Re: [dpdk-dev] [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and definitions

[Yongseok Koh <yskoh@mellanox.com>]

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 <viacheslavo@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> > Subject: Re: [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and
> > definitions
> > 
> > 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_flow.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)
> > 
> > MLX5_ACTION_* has been changed to MLX5_FLOW_ACTION_* as you can
> > see above.
> OK. Miscopied from previous version of patch.
> 
> > And make it alphabetical order; decap first and encap later? Or, at least make
> > it consistent. The order (case clause) is different among validate, prepare and
> > translate.
> OK. Will reorder.
> 
> > 
> > >  #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. */
> > 
> > It is used only for assert(), but if prepare() is trusted, why do we need to
> > keep it? I don't it is needed.
> > 
> Q? Let's keep the nlsize under NDEBUG flag? 
> 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. */
> > 
> > This isn't used at all.
> OK, now no dependencies on it, should be removed, good.
> 
> > 
> > > +	uint64_t action_flags; /**< Action flags. */
> > 
> > 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 struct
> > mlx5_flow in mlx5_flow.h
> 
> When translating item list into NL-message we have to know whether there is 
> some tunneling action in the actions list. This is due to possible 
> 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  use these
> gathered results in translation routine. As we can see from mlx5_flow_list_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_flags is not
> needed, should be removed. action_flags is in use.
> 
> 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 that
it can be used by translate and others.

http://git.dpdk.org/next/dpdk-next-net-mlx/commit/?id=4fa7d5e88165745523b9b6682c4092fb943a7d49

So, you don't need to keep this field here.

> 
> > >  	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;
> > > +	};
> > 
> > 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?
> 
> Sizes differ, but not a lot (especially comparing with DV rule size). 
> 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[];
> 
> and eliminate the pointer at all. The buf beginning contains either tunnel 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