netdev - Re: Question about flow table offload in mlx5e

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 21 Nov 2019 16:28:56 +0800
From:   wenxu <wenxu@...oud.cn>
To:     Paul Blakey <paulb@...lanox.com>
Cc:     "pablo@...filter.org" <pablo@...filter.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Mark Bloch <markb@...lanox.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@...oud.cn>
>> Sent: Thursday, November 21, 2019 9:30 AM
>> To: Paul Blakey <paulb@...lanox.com>
>> Cc: pablo@...filter.org; netdev@...r.kernel.org; Mark Bloch
>> <markb@...lanox.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
>>