> On Nov 4, 2018, at 10:08 PM, Ori Kam <or...@mellanox.com> wrote: > > > >> -----Original Message----- >> From: Yongseok Koh >> Sent: Monday, November 5, 2018 7:38 AM >> To: Ori Kam <or...@mellanox.com> >> Cc: Shahaf Shuler <shah...@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 <shah...@mellanox.com> >>>> Cc: dev@dpdk.org; Yongseok Koh <ys...@mellanox.com>; Ori Kam >>>> <or...@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,
