On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote:

On Fri, Nov 25, 2022 at 10:20 PM  Marcelo Leitner [off-list ref]

quoted

On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote:

quoted

On Fri, Nov 25, 2022 at 10:21 AM  Jamal Hadi Salim [off-list ref]

wrote:

quoted

quoted

I am not sure if the mlx5 changes will work since  they both seem to
be calling
mlx5e_tc_act_get() which expects the act->id to exist in tc_acts_xxx
tables, meaning mlx5e_tc_act_get() will always return you NULL  and
that check is hit before you check for ACT_PIPE.

Something not obvious to me:
Would all these drivers now be able to handle ACT_PIPE transparently
as if no action is specified? Cant see the obvious connection to
POLICE by just staring at the patch - is there and ACT_PIPE first then a

POLICE?

quoted

quoted

 Another question:
If the ACT_PIPE count is not being updated in s/w - is there a h/w
equivalent stat being updated?

cheers,
jamal

Thanks Jamal for your review.

About mlx5e_tc_act_get(), I'll later add PIPE action in tc_acts_nic so
that mlx5e_tc_act_get() will return the right act_id.

In driver we choose just ignore this gact with ACT_PIPE, so after
parsing the filter(rule) from kernel, the remaining actions are just like what

they used to be without changes in this patch. So the flow could be
processed as before.

quoted

The connection between POLICE and ACT_PIPE may exist in userspace
(e.g. ovs), we could put a gact (PIPE) at the beginning place in each tc filter.

We will also have an OVS patch for this propose.

quoted

I'm not very clear with your last case, but in expectation, the once
the traffic is offloaded in h/w tc datapath, the stats will be updated
by the flower stats from hardware. And when the traffic is using s/w tc

datapath, the stats are from software.

I'm still confused here. Take, for example cxgb4 driver below. It will simply
ignore this action AFAICT. This is good because it will still offload whatever
vswitchd would be offloading but then, I don't see how the stats will be right
in the end. I think the hw stats will be zeroed, no? (this is already considering
the per action stats change that Oz is working on, see [ RFC  net-next v2 0/2]
net: flow_offload:
add support for per action hw stats)

I think the drivers have to reject the action if they don't support it, and
vswitchd will have to probe for proper support when starting.

Other than this, patch seems good.

Thanks,
Marcelo

Thanks Marcelo for your reply

Likewise :)

As far as I've seen, in kernel tc (fl_hw_update_stats()), the flower stats from hw datapath is
updated as following:
1) get the flower stats from each driver directly using the registered offload api and store it
in cls_flower
2) using the dumped stats in cls_flower to update stats for each action in this filter
(tcf_exts_hw_stats_update())

So the stats of gact with PIPE will still be updated using the stats above even though this action
is ignored and actually not processed by hw.

Right, but...

Furthermore, I think the current stats for each action mentioned in 2) cannot represent the real
hw stats and this is why [ RFC  net-next v2 0/2] (net: flow_offload: add support for per action
hw stats) will come up.

Exactly. Then, when this patchset (or similar) come up, it won't
update all actions with the same stats anymore. It will require a set
of stats from hw for the gact with PIPE action here. But if drivers
are ignoring this action, they can't have specific stats for it. Or am
I missing something?

So it is better for the drivers to reject the whole flow instead of
simply ignoring it, and let vswitchd probe if it should or should not
use this action.

Cheers,
Marcelo

Cheers,
Tianyu

quoted

quoted

B.R.
Tianyu

quoted

On Tue, Nov 22, 2022 at 6:21 AM Simon Horman
[off-list ref] wrote:

quoted

From: Tianyu Yuan <redacted>

Support gact with PIPE action when setting up gact in TC.
This PIPE gact could come first in each tc filter to update the
filter(flow) stats.

The stats for each actons in a filter are updated by the flower
stats from HW(via netdev drivers) in kernel TC rather than drivers.

In each netdev driver, we don't have to process this gact, but
only to ignore it to make sure the whole rule can be offloaded.

Background:

This is a proposed solution to a problem with a miss-match between
TC police action instances - which may be shared between flows -
and OpenFlow meter actions - the action is per flow, while the
underlying meter may be shared. The key problem being that the
police action statistics are shared between flows, and this does
not match the requirement of OpenFlow for per-flow statistics.

