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

[Ori Kam <orika@mellanox.com>]

> -----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,
> Yongseok

Thanks,
Ori