With some questions for clarification:
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
> On Jun 24, 2015, at 10:57 AM, Ben Pfaff <b...@nicira.com> wrote:
>
>
(snip)
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 495f787..b7bf734 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -246,9 +246,16 @@ struct oftable {
> struct hmap eviction_groups_by_id;
> struct heap eviction_groups_by_size;
>
> - /* Table configuration. */
> + /* Flow table miss handling configuration. */
> ATOMIC(enum ofputil_table_miss) miss_config;
>
> + /* Eviction is enabled if either the client (vswitchd) enables it or an
> + * OpenFlow controller enables it; thus, a nonzero value indicates that
> + * eivction is enabled. */
->'eviction'
> +#define EVICTION_CLIENT (1 << 0) /* Set to 1 if client enables eviction. */
> +#define EVICTION_OPENFLOW (1 << 1) /* Set to 1 if OpenFlow enables eviction.
> */
> + unsigned int eviction;
> +
> atomic_ulong n_matched;
> atomic_ulong n_missed;
> };
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index fd14030..66edc87 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -83,11 +83,10 @@ static void oftable_set_name(struct oftable *, const char
> *name);
>
> static enum ofperr evict_rules_from_table(struct oftable *)
> OVS_REQUIRES(ofproto_mutex);
> -static void oftable_disable_eviction(struct oftable *)
> - OVS_REQUIRES(ofproto_mutex);
> -static void oftable_enable_eviction(struct oftable *,
> - const struct mf_subfield *fields,
> - size_t n_fields)
> +static void oftable_configure_eviction(struct oftable *,
> + unsigned int eviction,
> + const struct mf_subfield *fields,
> + size_t n_fields)
> OVS_REQUIRES(ofproto_mutex);
>
> /* A set of rules within a single OpenFlow table (oftable) that have the same
> @@ -1421,18 +1420,16 @@ ofproto_configure_table(struct ofproto *ofproto, int
> table_id,
> return;
> }
>
> -
> if (classifier_set_prefix_fields(&table->cls,
> s->prefix_fields, s->n_prefix_fields)) {
> /* XXX: Trigger revalidation. */
> }
>
> ovs_mutex_lock(&ofproto_mutex);
> - if (s->groups) {
> - oftable_enable_eviction(table, s->groups, s->n_groups);
> - } else {
> - oftable_disable_eviction(table);
> - }
> + unsigned int new_eviction = (s->enable_eviction
> + ? table->eviction | EVICTION_CLIENT
> + : table->eviction & ~EVICTION_CLIENT);
> + oftable_configure_eviction(table, new_eviction, s->groups, s->n_groups);
> table->max_flows = s->max_flows;
> evict_rules_from_table(table);
> ovs_mutex_unlock(&ofproto_mutex);
> @@ -1695,7 +1692,7 @@ ofproto_run(struct ofproto *p)
> struct eviction_group *evg;
> struct rule *rule;
>
> - if (!table->eviction_fields) {
> + if (!table->eviction) {
> continue;
> }
>
> @@ -6550,10 +6547,34 @@ handle_group_mod(struct ofconn *ofconn, const struct
> ofp_header *oh)
> enum ofputil_table_miss
> ofproto_table_get_miss_config(const struct ofproto *ofproto, uint8_t table_id)
> {
> - enum ofputil_table_miss value;
> + enum ofputil_table_miss miss;
> +
> + atomic_read_relaxed(&ofproto->tables[table_id].miss_config, &miss);
> + return miss;
> +}
> +
> +static void
> +table_mod__(struct oftable *oftable,
> + enum ofputil_table_miss miss, enum ofputil_table_eviction
> eviction)
> +{
> + if (miss != OFPUTIL_TABLE_MISS_DEFAULT) {
> + atomic_store_relaxed(&oftable->miss_config, miss);
> + }
> +
Would this not allow miss_config to be changed back to MISS_DEFAULT? If this is
intentional, could you add a comment?
> + unsigned int new_eviction = oftable->eviction;
> + if (eviction == OFPUTIL_TABLE_EVICTION_ON) {
> + new_eviction |= EVICTION_OPENFLOW;
> + } else if (eviction == OFPUTIL_TABLE_EVICTION_OFF) {
> + new_eviction &= ~EVICTION_OPENFLOW;
> + }
>
> - atomic_read_relaxed(&ofproto->tables[table_id].miss_config, &value);
> - return value;
> + if (new_eviction != oftable->eviction) {
> + ovs_mutex_lock(&ofproto_mutex);
> + oftable_configure_eviction(oftable, new_eviction,
> + oftable->eviction_fields,
> + oftable->n_eviction_fields);
> + ovs_mutex_unlock(&ofproto_mutex);
> + }
> }
>
> static enum ofperr
> @@ -6561,18 +6582,33 @@ table_mod(struct ofproto *ofproto, const struct
> ofputil_table_mod *tm)
> {
> if (!check_table_id(ofproto, tm->table_id)) {
> return OFPERR_OFPTMFC_BAD_TABLE;
> - } else if (tm->miss_config != OFPUTIL_TABLE_MISS_DEFAULT) {
> - if (tm->table_id == OFPTT_ALL) {
> - int i;
> - for (i = 0; i < ofproto->n_tables; i++) {
> - atomic_store_relaxed(&ofproto->tables[i].miss_config,
> - tm->miss_config);
> + }
> +
> + /* Don't allow the eviction flags to be changed. OF1.4 says this is
> + * normal: "The OFPTMPT_EVICTION property usually cannot be modified
> using
> + * a OFP_TABLE_MOD request, because the eviction mechanism is switch
> + * defined". */
> + if (tm->eviction_flags != UINT32_MAX
> + && tm->eviction_flags != (OFPTMPEF14_OTHER | OFPTMPEF14_IMPORTANCE
> + | OFPTMPEF14_LIFETIME)) {
Why is this one specific combination of three flags allowed?
> + return OFPERR_OFPTMFC_BAD_CONFIG;
> + }
> +
> + if (tm->table_id == OFPTT_ALL) {
> + struct oftable *oftable;
> + OFPROTO_FOR_EACH_TABLE (oftable, ofproto) {
> + if (!(oftable->flags & (OFTABLE_HIDDEN | OFTABLE_READONLY))) {
> + table_mod__(oftable, tm->miss, tm->eviction);
> }
> - } else {
> - atomic_store_relaxed(&ofproto->tables[tm->table_id].miss_config,
> - tm->miss_config);
> }
> + } else {
> + struct oftable *oftable = &ofproto->tables[tm->table_id];
> + if (oftable->flags & OFTABLE_READONLY) {
> + return OFPERR_OFPTMFC_EPERM;
> + }
> + table_mod__(oftable, tm->miss, tm->eviction);
> }
> +
> return 0;
> }
>
(snip)
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 63c2ecc..92edd91 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -62,20 +62,6 @@ Prints to the console statistics for each of the flow
> tables used by
> \fBdump\-table\-features \fIswitch\fR
> Prints to the console features for each of the flow tables used by
> \fIswitch\fR.
> -.
> -.IP "\fBmod\-table \fIswitch\fR \fItable_id\fR \fIflow_miss_handling\fR"
> -An OpenFlow 1.0 switch looks up each packet that arrives at the switch
> -in table 0, then in table 1 if there is no match in table 0, then in
> -table 2, and so on until the packet finds a match in some table.
> -Finally, if no match was found, the switch sends the packet to the
> -controller
> -.IP
> -OpenFlow 1.1 and later offer more flexibility. This command
> -configures the flow table miss handling configuration for table
> -\fItable_id\fR in \fIswitch\fR. \fItable_id\fR may be an OpenFlow
> -table number between 0 and 254, inclusive, or the keyword \fBALL\fR to
> -modify all tables. \fIflow_miss_handling\fR may be any one of the
> -following:
Why was this documentation removed?
> .RS
> .IP \fBdrop\fR
> Drop the packet.
> @@ -87,6 +73,21 @@ tables other than the last one.)
> Send to controller. (This is how an OpenFlow 1.0 switch always
> handles packets that do not match any flow in the last table.)
> .RE
> +.IP
> +In OpenFlow 1.4 and later (which must be enabled with the \fB\-O\fR
> +option) only, \fBmod\-table\fR configures the behavior when a
> +controller attempts to add a flow to a flow table that is full. The
> +following \fIsetting\fR values are available:
> +.RS
> +.IP \fBevict\fR
> +Delete some existing flow from the flow table, according to the
> +algorithm described for the \fBFlow_Table\fR table in
> +\fBovs-vswitchd.conf.db\fR(5).
> +.IP \fBnoevict\fR
> +Refuse to add the new flow. (Eviction might still be enabled through
> +the \fBoverflow_policy\fR oclumn in the \fBFlow_Table\fR table
> +documented in \fBovs-vswitchd.conf.db\fR(5).)
> +.RE
> .
> .TP
> \fBdump\-ports \fIswitch\fR [\fInetdev\fR]
>
(snip)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