Ref: [ovs-dev] [PATCH] tests: fix reference output for meter
offload stats

https://mail.openvswitch.org/pipermail/ovs-dev/2022-

October/398363.htm

quoted

l

Signed-off-by: Tianyu Yuan <redacted>
Signed-off-by: Simon Horman <redacted>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c                     | 5 +++++
 drivers/net/dsa/sja1105/sja1105_flower.c                   | 5 +++++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c       | 5 +++++
 drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 6

++++++

quoted

quoted

quoted

 drivers/net/ethernet/intel/ice/ice_tc_lib.c                | 5 +++++
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c       | 5 +++++
 drivers/net/ethernet/marvell/prestera/prestera_flower.c    | 5 +++++
 drivers/net/ethernet/mediatek/mtk_ppe_offload.c            | 5 +++++
 drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c        | 6 ++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c            | 5 +++++
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c      | 5

+++++

quoted

quoted

quoted

 drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c   | 4 ++++
 drivers/net/ethernet/mscc/ocelot_flower.c                  | 5 +++++
 drivers/net/ethernet/netronome/nfp/flower/action.c         | 5 +++++
 drivers/net/ethernet/qlogic/qede/qede_filter.c             | 5 +++++

                              | 5 +++++

quoted

 drivers/net/ethernet/ti/cpsw_priv.c                        | 5 +++++
 net/sched/act_gact.c                                       | 7 ++++---
 18 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c

b/drivers/net/dsa/ocelot/felix_vsc9959.c
index b0ae8d6156f6..e54eb8e28386 100644

--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c

