RE: Question about flow table offload in mlx5e

[Paul Blakey <paulb@mellanox.com>]

Also, can u CC me on those patches? Thanks.

> -----Original Message-----
> From: Paul Blakey
> Sent: Thursday, November 21, 2019 1:40 PM
> To: wenxu <wenxu@ucloud.cn>
> Cc: pablo@netfilter.org; netdev@vger.kernel.org; Mark Bloch
> <markb@mellanox.com>
> Subject: RE: Question about flow table offload in mlx5e
> 
> They are good fixes, exactly what we had when we tested this, thanks.
> 
> Regarding encap, I don't know what changes you did, how does the encap
> rule look? Is it a FWD to vxlan device? If not it should be, as our driver
> expects that.
> 
> I tried it on my setup via tc, by changing the callback of tc
> (mlx5e_rep_setup_tc_cb) to that of ft (mlx5e_rep_setup_ft_cb),
> and testing a vxlan encap rule:
> sudo tc qdisc add dev ens1f0_0 ingress
> sudo ifconfig ens1f0 7.7.7.7/24 up
> sudo ip link add name vxlan0 type vxlan dev ens1f0 remote 7.7.7.8 dstport
> 4789 external
> sudo ifconfig vxlan0 up
> sudo tc filter add dev ens1f0_0 ingress prio 1 chain 0 protocol ip flower
> dst_mac aa:bb:cc:dd:ee:ff ip_proto udp skip_sw  action tunnel_key set
> src_ip 0.0.0.0 dst_ip 7.7.7.8 id 1234 dst_port 4789 pipe action mirred egress
> redirect dev vxlan
> 
> then tc show:
> filter protocol ip pref 1 flower chain 0 handle 0x1 dst_mac aa:bb:cc:dd:ee:ff
> ip_proto udp skip_sw in_hw in_hw_count 1
>         tunnel_key set src_ip 0.0.0.0 dst_ip 7.7.7.8 key_id 1234 dst_port 4789
> csum pipe
>         Stats: used 119 sec      0 pkt
>         mirred (Egress Redirect to device vxlan0)
>         Stats: used 119 sec      0 pkt
> 
> 
> 
> > -----Original Message-----
> > From: wenxu <wenxu@ucloud.cn>
> > Sent: Thursday, November 21, 2019 10:29 AM
> > To: Paul Blakey <paulb@mellanox.com>
> > Cc: pablo@netfilter.org; netdev@vger.kernel.org; Mark Bloch
> > <markb@mellanox.com>
> > Subject: Re: Question about flow table offload in mlx5e
> >
> >
> > On 11/21/2019 3:42 PM, Paul Blakey wrote:
> > > Hi,
> > >
> > > The original design was the block setup to use TC_SETUP_FT type, and
> the
> > tc event type to be case TC_SETUP_CLSFLOWER.
> > > We will post a patch to change that. I would advise to wait till we fix that
> > 😊
> > > I'm not sure how you get to this function mlx5e_rep_setup_ft_cb() if it
> the
> > nf_flow_table_offload ndo_setup_tc event was TC_SETUP_BLOCK, and
> not
> > TC_SETUP_FT.
> >
> >
> > Yes I change the TC_SETUP_BLOCK to TC_SETUP_FT in the
> > nf_flow_table_offload_setup.
> >
> > Two fixes patch provide:
> >
> > http://patchwork.ozlabs.org/patch/1197818/
> >
> > http://patchwork.ozlabs.org/patch/1197876/
> >
> > So this change made by me is not correct currently?
> >
> > >
> > > In our driver en_rep.c we have:
> > >> -------switch (type) {
> > >> -------case TC_SETUP_BLOCK:
> > >> ------->-------return flow_block_cb_setup_simple(type_data,
> > >> ------->------->------->------->------->-------
> &mlx5e_rep_block_tc_cb_list,
> > >> ------->------->------->------->------->-------  mlx5e_rep_setup_tc_cb,
> > >> ------->------->------->------->------->-------  priv, priv, true);
> > >> -------case TC_SETUP_FT:
> > >> ------->-------return flow_block_cb_setup_simple(type_data,
> > >> ------->------->------->------->------->-------
> &mlx5e_rep_block_ft_cb_list,
> > >> ------->------->------->------->------->-------  mlx5e_rep_setup_ft_cb,
> > >> ------->------->------->------->------->-------  priv, priv, true);
> > >> -------default:
> > >> ------->-------return -EOPNOTSUPP;
> > >> -------}
> > > In nf_flow_table_offload.c:
> > >> -------bo.binder_type>-=
> FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
> > >> -------bo.extack>------= &extack;
> > >> -------INIT_LIST_HEAD(&bo.cb_list);
> > >> -------err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK,
> > &bo);
> > >> -------if (err < 0)
> > >> ------->-------return err;
> > >> -------return nf_flow_table_block_setup(flowtable, &bo, cmd);
> > > }
> > > EXPORT_SYMBOL_GPL(nf_flow_table_offload_setup);
> > >
> > >
> > > So unless you changed that as well, you should have gotten to
> > mlx5e_rep_setup_tc_cb and not mlx5e_rep_setup_tc_ft.
> > >
> > > Regarding the encap action, there should be no difference on which chain
> > the rule is on.
> >
> >
> > But for the same encap rule can be real offloaded when setup through
> > through TC_SETUP_BLOCK. But TC_SETUP_FT can't.
> >
> > So it is the problem of TC_SETUP_FT in mlx5e_rep_setup_ft_cb ?
> >
> > >
> > >
> > >> -----Original Message-----
> > >> From: wenxu <wenxu@ucloud.cn>
> > >> Sent: Thursday, November 21, 2019 9:30 AM
> > >> To: Paul Blakey <paulb@mellanox.com>
> > >> Cc: pablo@netfilter.org; netdev@vger.kernel.org; Mark Bloch
> > >> <markb@mellanox.com>
> > >> Subject: Question about flow table offload in mlx5e
> > >>
> > >> Hi  paul,
> > >>
> > >> The flow table offload in the mlx5e is based on TC_SETUP_FT.
> > >>
> > >>
> > >> It is almost the same as TC_SETUP_BLOCK.
> > >>
> > >> It just set MLX5_TC_FLAG(FT_OFFLOAD) flags and change
> > >> cls_flower.common.chain_index = FDB_FT_CHAIN;
> > >>
> > >> In following codes line 1380 and 1392
> > >>
> > >> 1368 static int mlx5e_rep_setup_ft_cb(enum tc_setup_type type, void
> > >> *type_data,
> > >> 1369                                  void *cb_priv)
> > >> 1370 {
> > >> 1371         struct flow_cls_offload *f = type_data;
> > >> 1372         struct flow_cls_offload cls_flower;
> > >> 1373         struct mlx5e_priv *priv = cb_priv;
> > >> 1374         struct mlx5_eswitch *esw;
> > >> 1375         unsigned long flags;
> > >> 1376         int err;
> > >> 1377
> > >> 1378         flags = MLX5_TC_FLAG(INGRESS) |
> > >> 1379                 MLX5_TC_FLAG(ESW_OFFLOAD) |
> > >> 1380                 MLX5_TC_FLAG(FT_OFFLOAD);
> > >> 1381         esw = priv->mdev->priv.eswitch;
> > >> 1382
> > >> 1383         switch (type) {
> > >> 1384         case TC_SETUP_CLSFLOWER:
> > >> 1385                 if (!mlx5_eswitch_prios_supported(esw) || f-
> > >>> common.chain_index)
> > >> 1386                         return -EOPNOTSUPP;
> > >> 1387
> > >> 1388                 /* Re-use tc offload path by moving the ft flow to the
> > >> 1389                  * reserved ft chain.
> > >> 1390                  */
> > >> 1391                 memcpy(&cls_flower, f, sizeof(*f));
> > >> 1392                cls_flower.common.chain_index = FDB_FT_CHAIN;
> > >> 1393                 err = mlx5e_rep_setup_tc_cls_flower(priv, &cls_flower,
> > flags);
> > >> 1394                 memcpy(&f->stats, &cls_flower.stats, sizeof(f->stats));
> > >>
> > >>
> > >> I want to add tunnel offload support in the flow table, I  add some
> patches
> > in
> > >> nf_flow_table_offload.
> > >>
> > >> Also add the indr setup support in the mlx driver. And Now I can  flow
> > table
> > >> offload with decap.
> > >>
> > >>
> > >> But I meet a problem with the encap.  The encap rule can be added in
> > >> hardware  successfully But it can't be offloaded.
> > >>
> > >> But I think the rule I added is correct.  If I mask the line 1392. The rule
> also
> > can
> > >> be add success and can be offloaded.
> > >>
> > >> So there are some limit for encap operation for FT_OFFLOAD in
> > >> FDB_FT_CHAIN?
> > >>
> > >>
> > >> BR
> > >>
> > >> wenxu
> > >>