From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and definitions Date: Thu, 25 Oct 2018 23:33:14 +0000 Message-ID: <20181025233306.GA6434@mtidpdk.mti.labs.mlnx> References: <1538461807-37507-1-git-send-email-viacheslavo@mellanox.com> <1539612815-47199-1-git-send-email-viacheslavo@mellanox.com> <1539612815-47199-2-git-send-email-viacheslavo@mellanox.com> <20181023100109.GA14792@mtidpdk.mti.labs.mlnx> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Shahaf Shuler , "dev@dpdk.org" To: Slava Ovsiienko Return-path: Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80053.outbound.protection.outlook.com [40.107.8.53]) by dpdk.org (Postfix) with ESMTP id 860615F16 for ; Fri, 26 Oct 2018 01:33:16 +0200 (CEST) In-Reply-To: Content-Language: en-US Content-ID: <6EBFEC82E314614E9FDA5C471976D1E1@eurprd05.prod.outlook.com> 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 Thu, Oct 25, 2018 at 05:50:26AM -0700, Slava Ovsiienko wrote: > > -----Original Message----- > > From: Yongseok Koh > > Sent: Tuesday, October 23, 2018 13:02 > > To: Slava Ovsiienko > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and > > definitions > >=20 > > On Mon, Oct 15, 2018 at 02:13:29PM +0000, Viacheslav Ovsiienko wrote: [...] > > > diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flo= w.h > > > index 840d645..b838ab0 100644 > > > --- a/drivers/net/mlx5/mlx5_flow.h > > > +++ b/drivers/net/mlx5/mlx5_flow.h > > > @@ -85,6 +85,8 @@ > > > #define MLX5_FLOW_ACTION_SET_TP_SRC (1u << 15) > > > #define MLX5_FLOW_ACTION_SET_TP_DST (1u << 16) > > > #define MLX5_FLOW_ACTION_JUMP (1u << 17) > > > +#define MLX5_ACTION_VXLAN_ENCAP (1u << 11) > > > +#define MLX5_ACTION_VXLAN_DECAP (1u << 12) > >=20 > > MLX5_ACTION_* has been changed to MLX5_FLOW_ACTION_* as you can > > see above. > OK. Miscopied from previous version of patch. >=20 > > And make it alphabetical order; decap first and encap later? Or, at lea= st make > > it consistent. The order (case clause) is different among validate, pre= pare and > > translate. > OK. Will reorder. >=20 > >=20 > > > #define MLX5_FLOW_FATE_ACTIONS \ > > > (MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_QUEUE | > > MLX5_FLOW_ACTION_RSS) > > > @@ -182,8 +184,17 @@ struct mlx5_flow_dv { > > > struct mlx5_flow_tcf { > > > struct nlmsghdr *nlh; > > > struct tcmsg *tcm; > > > + uint32_t nlsize; /**< Size of NL message buffer. */ > >=20 > > It is used only for assert(), but if prepare() is trusted, why do we ne= ed to > > keep it? I don't it is needed. > >=20 > Q? Let's keep the nlsize under NDEBUG flag?=20 > It's extremely useful to have assert() > on allocated size for debugging purposes. Totally agree. Please do so. > > > + uint32_t applied:1; /**< Whether rule is currently applied. */ > > > + uint64_t item_flags; /**< Item flags. */ > >=20 > > This isn't used at all. > OK, now no dependencies on it, should be removed, good. >=20 > >=20 > > > + uint64_t action_flags; /**< Action flags. */ > >=20 > > I checked following patches and it doesn't seem necessary. Please refer= to > > the > > comment on the translation func. But if you think it is really needed, = you > > could've used actions field of struct rte_flow and layers field of stru= ct > > mlx5_flow in mlx5_flow.h >=20 > When translating item list into NL-message we have to know whether there = is=20 > some tunneling action in the actions list. This is due to possible=20 > changing of the item meanings if tunneling action is present. For example= , > usually the ipv4 item provides IPv4 addresses for matching and translated= to > TCA_FLOWER_KEY_IPV4_SRC (+ xxx_DST) Netlink attribute(s), but if there is > VXLAN decap action specified, this item becames outer tunnel source IPs > and should be translated to TCA_FLOWER_KEY_ENC_IPV4_SRC. The action > list is scanned in the preperd list, so we can save action flags and us= e these > gathered results in translation routine. As we can see from mlx5_flow_lis= t_create() source, > it does not save item/actions flags, gathered by flow_drv_prepare(). That= 's why > there are item_flags/action_flags in the struct mlx5_flow_tcf. item_flag= s is not > needed, should be removed. action_flags is in use. >=20 > BTW, do we need item_flags, action_flags params in flow_drv_prepare() ? > We would avoid the item_flags field if flags were transferred from > flow_drv_prepare() to flow_drv_translate() (as local variable of > mlx5_flow_list_create(). That was a bug. I found it while I was reviewing your patches. Thanks :) Refer to my patch which is already merged. Prepare should return flags so t= hat it can be used by translate and others. http://git.dpdk.org/next/dpdk-next-net-mlx/commit/?id=3D4fa7d5e88165745523b= 9b6682c4092fb943a7d49 So, you don't need to keep this field here. >=20 > > > uint64_t hits; > > > uint64_t bytes; > > > + union { /**< Tunnel encap/decap descriptor. */ > > > + struct mlx5_flow_tcf_tunnel_hdr *tunnel; > > > + struct mlx5_flow_tcf_vxlan_decap *vxlan_decap; > > > + struct mlx5_flow_tcf_vxlan_encap *vxlan_encap; > > > + }; > >=20 > > What is the reason for keeping pointer even though the actual structure > > follows > > after mlx5_flow_tcf? Maybe you don't want to waste memory, as the size = of > > encap/decap struct differs a lot? >=20 > Sizes differ, but not a lot (especially comparing with DV rule size).=20 > Would you prefer to simplify and just include the union? > On other hand we could declare something like that: > ... > uint8_t tunnel_type; > alignas(struct mlx5_flow_tcf_tunnel_hdr) uint8_t buf[]; >=20 > and eliminate the pointer at all. The buf beginning contains either tunne= l structure > or Netlink message (if no tunnel), depending on the tunnel_type field. I was just curious. Either way looks good to me. Will defer it to you. Thanks, Yongseok