Re: [ovs-dev] [PATCH v2] xlate: Use dp_hash for select groups.

2016-09-16 Thread Jarno Rajahalme
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.

2016-09-15 Thread Ben Pfaff
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.

2016-09-15 Thread Jarno Rajahalme
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.

2016-09-14 Thread Ben Pfaff
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.

2016-09-14 Thread Ben Pfaff
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