Re: [ovs-dev] question about openvswitch
In the SE mode, i must implement the datapath module, dpif provider and dpif as showed in the graph without other module changes, is this right? right now i'm on the stage of doing some research and experiments, i prepare to accelerate OVS. then, maybe used in campus network in future. what i want to prove is just the feasibility right now. thanks to your answer. 2013/3/8 Justin Pettit > Please keep this on the list. > > I'm somewhat familiar with Cavium's platforms. One option would be to > bring up the cores in Linux, and I think you could run the kernel module > essentially unchanged. Another option, which would be a lot more work but > would almost certainly be faster, is to port dpif to their simple executive > and run the userspace portion on a core running Linux. > > I've heard rumors that Cavium ported OVS to their platform, so it may be > worth contacting your rep, too. > > What are you looking to do with this port? What solution are you looking > to provide? > > --Justin > > > On Mar 7, 2013, at 11:33 PM, 王国栋 wrote: > > > i use the Cavium NP and the kernel is linux2.6. is that means if i do > not have the TCAM, i only can accelerate the exact-match procedure through > the platform? > > do the datapath kernel module need to change or something while > porting,give me some suggestions please. > > thanks again! > > > > 2013/3/8 Justin Pettit > > What operating system and platform are you porting it to? The ofproto > provider pushes flows that have wildcards, and are very similar to OpenFlow > flows. The dpif provider only pushes exact-match flows. If you have > access to something like a TCAM that supports priorities and wildcards, > then implementing an ofproto provider would be the way to go. If this is > pure software, then you probably want to implement a dpif, since you can do > the flow lookups with a hash function, which is much faster in software. > Most of this should be explained in the PORTING file, so you may want to > take another look at it. > > > > --Justin > > > > > > On Mar 7, 2013, at 11:09 PM, 王国栋 wrote: > > > > > i have some question about PORTING file. there is a graph in this file > as follow: > > > > > > now i am willing to port the source code to a network processor ,which > is MIPS arch. my goal is implement the kernel module in the platform.but i > still cannot figure out a clear procedure about it. which part should i > change? and the difference between ofproto provider and dpif provider is > the challenge, can you tell me about it? > > > thank you in advance! > > > > > > regards, > > > > > > martin > > > ___ > > > dev mailing list > > > dev@openvswitch.org > > > http://openvswitch.org/mailman/listinfo/dev > > > > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: GOTO_TABLE recursion removal.
Remove recursion from GOTO_TABLE processing in do_xlate_actions(). This allows packet processing pipelines built with goto table be longer and not interact with each other via the resubmit recursion limit. Signed-off-by: Jarno Rajahalme --- ofproto/ofproto-dpif.c | 112 +++- tests/ofproto-dpif.at | 14 ++ 2 files changed, 87 insertions(+), 39 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 89f5bf4..f720095 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5816,61 +5816,73 @@ compose_output_action(struct action_xlate_ctx *ctx, uint16_t ofp_port) } static void +tag_the_flow(struct action_xlate_ctx *ctx, struct rule_dpif *rule) +{ +struct ofproto_dpif *ofproto = ctx->ofproto; +uint8_t table_id = ctx->table_id; + +if (table_id > 0 && table_id < N_TABLES) { +struct table_dpif *table = &ofproto->tables[table_id]; +if (table->other_table) { +ctx->tags |= (rule && rule->tag + ? rule->tag + : rule_calculate_tag(&ctx->flow, + &table->other_table->mask, + table->basis)); +} +} +} + +/* Common rule processing in one place to avoid duplicating code. */ +static struct rule_dpif * +ctx_rule_hooks(struct action_xlate_ctx *ctx, struct rule_dpif *rule, + bool may_packet_in) +{ +if (ctx->resubmit_hook) { +ctx->resubmit_hook(ctx, rule); +} +if (rule == NULL && may_packet_in) { +/* XXX + * check if table configuration flags + * OFPTC_TABLE_MISS_CONTROLLER, default. + * OFPTC_TABLE_MISS_CONTINUE, + * OFPTC_TABLE_MISS_DROP + * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? + */ +rule = rule_dpif_miss_rule(ctx->ofproto, &ctx->flow); +} +if (rule && ctx->resubmit_stats) { +rule_credit_stats(rule, ctx->resubmit_stats); +} +return rule; +} + +static void xlate_table_action(struct action_xlate_ctx *ctx, uint16_t in_port, uint8_t table_id, bool may_packet_in) { if (ctx->recurse < MAX_RESUBMIT_RECURSION) { -struct ofproto_dpif *ofproto = ctx->ofproto; struct rule_dpif *rule; -uint16_t old_in_port; -uint8_t old_table_id; +uint16_t old_in_port = ctx->flow.in_port; +uint8_t old_table_id = ctx->table_id; -old_table_id = ctx->table_id; ctx->table_id = table_id; /* Look up a flow with 'in_port' as the input port. */ -old_in_port = ctx->flow.in_port; ctx->flow.in_port = in_port; -rule = rule_dpif_lookup__(ofproto, &ctx->flow, table_id); - -/* Tag the flow. */ -if (table_id > 0 && table_id < N_TABLES) { -struct table_dpif *table = &ofproto->tables[table_id]; -if (table->other_table) { -ctx->tags |= (rule && rule->tag - ? rule->tag - : rule_calculate_tag(&ctx->flow, - &table->other_table->mask, - table->basis)); -} -} +rule = rule_dpif_lookup__(ctx->ofproto, &ctx->flow, table_id); + +tag_the_flow(ctx, rule); /* Restore the original input port. Otherwise OFPP_NORMAL and * OFPP_IN_PORT will have surprising behavior. */ ctx->flow.in_port = old_in_port; -if (ctx->resubmit_hook) { -ctx->resubmit_hook(ctx, rule); -} - -if (rule == NULL && may_packet_in) { -/* XXX - * check if table configuration flags - * OFPTC_TABLE_MISS_CONTROLLER, default. - * OFPTC_TABLE_MISS_CONTINUE, - * OFPTC_TABLE_MISS_DROP - * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? - */ -rule = rule_dpif_miss_rule(ofproto, &ctx->flow); -} +rule = ctx_rule_hooks(ctx, rule, may_packet_in); if (rule) { struct rule_dpif *old_rule = ctx->rule; -if (ctx->resubmit_stats) { -rule_credit_stats(rule, ctx->resubmit_stats); -} - ctx->recurse++; ctx->rule = rule; do_xlate_actions(rule->up.ofpacts, rule->up.ofpacts_len, ctx); @@ -6341,12 +6353,15 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, { bool was_evictable = true; const struct ofpact *a; +struct rule_dpif *orig_rule = ctx->rule; if (ctx->rule) { /* Don't let the rule we're working on get evicted underneath us. */ was_evictable = ctx->rule->up.evictable; ctx->rule->up.evictable = false; } + + do_xlate_actions_again: OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
Re: [ovs-dev] [PATCH] ofproto-dpif: GOTO_TABLE recursion removal.
On Fri, Mar 08, 2013 at 03:50:12PM +0200, Jarno Rajahalme wrote: > Remove recursion from GOTO_TABLE processing in do_xlate_actions(). > This allows packet processing pipelines built with goto table be > longer and not interact with each other via the resubmit recursion limit. > > Signed-off-by: Jarno Rajahalme Thank you, this is a nice refinement. This looks very close to correct to me. However, I don't think that the handling of the 'evictable' flags is quite right. The basic rule for 'evictable' is that we need to make sure that it is set for any rule that we're currently "using" in some way (one example is iterating over its actions) to tell "learn" actions not to delete that rule. As I read your patch, it didn't do this for rules reached via "goto_table". (Probably because the purpose of 'evictable' really isn't obvious.) I applied the following incremental to fix that. It seems correct, so I'll apply it to master soon, but please check my work. Thanks, Ben. diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 6628aa6..d911080 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -6305,7 +6305,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, { bool was_evictable = true; const struct ofpact *a; -struct rule_dpif *orig_rule = ctx->rule; if (ctx->rule) { /* Don't let the rule we're working on get evicted underneath us. */ @@ -6509,7 +6508,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, rule = ctx_rule_hooks(ctx, rule, true); if (rule) { +if (ctx->rule) { +ctx->rule->up.evictable = was_evictable; +} ctx->rule = rule; +was_evictable = rule->up.evictable; +rule->up.evictable = false; /* Tail recursion removal. */ ofpacts = rule->up.ofpacts; @@ -6522,7 +6526,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, } out: -ctx->rule = orig_rule; if (ctx->rule) { ctx->rule->up.evictable = was_evictable; } ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] question about openvswitch
Using SE, I would imagine you'd implement a datapath there. In the Linux core running ovs-vswitchd, I would imagine you'd implement a dpif provider that that spoke to the datapaths running in your simple executive cores. --Justin On Mar 8, 2013, at 12:16 AM, 王国栋 wrote: > In the SE mode, i must implement the datapath module, dpif provider and dpif > as showed in the graph without other module changes, is this right? > right now i'm on the stage of doing some research and experiments, i prepare > to accelerate OVS. then, maybe used in campus network in future. what i want > to prove is just the feasibility right now. > thanks to your answer. > > 2013/3/8 Justin Pettit > Please keep this on the list. > > I'm somewhat familiar with Cavium's platforms. One option would be to bring > up the cores in Linux, and I think you could run the kernel module > essentially unchanged. Another option, which would be a lot more work but > would almost certainly be faster, is to port dpif to their simple executive > and run the userspace portion on a core running Linux. > > I've heard rumors that Cavium ported OVS to their platform, so it may be > worth contacting your rep, too. > > What are you looking to do with this port? What solution are you looking to > provide? > > --Justin > > > On Mar 7, 2013, at 11:33 PM, 王国栋 wrote: > > > i use the Cavium NP and the kernel is linux2.6. is that means if i do not > > have the TCAM, i only can accelerate the exact-match procedure through the > > platform? > > do the datapath kernel module need to change or something while > > porting,give me some suggestions please. > > thanks again! > > > > 2013/3/8 Justin Pettit > > What operating system and platform are you porting it to? The ofproto > > provider pushes flows that have wildcards, and are very similar to OpenFlow > > flows. The dpif provider only pushes exact-match flows. If you have > > access to something like a TCAM that supports priorities and wildcards, > > then implementing an ofproto provider would be the way to go. If this is > > pure software, then you probably want to implement a dpif, since you can do > > the flow lookups with a hash function, which is much faster in software. > > Most of this should be explained in the PORTING file, so you may want to > > take another look at it. > > > > --Justin > > > > > > On Mar 7, 2013, at 11:09 PM, 王国栋 wrote: > > > > > i have some question about PORTING file. there is a graph in this file as > > > follow: > > > > > > now i am willing to port the source code to a network processor ,which is > > > MIPS arch. my goal is implement the kernel module in the platform.but i > > > still cannot figure out a clear procedure about it. which part should i > > > change? and the difference between ofproto provider and dpif provider is > > > the challenge, can you tell me about it? > > > thank you in advance! > > > > > > regards, > > > > > > martin > > > ___ > > > dev mailing list > > > dev@openvswitch.org > > > http://openvswitch.org/mailman/listinfo/dev > > > > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Add table_id to NXM flow_removed messages.
I'll hold my nose on the "table_id + 1", but otherwise looks good. :-) --Justin On Mar 6, 2013, at 9:13 AM, Ben Pfaff wrote: > Feature #15466. > Requested-by: Ronghua Zhang > Signed-off-by: Ben Pfaff > --- > NEWS |2 ++ > include/openflow/nicira-ext.h | 10 -- > lib/ofp-util.c|3 ++- > tests/ofp-print.at|4 ++-- > 4 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/NEWS b/NEWS > index 7fb0932..a33c14a 100644 > --- a/NEWS > +++ b/NEWS > @@ -9,6 +9,8 @@ post-v1.10.0 > - OpenFlow: > * The "dec_mpls_ttl" and "set_mpls_ttl" actions from OpenFlow > 1.1 and later are now implemented. > + * The NXM flow_removed message now reports the OpenFlow table ID > +from which the flow was removed. > > > v1.10.0 - xx xxx > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > index cdb1952..a1e5b58 100644 > --- a/include/openflow/nicira-ext.h > +++ b/include/openflow/nicira-ext.h > @@ -1770,12 +1770,18 @@ struct nx_flow_mod { > }; > OFP_ASSERT(sizeof(struct nx_flow_mod) == 32); > > -/* NXT_FLOW_REMOVED (analogous to OFPT_FLOW_REMOVED). */ > +/* NXT_FLOW_REMOVED (analogous to OFPT_FLOW_REMOVED). > + * > + * 'table_id' is present only in Open vSwitch 1.11 and later. In earlier > + * versions of Open vSwitch, this is a padding byte that is always zeroed. > + * Therefore, a 'table_id' value of 0 indicates that the table ID is not > known, > + * and other values may be interpreted as one more than the flow's former > table > + * ID. */ > struct nx_flow_removed { > ovs_be64 cookie; /* Opaque controller-issued identifier. */ > ovs_be16 priority;/* Priority level of flow entry. */ > uint8_t reason; /* One of OFPRR_*. */ > -uint8_t pad[1]; /* Align to 32-bits. */ > +uint8_t table_id; /* Flow's former table ID, plus one. */ > ovs_be32 duration_sec;/* Time flow was alive in seconds. */ > ovs_be32 duration_nsec; /* Time flow was alive in nanoseconds beyond > duration_sec. */ > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index dd6eed8..06e149a 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -2345,7 +2345,7 @@ ofputil_decode_flow_removed(struct ofputil_flow_removed > *fr, > fr->priority = ntohs(nfr->priority); > fr->cookie = nfr->cookie; > fr->reason = nfr->reason; > -fr->table_id = 255; > +fr->table_id = nfr->table_id ? nfr->table_id - 1 : 255; > fr->duration_sec = ntohl(nfr->duration_sec); > fr->duration_nsec = ntohl(nfr->duration_nsec); > fr->idle_timeout = ntohs(nfr->idle_timeout); > @@ -2424,6 +2424,7 @@ ofputil_encode_flow_removed(const struct > ofputil_flow_removed *fr, > nfr->cookie = fr->cookie; > nfr->priority = htons(fr->priority); > nfr->reason = fr->reason; > +nfr->table_id = fr->table_id + 1; > nfr->duration_sec = htonl(fr->duration_sec); > nfr->duration_nsec = htonl(fr->duration_nsec); > nfr->idle_timeout = htons(fr->idle_timeout); > diff --git a/tests/ofp-print.at b/tests/ofp-print.at > index 2939125..35f599c 100644 > --- a/tests/ofp-print.at > +++ b/tests/ofp-print.at > @@ -1699,7 +1699,7 @@ AT_SETUP([NXT_FLOW_REMOVED]) > AT_KEYWORDS([ofp-print]) > AT_CHECK([ovs-ofctl ofp-print "\ > 01 04 00 78 00 00 00 00 00 00 23 20 00 00 00 0e \ > -00 00 00 00 00 00 00 00 ff ff 00 00 00 00 00 06 \ > +00 00 00 00 00 00 00 00 ff ff 00 02 00 00 00 06 \ > 01 6e 36 00 00 05 00 3c 00 00 00 00 00 00 00 01 \ > 00 00 00 00 00 00 00 3c 00 00 00 02 00 03 00 00 \ > 02 06 50 54 00 00 00 06 00 00 04 06 50 54 00 00 \ > @@ -1707,7 +1707,7 @@ AT_CHECK([ovs-ofctl ofp-print "\ > 1e 02 00 02 00 00 20 04 c0 a8 00 01 00 00 22 04 \ > c0 a8 00 02 00 00 00 00 \ > "], [0], [dnl > -NXT_FLOW_REMOVED (xid=0x0): > priority=65535,arp,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2 > reason=idle duration6.024s idle5 pkts1 bytes60 > +NXT_FLOW_REMOVED (xid=0x0): > priority=65535,arp,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2 > reason=idle table_id=1 duration6.024s idle5 pkts1 bytes60 > ]) > AT_CLEANUP > > -- > 1.7.10.4 > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Fix up user specifying wrong bridge on "ofproto/trace".
I wonder if it would be better to only print the bridge name when there was a mismatch along with a warning. I'm not a heavy user of "ofproto/trace", so I don't have a good sense for which way would be better. Looks good, though. --Justin On Mar 5, 2013, at 4:49 PM, Ben Pfaff wrote: > If there is more than one bridge, then it's easy to specify the wrong one > on an ofproto/trace command. Previously, this would produce surprising > results. With this commit, "ofproto/trace" should silently fix up the > problem. > > It would be nice to not require the user to specify a bridge at all, but > it's theoretically possible to have more than one backer, in which case we > need some way to distinguish, and a bridge name is as good an identifier > as we have. We could ask the user to specify the datapath_type, I guess, > but that's a less familiar name to most users and it would be a somewhat > gratuitous change in synatx for ofproto/trace. > > Bug #15419. > Reported-by: Paul Ingram > Signed-off-by: Ben Pfaff > --- > ofproto/ofproto-dpif.c | 10 +- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 7035530..1087d72 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -7626,16 +7626,16 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int > argc, const char *argv[], > goto exit; > } > > -/* XXX: Since we allow the user to specify an ofproto, it's > - * possible they will specify a different ofproto than the one > the > - * port actually belongs too. Ideally we should simply remove > the > - * ability to specify the ofproto. */ > +/* The user might have specified the wrong ofproto but within the > + * same backer. That's OK, ofproto_receive() can find the right > + * one for us. */ > if (ofproto_receive(ofproto->backer, NULL, odp_key.data, > -odp_key.size, &flow, NULL, NULL, NULL, > +odp_key.size, &flow, NULL, &ofproto, NULL, > &initial_tci)) { > unixctl_command_reply_error(conn, "Invalid flow"); > goto exit; > } > +ds_put_format(&result, "Bridge: %s\n", ofproto->up.name); > } else { > char *error_s; > > -- > 1.7.2.5 > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] nicira-ext: Document that nx_flow_mod_table_id applies to nx_flow_mod too.
It wasn't clear from the comments that nx_flow_mod_table_id applies to nx_flow_mod as well as ofp10_flow_mod, but it does and always has. This commit makes it clear. Reported-by: Justin Pettit Signed-off-by: Ben Pfaff --- include/openflow/nicira-ext.h | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index f9b0af4..36b5cfe 100644 --- a/include/openflow/nicira-ext.h +++ b/include/openflow/nicira-ext.h @@ -116,12 +116,14 @@ enum nx_hash_fields { /* This command enables or disables an Open vSwitch extension that allows a * controller to specify the OpenFlow table to which a flow should be added, * instead of having the switch decide which table is most appropriate as - * required by OpenFlow 1.0. By default, the extension is disabled. + * required by OpenFlow 1.0. Because NXM was designed as an extension to + * OpenFlow 1.0, the extension applies equally to ofp10_flow_mod and + * nx_flow_mod. By default, the extension is disabled. * * When this feature is enabled, Open vSwitch treats struct ofp10_flow_mod's - * 16-bit 'command' member as two separate fields. The upper 8 bits are used - * as the table ID, the lower 8 bits specify the command as usual. A table ID - * of 0xff is treated like a wildcarded table ID. + * and struct nx_flow_mod's 16-bit 'command' member as two separate fields. + * The upper 8 bits are used as the table ID, the lower 8 bits specify the + * command as usual. A table ID of 0xff is treated like a wildcarded table ID. * * The specific treatment of the table ID depends on the type of flow mod: * @@ -1766,7 +1768,8 @@ OFP_ASSERT(sizeof(struct nx_set_flow_format) == 4); */ struct nx_flow_mod { ovs_be64 cookie; /* Opaque controller-issued identifier. */ -ovs_be16 command; /* One of OFPFC_*. */ +ovs_be16 command; /* OFPFC_* + possibly a table ID (see comment + * on struct nx_flow_mod_table_id). */ ovs_be16 idle_timeout;/* Idle time before discarding (seconds). */ ovs_be16 hard_timeout;/* Max time before discarding (seconds). */ ovs_be16 priority;/* Priority level of flow entry. */ -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] nicira-ext: Document that nx_flow_mod_table_id applies to nx_flow_mod too.
Looks good. Thank you! --Justin On Mar 8, 2013, at 10:24 AM, Ben Pfaff wrote: > It wasn't clear from the comments that nx_flow_mod_table_id applies to > nx_flow_mod as well as ofp10_flow_mod, but it does and always has. This > commit makes it clear. > > Reported-by: Justin Pettit > Signed-off-by: Ben Pfaff > --- > include/openflow/nicira-ext.h | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > index f9b0af4..36b5cfe 100644 > --- a/include/openflow/nicira-ext.h > +++ b/include/openflow/nicira-ext.h > @@ -116,12 +116,14 @@ enum nx_hash_fields { > /* This command enables or disables an Open vSwitch extension that allows a > * controller to specify the OpenFlow table to which a flow should be added, > * instead of having the switch decide which table is most appropriate as > - * required by OpenFlow 1.0. By default, the extension is disabled. > + * required by OpenFlow 1.0. Because NXM was designed as an extension to > + * OpenFlow 1.0, the extension applies equally to ofp10_flow_mod and > + * nx_flow_mod. By default, the extension is disabled. > * > * When this feature is enabled, Open vSwitch treats struct ofp10_flow_mod's > - * 16-bit 'command' member as two separate fields. The upper 8 bits are used > - * as the table ID, the lower 8 bits specify the command as usual. A table > ID > - * of 0xff is treated like a wildcarded table ID. > + * and struct nx_flow_mod's 16-bit 'command' member as two separate fields. > + * The upper 8 bits are used as the table ID, the lower 8 bits specify the > + * command as usual. A table ID of 0xff is treated like a wildcarded table > ID. > * > * The specific treatment of the table ID depends on the type of flow mod: > * > @@ -1766,7 +1768,8 @@ OFP_ASSERT(sizeof(struct nx_set_flow_format) == 4); > */ > struct nx_flow_mod { > ovs_be64 cookie; /* Opaque controller-issued identifier. */ > -ovs_be16 command; /* One of OFPFC_*. */ > +ovs_be16 command; /* OFPFC_* + possibly a table ID (see > comment > + * on struct nx_flow_mod_table_id). */ > ovs_be16 idle_timeout;/* Idle time before discarding (seconds). */ > ovs_be16 hard_timeout;/* Max time before discarding (seconds). */ > ovs_be16 priority;/* Priority level of flow entry. */ > -- > 1.7.10.4 > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Fix up user specifying wrong bridge on "ofproto/trace".
It doesn't seem like a big deal to me one way or another. Anyway, if it's a problem I'm sure we'll get a report. I applied this to master and branch-1.10. Thanks for the review. On Fri, Mar 08, 2013 at 10:20:24AM -0800, Justin Pettit wrote: > I wonder if it would be better to only print the bridge name when > there was a mismatch along with a warning. I'm not a heavy user of > "ofproto/trace", so I don't have a good sense for which way would be > better. > > Looks good, though. > > --Justin > > > On Mar 5, 2013, at 4:49 PM, Ben Pfaff wrote: > > > If there is more than one bridge, then it's easy to specify the wrong one > > on an ofproto/trace command. Previously, this would produce surprising > > results. With this commit, "ofproto/trace" should silently fix up the > > problem. > > > > It would be nice to not require the user to specify a bridge at all, but > > it's theoretically possible to have more than one backer, in which case we > > need some way to distinguish, and a bridge name is as good an identifier > > as we have. We could ask the user to specify the datapath_type, I guess, > > but that's a less familiar name to most users and it would be a somewhat > > gratuitous change in synatx for ofproto/trace. > > > > Bug #15419. > > Reported-by: Paul Ingram > > Signed-off-by: Ben Pfaff > > --- > > ofproto/ofproto-dpif.c | 10 +- > > 1 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 7035530..1087d72 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -7626,16 +7626,16 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, > > int argc, const char *argv[], > > goto exit; > > } > > > > -/* XXX: Since we allow the user to specify an ofproto, it's > > - * possible they will specify a different ofproto than the one > > the > > - * port actually belongs too. Ideally we should simply remove > > the > > - * ability to specify the ofproto. */ > > +/* The user might have specified the wrong ofproto but within > > the > > + * same backer. That's OK, ofproto_receive() can find the > > right > > + * one for us. */ > > if (ofproto_receive(ofproto->backer, NULL, odp_key.data, > > -odp_key.size, &flow, NULL, NULL, NULL, > > +odp_key.size, &flow, NULL, &ofproto, NULL, > > &initial_tci)) { > > unixctl_command_reply_error(conn, "Invalid flow"); > > goto exit; > > } > > +ds_put_format(&result, "Bridge: %s\n", ofproto->up.name); > > } else { > > char *error_s; > > > > -- > > 1.7.2.5 > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Add table_id to NXM flow_removed messages.
The things we sacrifice for compatibility! I'll apply this to master and branch-1.10 in a minute. On Fri, Mar 08, 2013 at 10:11:58AM -0800, Justin Pettit wrote: > I'll hold my nose on the "table_id + 1", but otherwise looks good. :-) > > --Justin > > > On Mar 6, 2013, at 9:13 AM, Ben Pfaff wrote: > > > Feature #15466. > > Requested-by: Ronghua Zhang > > Signed-off-by: Ben Pfaff > > --- > > NEWS |2 ++ > > include/openflow/nicira-ext.h | 10 -- > > lib/ofp-util.c|3 ++- > > tests/ofp-print.at|4 ++-- > > 4 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 7fb0932..a33c14a 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -9,6 +9,8 @@ post-v1.10.0 > > - OpenFlow: > > * The "dec_mpls_ttl" and "set_mpls_ttl" actions from OpenFlow > > 1.1 and later are now implemented. > > + * The NXM flow_removed message now reports the OpenFlow table ID > > +from which the flow was removed. > > > > > > v1.10.0 - xx xxx > > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > > index cdb1952..a1e5b58 100644 > > --- a/include/openflow/nicira-ext.h > > +++ b/include/openflow/nicira-ext.h > > @@ -1770,12 +1770,18 @@ struct nx_flow_mod { > > }; > > OFP_ASSERT(sizeof(struct nx_flow_mod) == 32); > > > > -/* NXT_FLOW_REMOVED (analogous to OFPT_FLOW_REMOVED). */ > > +/* NXT_FLOW_REMOVED (analogous to OFPT_FLOW_REMOVED). > > + * > > + * 'table_id' is present only in Open vSwitch 1.11 and later. In earlier > > + * versions of Open vSwitch, this is a padding byte that is always zeroed. > > + * Therefore, a 'table_id' value of 0 indicates that the table ID is not > > known, > > + * and other values may be interpreted as one more than the flow's former > > table > > + * ID. */ > > struct nx_flow_removed { > > ovs_be64 cookie; /* Opaque controller-issued identifier. */ > > ovs_be16 priority;/* Priority level of flow entry. */ > > uint8_t reason; /* One of OFPRR_*. */ > > -uint8_t pad[1]; /* Align to 32-bits. */ > > +uint8_t table_id; /* Flow's former table ID, plus one. */ > > ovs_be32 duration_sec;/* Time flow was alive in seconds. */ > > ovs_be32 duration_nsec; /* Time flow was alive in nanoseconds beyond > > duration_sec. */ > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > > index dd6eed8..06e149a 100644 > > --- a/lib/ofp-util.c > > +++ b/lib/ofp-util.c > > @@ -2345,7 +2345,7 @@ ofputil_decode_flow_removed(struct > > ofputil_flow_removed *fr, > > fr->priority = ntohs(nfr->priority); > > fr->cookie = nfr->cookie; > > fr->reason = nfr->reason; > > -fr->table_id = 255; > > +fr->table_id = nfr->table_id ? nfr->table_id - 1 : 255; > > fr->duration_sec = ntohl(nfr->duration_sec); > > fr->duration_nsec = ntohl(nfr->duration_nsec); > > fr->idle_timeout = ntohs(nfr->idle_timeout); > > @@ -2424,6 +2424,7 @@ ofputil_encode_flow_removed(const struct > > ofputil_flow_removed *fr, > > nfr->cookie = fr->cookie; > > nfr->priority = htons(fr->priority); > > nfr->reason = fr->reason; > > +nfr->table_id = fr->table_id + 1; > > nfr->duration_sec = htonl(fr->duration_sec); > > nfr->duration_nsec = htonl(fr->duration_nsec); > > nfr->idle_timeout = htons(fr->idle_timeout); > > diff --git a/tests/ofp-print.at b/tests/ofp-print.at > > index 2939125..35f599c 100644 > > --- a/tests/ofp-print.at > > +++ b/tests/ofp-print.at > > @@ -1699,7 +1699,7 @@ AT_SETUP([NXT_FLOW_REMOVED]) > > AT_KEYWORDS([ofp-print]) > > AT_CHECK([ovs-ofctl ofp-print "\ > > 01 04 00 78 00 00 00 00 00 00 23 20 00 00 00 0e \ > > -00 00 00 00 00 00 00 00 ff ff 00 00 00 00 00 06 \ > > +00 00 00 00 00 00 00 00 ff ff 00 02 00 00 00 06 \ > > 01 6e 36 00 00 05 00 3c 00 00 00 00 00 00 00 01 \ > > 00 00 00 00 00 00 00 3c 00 00 00 02 00 03 00 00 \ > > 02 06 50 54 00 00 00 06 00 00 04 06 50 54 00 00 \ > > @@ -1707,7 +1707,7 @@ AT_CHECK([ovs-ofctl ofp-print "\ > > 1e 02 00 02 00 00 20 04 c0 a8 00 01 00 00 22 04 \ > > c0 a8 00 02 00 00 00 00 \ > > "], [0], [dnl > > -NXT_FLOW_REMOVED (xid=0x0): > > priority=65535,arp,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2 > > reason=idle duration6.024s idle5 pkts1 bytes60 > > +NXT_FLOW_REMOVED (xid=0x0): > > priority=65535,arp,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2 > > reason=idle table_id=1 duration6.024s idle5 pkts1 bytes60 > > ]) > > AT_CLEANUP > > > > -- > > 1.7.10.4 > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev
[ovs-dev] [PATCH] NEWS: Fix release for table ID in NXM flow_removed feature.
I forgot that I was planning to backport this to branch-1.10. Signed-off-by: Ben Pfaff --- I already applied this to master. diff --git a/NEWS b/NEWS index 90c01a6..b6800bc 100644 --- a/NEWS +++ b/NEWS @@ -11,8 +11,6 @@ post-v1.10.0 1.1 and later are now implemented. * New "stack" extension for use in actions, to push and pop from NXM fields. - * The NXM flow_removed message now reports the OpenFlow table ID -from which the flow was removed. v1.10.0 - xx xxx @@ -36,6 +34,8 @@ v1.10.0 - xx xxx value of other-config:dp-desc in the Bridge table. - It is possible to request the OpenFlow port number with the "ofport_request" column in the Interface table. + - The NXM flow_removed message now reports the OpenFlow table ID +from which the flow was removed. - Tunneling: - New support for the VXLAN tunnel protocol (see the IETF draft here: http://tools.ietf.org/html/draft-mahalingam-dutt-dcops-vxlan-03). -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] nicira-ext: Document that nx_flow_mod_table_id applies to nx_flow_mod too.
Thanks, I applied this to master. On Fri, Mar 08, 2013 at 10:27:20AM -0800, Justin Pettit wrote: > Looks good. Thank you! > > --Justin > > > On Mar 8, 2013, at 10:24 AM, Ben Pfaff wrote: > > > It wasn't clear from the comments that nx_flow_mod_table_id applies to > > nx_flow_mod as well as ofp10_flow_mod, but it does and always has. This > > commit makes it clear. > > > > Reported-by: Justin Pettit > > Signed-off-by: Ben Pfaff > > --- > > include/openflow/nicira-ext.h | 13 - > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > > index f9b0af4..36b5cfe 100644 > > --- a/include/openflow/nicira-ext.h > > +++ b/include/openflow/nicira-ext.h > > @@ -116,12 +116,14 @@ enum nx_hash_fields { > > /* This command enables or disables an Open vSwitch extension that allows a > > * controller to specify the OpenFlow table to which a flow should be added, > > * instead of having the switch decide which table is most appropriate as > > - * required by OpenFlow 1.0. By default, the extension is disabled. > > + * required by OpenFlow 1.0. Because NXM was designed as an extension to > > + * OpenFlow 1.0, the extension applies equally to ofp10_flow_mod and > > + * nx_flow_mod. By default, the extension is disabled. > > * > > * When this feature is enabled, Open vSwitch treats struct ofp10_flow_mod's > > - * 16-bit 'command' member as two separate fields. The upper 8 bits are > > used > > - * as the table ID, the lower 8 bits specify the command as usual. A > > table ID > > - * of 0xff is treated like a wildcarded table ID. > > + * and struct nx_flow_mod's 16-bit 'command' member as two separate fields. > > + * The upper 8 bits are used as the table ID, the lower 8 bits specify the > > + * command as usual. A table ID of 0xff is treated like a wildcarded > > table ID. > > * > > * The specific treatment of the table ID depends on the type of flow mod: > > * > > @@ -1766,7 +1768,8 @@ OFP_ASSERT(sizeof(struct nx_set_flow_format) == 4); > > */ > > struct nx_flow_mod { > > ovs_be64 cookie; /* Opaque controller-issued identifier. */ > > -ovs_be16 command; /* One of OFPFC_*. */ > > +ovs_be16 command; /* OFPFC_* + possibly a table ID (see > > comment > > + * on struct nx_flow_mod_table_id). */ > > ovs_be16 idle_timeout;/* Idle time before discarding (seconds). > > */ > > ovs_be16 hard_timeout;/* Max time before discarding (seconds). > > */ > > ovs_be16 priority;/* Priority level of flow entry. */ > > -- > > 1.7.10.4 > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofpbuf: Remove OFPBUF_STACK_BUFFER.
On 27 February 2013 20:03, Ben Pfaff wrote: > Its alleged convenience just doesn't outweigh the syntactical ugliness, and > so it didn't have any users. Not entirely true: there's a consumer in lib/netdev-bsd.c. I don't have any time right now to look at it, but will try if nobody gets to it first. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofpbuf: Remove OFPBUF_STACK_BUFFER.
On 8 March 2013 16:06, Ed Maste wrote: > On 27 February 2013 20:03, Ben Pfaff wrote: >> Its alleged convenience just doesn't outweigh the syntactical ugliness, and >> so it didn't have any users. > > Not entirely true: there's a consumer in lib/netdev-bsd.c. I don't > have any time right now to look at it, but will try if nobody gets to > it first. Oh, sorry; I'm mistaken. The consumer is in my local patch set (the threading changes). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofpbuf: Remove OFPBUF_STACK_BUFFER.
On Fri, Mar 08, 2013 at 04:06:41PM -0500, Ed Maste wrote: > On 27 February 2013 20:03, Ben Pfaff wrote: > > Its alleged convenience just doesn't outweigh the syntactical ugliness, and > > so it didn't have any users. > > Not entirely true: there's a consumer in lib/netdev-bsd.c. I don't > have any time right now to look at it, but will try if nobody gets to > it first. Are you sure? I don't see any at all on master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: GOTO_TABLE recursion removal.
Ben, Thanks for fixing that up, looks good :-) Jarno On Mar 8, 2013, at 18:52 , ext Ben Pfaff wrote: > On Fri, Mar 08, 2013 at 03:50:12PM +0200, Jarno Rajahalme wrote: >> Remove recursion from GOTO_TABLE processing in do_xlate_actions(). >> This allows packet processing pipelines built with goto table be >> longer and not interact with each other via the resubmit recursion limit. >> >> Signed-off-by: Jarno Rajahalme > > Thank you, this is a nice refinement. > > This looks very close to correct to me. However, I don't think that the > handling of the 'evictable' flags is quite right. The basic rule for > 'evictable' is that we need to make sure that it is set for any rule > that we're currently "using" in some way (one example is iterating over > its actions) to tell "learn" actions not to delete that rule. As I read > your patch, it didn't do this for rules reached via "goto_table". > (Probably because the purpose of 'evictable' really isn't obvious.) > > I applied the following incremental to fix that. It seems correct, so > I'll apply it to master soon, but please check my work. > > Thanks, > > Ben. > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 6628aa6..d911080 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -6305,7 +6305,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t > ofpacts_len, > { > bool was_evictable = true; > const struct ofpact *a; > -struct rule_dpif *orig_rule = ctx->rule; > > if (ctx->rule) { > /* Don't let the rule we're working on get evicted underneath us. */ > @@ -6509,7 +6508,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t > ofpacts_len, > rule = ctx_rule_hooks(ctx, rule, true); > > if (rule) { > +if (ctx->rule) { > +ctx->rule->up.evictable = was_evictable; > +} > ctx->rule = rule; > +was_evictable = rule->up.evictable; > +rule->up.evictable = false; > > /* Tail recursion removal. */ > ofpacts = rule->up.ofpacts; > @@ -6522,7 +6526,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t > ofpacts_len, > } > > out: > -ctx->rule = orig_rule; > if (ctx->rule) { > ctx->rule->up.evictable = was_evictable; > } ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovs-appctl: dpif/show display per bridge stats
This is to fix the fallout of single datapath change. ovs-appctl dpif/show displays per bridge miss, hit and flow counts on the screen, but the backend is obtaining those information from the datapath. With a single datapath, all bridges of the same datapath would all display the same (global) counters maintained by the datapath, obviously not correct. This patch fixes the bug by maintaining per ofproto_dpif miss and hit counts, which are used for display output. The number of flows count is obtained by counting the number facets per ofproto. ovs-dpctl show still displays the counters maintain by the datapath, as before. Bug# 15369. Signed-off-by: Andy Zhou --- ofproto/ofproto-dpif.c | 39 --- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index ff93dc3..cce9736 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -661,6 +661,16 @@ static void drop_key_clear(struct dpif_backer *); static struct ofport_dpif * odp_port_to_ofport(const struct dpif_backer *, uint32_t odp_port); +/* + * Per ofproto stats maintain by ofproto_dpif. + */ +struct ofproto_dpif_stats { +uint64_t n_hit; +uint64_t n_missed; +}; +static void dpif_stats_update_hit_count(struct ofproto_dpif *ofproto, +uint64_t delta); + struct ofproto_dpif { struct hmap_node all_ofproto_dpifs_node; /* In 'all_ofproto_dpifs'. */ struct ofproto up; @@ -710,6 +720,9 @@ struct ofproto_dpif { struct sset ghost_ports; /* Ports with no datapath port. */ struct sset port_poll_set; /* Queued names for port_poll() reply. */ int port_poll_errno; /* Last errno for port_poll() reply. */ + +/* Per ofproto's dpif Stats. */ +struct ofproto_dpif_stats dpif_stats; }; /* Defer flow mod completion until "ovs-appctl ofproto/unclog"? (Useful only @@ -1287,6 +1300,8 @@ construct(struct ofproto *ofproto_) error = add_internal_flows(ofproto); ofproto->up.tables[TBL_INTERNAL].flags = OFTABLE_HIDDEN | OFTABLE_READONLY; +memset(&ofproto->dpif_stats, 0, sizeof ofproto->dpif_stats); + return error; } @@ -3816,6 +3831,8 @@ handle_miss_upcalls(struct dpif_backer *backer, struct dpif_upcall *upcalls, if (error) { continue; } + +ofproto->dpif_stats.n_missed++; flow_extract(upcall->packet, flow.skb_priority, flow.skb_mark, &flow.tunnel, flow.in_port, &miss->flow); @@ -4134,6 +4151,12 @@ update_stats(struct dpif_backer *backer) subfacet = subfacet_find(ofproto, key, key_len, key_hash); switch (subfacet ? subfacet->path : SF_NOT_INSTALLED) { case SF_FAST_PATH: +/* Update ofproto_dpif's hit count. */ +if (stats->n_packets > subfacet->dp_packet_count) { +uint64_t delta = stats->n_packets - subfacet->dp_packet_count; +dpif_stats_update_hit_count(ofproto, delta); +} + update_subfacet_stats(subfacet, stats); break; @@ -7994,19 +8017,15 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED, static void show_dp_format(const struct ofproto_dpif *ofproto, struct ds *ds) { -struct dpif_dp_stats s; +const struct ofproto_dpif_stats* s = &ofproto->dpif_stats; const struct shash_node **ports; int i; -dpif_get_dp_stats(ofproto->backer->dpif, &s); - ds_put_format(ds, "%s (%s):\n", ofproto->up.name, dpif_name(ofproto->backer->dpif)); -/* xxx It would be better to show bridge-specific stats instead - * xxx of dp ones. */ ds_put_format(ds, - "\tlookups: hit:%"PRIu64" missed:%"PRIu64" lost:%"PRIu64"\n", - s.n_hit, s.n_missed, s.n_lost); + "\tlookups: hit:%"PRIu64" missed:%"PRIu64"\n", + s->n_hit, s->n_missed); ds_put_format(ds, "\tflows: %zu\n", hmap_count(&ofproto->subfacets)); @@ -8415,6 +8434,12 @@ odp_port_to_ofp_port(const struct ofproto_dpif *ofproto, uint32_t odp_port) } } +static void +dpif_stats_update_hit_count(struct ofproto_dpif *ofproto, uint64_t delta) +{ +ofproto->dpif_stats.n_hit += delta; +} + const struct ofproto_class ofproto_dpif_class = { init, enumerate_types, -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev