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 <viachesl...@mellanox.com>
> > Cc: Shahaf Shuler <shah...@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
