| Message ID | 20221122112020.922691-1-simon.horman@corigine.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | [ovs-dev,PATCH/RFC,net-next] tc: allow drivers to accept gact with PIPE when offloading | expand |
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? 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 On Tue, Nov 22, 2022 at 6:21 AM Simon Horman <simon.horman@corigine.com> wrote: > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > 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.html > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > 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 ++++++ > 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 +++++ > 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 +++++ > 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, > sfi.fmid = index; > sfi.maxsdu = a->police.mtu; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > if (rc) > goto out; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > 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. > + * 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, > 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. > + * 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, > 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. > + * 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"); > 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, > case FLOW_ACTION_MARK: > mark = act->mark; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > if (err) > return err; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, struct flow_cls_offload *f) > data.pppoe.sid = act->pppoe.sid; > data.pppoe.num++; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > if (!act->offload_action) > continue; > > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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. > + * 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, > return err; > break; > } > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > 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. > + * 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, > } > filter->type = OCELOT_PSFP_FILTER_OFFLOAD; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, const struct flow_action_entry *act, > if (err) > return err; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > return -EINVAL; > } > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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, > } > *act = save; > break; > + /* Just ignore GACT with pipe action to let this action count the packets. > + * 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", > 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. > + * 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, > } 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"); > return -EOPNOTSUPP; > } else if (is_tcf_gact_reclassify(act)) { > NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" action is not supported"); > return -EOPNOTSUPP; > - } else if (is_tcf_gact_pipe(act)) { > - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is not supported"); > - return -EOPNOTSUPP; > } else { > NL_SET_ERR_MSG_MOD(extack, "Unsupported generic action offload"); > return -EOPNOTSUPP; > @@ -275,6 +274,8 @@ static int tcf_gact_offload_act_setup(struct tc_action *act, void *entry_data, > 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 >
On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > 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? > 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. 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. 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. B.R. Tianyu > > On Tue, Nov 22, 2022 at 6:21 AM Simon Horman > <simon.horman@corigine.com> wrote: > > > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > 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 > > l > > > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > --- > > 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 ++++++ > > 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 +++++ > > 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 +++++ > > 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, > > sfi.fmid = index; > > sfi.maxsdu = a->police.mtu; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > if (rc) > > goto out; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > 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. > > + * 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, > > 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. > > + * 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, > > 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. > > + * 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"); > > 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, > > case FLOW_ACTION_MARK: > > mark = act->mark; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > if (err) > > return err; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > struct flow_cls_offload *f) > > data.pppoe.sid = act->pppoe.sid; > > data.pppoe.num++; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > if (!act->offload_action) > > continue; > > > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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. > > + * 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, > > return err; > > break; > > } > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > 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. > > + * 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, > > } > > filter->type = OCELOT_PSFP_FILTER_OFFLOAD; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > const struct flow_action_entry *act, > > if (err) > > return err; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > return -EINVAL; > > } > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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, > > } > > *act = save; > > break; > > + /* Just ignore GACT with pipe action to let this action count the > packets. > > + * 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", > > 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. > > + * 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, > > } 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"); > > return -EOPNOTSUPP; > > } else if (is_tcf_gact_reclassify(act)) { > > NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" > action is not supported"); > > return -EOPNOTSUPP; > > - } else if (is_tcf_gact_pipe(act)) { > > - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is > not supported"); > > - return -EOPNOTSUPP; > > } else { > > NL_SET_ERR_MSG_MOD(extack, "Unsupported generic action > offload"); > > return -EOPNOTSUPP; @@ -275,6 +274,8 @@ static > > int tcf_gact_offload_act_setup(struct tc_action *act, void *entry_data, > > 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 > >
On Fri 25 Nov 2022 at 03:10, Tianyu Yuan <tianyu.yuan@corigine.com> wrote: > On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > >> >> 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? >> 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. Sorry for the late response. Jamal is correct and ACT_PIPE should indeed be properly handled in mlx5 by extending action array with correct type. You also need to extend tc_acts_fdb besides tc_acts_nic since "fdb" is responsible for actions in switchdev mode. I'll followup with a PoC patch that works on our hardware. > > 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. > > 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. > > 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. > > B.R. > Tianyu > >> >> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman >> <simon.horman@corigine.com> wrote: >> > >> > From: Tianyu Yuan <tianyu.yuan@corigine.com> >> > >> > 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 >> > l >> > >> > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> >> > Signed-off-by: Simon Horman <simon.horman@corigine.com> >> > --- >> > 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 ++++++ >> > 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 +++++ >> > 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 +++++ >> > 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, >> > sfi.fmid = index; >> > sfi.maxsdu = a->police.mtu; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > if (rc) >> > goto out; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > 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. >> > + * 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, >> > 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. >> > + * 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, >> > 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. >> > + * 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"); >> > 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, >> > case FLOW_ACTION_MARK: >> > mark = act->mark; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > if (err) >> > return err; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> struct flow_cls_offload *f) >> > data.pppoe.sid = act->pppoe.sid; >> > data.pppoe.num++; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > if (!act->offload_action) >> > continue; >> > >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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. >> > + * 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, >> > return err; >> > break; >> > } >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > 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. >> > + * 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, >> > } >> > filter->type = OCELOT_PSFP_FILTER_OFFLOAD; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> const struct flow_action_entry *act, >> > if (err) >> > return err; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > return -EINVAL; >> > } >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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, >> > } >> > *act = save; >> > break; >> > + /* Just ignore GACT with pipe action to let this action count the >> packets. >> > + * 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", >> > 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. >> > + * 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, >> > } 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"); >> > return -EOPNOTSUPP; >> > } else if (is_tcf_gact_reclassify(act)) { >> > NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" >> action is not supported"); >> > return -EOPNOTSUPP; >> > - } else if (is_tcf_gact_pipe(act)) { >> > - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is >> not supported"); >> > - return -EOPNOTSUPP; >> > } else { >> > NL_SET_ERR_MSG_MOD(extack, "Unsupported generic action >> offload"); >> > return -EOPNOTSUPP; @@ -275,6 +274,8 @@ static >> > int tcf_gact_offload_act_setup(struct tc_action *act, void *entry_data, >> > 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 >> >
On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: > On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > > 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? > > 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. > > 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. > > 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 > > B.R. > Tianyu > > > > > On Tue, Nov 22, 2022 at 6:21 AM Simon Horman > > <simon.horman@corigine.com> wrote: > > > > > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > > > 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 > > > l > > > > > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > > --- > > > 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 ++++++ > > > 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 +++++ > > > 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 +++++ > > > 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, > > > sfi.fmid = index; > > > sfi.maxsdu = a->police.mtu; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > if (rc) > > > goto out; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > 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. > > > + * 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, > > > 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. > > > + * 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, > > > 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. > > > + * 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"); > > > 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, > > > case FLOW_ACTION_MARK: > > > mark = act->mark; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > if (err) > > > return err; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > struct flow_cls_offload *f) > > > data.pppoe.sid = act->pppoe.sid; > > > data.pppoe.num++; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > if (!act->offload_action) > > > continue; > > > > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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. > > > + * 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, > > > return err; > > > break; > > > } > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > 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. > > > + * 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, > > > } > > > filter->type = OCELOT_PSFP_FILTER_OFFLOAD; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > const struct flow_action_entry *act, > > > if (err) > > > return err; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > return -EINVAL; > > > } > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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, > > > } > > > *act = save; > > > break; > > > + /* Just ignore GACT with pipe action to let this action count the > > packets. > > > + * 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", > > > 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. > > > + * 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, > > > } 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"); > > > return -EOPNOTSUPP; > > > } else if (is_tcf_gact_reclassify(act)) { > > > NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" > > action is not supported"); > > > return -EOPNOTSUPP; > > > - } else if (is_tcf_gact_pipe(act)) { > > > - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is > > not supported"); > > > - return -EOPNOTSUPP; > > > } else { > > > NL_SET_ERR_MSG_MOD(extack, "Unsupported generic action > > offload"); > > > return -EOPNOTSUPP; @@ -275,6 +274,8 @@ static > > > int tcf_gact_offload_act_setup(struct tc_action *act, void *entry_data, > > > 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 > > >
On 25 Nov 2022, at 15:19, Marcelo Leitner wrote: > On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: >> On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> >>> >>> 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? >>> 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. >> >> 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. >> >> 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. I guess OVS userspace needs a simple way to determine which approach to use, i.e. if the kernel has this patch series applied. Or else it would not be easy to migrate userspace to use this approach. > Other than this, patch seems good. > > Thanks, > Marcelo > >> >> B.R. >> Tianyu >> >>> >>> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman >>> <simon.horman@corigine.com> wrote: >>>> >>>> From: Tianyu Yuan <tianyu.yuan@corigine.com> >>>> >>>> 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 >>>> l >>>> >>>> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> >>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>> --- >>>> 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 ++++++ >>>> 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 +++++ >>>> 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 +++++ >>>> 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, >>>> sfi.fmid = index; >>>> sfi.maxsdu = a->police.mtu; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> if (rc) >>>> goto out; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> 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. >>>> + * 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, >>>> 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. >>>> + * 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, >>>> 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. >>>> + * 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"); >>>> 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, >>>> case FLOW_ACTION_MARK: >>>> mark = act->mark; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> if (err) >>>> return err; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>> struct flow_cls_offload *f) >>>> data.pppoe.sid = act->pppoe.sid; >>>> data.pppoe.num++; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> if (!act->offload_action) >>>> continue; >>>> >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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. >>>> + * 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, >>>> return err; >>>> break; >>>> } >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> 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. >>>> + * 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, >>>> } >>>> filter->type = OCELOT_PSFP_FILTER_OFFLOAD; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>> const struct flow_action_entry *act, >>>> if (err) >>>> return err; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> return -EINVAL; >>>> } >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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, >>>> } >>>> *act = save; >>>> break; >>>> + /* Just ignore GACT with pipe action to let this action count the >>> packets. >>>> + * 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", >>>> 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. >>>> + * 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, >>>> } 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"); >>>> return -EOPNOTSUPP; >>>> } else if (is_tcf_gact_reclassify(act)) { >>>> NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" >>> action is not supported"); >>>> return -EOPNOTSUPP; >>>> - } else if (is_tcf_gact_pipe(act)) { >>>> - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is >>> not supported"); >>>> - return -EOPNOTSUPP; >>>> } else { >>>> NL_SET_ERR_MSG_MOD(extack, "Unsupported generic action >>> offload"); >>>> return -EOPNOTSUPP; @@ -275,6 +274,8 @@ static >>>> int tcf_gact_offload_act_setup(struct tc_action *act, void *entry_data, >>>> 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 >>>>
Hi Tianyu, On Thu, Nov 24, 2022 at 10:10 PM Tianyu Yuan <tianyu.yuan@corigine.com> wrote: > On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> > wrote: > > > > > 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. > [..] > > 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. > Just noticed Vlad posted a patch for it ;-> I only looked at that change because i have that hardware and wanted to try it out. You are Ccing all the driver stakeholders hopefully they can double check. > 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. > > So in the simple case it is as if no action was specified? > 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. > > makes sense. > 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. > My initial thought was you want to cover two scenarios: 1) pipe followed by police. 2) pipe alone In both cases pipe serves as a placeholder for stats counters. That these counters come from hardware and will occasionally be updated to tc by the driver. i.e if i get/dump the filter or pipe action stats i can see the hw count. Am i correct? If the answer is yes, then would i see the hw stats and not the sw stat updates? Maybe if you have a filter dump in your test environment that you can show it will help satisfy my curiosity. cheers, jamal
On Fri, Nov 25, 2022 at 10:20 PM Marcelo Leitner <mleitner@redhat.com> > On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: > > On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> > wrote: > > > > > > > > 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? > > > 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. > > > > 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. > > > > 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 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. 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. Cheers, Tianyu > > > > B.R. > > Tianyu > > > > > > > > On Tue, Nov 22, 2022 at 6:21 AM Simon Horman > > > <simon.horman@corigine.com> wrote: > > > > > > > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > > > > > 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 > > > > l > > > > > > > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > > > --- > > > > 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 > ++++++ > > > > 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 > +++++ > > > > 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 +++++ > > > > 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, > > > > sfi.fmid = index; > > > > sfi.maxsdu = a->police.mtu; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > > if (rc) > > > > goto out; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > > 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. > > > > + * 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, > > > > 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. > > > > + * 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, > > > > 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. > > > > + * 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"); > > > > 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, > > > > case FLOW_ACTION_MARK: > > > > mark = act->mark; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > > if (err) > > > > return err; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > struct flow_cls_offload *f) > > > > data.pppoe.sid = act->pppoe.sid; > > > > data.pppoe.num++; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > > if (!act->offload_action) > > > > continue; > > > > > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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. > > > > + * 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, > > > > return err; > > > > break; > > > > } > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > > 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. > > > > + * 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, > > > > } > > > > filter->type = OCELOT_PSFP_FILTER_OFFLOAD; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > const struct flow_action_entry *act, > > > > if (err) > > > > return err; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this action > > > > + count the > > > packets. > > > > + * 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, > > > > return -EINVAL; > > > > } > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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, > > > > } > > > > *act = save; > > > > break; > > > > + /* Just ignore GACT with pipe action to let this > > > > + action count the > > > packets. > > > > + * 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", > > > > 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. > > > > + * 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, > > > > } 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"); > > > > return -EOPNOTSUPP; > > > > } else if (is_tcf_gact_reclassify(act)) { > > > > NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" > > > action is not supported"); > > > > return -EOPNOTSUPP; > > > > - } else if (is_tcf_gact_pipe(act)) { > > > > - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" > action is > > > not supported"); > > > > - return -EOPNOTSUPP; > > > > } else { > > > > NL_SET_ERR_MSG_MOD(extack, "Unsupported > > > > generic action > > > offload"); > > > > return -EOPNOTSUPP; @@ -275,6 +274,8 @@ > > > > static int tcf_gact_offload_act_setup(struct tc_action *act, void > *entry_data, > > > > 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 > > > >
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Friday, November 25, 2022 10:33 PM > To: Marcelo Leitner <mleitner@redhat.com> > Cc: Tianyu Yuan <tianyu.yuan@corigine.com>; Jamal Hadi Salim > <jhs@mojatatu.com>; Simon Horman <simon.horman@corigine.com>; > netdev@vger.kernel.org; Cong Wang <xiyou.wangcong@gmail.com>; Davide > Caratti <dcaratti@redhat.com>; Edward Cree <edward.cree@amd.com>; Ilya > Maximets <i.maximets@ovn.org>; Oz Shlomo <ozsh@nvidia.com>; Paul > Blakey <paulb@nvidia.com>; Vlad Buslov <vladbu@nvidia.com>; > dev@openvswitch.org; oss-drivers <oss-drivers@corigine.com>; Ziyang Chen > <ziyang.chen@corigine.com> > Subject: Re: [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE > when offloading > > > > On 25 Nov 2022, at 15:19, Marcelo Leitner wrote: > > > On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: > >> On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> > wrote: > >> > >>> > >>> 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? > >>> 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. > >> > >> 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. > >> > >> 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. > > I guess OVS userspace needs a simple way to determine which approach to > use, i.e. if the kernel has this patch series applied. Or else it would not be > easy to migrate userspace to use this approach. Yes, we could modify OVS' acinclude.m4 to check if the current has applied such a patch series. If so, it will use this approach, otherwise, put actions in tc flower as before. So that we could ensure the traffic will offloaded correctly. > > > Other than this, patch seems good. > > > > Thanks, > > Marcelo > > > >> > >> B.R. > >> Tianyu > >> > >>> > >>> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman > >>> <simon.horman@corigine.com> wrote: > >>>> > >>>> From: Tianyu Yuan <tianyu.yuan@corigine.com> > >>>> > >>>> 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 > >>>> l > >>>> > >>>> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > >>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> > >>>> --- > >>>> 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 > ++++++ > >>>> 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 > +++++ > >>>> 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 +++++ > >>>> 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, > >>>> sfi.fmid = index; > >>>> sfi.maxsdu = a->police.mtu; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>>> if (rc) > >>>> goto out; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>>> 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. > >>>> + * 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, > >>>> 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. > >>>> + * 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, > >>>> 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. > >>>> + * 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"); > >>>> 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, > >>>> case FLOW_ACTION_MARK: > >>>> mark = act->mark; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>>> if (err) > >>>> return err; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>> struct flow_cls_offload *f) > >>>> data.pppoe.sid = act->pppoe.sid; > >>>> data.pppoe.num++; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>>> if (!act->offload_action) > >>>> continue; > >>>> > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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. > >>>> + * 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, > >>>> return err; > >>>> break; > >>>> } > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>>> 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. > >>>> + * 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, > >>>> } > >>>> filter->type = OCELOT_PSFP_FILTER_OFFLOAD; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>> const struct flow_action_entry *act, > >>>> if (err) > >>>> return err; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this action > >>>> + count the > >>> packets. > >>>> + * 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, > >>>> return -EINVAL; > >>>> } > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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, > >>>> } > >>>> *act = save; > >>>> break; > >>>> + /* Just ignore GACT with pipe action to let this > >>>> + action count the > >>> packets. > >>>> + * 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", > >>>> 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. > >>>> + * 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, > >>>> } 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"); > >>>> return -EOPNOTSUPP; > >>>> } else if (is_tcf_gact_reclassify(act)) { > >>>> NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" > >>> action is not supported"); > >>>> return -EOPNOTSUPP; > >>>> - } else if (is_tcf_gact_pipe(act)) { > >>>> - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" > action is > >>> not supported"); > >>>> - return -EOPNOTSUPP; > >>>> } else { > >>>> NL_SET_ERR_MSG_MOD(extack, "Unsupported > >>>> generic action > >>> offload"); > >>>> return -EOPNOTSUPP; @@ -275,6 +274,8 @@ > >>>> static int tcf_gact_offload_act_setup(struct tc_action *act, void > *entry_data, > >>>> 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 > >>>>
Hi Jamal,
When no action is specified, there should not be gact with PIPE, rather than a gact with drop, like:
`
# tc -s -d filter show dev eth8 ingress
filter protocol ipv6 pref 3 flower chain 0
filter protocol ipv6 pref 3 flower chain 0 handle 0x1
eth_type ipv6
ip_flags nofrag
in_hw in_hw_count 1
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1 installed 315 sec used 67 sec
Action statistics:
Sent 1412 bytes 18 pkt (dropped 18, overlimits 0 requeues 0)
Sent software 0 bytes 0 pkt
Sent hardware 1412 bytes 18 pkt
backlog 0b 0p requeues 0
cookie 54ed8ed8714cc38e7a7abc9009199520
no_percpu
used_hw_stats delayed
`
Also attach the info about dumping a filter, in which there is a PIPE before POLICE:
`
[root@compute1 ovs-private]# tc -s -d filter show dev eth5 ingress
filter protocol ip pref 2 flower chain 0
filter protocol ip pref 2 flower chain 0 handle 0x1
eth_type ipv4
ip_flags nofrag
in_hw in_hw_count 1
action order 1: gact action pipe
random type none pass val 0
index 1508263657 ref 1 bind 1 installed 87 sec used 0 sec
Action statistics:
Sent 1422020 bytes 20648 pkt (dropped 0, overlimits 0 requeues 0)
Sent software 0 bytes 0 pkt
Sent hardware 1422020 bytes 20648 pkt
backlog 0b 0p requeues 0
no_percpu
used_hw_stats delayed
action order 2: police 0x10000000 rate 30Mbit burst 1250Kb mtu 64Kb action drop/pipe overhead 0b linklayer unspec
ref 4 bind 3 installed 121 sec used 0 sec firstused 120 sec
Action statistics:
Sent 333966615 bytes 239585 pkt (dropped 0, overlimits 0 requeues 0)
Sent software 696 bytes 12 pkt
Sent hardware 333965919 bytes 239573 pkt
backlog 0b 0p requeues 0
used_hw_stats delayed
action order 3: mirred (Egress Redirect to device eth6) stolen
index 8 ref 1 bind 1 installed 87 sec used 0 sec
Action statistics:
Sent 1422020 bytes 20648 pkt (dropped 0, overlimits 0 requeues 0)
Sent software 0 bytes 0 pkt
Sent hardware 1422020 bytes 20648 pkt
backlog 0b 0p requeues 0
cookie e946e659084028456b360ab443f77cd0
no_percpu
used_hw_stats delayed
`
About the second scenario of PIPE alone, I don’t think it should exist.
Besides this adding a PIPE at the first place of a tc filter to update the flow stats, another
attempt that directly store the flower stats, which is got from driver, in socket transacted
with userspace (e.g. OVS). In this approach, we don’t have to make changes in driver. Which
could be a better solution you think for this propose
cheers,
Tianyu
On Sat, Nov 26, 2022 at 12:54 AM Jamal Hadi Salim <jhs@mojatatu.com<mailto:jhs@mojatatu.com>> wrote:
Hi Tianyu,
On Thu, Nov 24, 2022 at 10:10 PM Tianyu Yuan <tianyu.yuan@corigine.com<mailto:tianyu.yuan@corigine.com>> wrote:
On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com<mailto:jhs@mojatatu.com>> wrote:
>
> 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.
[..]
>
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.
Just noticed Vlad posted a patch for it ;->
I only looked at that change because i have that hardware and wanted to try it out.
You are Ccing all the driver stakeholders hopefully they can double check.
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.
So in the simple case it is as if no action was specified?
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.
makes sense.
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.
My initial thought was you want to cover two scenarios:
1) pipe followed by police.
2) pipe alone
In both cases pipe serves as a placeholder for stats counters. That these counters
come from hardware and will occasionally be updated to tc by the driver.
i.e if i get/dump the filter or pipe action stats i can see the hw count. Am i correct? If the
answer is yes, then would i see the hw stats and not the sw stat updates?
Maybe if you have a filter dump in your test environment that you can show it will
help satisfy my curiosity.
cheers,
jamal
On 28 Nov 2022, at 8:18, Tianyu Yuan wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Friday, November 25, 2022 10:33 PM >> To: Marcelo Leitner <mleitner@redhat.com> >> Cc: Tianyu Yuan <tianyu.yuan@corigine.com>; Jamal Hadi Salim >> <jhs@mojatatu.com>; Simon Horman <simon.horman@corigine.com>; >> netdev@vger.kernel.org; Cong Wang <xiyou.wangcong@gmail.com>; Davide >> Caratti <dcaratti@redhat.com>; Edward Cree <edward.cree@amd.com>; Ilya >> Maximets <i.maximets@ovn.org>; Oz Shlomo <ozsh@nvidia.com>; Paul >> Blakey <paulb@nvidia.com>; Vlad Buslov <vladbu@nvidia.com>; >> dev@openvswitch.org; oss-drivers <oss-drivers@corigine.com>; Ziyang Chen >> <ziyang.chen@corigine.com> >> Subject: Re: [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE >> when offloading >> >> >> >> On 25 Nov 2022, at 15:19, Marcelo Leitner wrote: >> >>> On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: >>>> On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> >> wrote: >>>> >>>>> >>>>> 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? >>>>> 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. >>>> >>>> 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. >>>> >>>> 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. >> >> I guess OVS userspace needs a simple way to determine which approach to >> use, i.e. if the kernel has this patch series applied. Or else it would not be >> easy to migrate userspace to use this approach. > > Yes, we could modify OVS' acinclude.m4 to check if the current has applied such a patch > series. If so, it will use this approach, otherwise, put actions in tc flower as before. So that > we could ensure the traffic will offloaded correctly. This needs to be a runtime check, we can’t assume the kernel capabilities at build time. >>> Other than this, patch seems good. >>> >>> Thanks, >>> Marcelo >>> >>>> >>>> B.R. >>>> Tianyu >>>> >>>>> >>>>> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman >>>>> <simon.horman@corigine.com> wrote: >>>>>> >>>>>> From: Tianyu Yuan <tianyu.yuan@corigine.com> >>>>>> >>>>>> 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 >>>>>> l >>>>>> >>>>>> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> >>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>> --- >>>>>> 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 >> ++++++ >>>>>> 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 >> +++++ >>>>>> 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 +++++ >>>>>> 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, >>>>>> sfi.fmid = index; >>>>>> sfi.maxsdu = a->police.mtu; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> if (rc) >>>>>> goto out; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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"); >>>>>> 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, >>>>>> case FLOW_ACTION_MARK: >>>>>> mark = act->mark; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> if (err) >>>>>> return err; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>> struct flow_cls_offload *f) >>>>>> data.pppoe.sid = act->pppoe.sid; >>>>>> data.pppoe.num++; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> if (!act->offload_action) >>>>>> continue; >>>>>> >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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. >>>>>> + * 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, >>>>>> return err; >>>>>> break; >>>>>> } >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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, >>>>>> } >>>>>> filter->type = OCELOT_PSFP_FILTER_OFFLOAD; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>> const struct flow_action_entry *act, >>>>>> if (err) >>>>>> return err; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this action >>>>>> + count the >>>>> packets. >>>>>> + * 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, >>>>>> return -EINVAL; >>>>>> } >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> } >>>>>> *act = save; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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", >>>>>> 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. >>>>>> + * 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, >>>>>> } 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"); >>>>>> return -EOPNOTSUPP; >>>>>> } else if (is_tcf_gact_reclassify(act)) { >>>>>> NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" >>>>> action is not supported"); >>>>>> return -EOPNOTSUPP; >>>>>> - } else if (is_tcf_gact_pipe(act)) { >>>>>> - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" >> action is >>>>> not supported"); >>>>>> - return -EOPNOTSUPP; >>>>>> } else { >>>>>> NL_SET_ERR_MSG_MOD(extack, "Unsupported >>>>>> generic action >>>>> offload"); >>>>>> return -EOPNOTSUPP; @@ -275,6 +274,8 @@ >>>>>> static int tcf_gact_offload_act_setup(struct tc_action *act, void >> *entry_data, >>>>>> 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 >>>>>>
On Mon, Nov 28, 2022 at 3:56 AM Tianyu Yuan <tianyu.yuan@corigine.com> wrote: > > Hi Jamal, > > > When no action is specified, there should not be gact with PIPE, rather than a gact with drop, like: > Thanks for the example dumps. I think you should put them in your commit logs. [..] > > About the second scenario of PIPE alone, I don’t think it should exist. > Besides this adding a PIPE at the first place of a tc filter to update the flow stats, another > attempt that directly store the flower stats, which is got from driver, in socket transacted > with userspace (e.g. OVS). In this approach, we don’t have to make changes in driver. Which > could be a better solution you think for this propose I was thinking about a case of a filter with no actions but with interest in a counter for that match. i.e pseudo-tc-dsl as: tc filter add ... flower blah action count which translated is: tc filter add ... flower blah action pipe cheers, jamal
On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > > On Fri, Nov 25, 2022 at 10:20 PM Marcelo Leitner <mleitner@redhat.com> > > On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: > > > On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> > > wrote: > > > > > > > > > > > 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? > > > > 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. > > > > > > 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. > > > > > > 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 > > > > > > > B.R. > > > Tianyu > > > > > > > > > > > On Tue, Nov 22, 2022 at 6:21 AM Simon Horman > > > > <simon.horman@corigine.com> wrote: > > > > > > > > > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > > > > > > > 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 > > > > > l > > > > > > > > > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > > > > --- > > > > > 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 > > ++++++ > > > > > 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 > > +++++ > > > > > 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 +++++ > > > > > 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, > > > > > sfi.fmid = index; > > > > > sfi.maxsdu = a->police.mtu; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > > if (rc) > > > > > goto out; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > > 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. > > > > > + * 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, > > > > > 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. > > > > > + * 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, > > > > > 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. > > > > > + * 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"); > > > > > 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, > > > > > case FLOW_ACTION_MARK: > > > > > mark = act->mark; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > > if (err) > > > > > return err; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > struct flow_cls_offload *f) > > > > > data.pppoe.sid = act->pppoe.sid; > > > > > data.pppoe.num++; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > > if (!act->offload_action) > > > > > continue; > > > > > > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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. > > > > > + * 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, > > > > > return err; > > > > > break; > > > > > } > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > > 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. > > > > > + * 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, > > > > > } > > > > > filter->type = OCELOT_PSFP_FILTER_OFFLOAD; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > const struct flow_action_entry *act, > > > > > if (err) > > > > > return err; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this action > > > > > + count the > > > > packets. > > > > > + * 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, > > > > > return -EINVAL; > > > > > } > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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, > > > > > } > > > > > *act = save; > > > > > break; > > > > > + /* Just ignore GACT with pipe action to let this > > > > > + action count the > > > > packets. > > > > > + * 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", > > > > > 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. > > > > > + * 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, > > > > > } 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"); > > > > > return -EOPNOTSUPP; > > > > > } else if (is_tcf_gact_reclassify(act)) { > > > > > NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" > > > > action is not supported"); > > > > > return -EOPNOTSUPP; > > > > > - } else if (is_tcf_gact_pipe(act)) { > > > > > - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" > > action is > > > > not supported"); > > > > > - return -EOPNOTSUPP; > > > > > } else { > > > > > NL_SET_ERR_MSG_MOD(extack, "Unsupported > > > > > generic action > > > > offload"); > > > > > return -EOPNOTSUPP; @@ -275,6 +274,8 @@ > > > > > static int tcf_gact_offload_act_setup(struct tc_action *act, void > > *entry_data, > > > > > 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 > > > > > >
On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: >> >> On Fri, Nov 25, 2022 at 10:20 PM Marcelo Leitner <mleitner@redhat.com> >>> On Fri, Nov 25, 2022 at 03:10:37AM +0000, Tianyu Yuan wrote: >>>> On Fri, Nov 25, 2022 at 10:21 AM Jamal Hadi Salim <jhs@mojatatu.com> >>> wrote: >>>> >>>>> >>>>> 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? >>>>> 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. >>>> >>>> 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. >>>> >>>> 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. Please note that OVS does not probe features per interface, but does it per datapath. So if it’s supported in pipe in tc software, we will use it. If the driver rejects it, we will probably end up with the tc software rule only. >>>>> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman >>>>> <simon.horman@corigine.com> wrote: >>>>>> >>>>>> From: Tianyu Yuan <tianyu.yuan@corigine.com> >>>>>> >>>>>> 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 >>>>>> l >>>>>> >>>>>> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> >>>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com> >>>>>> --- >>>>>> 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 >>> ++++++ >>>>>> 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 >>> +++++ >>>>>> 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 +++++ >>>>>> 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, >>>>>> sfi.fmid = index; >>>>>> sfi.maxsdu = a->police.mtu; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> if (rc) >>>>>> goto out; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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"); >>>>>> 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, >>>>>> case FLOW_ACTION_MARK: >>>>>> mark = act->mark; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> if (err) >>>>>> return err; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>> struct flow_cls_offload *f) >>>>>> data.pppoe.sid = act->pppoe.sid; >>>>>> data.pppoe.num++; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> if (!act->offload_action) >>>>>> continue; >>>>>> >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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. >>>>>> + * 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, >>>>>> return err; >>>>>> break; >>>>>> } >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> 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. >>>>>> + * 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, >>>>>> } >>>>>> filter->type = OCELOT_PSFP_FILTER_OFFLOAD; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>> const struct flow_action_entry *act, >>>>>> if (err) >>>>>> return err; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this action >>>>>> + count the >>>>> packets. >>>>>> + * 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, >>>>>> return -EINVAL; >>>>>> } >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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, >>>>>> } >>>>>> *act = save; >>>>>> break; >>>>>> + /* Just ignore GACT with pipe action to let this >>>>>> + action count the >>>>> packets. >>>>>> + * 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", >>>>>> 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. >>>>>> + * 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, >>>>>> } 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"); >>>>>> return -EOPNOTSUPP; >>>>>> } else if (is_tcf_gact_reclassify(act)) { >>>>>> NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" >>>>> action is not supported"); >>>>>> return -EOPNOTSUPP; >>>>>> - } else if (is_tcf_gact_pipe(act)) { >>>>>> - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" >>> action is >>>>> not supported"); >>>>>> - return -EOPNOTSUPP; >>>>>> } else { >>>>>> NL_SET_ERR_MSG_MOD(extack, "Unsupported >>>>>> generic action >>>>> offload"); >>>>>> return -EOPNOTSUPP; @@ -275,6 +274,8 @@ >>>>>> static int tcf_gact_offload_act_setup(struct tc_action *act, void >>> *entry_data, >>>>>> 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 >>>>>> >>
On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: > > > On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > > > On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: ... > >> > >> 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. > > Please note that OVS does not probe features per interface, but does it per datapath. So if it’s supported in pipe in tc software, we will use it. If the driver rejects it, we will probably end up with the tc software rule only. Ah right. I remember it will pick 1 interface for testing and use those results everywhere, which then I don't know if it may or may not be a representor port or not. Anyhow, then it should use skip_sw, to try to probe for the offloading part. Otherwise I'm afraid tc sw will always accept this flow and trick the probing, yes. Marcelo
On Mon, Nov 28, 2022 at 7:36 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On Mon, Nov 28, 2022 at 3:56 AM Tianyu Yuan <tianyu.yuan@corigine.com> > wrote: > > > > Hi Jamal, > > > > > > When no action is specified, there should not be gact with PIPE, rather than > a gact with drop, like: > > Thanks for reminding, I'll add that :) > > Thanks for the example dumps. I think you should put them in your commit > logs. > > [..] > > > > > About the second scenario of PIPE alone, I don’t think it should exist. > > Besides this adding a PIPE at the first place of a tc filter to update > > the flow stats, another attempt that directly store the flower stats, > > which is got from driver, in socket transacted with userspace (e.g. > > OVS). In this approach, we don’t have to make changes in driver. Which > > could be a better solution you think for this propose > > I was thinking about a case of a filter with no actions but with interest in a > counter for that match. > > i.e pseudo-tc-dsl as: > tc filter add ... flower blah action count > > which translated is: > tc filter add ... flower blah action pipe > > cheers, > jamal I test the case that only add a gact pipe in a filter as a counter, the result is shown below: # tc filter add dev eth5 parent ffff: pref 4 flower action gact pipe # tc -s -d filter show dev eth5 ingress filter parent ffff: protocol ip pref 4 flower chain 0 filter parent ffff: protocol ip pref 4 flower chain 0 handle 0x1 eth_type ipv4 in_hw in_hw_count 1 action order 1: gact action pipe random type none pass val 0 index 2 ref 1 bind 1 installed 314 sec used 0 sec Action statistics: Sent 8449056 bytes 5588 pkt (dropped 0, overlimits 0 requeues 0) Sent software 0 bytes 0 pkt Sent hardware 8449056 bytes 5588 pkt backlog 0b 0p requeues 0 used_hw_stats delayed in which eth5 is a vf representor and packets are coming from corresponding vf. Packets are generated by iperf udp. This pipe(counter) still shows in_hw flag, although the only one action is ignored by driver. Best regards, Tianyu
On 28/11/2022 13:11, Marcelo Leitner wrote: > 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. I agree. Drivers should only accept a flow with a 'gact pipe' action if they are able to perform the stats offload it requests. Getting userland to autodiscover whether it can make use of this is a SMOP, better than having it always try to use it and then sometimes get confused that the resulting stats are wrong or meaningless. -ed
On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: > On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: >> >> >> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: >> >>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > ... >>>> >>>> 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. >> >> Please note that OVS does not probe features per interface, but does it per datapath. So if it’s supported in pipe in tc software, we will use it. If the driver rejects it, we will probably end up with the tc software rule only. > > Ah right. I remember it will pick 1 interface for testing and use > those results everywhere, which then I don't know if it may or may not > be a representor port or not. Anyhow, then it should use skip_sw, to > try to probe for the offloading part. Otherwise I'm afraid tc sw will > always accept this flow and trick the probing, yes. Well, it depends on how you look at it. In theory, we should be hardware agnostic, meaning what if you have different hardware in your system? OVS only supports global offload enablement. Tianyu how are you planning to support this from the OVS side? How would you probe kernel and/or hardware support for this change? //Eelco
On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: > > On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: > > > On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: > >> > >> > >> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > >> > >>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > > ... > >>>> > >>>> 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. > >> > >> Please note that OVS does not probe features per interface, but does it > per datapath. So if it’s supported in pipe in tc software, we will use it. If the > driver rejects it, we will probably end up with the tc software rule only. > > > > Ah right. I remember it will pick 1 interface for testing and use > > those results everywhere, which then I don't know if it may or may not > > be a representor port or not. Anyhow, then it should use skip_sw, to > > try to probe for the offloading part. Otherwise I'm afraid tc sw will > > always accept this flow and trick the probing, yes. > > Well, it depends on how you look at it. In theory, we should be hardware > agnostic, meaning what if you have different hardware in your system? OVS > only supports global offload enablement. > > Tianyu how are you planning to support this from the OVS side? How would > you probe kernel and/or hardware support for this change? Currently in the test demo, I just extend gact with PIPE (previously only SHOT as default and GOTO_CHAIN when chain exists), and then put such a gact with PIPE at the first place of each filter which will be transacted with kernel tc. About the tc sw datapath mentioned, we don't have to make changes because gact with PIPE has already been supported in current tc implementation and it could act like a 'counter' And for the hardware we just need to ignore this PIPE and the stats of this action will still be updated in kernel side and sent to userspace. I agree with that the unsupported actions should be rejected by drivers, so may another approach could work without ignoring PIPE in all the related drivers, that we directly make put the flower stats from driver into the socket which is used to transact with userspace and userspace(e.g. OVS) update the flow stats using this stats instead of the parsing the action stats. How do you think of this? Cheers, Tianyu > > //Eelco
On Wed, Nov 30, 2022 at 03:36:57AM +0000, Tianyu Yuan wrote: > > On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: > > > > On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: > > > > > On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: > > >> > > >> > > >> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > > >> > > >>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > > > ... > > >>>> > > >>>> 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. > > >> > > >> Please note that OVS does not probe features per interface, but does it > > per datapath. So if it’s supported in pipe in tc software, we will use it. If the > > driver rejects it, we will probably end up with the tc software rule only. > > > > > > Ah right. I remember it will pick 1 interface for testing and use > > > those results everywhere, which then I don't know if it may or may not > > > be a representor port or not. Anyhow, then it should use skip_sw, to > > > try to probe for the offloading part. Otherwise I'm afraid tc sw will > > > always accept this flow and trick the probing, yes. > > > > Well, it depends on how you look at it. In theory, we should be hardware > > agnostic, meaning what if you have different hardware in your system? OVS > > only supports global offload enablement. > > > > Tianyu how are you planning to support this from the OVS side? How would > > you probe kernel and/or hardware support for this change? > > Currently in the test demo, I just extend gact with PIPE (previously only SHOT as default and > GOTO_CHAIN when chain exists), and then put such a gact with PIPE at the first place of each > filter which will be transacted with kernel tc. > > About the tc sw datapath mentioned, we don't have to make changes because gact with PIPE > has already been supported in current tc implementation and it could act like a 'counter' And > for the hardware we just need to ignore this PIPE and the stats of this action will still be updated > in kernel side and sent to userspace. I can't see how the action would have stats from hw if the driver is ignoring the action. But maybe there was a misunderstanding here. I was reading more the cxgb4 driver here and AFAICT this patch will skip PIPE on the action validation, but not actually skip the action entirely. Then it will hit cxgb4_process_flow_actions() and maybe the driver will the right thing with a dummy action out of the blue. Was this your expectation, to just ignore it in the validation step, and let it fall through through the driver? If yes, the comments are misleading, as the NICs will have to process the packets. > > I agree with that the unsupported actions should be rejected by drivers, so may another approach > could work without ignoring PIPE in all the related drivers, that we directly make put the flower stats > from driver into the socket which is used to transact with userspace and userspace(e.g. OVS) update > the flow stats using this stats instead of the parsing the action stats. How do you think of this? I don't understand this approach. Can you please rephrase? Thanks, Marcelo > > Cheers, > Tianyu > > > > //Eelco >
On Thus, Dec 1 , 2022, at 2:05 AM, Marcelo Leitner wrote: > > On Wed, Nov 30, 2022 at 03:36:57AM +0000, Tianyu Yuan wrote: > > > > On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: > > > > > > On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: > > > > > > > On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: > > > >> > > > >> > > > >> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > > > >> > > > >>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > > > > ... > > > >>>> > > > >>>> 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. > > > >> > > > >> Please note that OVS does not probe features per interface, but > > > >> does it > > > per datapath. So if it’s supported in pipe in tc software, we will > > > use it. If the driver rejects it, we will probably end up with the tc software > rule only. > > > > > > > > Ah right. I remember it will pick 1 interface for testing and use > > > > those results everywhere, which then I don't know if it may or may > > > > not be a representor port or not. Anyhow, then it should use > > > > skip_sw, to try to probe for the offloading part. Otherwise I'm > > > > afraid tc sw will always accept this flow and trick the probing, yes. > > > > > > Well, it depends on how you look at it. In theory, we should be > > > hardware agnostic, meaning what if you have different hardware in > > > your system? OVS only supports global offload enablement. > > > > > > Tianyu how are you planning to support this from the OVS side? How > > > would you probe kernel and/or hardware support for this change? > > > > Currently in the test demo, I just extend gact with PIPE (previously > > only SHOT as default and GOTO_CHAIN when chain exists), and then put > > such a gact with PIPE at the first place of each filter which will be transacted > with kernel tc. > > > > About the tc sw datapath mentioned, we don't have to make changes > > because gact with PIPE has already been supported in current tc > > implementation and it could act like a 'counter' And for the hardware > > we just need to ignore this PIPE and the stats of this action will still be > > updated in kernel side and sent to userspace. > > I can't see how the action would have stats from hw if the driver is ignoring > the action. The stats for each actions in a filter is updated here in pkt_cls.h: static inline void tcf_exts_hw_stats_update(const struct tcf_exts *exts, u64 bytes, u64 packets, u64 drops, u64 lastuse, u8 used_hw_stats, bool used_hw_stats_valid) { #ifdef CONFIG_NET_CLS_ACT int i; for (i = 0; i < exts->nr_actions; i++) { struct tc_action *a = exts->actions[i]; /* if stats from hw, just skip */ if (tcf_action_update_hw_stats(a)) { preempt_disable(); tcf_action_stats_update(a, bytes, packets, drops, lastuse, true); preempt_enable(); a->used_hw_stats = used_hw_stats; a->used_hw_stats_valid = used_hw_stats_valid; } } #endif } In which bytes, packets, drops, lastuse are dumped from the driver, the stats of gact PIPE is updated here is kernel TC, rather than in driver directly. > > But maybe there was a misunderstanding here. I was reading more the > cxgb4 driver here and AFAICT this patch will skip PIPE on the action validation, > but not actually skip the action entirely. Then it will hit > cxgb4_process_flow_actions() and maybe the driver will the right thing with > a dummy action out of the blue. Was this your expectation, to just ignore it in > the validation step, and let it fall through through the driver? If yes, the > comments are misleading, as the NICs will have to process the packets. > Actually we want to ignore it not only in validation step but also the process step. We don't want to HW process this action and just want to let the driver treat this flow with PIPE at the first place as a offloadable one. > > > > I agree with that the unsupported actions should be rejected by > > drivers, so may another approach could work without ignoring PIPE in > > all the related drivers, that we directly make put the flower stats > > from driver into the socket which is used to transact with userspace and >> userspace(e.g. OVS) update the flow stats using this stats instead of the >> parsing the action stats. How do you think of this? > > I don't understand this approach. Can you please rephrase? > Sorry for not explaining clearly :) Basically in another approach, we let the cls_fl_filter records the stats for this filter and directly use this stats to update the flow stats in userspace, the key code changes are following: ...... //update stats in cls_fl_filter: @@ -500,6 +542,9 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f, tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false, rtnl_held); + cls_fl_filter_update_stats(f, cls_flower.stats.bytes, + cls_flower.stats.pkts, true); + tcf_exts_hw_stats_update(&f->exts, cls_flower.stats.bytes, cls_flower.stats.pkts, cls_flower.stats.drops, //add entries for netlink transaction @@ -714,6 +759,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { [TCA_FLOWER_KEY_PPPOE_SID] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_PPP_PROTO] = { .type = NLA_U16 }, [TCA_FLOWER_KEY_L2TPV3_SID] = { .type = NLA_U32 }, + [TCA_FLOWER_STATS] = { .type = NLA_NESTED}, + [TCA_FLOWER_LASTUSE] = { .type = NLA_U64 }, }; ...... //copy stats in cls_fl_filter into the socket which will be sent to userspace @@ -3319,6 +3392,9 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh, if (tcf_exts_dump(skb, &f->exts)) goto nla_put_failure; + if (tcf_filter_copy_stats(skb, f) < 0) + goto nla_put_failure; + nla_nest_end(skb, nest); if (tcf_exts_dump_stats(skb, &f->exts) < 0) @@ -3364,6 +3440,9 @@ static int fl_terse_dump(struct net *net, struct tcf_proto *tp, void *fh, if (tcf_exts_terse_dump(skb, &f->exts)) goto nla_put_failure; + if (tcf_filter_copy_stats(skb, f) < 0) + goto nla_put_failure; + nla_nest_end(skb, nest); return skb->len; ...... In this approach we don't have to make changes in drivers and they validate and process flows as before. I hope this could make it clear and if there is something wrong or misunderstanding, please let me know. Thanks, Tianyu > Thanks, > Marcelo > > > > > Cheers, > > Tianyu > > > > > > //Eelco > >
On 30 Nov 2022, at 4:36, Tianyu Yuan wrote: > On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: >> >> On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: >> >>> On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: >>>> >>>> >>>> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: >>>> >>>>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: >>> ... >>>>>> >>>>>> 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. >>>> >>>> Please note that OVS does not probe features per interface, but does it >> per datapath. So if it’s supported in pipe in tc software, we will use it. If the >> driver rejects it, we will probably end up with the tc software rule only. >>> >>> Ah right. I remember it will pick 1 interface for testing and use >>> those results everywhere, which then I don't know if it may or may not >>> be a representor port or not. Anyhow, then it should use skip_sw, to >>> try to probe for the offloading part. Otherwise I'm afraid tc sw will >>> always accept this flow and trick the probing, yes. >> >> Well, it depends on how you look at it. In theory, we should be hardware >> agnostic, meaning what if you have different hardware in your system? OVS >> only supports global offload enablement. >> >> Tianyu how are you planning to support this from the OVS side? How would >> you probe kernel and/or hardware support for this change? > > Currently in the test demo, I just extend gact with PIPE (previously only SHOT as default and > GOTO_CHAIN when chain exists), and then put such a gact with PIPE at the first place of each > filter which will be transacted with kernel tc. > > About the tc sw datapath mentioned, we don't have to make changes because gact with PIPE > has already been supported in current tc implementation and it could act like a 'counter' And > for the hardware we just need to ignore this PIPE and the stats of this action will still be updated > in kernel side and sent to userspace. I know it’s supported now, but if we implement it, it might fail in existing environments. So from an OVS userspace perspective, you need to implement something like: - Probe the kernel to see if this patch is applied, if not use the old method so we do not break existing deployments when upgrading OVS but not the kernel. - If we do have this newer kernel, do we assume all drivers that worked before, now also work? - If this is not the case, how will you determine what approach to use? We do not have a per-interface layer, but a per-datapath one, i.e. the kernel. We do not know at initialization time what NICs will be added later and we can not decide on the strategy to use. Thought? Maybe this should be discussed outside of the netdev mailing list, but what I want to highlight is that there should be a runtime way to determine if this patch is applied to the kernel (without using any actual hw driver). //Eelco > I agree with that the unsupported actions should be rejected by drivers, so may another approach > could work without ignoring PIPE in all the related drivers, that we directly make put the flower stats > from driver into the socket which is used to transact with userspace and userspace(e.g. OVS) update > the flow stats using this stats instead of the parsing the action stats. How do you think of this?
On Fri, Dec 2, 2022 at 8:18 PM , Eelco Chaudron wrote: > > On 30 Nov 2022, at 4:36, Tianyu Yuan wrote: > > > On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: > >> > >> On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: > >> > >>> On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: > >>>> > >>>> > >>>> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > >>>> > >>>>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > >>> ... > >>>>>> > >>>>>> 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. > >>>> > >>>> Please note that OVS does not probe features per interface, but > >>>> does it > >> per datapath. So if it’s supported in pipe in tc software, we will > >> use it. If the driver rejects it, we will probably end up with the tc software > rule only. > >>> > >>> Ah right. I remember it will pick 1 interface for testing and use > >>> those results everywhere, which then I don't know if it may or may > >>> not be a representor port or not. Anyhow, then it should use > >>> skip_sw, to try to probe for the offloading part. Otherwise I'm > >>> afraid tc sw will always accept this flow and trick the probing, yes. > >> > >> Well, it depends on how you look at it. In theory, we should be > >> hardware agnostic, meaning what if you have different hardware in > >> your system? OVS only supports global offload enablement. > >> > >> Tianyu how are you planning to support this from the OVS side? How > >> would you probe kernel and/or hardware support for this change? > > > > Currently in the test demo, I just extend gact with PIPE (previously > > only SHOT as default and GOTO_CHAIN when chain exists), and then put > > such a gact with PIPE at the first place of each filter which will be transacted > with kernel tc. > > > > About the tc sw datapath mentioned, we don't have to make changes > > because gact with PIPE has already been supported in current tc > > implementation and it could act like a 'counter' And for the hardware > > we just need to ignore this PIPE and the stats of this action will still be > updated in kernel side and sent to userspace. > > I know it’s supported now, but if we implement it, it might fail in existing > environments. So from an OVS userspace perspective, you need to > implement something like: I've got your point now, sorry for my misunderstanding previously. > > - Probe the kernel to see if this patch is applied, if not use the old method so > we do not break existing deployments when upgrading OVS but not the > kernel. > - If we do have this newer kernel, do we assume all drivers that worked > before, now also work? > - If this is not the case, how will you determine what approach to use? We > do not have a per-interface layer, but a per-datapath one, i.e. the kernel. We > do not know at initialization time what NICs will be added later and we can > not decide on the strategy to use. > > Thought? Maybe this should be discussed outside of the netdev mailing list, > but what I want to highlight is that there should be a runtime way to > determine if this patch is applied to the kernel (without using any actual hw > driver). I agree that whether the patch is applied in kernel should be checked at runtime rather than compiling (for the demo I made this check inacinlude.m4). I think I need some time to investigate how to implement it. We may discuss it later in an OVS mailing list. Thanks, Tianyu > > //Eelco > > > I agree with that the unsupported actions should be rejected by > > drivers, so may another approach could work without ignoring PIPE in > > all the related drivers, that we directly make put the flower stats > > from driver into the socket which is used to transact with userspace and > userspace(e.g. OVS) update the flow stats using this stats instead of the > parsing the action stats. How do you think of this?
On 2 Dec 2022, at 13:33, Tianyu Yuan wrote: > On Fri, Dec 2, 2022 at 8:18 PM , Eelco Chaudron wrote: >> >> On 30 Nov 2022, at 4:36, Tianyu Yuan wrote: >> >>> On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: >>>> >>>> On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: >>>> >>>>> On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: >>>>>> >>>>>> >>>>>> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: >>>>>> >>>>>>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: >>>>> ... >>>>>>>> >>>>>>>> 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. >>>>>> >>>>>> Please note that OVS does not probe features per interface, but >>>>>> does it >>>> per datapath. So if it’s supported in pipe in tc software, we will >>>> use it. If the driver rejects it, we will probably end up with the tc software >> rule only. >>>>> >>>>> Ah right. I remember it will pick 1 interface for testing and use >>>>> those results everywhere, which then I don't know if it may or may >>>>> not be a representor port or not. Anyhow, then it should use >>>>> skip_sw, to try to probe for the offloading part. Otherwise I'm >>>>> afraid tc sw will always accept this flow and trick the probing, yes. >>>> >>>> Well, it depends on how you look at it. In theory, we should be >>>> hardware agnostic, meaning what if you have different hardware in >>>> your system? OVS only supports global offload enablement. >>>> >>>> Tianyu how are you planning to support this from the OVS side? How >>>> would you probe kernel and/or hardware support for this change? >>> >>> Currently in the test demo, I just extend gact with PIPE (previously >>> only SHOT as default and GOTO_CHAIN when chain exists), and then put >>> such a gact with PIPE at the first place of each filter which will be transacted >> with kernel tc. >>> >>> About the tc sw datapath mentioned, we don't have to make changes >>> because gact with PIPE has already been supported in current tc >>> implementation and it could act like a 'counter' And for the hardware >>> we just need to ignore this PIPE and the stats of this action will still be >> updated in kernel side and sent to userspace. >> >> I know it’s supported now, but if we implement it, it might fail in existing >> environments. So from an OVS userspace perspective, you need to >> implement something like: > > I've got your point now, sorry for my misunderstanding previously. No problem, there are quite some emails around this patch. >> - Probe the kernel to see if this patch is applied, if not use the old method so >> we do not break existing deployments when upgrading OVS but not the >> kernel. >> - If we do have this newer kernel, do we assume all drivers that worked >> before, now also work? >> - If this is not the case, how will you determine what approach to use? We >> do not have a per-interface layer, but a per-datapath one, i.e. the kernel. We >> do not know at initialization time what NICs will be added later and we can >> not decide on the strategy to use. >> >> Thought? Maybe this should be discussed outside of the netdev mailing list, >> but what I want to highlight is that there should be a runtime way to >> determine if this patch is applied to the kernel (without using any actual hw >> driver). > > I agree that whether the patch is applied in kernel should be checked at runtime rather than > compiling (for the demo I made this check inacinlude.m4). I think I need some time to investigate > how to implement it. We may discuss it later in an OVS mailing list. No problem, but just want to make sure that if it needs changes to this patch to be able to do it, now is the time ;) >>> I agree with that the unsupported actions should be rejected by >>> drivers, so may another approach could work without ignoring PIPE in >>> all the related drivers, that we directly make put the flower stats >>> from driver into the socket which is used to transact with userspace and >> userspace(e.g. OVS) update the flow stats using this stats instead of the >> parsing the action stats. How do you think of this?
On 2 Dec 2022, at 8:40 PM, Eelco Chaudron wrote: > > On 2 Dec 2022, at 13:33, Tianyu Yuan wrote: > > > On Fri, Dec 2, 2022 at 8:18 PM , Eelco Chaudron wrote: > >> > >> On 30 Nov 2022, at 4:36, Tianyu Yuan wrote: > >> > >>> On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote: > >>>> > >>>> On 28 Nov 2022, at 14:33, Marcelo Leitner wrote: > >>>> > >>>>> On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote: > >>>>>> > >>>>>> > >>>>>> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote: > >>>>>> > >>>>>>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote: > >>>>> ... > >>>>>>>> > >>>>>>>> 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. > >>>>>> > >>>>>> Please note that OVS does not probe features per interface, but > >>>>>> does it > >>>> per datapath. So if it’s supported in pipe in tc software, we will > >>>> use it. If the driver rejects it, we will probably end up with the > >>>> tc software > >> rule only. > >>>>> > >>>>> Ah right. I remember it will pick 1 interface for testing and use > >>>>> those results everywhere, which then I don't know if it may or may > >>>>> not be a representor port or not. Anyhow, then it should use > >>>>> skip_sw, to try to probe for the offloading part. Otherwise I'm > >>>>> afraid tc sw will always accept this flow and trick the probing, yes. > >>>> > >>>> Well, it depends on how you look at it. In theory, we should be > >>>> hardware agnostic, meaning what if you have different hardware in > >>>> your system? OVS only supports global offload enablement. > >>>> > >>>> Tianyu how are you planning to support this from the OVS side? How > >>>> would you probe kernel and/or hardware support for this change? > >>> > >>> Currently in the test demo, I just extend gact with PIPE (previously > >>> only SHOT as default and GOTO_CHAIN when chain exists), and then put > >>> such a gact with PIPE at the first place of each filter which will > >>> be transacted > >> with kernel tc. > >>> > >>> About the tc sw datapath mentioned, we don't have to make changes > >>> because gact with PIPE has already been supported in current tc > >>> implementation and it could act like a 'counter' And for the > >>> hardware we just need to ignore this PIPE and the stats of this > >>> action will still be > >> updated in kernel side and sent to userspace. > >> > >> I know it’s supported now, but if we implement it, it might fail in > >> existing environments. So from an OVS userspace perspective, you need > >> to implement something like: > > > > I've got your point now, sorry for my misunderstanding previously. > > No problem, there are quite some emails around this patch. > > >> - Probe the kernel to see if this patch is applied, if not use the > >> old method so we do not break existing deployments when upgrading > OVS > >> but not the kernel. > >> - If we do have this newer kernel, do we assume all drivers that > >> worked before, now also work? > >> - If this is not the case, how will you determine what approach to > >> use? We do not have a per-interface layer, but a per-datapath one, > >> i.e. the kernel. We do not know at initialization time what NICs will > >> be added later and we can not decide on the strategy to use. > >> > >> Thought? Maybe this should be discussed outside of the netdev mailing > >> list, but what I want to highlight is that there should be a runtime > >> way to determine if this patch is applied to the kernel (without > >> using any actual hw driver). > > > > I agree that whether the patch is applied in kernel should be checked > > at runtime rather than compiling (for the demo I made this check > > inacinlude.m4). I think I need some time to investigate how to implement it. > We may discuss it later in an OVS mailing list. > > No problem, but just want to make sure that if it needs changes to this patch > to be able to do it, now is the time ;) Sorry to not reply for such a long time. I made some investigation on how to probe single pipe action offload support in OVS side and found that we don't have to make change in kernel for this probe purpose. Maybe we could process this kernel tc and driver changes after OVS one. Look forward to discussing with you in OVS side 😊 Cheers, Tianyu > > > >>> I agree with that the unsupported actions should be rejected by > >>> drivers, so may another approach could work without ignoring PIPE in > >>> all the related drivers, that we directly make put the flower stats > >>> from driver into the socket which is used to transact with userspace > >>> and > >> userspace(e.g. OVS) update the flow stats using this stats instead of > >> the parsing the action stats. How do you think of this?
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, sfi.fmid = index; sfi.maxsdu = a->police.mtu; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, if (rc) goto out; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, 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. + * 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, 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. + * 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, 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. + * 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"); 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, case FLOW_ACTION_MARK: mark = act->mark; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, if (err) return err; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, struct flow_cls_offload *f) data.pppoe.sid = act->pppoe.sid; data.pppoe.num++; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, if (!act->offload_action) continue; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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. + * 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, return err; break; } + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, 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. + * 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, } filter->type = OCELOT_PSFP_FILTER_OFFLOAD; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, const struct flow_action_entry *act, if (err) return err; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, return -EINVAL; } break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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, } *act = save; break; + /* Just ignore GACT with pipe action to let this action count the packets. + * 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", 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. + * 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, } 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"); return -EOPNOTSUPP; } else if (is_tcf_gact_reclassify(act)) { NL_SET_ERR_MSG_MOD(extack, "Offload of \"reclassify\" action is not supported"); return -EOPNOTSUPP; - } else if (is_tcf_gact_pipe(act)) { - NL_SET_ERR_MSG_MOD(extack, "Offload of \"pipe\" action is not supported"); - return -EOPNOTSUPP; } else { NL_SET_ERR_MSG_MOD(extack, "Unsupported generic action offload"); return -EOPNOTSUPP; @@ -275,6 +274,8 @@ static int tcf_gact_offload_act_setup(struct tc_action *act, void *entry_data, 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; }