Re: [ovs-dev] [PATCH v2] xlate: Use dp_hash for select groups.
No, thank you for remembering to prompt for the documentation, I would not have noticed the bug otherwise! v3 pushed to master, Jarno > On Sep 15, 2016, at 7:26 PM, Ben Pfaff wrote: > > OK. Thanks for being so careful! > > I'll look at v3. > > On Thu, Sep 15, 2016 at 06:43:39PM -0700, Jarno Rajahalme wrote: >> Thanks for the fix! >> >> While I was working with tightening the parsing, I found that I had earlier >> introduced a bug that crashes ovs-ofctl when a parsing error is found after >> parsing ‘fields’, essentially a double free. I sent a v3 fixing this, >> documenting the parsing tightening and then rebasing this patch with this >> fix, additional documentation and with the NEWS piece moved to the post-2.6b >> section. >> >> Jarno >> >>> On Sep 14, 2016, at 8:53 PM, Ben Pfaff wrote: >>> >>> On Wed, Sep 14, 2016 at 08:51:42PM -0700, Ben Pfaff wrote: On Mon, Sep 12, 2016 at 04:16:08PM -0700, Jarno Rajahalme wrote: > Add a new select group selection method "dp_hash", which uses minimal > number of bits from the datapath calculated packet hash to inform the > select group bucket selection. This makes the datapath flows more > generic resulting in less upcalls to userspace, but adds recirculation > prior to group selection. > > Signed-off-by: Jarno Rajahalme > --- > v2: Rebase and documentation. Thanks for adding the documentation! It describes the syntax, which is useful. To make it even more helpful, I would suggest adding some advice to the user to explain when one might best choose one or the other. I think that the dp_hash method ignores fields specified by the user if any are given, so the documentation might mention that for dp_hash the "fields" option should be omitted. Thanks! Acked-by: Ben Pfaff >>> >>> Oh, I forgot to say that I get a compiler warning: >>> >>> ../ofproto/ofproto-dpif-xlate.c: In function 'xlate_dp_hash_select_group': >>> ../ofproto/ofproto-dpif-xlate.c:3410:32: error: variable 'buckets' set >>> but not used [-Werror=unused-but-set-variable] >>>const struct ovs_list *buckets; >>> >>> Fixed by: >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index c83132c..a74daa7 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -3407,10 +3407,8 @@ xlate_dp_hash_select_group(struct xlate_ctx *ctx, >>> struct group_dpif *group) >>> >>>ctx_trigger_recirculate_with_hash(ctx, param >> 32, (uint32_t)param); >>>} else { >>> -const struct ovs_list *buckets; >>>uint32_t n_buckets; >>> - >>> -buckets = group_dpif_get_buckets(group, &n_buckets); >>> +group_dpif_get_buckets(group, &n_buckets); >>>if (n_buckets) { >>>/* Minimal mask to cover the number of buckets. */ >>>uint32_t mask = (1 << log_2_ceil(n_buckets)) - 1; >> ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] xlate: Use dp_hash for select groups.
OK. Thanks for being so careful! I'll look at v3. On Thu, Sep 15, 2016 at 06:43:39PM -0700, Jarno Rajahalme wrote: > Thanks for the fix! > > While I was working with tightening the parsing, I found that I had earlier > introduced a bug that crashes ovs-ofctl when a parsing error is found after > parsing ‘fields’, essentially a double free. I sent a v3 fixing this, > documenting the parsing tightening and then rebasing this patch with this > fix, additional documentation and with the NEWS piece moved to the post-2.6b > section. > > Jarno > > > On Sep 14, 2016, at 8:53 PM, Ben Pfaff wrote: > > > > On Wed, Sep 14, 2016 at 08:51:42PM -0700, Ben Pfaff wrote: > >> On Mon, Sep 12, 2016 at 04:16:08PM -0700, Jarno Rajahalme wrote: > >>> Add a new select group selection method "dp_hash", which uses minimal > >>> number of bits from the datapath calculated packet hash to inform the > >>> select group bucket selection. This makes the datapath flows more > >>> generic resulting in less upcalls to userspace, but adds recirculation > >>> prior to group selection. > >>> > >>> Signed-off-by: Jarno Rajahalme > >>> --- > >>> v2: Rebase and documentation. > >> > >> Thanks for adding the documentation! It describes the syntax, which is > >> useful. To make it even more helpful, I would suggest adding some > >> advice to the user to explain when one might best choose one or the > >> other. > >> > >> I think that the dp_hash method ignores fields specified by the user if > >> any are given, so the documentation might mention that for dp_hash the > >> "fields" option should be omitted. > >> > >> Thanks! > >> > >> Acked-by: Ben Pfaff > > > > Oh, I forgot to say that I get a compiler warning: > > > >../ofproto/ofproto-dpif-xlate.c: In function > > 'xlate_dp_hash_select_group': > >../ofproto/ofproto-dpif-xlate.c:3410:32: error: variable 'buckets' set > > but not used [-Werror=unused-but-set-variable] > > const struct ovs_list *buckets; > > > > Fixed by: > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index c83132c..a74daa7 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -3407,10 +3407,8 @@ xlate_dp_hash_select_group(struct xlate_ctx *ctx, > > struct group_dpif *group) > > > > ctx_trigger_recirculate_with_hash(ctx, param >> 32, > > (uint32_t)param); > > } else { > > -const struct ovs_list *buckets; > > uint32_t n_buckets; > > - > > -buckets = group_dpif_get_buckets(group, &n_buckets); > > +group_dpif_get_buckets(group, &n_buckets); > > if (n_buckets) { > > /* Minimal mask to cover the number of buckets. */ > > uint32_t mask = (1 << log_2_ceil(n_buckets)) - 1; > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] xlate: Use dp_hash for select groups.
Thanks for the fix! While I was working with tightening the parsing, I found that I had earlier introduced a bug that crashes ovs-ofctl when a parsing error is found after parsing ‘fields’, essentially a double free. I sent a v3 fixing this, documenting the parsing tightening and then rebasing this patch with this fix, additional documentation and with the NEWS piece moved to the post-2.6b section. Jarno > On Sep 14, 2016, at 8:53 PM, Ben Pfaff wrote: > > On Wed, Sep 14, 2016 at 08:51:42PM -0700, Ben Pfaff wrote: >> On Mon, Sep 12, 2016 at 04:16:08PM -0700, Jarno Rajahalme wrote: >>> Add a new select group selection method "dp_hash", which uses minimal >>> number of bits from the datapath calculated packet hash to inform the >>> select group bucket selection. This makes the datapath flows more >>> generic resulting in less upcalls to userspace, but adds recirculation >>> prior to group selection. >>> >>> Signed-off-by: Jarno Rajahalme >>> --- >>> v2: Rebase and documentation. >> >> Thanks for adding the documentation! It describes the syntax, which is >> useful. To make it even more helpful, I would suggest adding some >> advice to the user to explain when one might best choose one or the >> other. >> >> I think that the dp_hash method ignores fields specified by the user if >> any are given, so the documentation might mention that for dp_hash the >> "fields" option should be omitted. >> >> Thanks! >> >> Acked-by: Ben Pfaff > > Oh, I forgot to say that I get a compiler warning: > >../ofproto/ofproto-dpif-xlate.c: In function 'xlate_dp_hash_select_group': >../ofproto/ofproto-dpif-xlate.c:3410:32: error: variable 'buckets' set but > not used [-Werror=unused-but-set-variable] > const struct ovs_list *buckets; > > Fixed by: > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index c83132c..a74daa7 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3407,10 +3407,8 @@ xlate_dp_hash_select_group(struct xlate_ctx *ctx, > struct group_dpif *group) > > ctx_trigger_recirculate_with_hash(ctx, param >> 32, (uint32_t)param); > } else { > -const struct ovs_list *buckets; > uint32_t n_buckets; > - > -buckets = group_dpif_get_buckets(group, &n_buckets); > +group_dpif_get_buckets(group, &n_buckets); > if (n_buckets) { > /* Minimal mask to cover the number of buckets. */ > uint32_t mask = (1 << log_2_ceil(n_buckets)) - 1; ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] xlate: Use dp_hash for select groups.
On Wed, Sep 14, 2016 at 08:51:42PM -0700, Ben Pfaff wrote: > On Mon, Sep 12, 2016 at 04:16:08PM -0700, Jarno Rajahalme wrote: > > Add a new select group selection method "dp_hash", which uses minimal > > number of bits from the datapath calculated packet hash to inform the > > select group bucket selection. This makes the datapath flows more > > generic resulting in less upcalls to userspace, but adds recirculation > > prior to group selection. > > > > Signed-off-by: Jarno Rajahalme > > --- > > v2: Rebase and documentation. > > Thanks for adding the documentation! It describes the syntax, which is > useful. To make it even more helpful, I would suggest adding some > advice to the user to explain when one might best choose one or the > other. > > I think that the dp_hash method ignores fields specified by the user if > any are given, so the documentation might mention that for dp_hash the > "fields" option should be omitted. > > Thanks! > > Acked-by: Ben Pfaff Oh, I forgot to say that I get a compiler warning: ../ofproto/ofproto-dpif-xlate.c: In function 'xlate_dp_hash_select_group': ../ofproto/ofproto-dpif-xlate.c:3410:32: error: variable 'buckets' set but not used [-Werror=unused-but-set-variable] const struct ovs_list *buckets; Fixed by: diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index c83132c..a74daa7 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3407,10 +3407,8 @@ xlate_dp_hash_select_group(struct xlate_ctx *ctx, struct group_dpif *group) ctx_trigger_recirculate_with_hash(ctx, param >> 32, (uint32_t)param); } else { -const struct ovs_list *buckets; uint32_t n_buckets; - -buckets = group_dpif_get_buckets(group, &n_buckets); +group_dpif_get_buckets(group, &n_buckets); if (n_buckets) { /* Minimal mask to cover the number of buckets. */ uint32_t mask = (1 << log_2_ceil(n_buckets)) - 1; ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] xlate: Use dp_hash for select groups.
On Mon, Sep 12, 2016 at 04:16:08PM -0700, Jarno Rajahalme wrote: > Add a new select group selection method "dp_hash", which uses minimal > number of bits from the datapath calculated packet hash to inform the > select group bucket selection. This makes the datapath flows more > generic resulting in less upcalls to userspace, but adds recirculation > prior to group selection. > > Signed-off-by: Jarno Rajahalme > --- > v2: Rebase and documentation. Thanks for adding the documentation! It describes the syntax, which is useful. To make it even more helpful, I would suggest adding some advice to the user to explain when one might best choose one or the other. I think that the dp_hash method ignores fields specified by the user if any are given, so the documentation might mention that for dp_hash the "fields" option should be omitted. Thanks! Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev