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

[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 <blp at ovn.org> 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 <blp at ovn.org> 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 <jarno at ovn.org>
>>>>> ---
>>>>> 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 <blp at ovn.org>
>>> 
>>> 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;
>>