@@ -2217,6 +2217,11 @@ static int vsc9959_psfp_filter_add(struct

ocelot

*ocelot, int port,

quoted

                        sfi.fmid = index;
                        sfi.maxsdu = a->police.mtu;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        mutex_unlock(&psfp->lock);
                        return -EOPNOTSUPP; diff --git
a/drivers/net/dsa/sja1105/sja1105_flower.c
b/drivers/net/dsa/sja1105/sja1105_flower.c
index fad5afe3819c..d3eeeeea152a 100644

--- a/drivers/net/dsa/sja1105/sja1105_flower.c
+++ b/drivers/net/dsa/sja1105/sja1105_flower.c

@@ -426,6 +426,11 @@ int sja1105_cls_flower_add(struct dsa_switch

*ds,

int port,

quoted

                        if (rc)
                                goto out;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        NL_SET_ERR_MSG_MOD(extack,
                                           "Action not
supported"); diff --git
a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index dd9be229819a..443f405c0ed4 100644

--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c

@@ -770,6 +770,11 @@ int cxgb4_validate_flow_actions(struct

net_device

*dev,

quoted

                case FLOW_ACTION_QUEUE:
                        /* Do nothing. cxgb4_set_filter will validate */
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        netdev_err(dev, "%s: Unsupported action\n", __func__);
                        return -EOPNOTSUPP; diff --git
a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c
b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c
index cacd454ac696..cfbf2f76e83a 100644

--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c

@@ -378,6 +378,11 @@ static int

dpaa2_switch_tc_parse_action_acl(struct

ethsw_core *ethsw,

quoted

        case FLOW_ACTION_DROP:
                dpsw_act->action = DPSW_ACL_ACTION_DROP;
                break;
+       /* Just ignore GACT with pipe action to let this action
+ count the

packets.

quoted

+        * The NIC doesn't have to process this action
+        */
+       case FLOW_ACTION_PIPE:
+               break;
        default:
                NL_SET_ERR_MSG_MOD(extack,
                                   "Action not supported"); @@
-651,6
+656,7 @@ int dpaa2_switch_cls_flower_replace(struct

dpaa2_switch_filter_block *block,

quoted

        case FLOW_ACTION_REDIRECT:
        case FLOW_ACTION_TRAP:
        case FLOW_ACTION_DROP:
+       case FLOW_ACTION_PIPE:
                return dpaa2_switch_cls_flower_replace_acl(block, cls);
        case FLOW_ACTION_MIRRED:
                return
dpaa2_switch_cls_flower_replace_mirror(block,
cls); diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index faba0f857cd9..5908ad4d0170 100644

--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c

@@ -642,6 +642,11 @@ ice_eswitch_tc_parse_action(struct

ice_tc_flower_fltr *fltr,

                break;

+       /* Just ignore GACT with pipe action to let this action
+ count the

packets.

quoted

+        * The NIC doesn't have to process this action
+        */
+       case FLOW_ACTION_PIPE:
+               break;
        default:
                NL_SET_ERR_MSG_MOD(fltr->extack, "Unsupported
action in

switchdev mode");

quoted

                return -EINVAL;

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c

b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
index e64318c110fd..fc05897adb70 100644

--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c

@@ -450,6 +450,11 @@ static int otx2_tc_parse_actions(struct

otx2_nic

*nic,

quoted

                case FLOW_ACTION_MARK:
                        mark = act->mark;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        return -EOPNOTSUPP;
                }
diff --git
a/drivers/net/ethernet/marvell/prestera/prestera_flower.c
b/drivers/net/ethernet/marvell/prestera/prestera_flower.c
index 91a478b75cbf..9686ed086e35 100644

--- a/drivers/net/ethernet/marvell/prestera/prestera_flower.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_flower.c

@@ -126,6 +126,11 @@ static int

prestera_flower_parse_actions(struct

prestera_flow_block *block,

quoted

                        if (err)
                                return err;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
                        pr_err("Unsupported action\n"); diff --git
a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 81afd5ee3fbf..91e4d3fcc756 100644

--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c

@@ -344,6 +344,11 @@ mtk_flow_offload_replace(struct mtk_eth

*eth,

quoted

quoted

struct flow_cls_offload *f)

quoted

                        data.pppoe.sid = act->pppoe.sid;
                        data.pppoe.num++;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        return -EOPNOTSUPP;
                }

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c

b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
index b08339d986d5..231660cb1daf 100644

--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c

@@ -544,6 +544,12 @@ mlx5e_rep_indr_replace_act(struct

mlx5e_rep_priv *rpriv,

quoted

                if (!act->offload_action)
                        continue;

+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               if (action->id == FLOW_ACTION_PIPE)
+                       continue;
+
                if (!act->offload_action(priv, fl_act, action))
                        add = true;
        }

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 3782f0097292..adac2ce9b24f 100644

--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

@@ -3853,6 +3853,11 @@ parse_tc_actions(struct

mlx5e_tc_act_parse_state *parse_state,

        flow_action_for_each(i, _act, &flow_action_reorder) {
                act = *_act;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               if (act->id == FLOW_ACTION_PIPE)
+                       continue;
                tc_act = mlx5e_tc_act_get(act->id, ns_type);
                if (!tc_act) {
                        NL_SET_ERR_MSG_MOD(extack, "Not
implemented offload action"); diff --git
a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index e91fb205e0b4..9270bf9581c7 100644

--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c

@@ -266,6 +266,11 @@ static int

mlxsw_sp_flower_parse_actions(struct

mlxsw_sp *mlxsw_sp,

quoted

                                return err;
                        break;
                        }
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
                        dev_err(mlxsw_sp->bus_info->dev,
"Unsupported action\n"); diff --git
a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
index bd6bd380ba34..e32f5b5d1e95 100644

--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c

@@ -692,6 +692,10 @@ static int sparx5_tc_flower_replace(struct

net_device *ndev,

quoted

                        break;
                case FLOW_ACTION_GOTO:
                        /* Links between VCAPs will be added later
*/
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
                        break;
                default:
                        NL_SET_ERR_MSG_MOD(fco->common.extack,

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c

b/drivers/net/ethernet/mscc/ocelot_flower.c
index 7c0897e779dc..b8e01af0fb48 100644

--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c

@@ -492,6 +492,11 @@ static int ocelot_flower_parse_action(struct

ocelot

*ocelot, int port,

quoted

                        }
                        filter->type = OCELOT_PSFP_FILTER_OFFLOAD;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        NL_SET_ERR_MSG_MOD(extack, "Cannot offload action");
                        return -EOPNOTSUPP; diff --git
a/drivers/net/ethernet/netronome/nfp/flower/action.c
b/drivers/net/ethernet/netronome/nfp/flower/action.c
index 2b383d92d7f5..57fd83b8e54a 100644

--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c

@@ -1209,6 +1209,11 @@ nfp_flower_loop_action(struct nfp_app

*app,

quoted

quoted

const struct flow_action_entry *act,

quoted

                if (err)
                        return err;
                break;
+       /* Just ignore GACT with pipe action to let this action
+ count the

packets.

quoted

+        * The NIC doesn't have to process this action
+        */
+       case FLOW_ACTION_PIPE:
+               break;
        default:
                /* Currently we do not handle any other actions. */
                NL_SET_ERR_MSG_MOD(extack, "unsupported offload:
unsupported action in action list"); diff --git
a/drivers/net/ethernet/qlogic/qede/qede_filter.c
b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..69110d5978d8 100644

--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c

@@ -1691,6 +1691,11 @@ static int qede_parse_actions(struct

qede_dev

*edev,

quoted

                                return -EINVAL;
                        }
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        return -EINVAL;
                }

diff --git a/drivers/net/ethernet/sfc/tc.c

b/drivers/net/ethernet/sfc/tc.c index deeaab9ee761..7256bbcdcc59
100644

--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c

@@ -494,6 +494,11 @@ static int efx_tc_flower_replace(struct

efx_nic

*efx,

quoted

                        }
                        *act = save;
                        break;
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled

action %u",

quoted

quoted

quoted

                                               fa->id); diff --git
a/drivers/net/ethernet/ti/cpsw_priv.c
b/drivers/net/ethernet/ti/cpsw_priv.c
index 758295c898ac..c0ac58db64d4 100644

--- a/drivers/net/ethernet/ti/cpsw_priv.c
+++ b/drivers/net/ethernet/ti/cpsw_priv.c

@@ -1492,6 +1492,11 @@ static int

cpsw_qos_configure_clsflower(struct
cpsw_priv *priv, struct flow_cls_

                        return
cpsw_qos_clsflower_add_policer(priv, extack, cls,

act->police.rate_pkt_ps);
+               /* Just ignore GACT with pipe action to let this
+ action count the

packets.

quoted

+                * The NIC doesn't have to process this action
+                */
+               case FLOW_ACTION_PIPE:
+                       break;
                default:
                        NL_SET_ERR_MSG_MOD(extack, "Action not supported");
                        return -EOPNOTSUPP; diff --git
a/net/sched/act_gact.c b/net/sched/act_gact.c index
62d682b96b88..82d1371e251e 100644

--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c

@@ -250,15 +250,14 @@ static int tcf_gact_offload_act_setup(struct

tc_action *act, void *entry_data,

quoted

                } else if (is_tcf_gact_goto_chain(act)) {
                        entry->id = FLOW_ACTION_GOTO;
                        entry->chain_index =
tcf_gact_goto_chain_index(act);
+               } else if (is_tcf_gact_pipe(act)) {
+                       entry->id = FLOW_ACTION_PIPE;
                } else if (is_tcf_gact_continue(act)) {
                        NL_SET_ERR_MSG_MOD(extack, "Offload of \"continue\"

action is not supported");

quoted

                        return -EOPNOTSUPP;
                } else if (is_tcf_gact_reclassify(act)) {
                        NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\"

action is not supported");

quoted

                        return -EOPNOTSUPP;
-               } else if (is_tcf_gact_pipe(act)) {
-                       NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\"

action is

quoted

quoted

not supported");

quoted

-                       return -EOPNOTSUPP;
                } else {
                        NL_SET_ERR_MSG_MOD(extack, "Unsupported
generic action

offload");

quoted

                        return -EOPNOTSUPP; @@ -275,6 +274,8 @@
static int tcf_gact_offload_act_setup(struct tc_action *act, void

*entry_data,

quoted

quoted

quoted

                        fl_action->id = FLOW_ACTION_TRAP;
                else if (is_tcf_gact_goto_chain(act))
                        fl_action->id = FLOW_ACTION_GOTO;
+               else if (is_tcf_gact_pipe(act))
+                       fl_action->id = FLOW_ACTION_PIPE;
                else
                        return -EOPNOTSUPP;
        }
--
2.30.2