From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH 2/3] net/mlx5: fix Direct Verbs flow tunnel Date: Mon, 5 Nov 2018 05:37:42 +0000 Message-ID: <20181105053732.GE15737@mtidpdk.mti.labs.mlnx> References: <20181102210801.28370-1-yskoh@mellanox.com> <20181102210801.28370-2-yskoh@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Shahaf Shuler , "dev@dpdk.org" To: Ori Kam Return-path: Received: from EUR03-DB5-obe.outbound.protection.outlook.com (mail-eopbgr40046.outbound.protection.outlook.com [40.107.4.46]) by dpdk.org (Postfix) with ESMTP id D62072C52 for ; Mon, 5 Nov 2018 06:37:43 +0100 (CET) In-Reply-To: Content-Language: en-US Content-ID: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Sun, Nov 04, 2018 at 01:22:34AM -0700, Ori Kam wrote: >=20 > > -----Original Message----- > > From: Yongseok Koh > > Sent: Friday, November 2, 2018 11:08 PM > > To: Shahaf Shuler > > Cc: dev@dpdk.org; Yongseok Koh ; Ori Kam > > > > Subject: [PATCH 2/3] net/mlx5: fix Direct Verbs flow tunnel > >=20 > > 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 a= s > > inner as dev_flow->layer already has all the items. Dynamic parsing > > (item_flags) is added as there's no such code. > >=20 > > 2) Refactoring code > > - flow_dv_create_item() and flow_dv_create_action() are merged into > > flow_dv_translate() for consistency with Verbs and *_validate(). >=20 > 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 shou= ld not > be combined. > If you want to align both the Direct Verbs and Verbs I think we can spli= t 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 as= k why. I didn't like the lengthy function from the beginning but you wanted t= o keep it. Of course, I considered to split the Verbs one but that's the reas= on why I chose to merge DV code. If we feel this lengthy func is really comple= x and gets error prone, then we can refactor all the code at once later. Or, I st= ill prefer the graph approach. That would be simpler. Thanks, Yongseok