Re: [dpdk-dev] [PATCH 2/3] net/mlx5: fix Direct Verbs flow tunnel

[Yongseok Koh <yskoh@mellanox.com>]

> On Nov 4, 2018, at 10:08 PM, Ori Kam <orika@mellanox.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Yongseok Koh
>> Sent: Monday, November 5, 2018 7:38 AM
>> To: Ori Kam <orika@mellanox.com>
>> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
>> Subject: Re: [PATCH 2/3] net/mlx5: fix Direct Verbs flow tunnel
>> 
>> On Sun, Nov 04, 2018 at 01:22:34AM -0700, Ori Kam wrote:
>>> 
>>>> -----Original Message-----
>>>> From: Yongseok Koh
>>>> Sent: Friday, November 2, 2018 11:08 PM
>>>> To: Shahaf Shuler <shahafs@mellanox.com>
>>>> Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Ori Kam
>>>> <orika@mellanox.com>
>>>> Subject: [PATCH 2/3] net/mlx5: fix Direct Verbs flow tunnel
>>>> 
>>>> 1) Fix layer parsing
>>>> In translation of tunneled flows, dev_flow->layers must not be used to
>>>> check tunneled layer as it contains all the layers parsed from
>>>> flow_drv_prepare(). Checking tunneled layer is needed to distinguish
>>>> between outer and inner item. This should be based on dynamic parsing.
>> With
>>>> dev_flow->layers on a tunneled flow, items will always be interpreted as
>>>> inner as dev_flow->layer already has all the items. Dynamic parsing
>>>> (item_flags) is added as there's no such code.
>>>> 
>>>> 2) Refactoring code
>>>> - flow_dv_create_item() and flow_dv_create_action() are merged into
>>>>  flow_dv_translate() for consistency with Verbs and *_validate().
>>> 
>>> I don't like the idea of combining 2 distinct functions into one.
>>> I think a function should be as short as possible and do only one thing,
>>> if there is no good reason why two functions should be combined they should
>> not
>>> be combined.
>>> If you want to align both the Direct Verbs and  Verbs I think we can split the
>> Verbs
>>> code.
>> 
>> Look at the other lengthy switch-case clauses in validate/prepare/translate in
>> each driver. This DV translate is the only exception. I'd rather like to ask
>> why. I didn't like the lengthy function from the beginning but you wanted to
>> keep it. Of course, I considered to split the Verbs one but that's the reason
>> why I chose to merge DV code. If we feel this lengthy func is really complex and
>> gets error prone, then we can refactor all the code at once later. Or, I still
>> prefer the graph approach. That would be simpler.
>> 
> 
> I agree with you that all functions should have been split( No excuse also kept the basic
> structure as it was), specific in this one I had extra reason to make the split since
> creation of items also need adding matcher so it was different.
> In any case I agree that we should have consistency with other functions so ether we change
> them all or just this one. I think do to time let's do what you suggested and change only this one
> or just leave it as is. 
> Regarding the graph approach I think we should wait to see if the current approach is good
> enough and in next releases maybe switch to the graph approach.

Thanks for understanding. Will push v2 once the unit test is done.

Yongseok,