[ovs-dev] [PATCH] ovs-ofctl:To set importance of a rule for eviction(OF14)
Ben Pfaff
blp at nicira.com
Fri Sep 26 17:19:23 UTC 2014
On Fri, Sep 26, 2014 at 04:27:06PM +0530, Rishi Bamba wrote:
> This patch enables a user to set importance for a new rule via add-flow
> OF1.1+ in the OVS and display the same via dump-flows command OF1.1+ .
> The changes are made in accordance with OpenFlow 1.4 specs to implement
> Eviction on the basis of "importance".
>
> Signed-off-by: Rishi Bamba <rishi.bamba at tcs.com>
The Clang compiler reports:
../ofproto/ofproto.c:3830:29: error: reading variable 'importance' requires
holding any mutex [-Werror,-Wthread-safety-analysis]
fs.importance=rule->importance;
^
Please run the tests before you submit. This change causes 40 tests
to fail. I think that you must have failed to initialize the
importance somewhere, because I see random numbers like 33691 and
23130 in the output.
Please add a test that sets the importance of a flow and verifies that
it dumps correctly.
Please read CodingStyle and follow it. In particular, we always put a
space on each side of =.
I think that only an OFPPC_ADD flow_mod should set the importance,
because that is what OF1.4 says for hard_timeout and idle_timeout.
Please change the code to do that, and then fold in the following
update to DESIGN:
diff --git a/DESIGN b/DESIGN
index f864135..c6e9096 100644
--- a/DESIGN
+++ b/DESIGN
@@ -269,7 +269,11 @@ The table for 1.3 is the same as the one shown above for 1.2.
OpenFlow 1.4
------------
-OpenFlow 1.4 does not change flow_mod semantics.
+OpenFlow 1.4 adds the "importance" field to flow_mods, but it does not
+explicitly specify which kinds of flow_mods set the importance. For
+consistency, Open vSwitch uses the same rule for importance as for
+idle_timeout and hard_timeout, that is, only an "ADD" flow_mod sets
+the importance. (This issue has been filed with the ONF as EXT-496.)
OFPT_PACKET_IN
> ---
> include/openflow/openflow-1.1.h | 5 +++--
> lib/ofp-parse.c | 6 +++++-
> lib/ofp-print.c | 3 +++
> lib/ofp-util.c | 5 +++++
> lib/ofp-util.h | 3 +++
> ofproto/ofproto-provider.h | 3 +++
> ofproto/ofproto.c | 5 ++++-
> 7 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
> index f87c5cf..d881192 100644
> --- a/include/openflow/openflow-1.1.h
> +++ b/include/openflow/openflow-1.1.h
> @@ -340,7 +340,7 @@ struct ofp11_flow_mod {
> output group. A value of OFPG11_ANY
> indicates no restriction. */
> ovs_be16 flags; /* One of OFPFF_*. */
> - uint8_t pad[2];
> + ovs_be16 importance; /* Eviction Precedence */
> /* Followed by an ofp11_match structure. */
> /* Followed by an instruction set. */
> };
> @@ -451,7 +451,8 @@ struct ofp11_flow_stats {
> ovs_be16 idle_timeout; /* Number of seconds idle before expiration. */
> ovs_be16 hard_timeout; /* Number of seconds before expiration. */
> ovs_be16 flags; /* OF 1.3: Set of OFPFF*. */
> - uint8_t pad2[4]; /* Align to 64-bits. */
> + ovs_be16 importance; /* Eviction Precedence */
> + uint8_t pad2[2]; /* Align to 64-bits. */
> ovs_be64 cookie; /* Opaque controller-issued identifier. */
> ovs_be64 packet_count; /* Number of packets in flow. */
> ovs_be64 byte_count; /* Number of bytes in flow. */
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index eed5a08..23626cb 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -248,6 +248,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
> enum {
> F_OUT_PORT = 1 << 0,
> F_ACTIONS = 1 << 1,
> + F_IMPORTANCE=1 << 2,
> F_TIMEOUT = 1 << 3,
> F_PRIORITY = 1 << 4,
> F_FLAGS = 1 << 5,
> @@ -264,7 +265,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
> break;
>
> case OFPFC_ADD:
> - fields = F_ACTIONS | F_TIMEOUT | F_PRIORITY | F_FLAGS;
> + fields = F_ACTIONS | F_TIMEOUT | F_PRIORITY | F_FLAGS | F_IMPORTANCE;
> break;
>
> case OFPFC_DELETE:
> @@ -305,6 +306,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
> fm->buffer_id = UINT32_MAX;
> fm->out_port = OFPP_ANY;
> fm->flags = 0;
> + fm->importance = OFP_FLOW_PERMANENT;
> fm->out_group = OFPG11_ANY;
> fm->delete_reason = OFPRR_DELETE;
> if (fields & F_ACTIONS) {
> @@ -366,6 +368,8 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
> error = str_to_u16(value, name, &fm->idle_timeout);
> } else if (fields & F_TIMEOUT && !strcmp(name, "hard_timeout")) {
> error = str_to_u16(value, name, &fm->hard_timeout);
> + } else if (fields & F_IMPORTANCE && !strcmp(name, "importance")) {
> + error = str_to_u16(value, name, &fm->importance);
> } else if (!strcmp(name, "cookie")) {
> char *mask = strchr(value, '/');
>
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 43bfa17..7f45858 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1431,6 +1431,9 @@ ofp_print_flow_stats(struct ds *string, struct ofputil_flow_stats *fs)
> if (fs->flags) {
> ofp_print_flow_flags(string, fs->flags);
> }
> + if (fs->importance != OFP_FLOW_PERMANENT) {
> + ds_put_format(string, "importance=%"PRIu16", ", fs->importance);
> + }
> if (fs->idle_age >= 0) {
> ds_put_format(string, "idle_age=%d, ", fs->idle_age);
> }
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index c8d38e8..9c3f219 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1700,6 +1700,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
>
> fm->idle_timeout = ntohs(ofm->idle_timeout);
> fm->hard_timeout = ntohs(ofm->hard_timeout);
> + fm->importance = ntohs(ofm->importance);
> fm->buffer_id = ntohl(ofm->buffer_id);
> error = ofputil_port_from_ofp11(ofm->out_port, &fm->out_port);
> if (error) {
> @@ -1738,6 +1739,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
> fm->new_cookie = ofm->cookie;
> fm->idle_timeout = ntohs(ofm->idle_timeout);
> fm->hard_timeout = ntohs(ofm->hard_timeout);
> + fm->importance = 0;
> fm->buffer_id = ntohl(ofm->buffer_id);
> fm->out_port = u16_to_ofp(ntohs(ofm->out_port));
> fm->out_group = OFPG11_ANY;
> @@ -2248,6 +2250,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
> ofm->out_port = ofputil_port_to_ofp11(fm->out_port);
> ofm->out_group = htonl(fm->out_group);
> ofm->flags = raw_flags;
> + ofm->importance = htons(fm->importance);
> ofputil_put_ofp11_match(msg, &fm->match, protocol);
> ofpacts_put_openflow_instructions(fm->ofpacts, fm->ofpacts_len, msg,
> version);
> @@ -2834,6 +2837,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
> fs->duration_nsec = ntohl(ofs->duration_nsec);
> fs->idle_timeout = ntohs(ofs->idle_timeout);
> fs->hard_timeout = ntohs(ofs->hard_timeout);
> + fs->importance = ntohs(ofs->importance);
> if (raw == OFPRAW_OFPST13_FLOW_REPLY) {
> error = ofputil_decode_flow_mod_flags(ofs->flags, -1, oh->version,
> &fs->flags);
> @@ -2977,6 +2981,7 @@ ofputil_append_flow_stats_reply(const struct ofputil_flow_stats *fs,
> ofs->priority = htons(fs->priority);
> ofs->idle_timeout = htons(fs->idle_timeout);
> ofs->hard_timeout = htons(fs->hard_timeout);
> + ofs->importance = htons(fs->importance);
> if (raw == OFPRAW_OFPST13_FLOW_REPLY) {
> ofs->flags = ofputil_encode_flow_mod_flags(fs->flags, version);
> } else {
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index af1a2a3..3173ab4 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -306,6 +306,7 @@ struct ofputil_flow_mod {
> ofp_port_t out_port;
> uint32_t out_group;
> enum ofputil_flow_mod_flags flags;
> + uint16_t importance; /* Eviction Precedence */
> struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
> size_t ofpacts_len; /* Length of ofpacts, in bytes. */
>
> @@ -355,6 +356,7 @@ struct ofputil_flow_stats {
> const struct ofpact *ofpacts;
> size_t ofpacts_len;
> enum ofputil_flow_mod_flags flags;
> + uint16_t importance; /* Eviction Precedence */
> };
>
> int ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *,
> @@ -391,6 +393,7 @@ struct ofputil_flow_removed {
> uint16_t hard_timeout;
> uint64_t packet_count; /* Packet count, UINT64_MAX if unknown. */
> uint64_t byte_count; /* Byte count, UINT64_MAX if unknown. */
> + uint16_t importance; /* Eviction Precedence */
> };
>
> enum ofperr ofputil_decode_flow_removed(struct ofputil_flow_removed *,
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 158f86e..106d4e6 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -346,6 +346,9 @@ struct rule {
> uint16_t hard_timeout OVS_GUARDED; /* In seconds from ->modified. */
> uint16_t idle_timeout OVS_GUARDED; /* In seconds from ->used. */
>
> + /* "Importance" is the eviction precedence of a flow */
> + uint16_t importance OVS_GUARDED;
> +
> /* Eviction groups (see comment on struct eviction_group for explanation) .
> *
> * 'eviction_group' is this rule's eviction group, or NULL if it is not in
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 5233a4d..f2835f7 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -3827,6 +3827,7 @@ handle_flow_stats_request(struct ofconn *ofconn,
> fs.ofpacts_len = actions->ofpacts_len;
>
> fs.flags = flags;
> + fs.importance=rule->importance;
> ofputil_append_flow_stats_reply(&fs, &replies);
> }
>
> @@ -4237,6 +4238,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
> ovs_mutex_lock(&rule->mutex);
> rule->idle_timeout = fm->idle_timeout;
> rule->hard_timeout = fm->hard_timeout;
> + rule->importance = fm->importance;
> ovs_mutex_unlock(&rule->mutex);
>
> *CONST_CAST(uint8_t *, &rule->table_id) = table - ofproto->tables;
> @@ -4354,6 +4356,7 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
> if (fm->command == OFPFC_ADD) {
> rule->idle_timeout = fm->idle_timeout;
> rule->hard_timeout = fm->hard_timeout;
> + rule->importance = fm->importance;
> rule->flags = fm->flags & OFPUTIL_FF_STATE;
> rule->created = now;
> }
> @@ -4367,7 +4370,7 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
> cookies_insert(ofproto, rule);
> }
> if (fm->command == OFPFC_ADD) {
> - if (fm->idle_timeout || fm->hard_timeout) {
> + if (fm->idle_timeout || fm->hard_timeout || fm->importance) {
> if (!rule->eviction_group) {
> eviction_group_add_rule(rule);
> }
> --
> 2.1.1
>
>
>
> Thank You
> Regards
> Rishi
>
>
> ----- Original Message -----
> From: "Ben Pfaff" <blp at nicira.com>
> To: "Rishi Bamba" <rishi.bamba at tcs.com>
> Cc: dev at openvswitch.org
> Sent: Monday, September 22, 2014 10:31:37 PM
> Subject: Re: [ovs-dev] Addition of importance parameter in flow_mod structure for implementation of eviction according to OF1.4
>
> It would save a lot of time for both of us if you would just post your
> patches.
>
> On Mon, Sep 22, 2014 at 08:09:03PM +0530, Rishi Bamba wrote:
> > Hi Ben,
> >
> > In reference to the mail dated 15.09.2014 I would like to thank you for your continued support to the team.Attached is the mail for reference.
> >
> > In progress to the same ,so far we have achieved the below mentioned.Wanted to share the understanding and confirmation on the same.
> >
> >
> > Case 1: Flow Added with default active version on OpenvSwitch ( OF10 ) alongwith importance
> > ( Eviction implementation in accordance with OF1.4 ).
> >
> > Add Flow Command(OF10) : ovs-ofctl add-flow br0 priority=21,importance=21,action=normal
> > Output: No Error , rule saved but with default importance( i.e zero ) rather than with the specified value by user.
> >
> >
> > Dump-Flow Command(OF 10) : ovs-ofctl dump-flows br0
> > Output: cookie=0x0, duration=8.736s, table=0, n_packets=0, n_bytes=0, idle_age=8, priority=21 actions=NORMAL
> >
> > Dump-Flow Command(OF 11+) : ovs-ofctl -O OpenFlow13 dump-flows br0
> > Output: cookie=0x0, duration=116.980s, table=0, n_packets=6, n_bytes=546, priority=21 actions=NORMAL
> >
> > The understanding is ,if the flow is added by the default active version i.e OF10 , the added flow will have importance set as zero whether provided by the user or not.The same is reflected in the dump flows either via OF10 or OF11+ . If importance is zero it will not be visible in that case.
> >
> > Query 1: Is this handling correct or we need to throw a message to the user that "importance" is supported by a particular version only rather than making it zero. Also as we are using OF11 flow mod structure as mentioned in previous mail, will importance parameter in add-flow should be supported on 1.1+ or we have to make it OF1.4 specific only.
> >
> >
> >
> > Case 2: Flow Added with specific version on OpenvSwitch ( OF11+ ) alongwith importance
> > (Eviction implementation in accordance with OF1.4 ).
> >
> > Add Flow Command(OF11+) : ovs-ofctl -O OpenFlow13 add-flow br0 priority=22,importance=22,action=drop
> > Output: No Error , rule saved with the value provided by the user and else zero if not provided by the user.
> >
> >
> > Dump-Flow Command(OF 10) : ovs-ofctl dump-flows br0
> > Output: (IMPORTANCE NOT VISIBLE)
> > cookie=0x0, duration=47.841s, table=0, n_packets=2, n_bytes=182, idle_age=24, priority=22 actions=drop
> > cookie=0x0, duration=243.481s, table=0, n_packets=6, n_bytes=546, idle_age=152, priority=21 actions=NORMAL
> >
> > Dump-Flow Command(OF 11+) : ovs-ofctl -O OpenFlow13 dump-flows br0
> > Output: (IMPORTANCE VISIBLE FOR THE FLOWS HAVING VALUE SET FOR THE FIELD)
> > cookie=0x0, duration=141.275s, table=0, n_packets=2, n_bytes=182, importance=22, priority=22 actions=drop
> > cookie=0x0, duration=336.915s, table=0, n_packets=6, n_bytes=546, priority=21 actions=NORMAL
> >
> > The understanding is,if the flow is added by a specific version (OF11+), the added flow will have importance set as the value provided by the user else zero.Now if the user dump-flows with the default version (OF10) none of the flows importance parameter will be visible even for the set one's else if dump-flows via OF11+ , the importance parameter will be visible for the flows which have the parameter set of.
> >
> > Query 2: Do we need to show the importance field if a flow is added by OF11+ and dump-flows via OF10. Currently we are not showing as mentioned above.Also this should also be only 1.4 specific or 1.1+ is fine.
> >
> >
> > Note: On finalizing the the above mentioned approach we will be submitting the patch for the same at the earliest. Need your confirmation on the above working.
> >
> >
> > Thank You
> > Regards
> > Rishi
> >
> > =====-----=====-----=====
> > Notice: The information contained in this e-mail
> > message and/or attachments to it may contain
> > confidential or privileged information. If you are
> > not the intended recipient, any dissemination, use,
> > review, distribution, printing or copying of the
> > information contained in this e-mail message
> > and/or attachments to it are strictly prohibited. If
> > you have received this communication in error,
> > please notify us by reply e-mail or telephone and
> > immediately and permanently delete the message
> > and any attachments. Thank you
> >
> >
>
> > Date: Sun, 14 Sep 2014 13:50:23 -0700
> > From: Ben Pfaff <blp at nicira.com>
> > To: Rishi Bamba <rishi.bamba at tcs.com>
> > Cc: dev at openvswitch.org
> > Subject: Re: [ovs-dev] Addition of importance parameter in flow_mod
> > structure for implementation of eviction according to OF1.4
> > User-Agent: Mutt/1.5.21 (2010-09-15)
> >
> > On Fri, Sep 12, 2014 at 05:55:18PM +0530, Rishi Bamba wrote:
> > > 1. Currently we have modified ofp11_flow_mod in openflow-1.1.h for
> > > addition of "ovs_be16 importance" parameter.
> > >
> > > Query: Do we need to create ofp14_flow_mod as the structure is
> > > currently not present in openflow-1.4.h or the enhancement to
> > > ofp11_flow_mod in openflow-1.1.h is justified.
> >
> > It's OK to update ofp11_flow_mod because the new member just assigns a
> > meaning to a field previously used for padding.
> >
> > > 2. We noticed that when using "add-flow" command with argument "-O
> > > OpenFlow14" a error message is thrown
> >
> > You should enable OF1.4. See the FAQ.
> >
> > Q: What versions of OpenFlow does Open vSwitch support?
> >
> > A: The following table lists the versions of OpenFlow supported by
> > each version of Open vSwitch:
> >
> > Open vSwitch OF1.0 OF1.1 OF1.2 OF1.3 OF1.4 OF1.5
> > =============== ===== ===== ===== ===== ===== =====
> > 1.9 and earlier yes --- --- --- --- ---
> > 1.10 yes --- [*] [*] --- ---
> > 1.11 yes --- [*] [*] --- ---
> > 2.0 yes [*] [*] [*] --- ---
> > 2.1 yes [*] [*] [*] --- ---
> > 2.2 yes [*] [*] [*] [%] [*]
> > 2.3 yes yes yes yes [*] [*]
> >
> > [*] Supported, with one or more missing features.
> > [%] Experimental, unsafe implementation.
> >
> > Open vSwitch 2.3 enables OpenFlow 1.0, 1.1, 1.2, and 1.3 by default
> > in ovs-vswitchd. In Open vSwitch 1.10 through 2.2, OpenFlow 1.1,
> > 1.2, and 1.3 must be enabled manually in ovs-vswitchd. OpenFlow
> > 1.4 and 1.5 are also supported, with missing features, in Open
> > vSwitch 2.3 and later, but not enabled by default. In any case,
> > the user may override the default:
> >
> > - To enable OpenFlow 1.0, 1.1, 1.2, and 1.3 on bridge br0:
> >
> > ovs-vsctl set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13
> >
> > - To enable OpenFlow 1.0, 1.1, 1.2, 1.3, 1.4, and 1.5 on bridge br0:
> >
> > ovs-vsctl set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15
> >
> > - To enable only OpenFlow 1.0 on bridge br0:
> >
> > ovs-vsctl set bridge br0 protocols=OpenFlow10
> >
> > All current versions of ovs-ofctl enable only OpenFlow 1.0 by
> > default. Use the -O option to enable support for later versions of
> > OpenFlow in ovs-ofctl. For example:
> >
> > ovs-ofctl -O OpenFlow13 dump-flows br0
> >
> > (Open vSwitch 2.2 had an experimental implementation of OpenFlow
> > 1.4 that could cause crashes. We don't recommend enabling it.)
> >
> > OPENFLOW-1.1+ in the Open vSwitch source tree tracks support for
> > OpenFlow 1.1 and later features. When support for OpenFlow 1.4 and
> > 1.5 is solidly implemented, Open vSwitch will enable those version
> > by default. Also, the OpenFlow 1.5 specification is still under
> > development and thus subject to change.
>
More information about the dev
mailing list