* [PATCH v4 net-next 0/10] allow user to offload tc action to net device
@ 2021-11-18 13:07 Simon Horman
2021-11-18 13:07 ` [PATCH v4 01/10] flow_offload: fill flags to action structure Simon Horman
` (10 more replies)
0 siblings, 11 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:07 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police rate 100mbit burst 10000k index 200 skip_sw
tc -s -d actions list action police
tc filter add dev $DEV protocol ip parent ffff: \
flower skip_sw ip_proto tcp action police index 200
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff: \
flower skip_sw ip_proto tcp action police index 200
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc -s -d actions list action police
Output for the tc action verbose dump:
action order 0: police index 200 rate 100Mbit burst 10000Kb mtu 2Kb action drop overhead 0 linklayer unspec ref 3 bind 2 installed 52 sec used 0 sec firstused 30 sec
Action statistics:
Sent 136094386 bytes 91110 pkt (dropped 0, overlimits 0 requeues 0)
Sent software 0 bytes 0 pkt
Sent hardware 136094386 bytes 91110 pkt
backlog 0b 0p requeues 0
skip_sw in_hw in_hw_count 1
used_hw_stats delayed
Tc cli cleanup commands
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 200
Changes compared to v3 patches:
* Made changes according to the public review comments.
* Validate flags inside tcf_action_init() instead of creating new
tcf_exts_validate_actions() function.
* Exactly match when validating flags of actions and filters.
* Add index to flow_action_entry for driver to identify actions.
Baowen Zheng (10):
flow_offload: fill flags to action structure
flow_offload: reject to offload tc actions in offload drivers
flow_offload: add index to flow_action_entry structure
flow_offload: allow user to offload tc action to net device
flow_offload: add skip_hw and skip_sw to control if offload the action
flow_offload: add process to update action stats from hardware
net: sched: save full flags for tc action
flow_offload: add reoffload process to update hw_count
flow_offload: validate flags of filter and actions
selftests: tc-testing: add action offload selftest for action and
filter
drivers/net/dsa/sja1105/sja1105_flower.c | 2 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
.../net/ethernet/freescale/enetc/enetc_qos.c | 6 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +
.../ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +-
.../ethernet/netronome/nfp/flower/offload.c | 3 +
include/linux/netdevice.h | 1 +
include/net/act_api.h | 34 +-
include/net/flow_offload.h | 20 +-
include/net/pkt_cls.h | 56 ++-
include/net/tc_act/tc_gate.h | 5 -
include/uapi/linux/pkt_cls.h | 9 +-
net/core/flow_offload.c | 47 +-
net/sched/act_api.c | 451 +++++++++++++++++-
net/sched/act_bpf.c | 2 +-
net/sched/act_connmark.c | 2 +-
net/sched/act_ctinfo.c | 2 +-
net/sched/act_gate.c | 2 +-
net/sched/act_ife.c | 2 +-
net/sched/act_ipt.c | 2 +-
net/sched/act_mpls.c | 2 +-
net/sched/act_nat.c | 2 +-
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/act_sample.c | 2 +-
net/sched/act_simple.c | 2 +-
net/sched/act_skbedit.c | 2 +-
net/sched/act_skbmod.c | 2 +-
net/sched/cls_api.c | 52 +-
net/sched/cls_flower.c | 9 +-
net/sched/cls_matchall.c | 9 +-
net/sched/cls_u32.c | 12 +-
.../tc-testing/tc-tests/actions/police.json | 24 +
.../tc-testing/tc-tests/filters/matchall.json | 24 +
34 files changed, 719 insertions(+), 80 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 01/10] flow_offload: fill flags to action structure
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
@ 2021-11-18 13:07 ` Simon Horman
2021-11-18 13:07 ` [PATCH v4 02/10] flow_offload: reject to offload tc actions in offload drivers Simon Horman
` (9 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:07 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Fill flags to action structure to allow user control if
the action should be offloaded to hardware or not.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
net/sched/act_bpf.c | 2 +-
net/sched/act_connmark.c | 2 +-
net/sched/act_ctinfo.c | 2 +-
net/sched/act_gate.c | 2 +-
net/sched/act_ife.c | 2 +-
net/sched/act_ipt.c | 2 +-
net/sched/act_mpls.c | 2 +-
net/sched/act_nat.c | 2 +-
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/act_sample.c | 2 +-
net/sched/act_simple.c | 2 +-
net/sched/act_skbedit.c | 2 +-
net/sched/act_skbmod.c | 2 +-
14 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index f2bf896331a5..a77d8908e737 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -305,7 +305,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
ret = tcf_idr_check_alloc(tn, &index, act, bind);
if (!ret) {
ret = tcf_idr_create(tn, index, est, act,
- &act_bpf_ops, bind, true, 0);
+ &act_bpf_ops, bind, true, flags);
if (ret < 0) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 94e78ac7a748..09e2aafc8943 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -124,7 +124,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
ret = tcf_idr_check_alloc(tn, &index, a, bind);
if (!ret) {
ret = tcf_idr_create(tn, index, est, a,
- &act_connmark_ops, bind, false, 0);
+ &act_connmark_ops, bind, false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 549374a2d008..0281e45987a4 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -212,7 +212,7 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
err = tcf_idr_check_alloc(tn, &index, a, bind);
if (!err) {
ret = tcf_idr_create(tn, index, est, a,
- &act_ctinfo_ops, bind, false, 0);
+ &act_ctinfo_ops, bind, false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 7df72a4197a3..ac985c53ebaf 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -357,7 +357,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
if (!err) {
ret = tcf_idr_create(tn, index, est, a,
- &act_gate_ops, bind, false, 0);
+ &act_gate_ops, bind, false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index b757f90a2d58..41ba55e60b1b 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -553,7 +553,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a, &act_ife_ops,
- bind, true, 0);
+ bind, true, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
kfree(p);
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 265b1443e252..2f3d507c24a1 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -145,7 +145,7 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a, ops, bind,
- false, 0);
+ false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 8faa4c58305e..2b30dc562743 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -248,7 +248,7 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a,
- &act_mpls_ops, bind, true, 0);
+ &act_mpls_ops, bind, true, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 7dd6b586ba7f..2a39b3729e84 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -61,7 +61,7 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
err = tcf_idr_check_alloc(tn, &index, a, bind);
if (!err) {
ret = tcf_idr_create(tn, index, est, a,
- &act_nat_ops, bind, false, 0);
+ &act_nat_ops, bind, false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index c6c862c459cc..cd3b8aad3192 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -189,7 +189,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
err = tcf_idr_check_alloc(tn, &index, a, bind);
if (!err) {
ret = tcf_idr_create(tn, index, est, a,
- &act_pedit_ops, bind, false, 0);
+ &act_pedit_ops, bind, false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
goto out_free;
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 9e77ba8401e5..c13a6245dfba 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -90,7 +90,7 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, NULL, a,
- &act_police_ops, bind, true, 0);
+ &act_police_ops, bind, true, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index ce859b0e0deb..91a7a93d5f6a 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -70,7 +70,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a,
- &act_sample_ops, bind, true, 0);
+ &act_sample_ops, bind, true, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index e617ab4505ca..8c1d60bde93e 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -129,7 +129,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a,
- &act_simp_ops, bind, false, 0);
+ &act_simp_ops, bind, false, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index d30ecbfc8f84..cb2d10d3dcc0 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -176,7 +176,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a,
- &act_skbedit_ops, bind, true, 0);
+ &act_skbedit_ops, bind, true, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 9b6b52c5e24e..2083612d8780 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -168,7 +168,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, index, est, a,
- &act_skbmod_ops, bind, true, 0);
+ &act_skbmod_ops, bind, true, flags);
if (ret) {
tcf_idr_cleanup(tn, index);
return ret;
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 02/10] flow_offload: reject to offload tc actions in offload drivers
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
2021-11-18 13:07 ` [PATCH v4 01/10] flow_offload: fill flags to action structure Simon Horman
@ 2021-11-18 13:07 ` Simon Horman
2021-11-18 13:07 ` [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure Simon Horman
` (8 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:07 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
A follow-up patch will allow users to offload tc actions independent of
classifier in the software datapath.
In preparation for this, teach all drivers that support offload of the flow
tables to reject such configuration as currently none of them support it.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +++
drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index e6a4a768b10b..8c9bab932478 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1962,7 +1962,7 @@ static int bnxt_tc_setup_indr_cb(struct net_device *netdev, struct Qdisc *sch, v
void *data,
void (*cleanup)(struct flow_block_cb *block_cb))
{
- if (!bnxt_is_netdev_indr_offload(netdev))
+ if (!netdev || !bnxt_is_netdev_indr_offload(netdev))
return -EOPNOTSUPP;
switch (type) {
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 fcb0892c08a9..0991345c4ae5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
@@ -517,6 +517,9 @@ int mlx5e_rep_indr_setup_cb(struct net_device *netdev, struct Qdisc *sch, void *
void *data,
void (*cleanup)(struct flow_block_cb *block_cb))
{
+ if (!netdev)
+ return -EOPNOTSUPP;
+
switch (type) {
case TC_SETUP_BLOCK:
return mlx5e_rep_indr_setup_block(netdev, sch, cb_priv, type_data,
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 224089d04d98..f97eff5afd12 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -1867,6 +1867,9 @@ nfp_flower_indr_setup_tc_cb(struct net_device *netdev, struct Qdisc *sch, void *
void *data,
void (*cleanup)(struct flow_block_cb *block_cb))
{
+ if (!netdev)
+ return -EOPNOTSUPP;
+
if (!nfp_fl_is_netdev_to_offload(netdev))
return -EOPNOTSUPP;
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
2021-11-18 13:07 ` [PATCH v4 01/10] flow_offload: fill flags to action structure Simon Horman
2021-11-18 13:07 ` [PATCH v4 02/10] flow_offload: reject to offload tc actions in offload drivers Simon Horman
@ 2021-11-18 13:07 ` Simon Horman
2021-11-19 6:31 ` Jakub Kicinski
2021-11-18 13:07 ` [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device Simon Horman
` (7 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:07 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Add index to flow_action_entry structure and delete index from police and
gate child structure.
We make this change to offload tc action for driver to identify a tc
action.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
drivers/net/dsa/sja1105/sja1105_flower.c | 2 +-
drivers/net/ethernet/freescale/enetc/enetc_qos.c | 6 +++---
drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +-
include/net/flow_offload.h | 3 +--
include/net/tc_act/tc_gate.h | 5 -----
net/sched/cls_api.c | 3 +--
6 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
index 72b9b39b0989..ff0b48d48576 100644
--- a/drivers/net/dsa/sja1105/sja1105_flower.c
+++ b/drivers/net/dsa/sja1105/sja1105_flower.c
@@ -379,7 +379,7 @@ int sja1105_cls_flower_add(struct dsa_switch *ds, int port,
vl_rule = true;
rc = sja1105_vl_gate(priv, port, extack, cookie,
- &key, act->gate.index,
+ &key, act->index,
act->gate.prio,
act->gate.basetime,
act->gate.cycletime,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 0536d2c76fbc..04a81bba14b2 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -1182,7 +1182,7 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
}
/* parsing gate action */
- if (entryg->gate.index >= priv->psfp_cap.max_psfp_gate) {
+ if (entryg->index >= priv->psfp_cap.max_psfp_gate) {
NL_SET_ERR_MSG_MOD(extack, "No Stream Gate resource!");
err = -ENOSPC;
goto free_filter;
@@ -1202,7 +1202,7 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
}
refcount_set(&sgi->refcount, 1);
- sgi->index = entryg->gate.index;
+ sgi->index = entryg->index;
sgi->init_ipv = entryg->gate.prio;
sgi->basetime = entryg->gate.basetime;
sgi->cycletime = entryg->gate.cycletime;
@@ -1244,7 +1244,7 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
refcount_set(&fmi->refcount, 1);
fmi->cir = entryp->police.rate_bytes_ps;
fmi->cbs = entryp->police.burst;
- fmi->index = entryp->police.index;
+ fmi->index = entryp->index;
filter->flags |= ENETC_PSFP_FLAGS_FMI;
filter->fmi_index = fmi->index;
sfi->meter_id = fmi->index;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index be3791ca6069..06c006a8b9b4 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -203,7 +203,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
*/
burst = roundup_pow_of_two(act->police.burst);
err = mlxsw_sp_acl_rulei_act_police(mlxsw_sp, rulei,
- act->police.index,
+ act->index,
act->police.rate_bytes_ps,
burst, extack);
if (err)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 3961461d9c8b..f6970213497a 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -197,6 +197,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
struct flow_action_entry {
enum flow_action_id id;
+ u32 index;
enum flow_action_hw_stats hw_stats;
action_destr destructor;
void *destructor_priv;
@@ -232,7 +233,6 @@ struct flow_action_entry {
bool truncate;
} sample;
struct { /* FLOW_ACTION_POLICE */
- u32 index;
u32 burst;
u64 rate_bytes_ps;
u64 burst_pkt;
@@ -267,7 +267,6 @@ struct flow_action_entry {
u8 ttl;
} mpls_mangle;
struct {
- u32 index;
s32 prio;
u64 basetime;
u64 cycletime;
diff --git a/include/net/tc_act/tc_gate.h b/include/net/tc_act/tc_gate.h
index 8bc6be81a7ad..c8fa11ebb397 100644
--- a/include/net/tc_act/tc_gate.h
+++ b/include/net/tc_act/tc_gate.h
@@ -60,11 +60,6 @@ static inline bool is_tcf_gate(const struct tc_action *a)
return false;
}
-static inline u32 tcf_gate_index(const struct tc_action *a)
-{
- return a->tcfa_index;
-}
-
static inline s32 tcf_gate_prio(const struct tc_action *a)
{
s32 tcfg_prio;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..d9d6ff0bf361 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3568,6 +3568,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
goto err_out_locked;
entry->hw_stats = tc_act_hw_stats(act->hw_stats);
+ entry->index = act->tcfa_index;
if (is_tcf_gact_ok(act)) {
entry->id = FLOW_ACTION_ACCEPT;
@@ -3659,7 +3660,6 @@ int tc_setup_flow_action(struct flow_action *flow_action,
entry->police.rate_pkt_ps =
tcf_police_rate_pkt_ps(act);
entry->police.mtu = tcf_police_tcfp_mtu(act);
- entry->police.index = act->tcfa_index;
} else if (is_tcf_ct(act)) {
entry->id = FLOW_ACTION_CT;
entry->ct.action = tcf_ct_action(act);
@@ -3697,7 +3697,6 @@ int tc_setup_flow_action(struct flow_action *flow_action,
entry->priority = tcf_skbedit_priority(act);
} else if (is_tcf_gate(act)) {
entry->id = FLOW_ACTION_GATE;
- entry->gate.index = tcf_gate_index(act);
entry->gate.prio = tcf_gate_prio(act);
entry->gate.basetime = tcf_gate_basetime(act);
entry->gate.cycletime = tcf_gate_cycletime(act);
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (2 preceding siblings ...)
2021-11-18 13:07 ` [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure Simon Horman
@ 2021-11-18 13:07 ` Simon Horman
2021-11-19 19:05 ` Vlad Buslov
2021-11-22 12:24 ` Jamal Hadi Salim
2021-11-18 13:08 ` [PATCH v4 05/10] flow_offload: add skip_hw and skip_sw to control if offload the action Simon Horman
` (6 subsequent siblings)
10 siblings, 2 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:07 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We need to call tc_cleanup_flow_action to clean up tc action entry since
in tc_setup_action, some actions may hold dev refcnt, especially the mirror
action.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
include/linux/netdevice.h | 1 +
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 12 +++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 164 +++++++++++++++++++++++++++++++++++++
net/sched/cls_api.c | 31 ++++++-
6 files changed, 256 insertions(+), 12 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4f4a299e92de..ae189fcff3c6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -916,6 +916,7 @@ enum tc_setup_type {
TC_SETUP_QDISC_TBF,
TC_SETUP_QDISC_FIFO,
TC_SETUP_QDISC_HTB,
+ TC_SETUP_ACT,
};
/* These structures hold the attributes of bpf state that are being passed
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index f6970213497a..15662cad5bca 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -551,6 +551,23 @@ struct flow_cls_offload {
u32 classid;
};
+enum flow_act_command {
+ FLOW_ACT_REPLACE,
+ FLOW_ACT_DESTROY,
+ FLOW_ACT_STATS,
+};
+
+struct flow_offload_action {
+ struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/
+ enum flow_act_command command;
+ enum flow_action_id id;
+ u32 index;
+ struct flow_stats stats;
+ struct flow_action action;
+};
+
+struct flow_offload_action *flow_action_alloc(unsigned int num_actions);
+
static inline struct flow_rule *
flow_cls_offload_flow_rule(struct flow_cls_offload *flow_cmd)
{
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 193f88ebf629..14d098a887d0 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -258,6 +258,14 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
for (; 0; (void)(i), (void)(a), (void)(exts))
#endif
+#define tcf_act_for_each_action(i, a, actions) \
+ for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
+
+static inline bool tc_act_bind(u32 flags)
+{
+ return !!(flags & TCA_ACT_FLAGS_BIND);
+}
+
static inline void
tcf_exts_stats_update(const struct tcf_exts *exts,
u64 bytes, u64 packets, u64 drops, u64 lastuse,
@@ -534,6 +542,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
int tc_setup_flow_action(struct flow_action *flow_action,
const struct tcf_exts *exts);
+
+int tc_setup_action(struct flow_action *flow_action,
+ struct tc_action *actions[]);
void tc_cleanup_flow_action(struct flow_action *flow_action);
int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
@@ -554,6 +565,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp,
enum tc_setup_type type, void *type_data,
void *cb_priv, u32 *flags, unsigned int *in_hw_count);
unsigned int tcf_exts_num_actions(struct tcf_exts *exts);
+unsigned int tcf_act_num_actions_single(struct tc_action *act);
#ifdef CONFIG_NET_CLS_ACT
int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 6beaea13564a..6676431733ef 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int num_actions)
}
EXPORT_SYMBOL(flow_rule_alloc);
+struct flow_offload_action *flow_action_alloc(unsigned int num_actions)
+{
+ struct flow_offload_action *fl_action;
+ int i;
+
+ fl_action = kzalloc(struct_size(fl_action, action.entries, num_actions),
+ GFP_KERNEL);
+ if (!fl_action)
+ return NULL;
+
+ fl_action->action.num_entries = num_actions;
+ /* Pre-fill each action hw_stats with DONT_CARE.
+ * Caller can override this if it wants stats for a given action.
+ */
+ for (i = 0; i < num_actions; i++)
+ fl_action->action.entries[i].hw_stats = FLOW_ACTION_HW_STATS_DONT_CARE;
+
+ return fl_action;
+}
+EXPORT_SYMBOL(flow_action_alloc);
+
#define FLOW_DISSECTOR_MATCH(__rule, __type, __out) \
const struct flow_match *__m = &(__rule)->match; \
struct flow_dissector *__d = (__m)->dissector; \
@@ -549,19 +570,25 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
void (*cleanup)(struct flow_block_cb *block_cb))
{
struct flow_indr_dev *this;
+ u32 count = 0;
+ int err;
mutex_lock(&flow_indr_block_lock);
+ if (bo) {
+ if (bo->command == FLOW_BLOCK_BIND)
+ indir_dev_add(data, dev, sch, type, cleanup, bo);
+ else if (bo->command == FLOW_BLOCK_UNBIND)
+ indir_dev_remove(data);
+ }
- if (bo->command == FLOW_BLOCK_BIND)
- indir_dev_add(data, dev, sch, type, cleanup, bo);
- else if (bo->command == FLOW_BLOCK_UNBIND)
- indir_dev_remove(data);
-
- list_for_each_entry(this, &flow_block_indr_dev_list, list)
- this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup);
+ list_for_each_entry(this, &flow_block_indr_dev_list, list) {
+ err = this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup);
+ if (!err)
+ count++;
+ }
mutex_unlock(&flow_indr_block_lock);
- return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
+ return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
}
EXPORT_SYMBOL(flow_indr_dev_setup_offload);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3258da3d5bed..c3d08b710661 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -21,6 +21,19 @@
#include <net/pkt_cls.h>
#include <net/act_api.h>
#include <net/netlink.h>
+#include <net/tc_act/tc_pedit.h>
+#include <net/tc_act/tc_mirred.h>
+#include <net/tc_act/tc_vlan.h>
+#include <net/tc_act/tc_tunnel_key.h>
+#include <net/tc_act/tc_csum.h>
+#include <net/tc_act/tc_gact.h>
+#include <net/tc_act/tc_police.h>
+#include <net/tc_act/tc_sample.h>
+#include <net/tc_act/tc_skbedit.h>
+#include <net/tc_act/tc_ct.h>
+#include <net/tc_act/tc_mpls.h>
+#include <net/tc_act/tc_gate.h>
+#include <net/flow_offload.h>
#ifdef CONFIG_INET
DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
@@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
kfree(p);
}
+static int flow_action_init(struct flow_offload_action *fl_action,
+ struct tc_action *act,
+ enum flow_act_command cmd,
+ struct netlink_ext_ack *extack)
+{
+ if (!fl_action)
+ return -EINVAL;
+
+ fl_action->extack = extack;
+ fl_action->command = cmd;
+ fl_action->index = act->tcfa_index;
+
+ if (is_tcf_gact_ok(act)) {
+ fl_action->id = FLOW_ACTION_ACCEPT;
+ } else if (is_tcf_gact_shot(act)) {
+ fl_action->id = FLOW_ACTION_DROP;
+ } else if (is_tcf_gact_trap(act)) {
+ fl_action->id = FLOW_ACTION_TRAP;
+ } else if (is_tcf_gact_goto_chain(act)) {
+ fl_action->id = FLOW_ACTION_GOTO;
+ } else if (is_tcf_mirred_egress_redirect(act)) {
+ fl_action->id = FLOW_ACTION_REDIRECT;
+ } else if (is_tcf_mirred_egress_mirror(act)) {
+ fl_action->id = FLOW_ACTION_MIRRED;
+ } else if (is_tcf_mirred_ingress_redirect(act)) {
+ fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
+ } else if (is_tcf_mirred_ingress_mirror(act)) {
+ fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
+ } else if (is_tcf_vlan(act)) {
+ switch (tcf_vlan_action(act)) {
+ case TCA_VLAN_ACT_PUSH:
+ fl_action->id = FLOW_ACTION_VLAN_PUSH;
+ break;
+ case TCA_VLAN_ACT_POP:
+ fl_action->id = FLOW_ACTION_VLAN_POP;
+ break;
+ case TCA_VLAN_ACT_MODIFY:
+ fl_action->id = FLOW_ACTION_VLAN_MANGLE;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ } else if (is_tcf_tunnel_set(act)) {
+ fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
+ } else if (is_tcf_tunnel_release(act)) {
+ fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
+ } else if (is_tcf_csum(act)) {
+ fl_action->id = FLOW_ACTION_CSUM;
+ } else if (is_tcf_skbedit_mark(act)) {
+ fl_action->id = FLOW_ACTION_MARK;
+ } else if (is_tcf_sample(act)) {
+ fl_action->id = FLOW_ACTION_SAMPLE;
+ } else if (is_tcf_police(act)) {
+ fl_action->id = FLOW_ACTION_POLICE;
+ } else if (is_tcf_ct(act)) {
+ fl_action->id = FLOW_ACTION_CT;
+ } else if (is_tcf_mpls(act)) {
+ switch (tcf_mpls_action(act)) {
+ case TCA_MPLS_ACT_PUSH:
+ fl_action->id = FLOW_ACTION_MPLS_PUSH;
+ break;
+ case TCA_MPLS_ACT_POP:
+ fl_action->id = FLOW_ACTION_MPLS_POP;
+ break;
+ case TCA_MPLS_ACT_MODIFY:
+ fl_action->id = FLOW_ACTION_MPLS_MANGLE;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ } else if (is_tcf_skbedit_ptype(act)) {
+ fl_action->id = FLOW_ACTION_PTYPE;
+ } else if (is_tcf_skbedit_priority(act)) {
+ fl_action->id = FLOW_ACTION_PRIORITY;
+ } else if (is_tcf_gate(act)) {
+ fl_action->id = FLOW_ACTION_GATE;
+ } else {
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
+ struct netlink_ext_ack *extack)
+{
+ int err;
+
+ if (IS_ERR(fl_act))
+ return PTR_ERR(fl_act);
+
+ err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
+ fl_act, NULL, NULL);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
+/* offload the tc command after inserted */
+static int tcf_action_offload_add(struct tc_action *action,
+ struct netlink_ext_ack *extack)
+{
+ struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
+ [0] = action,
+ };
+ struct flow_offload_action *fl_action;
+ int err = 0;
+
+ fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
+ if (!fl_action)
+ return -ENOMEM;
+
+ err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack);
+ if (err)
+ goto fl_err;
+
+ err = tc_setup_action(&fl_action->action, actions);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Failed to setup tc actions for offload\n");
+ goto fl_err;
+ }
+
+ err = tcf_action_offload_cmd(fl_action, extack);
+ tc_cleanup_flow_action(&fl_action->action);
+
+fl_err:
+ kfree(fl_action);
+
+ return err;
+}
+
+static int tcf_action_offload_del(struct tc_action *action)
+{
+ struct flow_offload_action fl_act;
+ int err = 0;
+
+ if (!action)
+ return -EINVAL;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
+ if (err)
+ return err;
+
+ return tcf_action_offload_cmd(&fl_act, NULL);
+}
+
static void tcf_action_cleanup(struct tc_action *p)
{
+ tcf_action_offload_del(p);
if (p->ops->cleanup)
p->ops->cleanup(p);
@@ -1103,6 +1265,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!tc_act_bind(flags))
+ tcf_action_offload_add(act, extack);
}
/* We have to commit them all together, because if any error happened in
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d9d6ff0bf361..55fa48999d43 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats)
return hw_stats;
}
-int tc_setup_flow_action(struct flow_action *flow_action,
- const struct tcf_exts *exts)
+int tc_setup_action(struct flow_action *flow_action,
+ struct tc_action *actions[])
{
struct tc_action *act;
int i, j, k, err = 0;
@@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action *flow_action,
BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE);
BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED != FLOW_ACTION_HW_STATS_DELAYED);
- if (!exts)
+ if (!actions)
return 0;
j = 0;
- tcf_exts_for_each_action(i, act, exts) {
+ tcf_act_for_each_action(i, act, actions) {
struct flow_action_entry *entry;
entry = &flow_action->entries[j];
@@ -3724,6 +3724,20 @@ int tc_setup_flow_action(struct flow_action *flow_action,
spin_unlock_bh(&act->tcfa_lock);
goto err_out;
}
+EXPORT_SYMBOL(tc_setup_action);
+
+int tc_setup_flow_action(struct flow_action *flow_action,
+ const struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+ if (!exts)
+ return 0;
+
+ return tc_setup_action(flow_action, exts->actions);
+#else
+ return 0;
+#endif
+}
EXPORT_SYMBOL(tc_setup_flow_action);
unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
@@ -3742,6 +3756,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
}
EXPORT_SYMBOL(tcf_exts_num_actions);
+unsigned int tcf_act_num_actions_single(struct tc_action *act)
+{
+ if (is_tcf_pedit(act))
+ return tcf_pedit_nkeys(act);
+ else
+ return 1;
+}
+EXPORT_SYMBOL(tcf_act_num_actions_single);
+
#ifdef CONFIG_NET_CLS_ACT
static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
u32 *p_block_index,
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 05/10] flow_offload: add skip_hw and skip_sw to control if offload the action
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (3 preceding siblings ...)
2021-11-18 13:07 ` [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-11-18 13:08 ` Simon Horman
2021-11-18 13:08 ` [PATCH v4 06/10] flow_offload: add process to update action stats from hardware Simon Horman
` (5 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
We add skip_hw and skip_sw for user to control if offload the action
to hardware.
We also add in_hw_count for user to indicate if the action is offloaded
to any hardware.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
include/net/act_api.h | 7 +++++
include/net/pkt_cls.h | 23 +++++++++++++++
include/uapi/linux/pkt_cls.h | 9 ++++--
net/sched/act_api.c | 54 ++++++++++++++++++++++++++++++++----
4 files changed, 84 insertions(+), 9 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index b5b624c7e488..68d6f245f7e9 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -44,6 +44,7 @@ struct tc_action {
u8 hw_stats;
u8 used_hw_stats;
bool used_hw_stats_valid;
+ u32 in_hw_count;
};
#define tcf_index common.tcfa_index
#define tcf_refcnt common.tcfa_refcnt
@@ -236,6 +237,12 @@ static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
spin_unlock(&a->tcfa_lock);
}
+static inline void flow_action_hw_count_set(struct tc_action *act,
+ u32 hw_count)
+{
+ act->in_hw_count = hw_count;
+}
+
void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
u64 drops, bool hw);
int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 14d098a887d0..b00fd421e7c0 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -261,6 +261,29 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
#define tcf_act_for_each_action(i, a, actions) \
for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
+static inline bool tc_act_skip_hw(u32 flags)
+{
+ return (flags & TCA_ACT_FLAGS_SKIP_HW) ? true : false;
+}
+
+static inline bool tc_act_skip_sw(u32 flags)
+{
+ return (flags & TCA_ACT_FLAGS_SKIP_SW) ? true : false;
+}
+
+static inline bool tc_act_in_hw(struct tc_action *act)
+{
+ return !!act->in_hw_count;
+}
+
+/* SKIP_HW and SKIP_SW are mutually exclusive flags. */
+static inline bool tc_act_flags_valid(u32 flags)
+{
+ flags &= TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW;
+
+ return flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW);
+}
+
static inline bool tc_act_bind(u32 flags)
{
return !!(flags & TCA_ACT_FLAGS_BIND);
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 6836ccb9c45d..ee38b35c3f57 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -19,13 +19,16 @@ enum {
TCA_ACT_FLAGS,
TCA_ACT_HW_STATS,
TCA_ACT_USED_HW_STATS,
+ TCA_ACT_IN_HW_COUNT,
__TCA_ACT_MAX
};
/* See other TCA_ACT_FLAGS_ * flags in include/net/act_api.h. */
-#define TCA_ACT_FLAGS_NO_PERCPU_STATS 1 /* Don't use percpu allocator for
- * actions stats.
- */
+#define TCA_ACT_FLAGS_NO_PERCPU_STATS (1 << 0) /* Don't use percpu allocator for
+ * actions stats.
+ */
+#define TCA_ACT_FLAGS_SKIP_HW (1 << 1) /* don't offload action to HW */
+#define TCA_ACT_FLAGS_SKIP_SW (1 << 2) /* don't use action in SW */
/* tca HW stats type
* When user does not pass the attribute, he does not care.
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index c3d08b710661..29fba4fa1616 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -226,6 +226,7 @@ static int flow_action_init(struct flow_offload_action *fl_action,
}
static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
+ u32 *hw_count,
struct netlink_ext_ack *extack)
{
int err;
@@ -238,6 +239,9 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
if (err < 0)
return err;
+ if (hw_count)
+ *hw_count = err;
+
return 0;
}
@@ -245,12 +249,17 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
static int tcf_action_offload_add(struct tc_action *action,
struct netlink_ext_ack *extack)
{
+ bool skip_sw = tc_act_skip_sw(action->tcfa_flags);
struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
[0] = action,
};
struct flow_offload_action *fl_action;
+ u32 in_hw_count = 0;
int err = 0;
+ if (tc_act_skip_hw(action->tcfa_flags))
+ return 0;
+
fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
if (!fl_action)
return -ENOMEM;
@@ -266,7 +275,13 @@ static int tcf_action_offload_add(struct tc_action *action,
goto fl_err;
}
- err = tcf_action_offload_cmd(fl_action, extack);
+ err = tcf_action_offload_cmd(fl_action, &in_hw_count, extack);
+ if (!err)
+ flow_action_hw_count_set(action, in_hw_count);
+
+ if (skip_sw && !tc_act_in_hw(action))
+ err = -EINVAL;
+
tc_cleanup_flow_action(&fl_action->action);
fl_err:
@@ -278,16 +293,27 @@ static int tcf_action_offload_add(struct tc_action *action,
static int tcf_action_offload_del(struct tc_action *action)
{
struct flow_offload_action fl_act;
+ u32 in_hw_count = 0;
int err = 0;
if (!action)
return -EINVAL;
+ if (!tc_act_in_hw(action))
+ return 0;
+
err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
if (err)
return err;
- return tcf_action_offload_cmd(&fl_act, NULL);
+ err = tcf_action_offload_cmd(&fl_act, &in_hw_count, NULL);
+ if (err)
+ return err;
+
+ if (action->in_hw_count != in_hw_count)
+ return -EINVAL;
+
+ return 0;
}
static void tcf_action_cleanup(struct tc_action *p)
@@ -897,6 +923,9 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
jmp_prgcnt -= 1;
continue;
}
+
+ if (tc_act_skip_sw(a->tcfa_flags))
+ continue;
repeat:
ret = a->ops->act(skb, a, res);
if (ret == TC_ACT_REPEAT)
@@ -1002,6 +1031,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
a->tcfa_flags, a->tcfa_flags))
goto nla_put_failure;
+ if (nla_put_u32(skb, TCA_ACT_IN_HW_COUNT, a->in_hw_count))
+ goto nla_put_failure;
+
nest = nla_nest_start_noflag(skb, TCA_OPTIONS);
if (nest == NULL)
goto nla_put_failure;
@@ -1081,7 +1113,9 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
[TCA_ACT_COOKIE] = { .type = NLA_BINARY,
.len = TC_COOKIE_MAX_SIZE },
[TCA_ACT_OPTIONS] = { .type = NLA_NESTED },
- [TCA_ACT_FLAGS] = NLA_POLICY_BITFIELD32(TCA_ACT_FLAGS_NO_PERCPU_STATS),
+ [TCA_ACT_FLAGS] = NLA_POLICY_BITFIELD32(TCA_ACT_FLAGS_NO_PERCPU_STATS |
+ TCA_ACT_FLAGS_SKIP_HW |
+ TCA_ACT_FLAGS_SKIP_SW),
[TCA_ACT_HW_STATS] = NLA_POLICY_BITFIELD32(TCA_ACT_HW_STATS_ANY),
};
@@ -1194,8 +1228,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
}
}
hw_stats = tcf_action_hw_stats_get(tb[TCA_ACT_HW_STATS]);
- if (tb[TCA_ACT_FLAGS])
+ if (tb[TCA_ACT_FLAGS]) {
userflags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]);
+ if (!tc_act_flags_valid(userflags.value)) {
+ err = -EINVAL;
+ goto err_out;
+ }
+ }
err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp,
userflags.value | flags, extack);
@@ -1265,8 +1304,11 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
- if (!tc_act_bind(flags))
- tcf_action_offload_add(act, extack);
+ if (!tc_act_bind(flags)) {
+ err = tcf_action_offload_add(act, extack);
+ if (tc_act_skip_sw(act->tcfa_flags) && err)
+ goto err;
+ }
}
/* We have to commit them all together, because if any error happened in
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 06/10] flow_offload: add process to update action stats from hardware
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (4 preceding siblings ...)
2021-11-18 13:08 ` [PATCH v4 05/10] flow_offload: add skip_hw and skip_sw to control if offload the action Simon Horman
@ 2021-11-18 13:08 ` Simon Horman
2021-11-18 13:08 ` [PATCH v4 07/10] net: sched: save full flags for tc action Simon Horman
` (4 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
When collecting stats for actions update them using both
hardware and software counters.
Stats update process should not run in context of preempt_disable.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
include/net/act_api.h | 1 +
include/net/pkt_cls.h | 18 ++++++++++--------
net/sched/act_api.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 68d6f245f7e9..7900598d2dd3 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -247,6 +247,7 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
u64 drops, bool hw);
int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
+int tcf_action_update_hw_stats(struct tc_action *action);
int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
struct tcf_chain **handle,
struct netlink_ext_ack *newchain);
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index b00fd421e7c0..c8e1aac82752 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -297,18 +297,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts,
#ifdef CONFIG_NET_CLS_ACT
int i;
- preempt_disable();
-
for (i = 0; i < exts->nr_actions; i++) {
struct tc_action *a = exts->actions[i];
- tcf_action_stats_update(a, bytes, packets, drops,
- lastuse, true);
- a->used_hw_stats = used_hw_stats;
- a->used_hw_stats_valid = used_hw_stats_valid;
- }
+ /* 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();
- preempt_enable();
+ a->used_hw_stats = used_hw_stats;
+ a->used_hw_stats_valid = used_hw_stats_valid;
+ }
+ }
#endif
}
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 29fba4fa1616..01f0bed9c399 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -290,6 +290,37 @@ static int tcf_action_offload_add(struct tc_action *action,
return err;
}
+int tcf_action_update_hw_stats(struct tc_action *action)
+{
+ struct flow_offload_action fl_act = {};
+ int err;
+
+ if (!tc_act_in_hw(action))
+ return -EOPNOTSUPP;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
+ if (err)
+ return err;
+
+ err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
+ if (!err) {
+ preempt_disable();
+ tcf_action_stats_update(action, fl_act.stats.bytes,
+ fl_act.stats.pkts,
+ fl_act.stats.drops,
+ fl_act.stats.lastused,
+ true);
+ preempt_enable();
+ action->used_hw_stats = fl_act.stats.used_hw_stats;
+ action->used_hw_stats_valid = true;
+ } else {
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(tcf_action_update_hw_stats);
+
static int tcf_action_offload_del(struct tc_action *action)
{
struct flow_offload_action fl_act;
@@ -1360,6 +1391,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
if (p == NULL)
goto errout;
+ /* update hw stats for this action */
+ tcf_action_update_hw_stats(p);
+
/* compat_mode being true specifies a call that is supposed
* to add additional backward compatibility statistic TLVs.
*/
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 07/10] net: sched: save full flags for tc action
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (5 preceding siblings ...)
2021-11-18 13:08 ` [PATCH v4 06/10] flow_offload: add process to update action stats from hardware Simon Horman
@ 2021-11-18 13:08 ` Simon Horman
2021-11-18 13:08 ` [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count Simon Horman
` (3 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Save full action flags and return user flags when return flags to
user space.
Save full action flags to distinguish if the action is created
independent from classifier.
We made this change mainly for further patch to reoffload tc actions.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
net/sched/act_api.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 01f0bed9c399..f5834d47a392 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -716,7 +716,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
p->tcfa_tm.install = jiffies;
p->tcfa_tm.lastuse = jiffies;
p->tcfa_tm.firstuse = 0;
- p->tcfa_flags = flags & TCA_ACT_FLAGS_USER_MASK;
+ p->tcfa_flags = flags;
if (est) {
err = gen_new_estimator(&p->tcfa_bstats, p->cpu_bstats,
&p->tcfa_rate_est,
@@ -1043,6 +1043,7 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
int err = -EINVAL;
unsigned char *b = skb_tail_pointer(skb);
struct nlattr *nest;
+ u32 flags;
if (tcf_action_dump_terse(skb, a, false))
goto nla_put_failure;
@@ -1057,9 +1058,10 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
a->used_hw_stats, TCA_ACT_HW_STATS_ANY))
goto nla_put_failure;
- if (a->tcfa_flags &&
+ flags = a->tcfa_flags & TCA_ACT_FLAGS_USER_MASK;
+ if (flags &&
nla_put_bitfield32(skb, TCA_ACT_FLAGS,
- a->tcfa_flags, a->tcfa_flags))
+ flags, flags))
goto nla_put_failure;
if (nla_put_u32(skb, TCA_ACT_IN_HW_COUNT, a->in_hw_count))
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (6 preceding siblings ...)
2021-11-18 13:08 ` [PATCH v4 07/10] net: sched: save full flags for tc action Simon Horman
@ 2021-11-18 13:08 ` Simon Horman
2021-11-19 20:09 ` Vlad Buslov
2021-11-18 13:08 ` [PATCH v4 09/10] flow_offload: validate flags of filter and actions Simon Horman
` (2 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Add reoffload process to update hw_count when driver
is inserted or removed.
When reoffloading actions, we still offload the actions
that are added independent of filters.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
include/net/act_api.h | 24 +++++
net/core/flow_offload.c | 4 +
net/sched/act_api.c | 213 ++++++++++++++++++++++++++++++++++++----
3 files changed, 222 insertions(+), 19 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 7900598d2dd3..e5e6e58df618 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -7,6 +7,7 @@
*/
#include <linux/refcount.h>
+#include <net/flow_offload.h>
#include <net/sch_generic.h>
#include <net/pkt_sched.h>
#include <net/net_namespace.h>
@@ -243,11 +244,26 @@ static inline void flow_action_hw_count_set(struct tc_action *act,
act->in_hw_count = hw_count;
}
+static inline void flow_action_hw_count_inc(struct tc_action *act,
+ u32 hw_count)
+{
+ act->in_hw_count += hw_count;
+}
+
+static inline void flow_action_hw_count_dec(struct tc_action *act,
+ u32 hw_count)
+{
+ act->in_hw_count = act->in_hw_count > hw_count ?
+ act->in_hw_count - hw_count : 0;
+}
+
void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
u64 drops, bool hw);
int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
int tcf_action_update_hw_stats(struct tc_action *action);
+int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
+ void *cb_priv, bool add);
int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
struct tcf_chain **handle,
struct netlink_ext_ack *newchain);
@@ -259,6 +275,14 @@ DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
#endif
int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
+
+#else /* !CONFIG_NET_CLS_ACT */
+
+static inline int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
+ void *cb_priv, bool add) {
+ return 0;
+}
+
#endif /* CONFIG_NET_CLS_ACT */
static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 6676431733ef..92000164ac37 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include <linux/kernel.h>
#include <linux/slab.h>
+#include <net/act_api.h>
#include <net/flow_offload.h>
#include <linux/rtnetlink.h>
#include <linux/mutex.h>
@@ -418,6 +419,8 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
existing_qdiscs_register(cb, cb_priv);
mutex_unlock(&flow_indr_block_lock);
+ tcf_action_reoffload_cb(cb, cb_priv, true);
+
return 0;
}
EXPORT_SYMBOL(flow_indr_dev_register);
@@ -470,6 +473,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
__flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
mutex_unlock(&flow_indr_block_lock);
+ tcf_action_reoffload_cb(cb, cb_priv, false);
flow_block_indr_notify(&cleanup_list);
kfree(indr_dev);
}
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f5834d47a392..ada51b2df851 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -225,15 +225,11 @@ static int flow_action_init(struct flow_offload_action *fl_action,
return 0;
}
-static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
- u32 *hw_count,
- struct netlink_ext_ack *extack)
+static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
+ u32 *hw_count)
{
int err;
- if (IS_ERR(fl_act))
- return PTR_ERR(fl_act);
-
err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
fl_act, NULL, NULL);
if (err < 0)
@@ -245,9 +241,41 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
return 0;
}
+static int tcf_action_offload_cmd_cb_ex(struct flow_offload_action *fl_act,
+ u32 *hw_count,
+ flow_indr_block_bind_cb_t *cb,
+ void *cb_priv)
+{
+ int err;
+
+ err = cb(NULL, NULL, cb_priv, TC_SETUP_ACT, NULL, fl_act, NULL);
+ if (err < 0)
+ return err;
+
+ if (hw_count)
+ *hw_count = 1;
+
+ return 0;
+}
+
+static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
+ u32 *hw_count,
+ flow_indr_block_bind_cb_t *cb,
+ void *cb_priv)
+{
+ if (IS_ERR(fl_act))
+ return PTR_ERR(fl_act);
+
+ return cb ? tcf_action_offload_cmd_cb_ex(fl_act, hw_count,
+ cb, cb_priv) :
+ tcf_action_offload_cmd_ex(fl_act, hw_count);
+}
+
/* offload the tc command after inserted */
-static int tcf_action_offload_add(struct tc_action *action,
- struct netlink_ext_ack *extack)
+static int tcf_action_offload_add_ex(struct tc_action *action,
+ struct netlink_ext_ack *extack,
+ flow_indr_block_bind_cb_t *cb,
+ void *cb_priv)
{
bool skip_sw = tc_act_skip_sw(action->tcfa_flags);
struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
@@ -275,9 +303,10 @@ static int tcf_action_offload_add(struct tc_action *action,
goto fl_err;
}
- err = tcf_action_offload_cmd(fl_action, &in_hw_count, extack);
+ err = tcf_action_offload_cmd(fl_action, &in_hw_count, cb, cb_priv);
if (!err)
- flow_action_hw_count_set(action, in_hw_count);
+ cb ? flow_action_hw_count_inc(action, in_hw_count) :
+ flow_action_hw_count_set(action, in_hw_count);
if (skip_sw && !tc_act_in_hw(action))
err = -EINVAL;
@@ -290,6 +319,12 @@ static int tcf_action_offload_add(struct tc_action *action,
return err;
}
+static int tcf_action_offload_add(struct tc_action *action,
+ struct netlink_ext_ack *extack)
+{
+ return tcf_action_offload_add_ex(action, extack, NULL, NULL);
+}
+
int tcf_action_update_hw_stats(struct tc_action *action)
{
struct flow_offload_action fl_act = {};
@@ -302,7 +337,7 @@ int tcf_action_update_hw_stats(struct tc_action *action)
if (err)
return err;
- err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
+ err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
if (!err) {
preempt_disable();
tcf_action_stats_update(action, fl_act.stats.bytes,
@@ -321,7 +356,9 @@ int tcf_action_update_hw_stats(struct tc_action *action)
}
EXPORT_SYMBOL(tcf_action_update_hw_stats);
-static int tcf_action_offload_del(struct tc_action *action)
+static int tcf_action_offload_del_ex(struct tc_action *action,
+ flow_indr_block_bind_cb_t *cb,
+ void *cb_priv)
{
struct flow_offload_action fl_act;
u32 in_hw_count = 0;
@@ -337,16 +374,25 @@ static int tcf_action_offload_del(struct tc_action *action)
if (err)
return err;
- err = tcf_action_offload_cmd(&fl_act, &in_hw_count, NULL);
- if (err)
+ err = tcf_action_offload_cmd(&fl_act, &in_hw_count, cb, cb_priv);
+ if (err < 0)
return err;
- if (action->in_hw_count != in_hw_count)
+ if (!cb && action->in_hw_count != in_hw_count)
return -EINVAL;
+ /* do not need to update hw state when deleting action */
+ if (cb && in_hw_count)
+ flow_action_hw_count_dec(action, in_hw_count);
+
return 0;
}
+static int tcf_action_offload_del(struct tc_action *action)
+{
+ return tcf_action_offload_del_ex(action, NULL, NULL);
+}
+
static void tcf_action_cleanup(struct tc_action *p)
{
tcf_action_offload_del(p);
@@ -841,6 +887,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);
static LIST_HEAD(act_base);
static DEFINE_RWLOCK(act_mod_lock);
+/* since act ops id is stored in pernet subsystem list,
+ * then there is no way to walk through only all the action
+ * subsystem, so we keep tc action pernet ops id for
+ * reoffload to walk through.
+ */
+static LIST_HEAD(act_pernet_id_list);
+static DEFINE_MUTEX(act_id_mutex);
+struct tc_act_pernet_id {
+ struct list_head list;
+ unsigned int id;
+};
+
+static int tcf_pernet_add_id_list(unsigned int id)
+{
+ struct tc_act_pernet_id *id_ptr;
+ int ret = 0;
+
+ mutex_lock(&act_id_mutex);
+ list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
+ if (id_ptr->id == id) {
+ ret = -EEXIST;
+ goto err_out;
+ }
+ }
+
+ id_ptr = kzalloc(sizeof(*id_ptr), GFP_KERNEL);
+ if (!id_ptr) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+ id_ptr->id = id;
+
+ list_add_tail(&id_ptr->list, &act_pernet_id_list);
+
+err_out:
+ mutex_unlock(&act_id_mutex);
+ return ret;
+}
+
+static void tcf_pernet_del_id_list(unsigned int id)
+{
+ struct tc_act_pernet_id *id_ptr;
+
+ mutex_lock(&act_id_mutex);
+ list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
+ if (id_ptr->id == id) {
+ list_del(&id_ptr->list);
+ kfree(id_ptr);
+ break;
+ }
+ }
+ mutex_unlock(&act_id_mutex);
+}
int tcf_register_action(struct tc_action_ops *act,
struct pernet_operations *ops)
@@ -859,18 +958,30 @@ int tcf_register_action(struct tc_action_ops *act,
if (ret)
return ret;
+ if (ops->id) {
+ ret = tcf_pernet_add_id_list(*ops->id);
+ if (ret)
+ goto id_err;
+ }
+
write_lock(&act_mod_lock);
list_for_each_entry(a, &act_base, head) {
if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
- write_unlock(&act_mod_lock);
- unregister_pernet_subsys(ops);
- return -EEXIST;
+ ret = -EEXIST;
+ goto err_out;
}
}
list_add_tail(&act->head, &act_base);
write_unlock(&act_mod_lock);
return 0;
+
+err_out:
+ write_unlock(&act_mod_lock);
+ tcf_pernet_del_id_list(*ops->id);
+id_err:
+ unregister_pernet_subsys(ops);
+ return ret;
}
EXPORT_SYMBOL(tcf_register_action);
@@ -889,12 +1000,76 @@ int tcf_unregister_action(struct tc_action_ops *act,
}
}
write_unlock(&act_mod_lock);
- if (!err)
+ if (!err) {
unregister_pernet_subsys(ops);
+ if (ops->id)
+ tcf_pernet_del_id_list(*ops->id);
+ }
return err;
}
EXPORT_SYMBOL(tcf_unregister_action);
+int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
+ void *cb_priv, bool add)
+{
+ struct tc_act_pernet_id *id_ptr;
+ struct tcf_idrinfo *idrinfo;
+ struct tc_action_net *tn;
+ struct tc_action *p;
+ unsigned int act_id;
+ unsigned long tmp;
+ unsigned long id;
+ struct idr *idr;
+ struct net *net;
+ int ret;
+
+ if (!cb)
+ return -EINVAL;
+
+ down_read(&net_rwsem);
+ mutex_lock(&act_id_mutex);
+
+ for_each_net(net) {
+ list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
+ act_id = id_ptr->id;
+ tn = net_generic(net, act_id);
+ if (!tn)
+ continue;
+ idrinfo = tn->idrinfo;
+ if (!idrinfo)
+ continue;
+
+ mutex_lock(&idrinfo->lock);
+ idr = &idrinfo->action_idr;
+ idr_for_each_entry_ul(idr, p, tmp, id) {
+ if (IS_ERR(p) || tc_act_bind(p->tcfa_flags))
+ continue;
+ if (add) {
+ tcf_action_offload_add_ex(p, NULL, cb,
+ cb_priv);
+ continue;
+ }
+
+ /* cb unregister to update hw count */
+ ret = tcf_action_offload_del_ex(p, cb, cb_priv);
+ if (ret < 0)
+ continue;
+ if (tc_act_skip_sw(p->tcfa_flags) &&
+ !tc_act_in_hw(p)) {
+ ret = tcf_idr_release_unsafe(p);
+ if (ret == ACT_P_DELETED)
+ module_put(p->ops->owner);
+ }
+ }
+ mutex_unlock(&idrinfo->lock);
+ }
+ }
+ mutex_unlock(&act_id_mutex);
+ up_read(&net_rwsem);
+
+ return 0;
+}
+
/* lookup by name */
static struct tc_action_ops *tc_lookup_action_n(char *kind)
{
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 09/10] flow_offload: validate flags of filter and actions
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (7 preceding siblings ...)
2021-11-18 13:08 ` [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count Simon Horman
@ 2021-11-18 13:08 ` Simon Horman
2021-11-18 13:08 ` [PATCH v4 10/10] selftests: tc-testing: add action offload selftest for action and filter Simon Horman
2021-11-22 12:17 ` [PATCH v4 net-next 0/10] allow user to offload tc action to net device Jamal Hadi Salim
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Add process to validate flags of filter and actions when adding
a tc filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
include/net/act_api.h | 2 +-
include/net/pkt_cls.h | 3 +++
net/sched/act_api.c | 18 +++++++++++++++---
net/sched/cls_api.c | 18 ++++++++++++++----
net/sched/cls_flower.c | 9 ++++++---
net/sched/cls_matchall.c | 9 +++++----
net/sched/cls_u32.c | 12 +++++++-----
7 files changed, 51 insertions(+), 20 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index e5e6e58df618..10cac4486a1c 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -191,7 +191,7 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est,
struct tc_action *actions[], int init_res[], size_t *attr_size,
- u32 flags, struct netlink_ext_ack *extack);
+ u32 flags, u32 fl_flags, struct netlink_ext_ack *extack);
struct tc_action_ops *tc_action_load_ops(struct nlattr *nla, bool police,
bool rtnl_held,
struct netlink_ext_ack *extack);
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index c8e1aac82752..94439d0521e3 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -354,6 +354,9 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
struct nlattr **tb, struct nlattr *rate_tlv,
struct tcf_exts *exts, u32 flags,
struct netlink_ext_ack *extack);
+int tcf_exts_validate_ex(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
+ struct nlattr *rate_tlv, struct tcf_exts *exts,
+ u32 flags, u32 fl_flags, struct netlink_ext_ack *extack);
void tcf_exts_destroy(struct tcf_exts *exts);
void tcf_exts_change(struct tcf_exts *dst, struct tcf_exts *src);
int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index ada51b2df851..23ef884c1681 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1474,7 +1474,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est, struct tc_action *actions[],
- int init_res[], size_t *attr_size, u32 flags,
+ int init_res[], size_t *attr_size,
+ u32 flags, u32 fl_flags,
struct netlink_ext_ack *extack)
{
struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {};
@@ -1512,7 +1513,18 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
- if (!tc_act_bind(flags)) {
+ if (tc_act_bind(flags)) {
+ bool skip_sw = tc_skip_sw(fl_flags);
+ bool skip_hw = tc_skip_hw(fl_flags);
+
+ if (tc_act_bind(act->tcfa_flags))
+ continue;
+ if (skip_sw != tc_act_skip_sw(act->tcfa_flags) ||
+ skip_hw != tc_act_skip_hw(act->tcfa_flags)) {
+ err = -EINVAL;
+ goto err;
+ }
+ } else {
err = tcf_action_offload_add(act, extack);
if (tc_act_skip_sw(act->tcfa_flags) && err)
goto err;
@@ -1925,7 +1937,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
for (loop = 0; loop < 10; loop++) {
ret = tcf_action_init(net, NULL, nla, NULL, actions, init_res,
- &attr_size, flags, extack);
+ &attr_size, flags, 0, extack);
if (ret != -EAGAIN)
break;
}
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 55fa48999d43..e0426202215e 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3025,9 +3025,9 @@ void tcf_exts_destroy(struct tcf_exts *exts)
}
EXPORT_SYMBOL(tcf_exts_destroy);
-int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
- struct nlattr *rate_tlv, struct tcf_exts *exts,
- u32 flags, struct netlink_ext_ack *extack)
+int tcf_exts_validate_ex(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
+ struct nlattr *rate_tlv, struct tcf_exts *exts,
+ u32 flags, u32 fl_flags, struct netlink_ext_ack *extack)
{
#ifdef CONFIG_NET_CLS_ACT
{
@@ -3061,7 +3061,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
flags |= TCA_ACT_FLAGS_BIND;
err = tcf_action_init(net, tp, tb[exts->action],
rate_tlv, exts->actions, init_res,
- &attr_size, flags, extack);
+ &attr_size, flags, fl_flags,
+ extack);
if (err < 0)
return err;
exts->nr_actions = err;
@@ -3077,6 +3078,15 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
return 0;
}
+EXPORT_SYMBOL(tcf_exts_validate_ex);
+
+int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
+ struct nlattr *rate_tlv, struct tcf_exts *exts,
+ u32 flags, struct netlink_ext_ack *extack)
+{
+ return tcf_exts_validate_ex(net, tp, tb, rate_tlv, exts,
+ flags, 0, extack);
+}
EXPORT_SYMBOL(tcf_exts_validate);
void tcf_exts_change(struct tcf_exts *dst, struct tcf_exts *src)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index aab13ba11767..c3a104832a17 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1917,12 +1917,14 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
struct cls_fl_filter *f, struct fl_flow_mask *mask,
unsigned long base, struct nlattr **tb,
struct nlattr *est,
- struct fl_flow_tmplt *tmplt, u32 flags,
+ struct fl_flow_tmplt *tmplt,
+ u32 flags, u32 fl_flags,
struct netlink_ext_ack *extack)
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &f->exts, flags, extack);
+ err = tcf_exts_validate_ex(net, tp, tb, est, &f->exts, flags,
+ fl_flags, extack);
if (err < 0)
return err;
@@ -2036,7 +2038,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
}
err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE],
- tp->chain->tmplt_priv, flags, extack);
+ tp->chain->tmplt_priv, flags, fnew->flags,
+ extack);
if (err)
goto errout;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 24f0046ce0b3..a0c2a81d5762 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -163,12 +163,13 @@ static const struct nla_policy mall_policy[TCA_MATCHALL_MAX + 1] = {
static int mall_set_parms(struct net *net, struct tcf_proto *tp,
struct cls_mall_head *head,
unsigned long base, struct nlattr **tb,
- struct nlattr *est, u32 flags,
+ struct nlattr *est, u32 flags, u32 fl_flags,
struct netlink_ext_ack *extack)
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &head->exts, flags, extack);
+ err = tcf_exts_validate_ex(net, tp, tb, est, &head->exts, flags,
+ fl_flags, extack);
if (err < 0)
return err;
@@ -226,8 +227,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
goto err_alloc_percpu;
}
- err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE], flags,
- extack);
+ err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE],
+ flags, new->flags, extack);
if (err)
goto err_set_parms;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4272814487f0..cf5649292ee0 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -709,12 +709,13 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
static int u32_set_parms(struct net *net, struct tcf_proto *tp,
unsigned long base,
struct tc_u_knode *n, struct nlattr **tb,
- struct nlattr *est, u32 flags,
+ struct nlattr *est, u32 flags, u32 fl_flags,
struct netlink_ext_ack *extack)
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &n->exts, flags, extack);
+ err = tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flags,
+ fl_flags, extack);
if (err < 0)
return err;
@@ -895,7 +896,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return -ENOMEM;
err = u32_set_parms(net, tp, base, new, tb,
- tca[TCA_RATE], flags, extack);
+ tca[TCA_RATE], flags, new->flags,
+ extack);
if (err) {
u32_destroy_key(new, false);
@@ -1060,8 +1062,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
}
#endif
- err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], flags,
- extack);
+ err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE],
+ flags, n->flags, extack);
if (err == 0) {
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 10/10] selftests: tc-testing: add action offload selftest for action and filter
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (8 preceding siblings ...)
2021-11-18 13:08 ` [PATCH v4 09/10] flow_offload: validate flags of filter and actions Simon Horman
@ 2021-11-18 13:08 ` Simon Horman
2021-11-22 12:17 ` [PATCH v4 net-next 0/10] allow user to offload tc action to net device Jamal Hadi Salim
10 siblings, 0 replies; 30+ messages in thread
From: Simon Horman @ 2021-11-18 13:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko, Oz Shlomo,
Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
From: Baowen Zheng <baowen.zheng@corigine.com>
Add selftest cases in action police with skip_hw.
Add selftest case to validate flags of filter and action.
These tests depend on corresponding iproute2 command support.
Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
.../tc-testing/tc-tests/actions/police.json | 24 +++++++++++++++++++
.../tc-testing/tc-tests/filters/matchall.json | 24 +++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
index 8e45792703ed..b7205a069534 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
@@ -812,5 +812,29 @@
"teardown": [
"$TC actions flush action police"
]
+ },
+ {
+ "id": "7d64",
+ "name": "Add police action with skip_hw option",
+ "category": [
+ "actions",
+ "police"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action police",
+ 0,
+ 1,
+ 255
+ ]
+ ],
+ "cmdUnderTest": "$TC actions add action police rate 1kbit burst 10k index 100 skip_hw",
+ "expExitCode": "0",
+ "verifyCmd": "$TC actions ls action police | grep skip_hw",
+ "matchPattern": "skip_hw",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action police"
+ ]
}
]
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/matchall.json b/tools/testing/selftests/tc-testing/tc-tests/filters/matchall.json
index 51799874a972..c17d277f0ab6 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/matchall.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/matchall.json
@@ -387,5 +387,29 @@
"$TC qdisc del dev $DUMMY ingress",
"$IP link del dev $DUMMY type dummy"
]
+ },
+ {
+ "id": "3329",
+ "name": "Validate flags of the matchall filter and police action with skip_sw",
+ "category": [
+ "filter",
+ "matchall"
+ ],
+ "setup": [
+ "$IP link add dev $DUMMY type dummy || /bin/true",
+ "$TC qdisc add dev $DUMMY ingress",
+ "$TC actions flush action police",
+ "$TC actions add action police rate 1mbit burst 100k index 199 skip_sw"
+ ],
+ "cmdUnderTest": "$TC filter add dev $DUMMY parent ffff: handle 0x1 prio 65535 protocol ipv4 matchall skip_hw action police index 199",
+ "expExitCode": "2",
+ "verifyCmd": "$TC filter get dev $DUMMY parent ffff: handle 1 prio 65535 protocol ipv4 matchall",
+ "matchPattern": "^filter parent ffff: protocol ip pref 655355 matchall.*handle 0x1.",
+ "matchCount": "0",
+ "teardown": [
+ "$TC qdisc del dev $DUMMY ingress",
+ "$IP link del dev $DUMMY type dummy",
+ "$TC actions del action police index 199"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure
2021-11-18 13:07 ` [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure Simon Horman
@ 2021-11-19 6:31 ` Jakub Kicinski
2021-11-19 7:03 ` Baowen Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Jakub Kicinski @ 2021-11-19 6:31 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko,
Oz Shlomo, Roi Dayan, Vlad Buslov, Baowen Zheng, Louis Peens,
oss-drivers
On Thu, 18 Nov 2021 14:07:58 +0100 Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> Add index to flow_action_entry structure and delete index from police and
> gate child structure.
>
> We make this change to offload tc action for driver to identify a tc
> action.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
drivers/net/ethernet/mscc/ocelot_flower.c:306:43: error: ‘const struct <anonymous>’ has no member named ‘index’
306 | pol_ix = a->police.index + ocelot->vcap_pol.base;
| ^
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure
2021-11-19 6:31 ` Jakub Kicinski
@ 2021-11-19 7:03 ` Baowen Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Baowen Zheng @ 2021-11-19 7:03 UTC (permalink / raw)
To: Jakub Kicinski, Simon Horman
Cc: netdev, Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko,
Oz Shlomo, Roi Dayan, Vlad Buslov, Louis Peens, oss-drivers
Thanks Jakub for bring this to us.
On November 19, 2021 2:31 PM, Jakub Kicinski wrote:
>On Thu, 18 Nov 2021 14:07:58 +0100 Simon Horman wrote:
>> From: Baowen Zheng <baowen.zheng@corigine.com>
>>
>> Add index to flow_action_entry structure and delete index from police
>> and gate child structure.
>>
>> We make this change to offload tc action for driver to identify a tc
>> action.
>>
>> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>
>drivers/net/ethernet/mscc/ocelot_flower.c:306:43: error: ‘const struct
><anonymous>’ has no member named ‘index’
> 306 | pol_ix = a->police.index + ocelot->vcap_pol.base;
> | ^
Thanks, this new change was delivered yesterday and our patch broke it. We will make the change in the V5 patches.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-18 13:07 ` [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-11-19 19:05 ` Vlad Buslov
2021-11-22 2:18 ` Baowen Zheng
2021-11-22 12:24 ` Jamal Hadi Salim
1 sibling, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2021-11-19 19:05 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko,
Oz Shlomo, Roi Dayan, Baowen Zheng, Louis Peens, oss-drivers
On Thu 18 Nov 2021 at 15:07, Simon Horman <simon.horman@corigine.com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> Use flow_indr_dev_register/flow_indr_dev_setup_offload to
> offload tc action.
>
> We need to call tc_cleanup_flow_action to clean up tc action entry since
> in tc_setup_action, some actions may hold dev refcnt, especially the mirror
> action.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
> include/linux/netdevice.h | 1 +
> include/net/flow_offload.h | 17 ++++
> include/net/pkt_cls.h | 12 +++
> net/core/flow_offload.c | 43 ++++++++--
> net/sched/act_api.c | 164 +++++++++++++++++++++++++++++++++++++
> net/sched/cls_api.c | 31 ++++++-
> 6 files changed, 256 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 4f4a299e92de..ae189fcff3c6 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -916,6 +916,7 @@ enum tc_setup_type {
> TC_SETUP_QDISC_TBF,
> TC_SETUP_QDISC_FIFO,
> TC_SETUP_QDISC_HTB,
> + TC_SETUP_ACT,
> };
>
> /* These structures hold the attributes of bpf state that are being passed
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index f6970213497a..15662cad5bca 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -551,6 +551,23 @@ struct flow_cls_offload {
> u32 classid;
> };
>
> +enum flow_act_command {
> + FLOW_ACT_REPLACE,
> + FLOW_ACT_DESTROY,
> + FLOW_ACT_STATS,
> +};
> +
> +struct flow_offload_action {
> + struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/
> + enum flow_act_command command;
> + enum flow_action_id id;
> + u32 index;
> + struct flow_stats stats;
> + struct flow_action action;
> +};
> +
> +struct flow_offload_action *flow_action_alloc(unsigned int num_actions);
> +
> static inline struct flow_rule *
> flow_cls_offload_flow_rule(struct flow_cls_offload *flow_cmd)
> {
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 193f88ebf629..14d098a887d0 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -258,6 +258,14 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
> for (; 0; (void)(i), (void)(a), (void)(exts))
> #endif
>
> +#define tcf_act_for_each_action(i, a, actions) \
> + for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
> +
> +static inline bool tc_act_bind(u32 flags)
> +{
> + return !!(flags & TCA_ACT_FLAGS_BIND);
> +}
> +
> static inline void
> tcf_exts_stats_update(const struct tcf_exts *exts,
> u64 bytes, u64 packets, u64 drops, u64 lastuse,
> @@ -534,6 +542,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
>
> int tc_setup_flow_action(struct flow_action *flow_action,
> const struct tcf_exts *exts);
> +
> +int tc_setup_action(struct flow_action *flow_action,
> + struct tc_action *actions[]);
> void tc_cleanup_flow_action(struct flow_action *flow_action);
>
> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
> @@ -554,6 +565,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp,
> enum tc_setup_type type, void *type_data,
> void *cb_priv, u32 *flags, unsigned int *in_hw_count);
> unsigned int tcf_exts_num_actions(struct tcf_exts *exts);
> +unsigned int tcf_act_num_actions_single(struct tc_action *act);
>
> #ifdef CONFIG_NET_CLS_ACT
> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 6beaea13564a..6676431733ef 100644
> --- a/net/core/flow_offload.c
> +++ b/net/core/flow_offload.c
> @@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int num_actions)
> }
> EXPORT_SYMBOL(flow_rule_alloc);
>
> +struct flow_offload_action *flow_action_alloc(unsigned int num_actions)
> +{
> + struct flow_offload_action *fl_action;
> + int i;
> +
> + fl_action = kzalloc(struct_size(fl_action, action.entries, num_actions),
> + GFP_KERNEL);
> + if (!fl_action)
> + return NULL;
> +
> + fl_action->action.num_entries = num_actions;
> + /* Pre-fill each action hw_stats with DONT_CARE.
> + * Caller can override this if it wants stats for a given action.
> + */
> + for (i = 0; i < num_actions; i++)
> + fl_action->action.entries[i].hw_stats = FLOW_ACTION_HW_STATS_DONT_CARE;
> +
> + return fl_action;
> +}
> +EXPORT_SYMBOL(flow_action_alloc);
> +
> #define FLOW_DISSECTOR_MATCH(__rule, __type, __out) \
> const struct flow_match *__m = &(__rule)->match; \
> struct flow_dissector *__d = (__m)->dissector; \
> @@ -549,19 +570,25 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
> void (*cleanup)(struct flow_block_cb *block_cb))
> {
> struct flow_indr_dev *this;
> + u32 count = 0;
> + int err;
>
> mutex_lock(&flow_indr_block_lock);
> + if (bo) {
> + if (bo->command == FLOW_BLOCK_BIND)
> + indir_dev_add(data, dev, sch, type, cleanup, bo);
> + else if (bo->command == FLOW_BLOCK_UNBIND)
> + indir_dev_remove(data);
> + }
>
> - if (bo->command == FLOW_BLOCK_BIND)
> - indir_dev_add(data, dev, sch, type, cleanup, bo);
> - else if (bo->command == FLOW_BLOCK_UNBIND)
> - indir_dev_remove(data);
> -
> - list_for_each_entry(this, &flow_block_indr_dev_list, list)
> - this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup);
> + list_for_each_entry(this, &flow_block_indr_dev_list, list) {
> + err = this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup);
> + if (!err)
> + count++;
> + }
>
> mutex_unlock(&flow_indr_block_lock);
>
> - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
> + return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
> }
> EXPORT_SYMBOL(flow_indr_dev_setup_offload);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 3258da3d5bed..c3d08b710661 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -21,6 +21,19 @@
> #include <net/pkt_cls.h>
> #include <net/act_api.h>
> #include <net/netlink.h>
> +#include <net/tc_act/tc_pedit.h>
> +#include <net/tc_act/tc_mirred.h>
> +#include <net/tc_act/tc_vlan.h>
> +#include <net/tc_act/tc_tunnel_key.h>
> +#include <net/tc_act/tc_csum.h>
> +#include <net/tc_act/tc_gact.h>
> +#include <net/tc_act/tc_police.h>
> +#include <net/tc_act/tc_sample.h>
> +#include <net/tc_act/tc_skbedit.h>
> +#include <net/tc_act/tc_ct.h>
> +#include <net/tc_act/tc_mpls.h>
> +#include <net/tc_act/tc_gate.h>
> +#include <net/flow_offload.h>
>
> #ifdef CONFIG_INET
> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
> kfree(p);
> }
>
> +static int flow_action_init(struct flow_offload_action *fl_action,
> + struct tc_action *act,
> + enum flow_act_command cmd,
> + struct netlink_ext_ack *extack)
> +{
> + if (!fl_action)
> + return -EINVAL;
Multiple functions in this patch verify action argument that seemingly
can't be NULL neither in this change nor in the following ones in
series. Any particular motivation for this?
> +
> + fl_action->extack = extack;
> + fl_action->command = cmd;
> + fl_action->index = act->tcfa_index;
> +
> + if (is_tcf_gact_ok(act)) {
> + fl_action->id = FLOW_ACTION_ACCEPT;
> + } else if (is_tcf_gact_shot(act)) {
> + fl_action->id = FLOW_ACTION_DROP;
> + } else if (is_tcf_gact_trap(act)) {
> + fl_action->id = FLOW_ACTION_TRAP;
> + } else if (is_tcf_gact_goto_chain(act)) {
> + fl_action->id = FLOW_ACTION_GOTO;
> + } else if (is_tcf_mirred_egress_redirect(act)) {
> + fl_action->id = FLOW_ACTION_REDIRECT;
> + } else if (is_tcf_mirred_egress_mirror(act)) {
> + fl_action->id = FLOW_ACTION_MIRRED;
> + } else if (is_tcf_mirred_ingress_redirect(act)) {
> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
> + } else if (is_tcf_mirred_ingress_mirror(act)) {
> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
> + } else if (is_tcf_vlan(act)) {
> + switch (tcf_vlan_action(act)) {
> + case TCA_VLAN_ACT_PUSH:
> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
> + break;
> + case TCA_VLAN_ACT_POP:
> + fl_action->id = FLOW_ACTION_VLAN_POP;
> + break;
> + case TCA_VLAN_ACT_MODIFY:
> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + } else if (is_tcf_tunnel_set(act)) {
> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
> + } else if (is_tcf_tunnel_release(act)) {
> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
> + } else if (is_tcf_csum(act)) {
> + fl_action->id = FLOW_ACTION_CSUM;
> + } else if (is_tcf_skbedit_mark(act)) {
> + fl_action->id = FLOW_ACTION_MARK;
> + } else if (is_tcf_sample(act)) {
> + fl_action->id = FLOW_ACTION_SAMPLE;
> + } else if (is_tcf_police(act)) {
> + fl_action->id = FLOW_ACTION_POLICE;
> + } else if (is_tcf_ct(act)) {
> + fl_action->id = FLOW_ACTION_CT;
> + } else if (is_tcf_mpls(act)) {
> + switch (tcf_mpls_action(act)) {
> + case TCA_MPLS_ACT_PUSH:
> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
> + break;
> + case TCA_MPLS_ACT_POP:
> + fl_action->id = FLOW_ACTION_MPLS_POP;
> + break;
> + case TCA_MPLS_ACT_MODIFY:
> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + } else if (is_tcf_skbedit_ptype(act)) {
> + fl_action->id = FLOW_ACTION_PTYPE;
> + } else if (is_tcf_skbedit_priority(act)) {
> + fl_action->id = FLOW_ACTION_PRIORITY;
> + } else if (is_tcf_gate(act)) {
> + fl_action->id = FLOW_ACTION_GATE;
> + } else {
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
> + struct netlink_ext_ack *extack)
> +{
> + int err;
> +
> + if (IS_ERR(fl_act))
> + return PTR_ERR(fl_act);
Ditto.
> +
> + err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
> + fl_act, NULL, NULL);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> +/* offload the tc command after inserted */
> +static int tcf_action_offload_add(struct tc_action *action,
> + struct netlink_ext_ack *extack)
> +{
> + struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
> + [0] = action,
> + };
> + struct flow_offload_action *fl_action;
> + int err = 0;
> +
> + fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
> + if (!fl_action)
> + return -ENOMEM;
> +
> + err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack);
> + if (err)
> + goto fl_err;
> +
> + err = tc_setup_action(&fl_action->action, actions);
> + if (err) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Failed to setup tc actions for offload\n");
> + goto fl_err;
> + }
> +
> + err = tcf_action_offload_cmd(fl_action, extack);
> + tc_cleanup_flow_action(&fl_action->action);
> +
> +fl_err:
> + kfree(fl_action);
> +
> + return err;
> +}
> +
> +static int tcf_action_offload_del(struct tc_action *action)
> +{
> + struct flow_offload_action fl_act;
This is the only usage of uninitialized flow_offload_action instance.
Since there is no reference driver implementation, it is not clear to me
whether this is intentional because flow_action_init() is guaranteed to
initialize all data that is necessary for delete, or just an omission.
> + int err = 0;
> +
> + if (!action)
> + return -EINVAL;
Seemingly redundant check again.
> +
> + err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
> + if (err)
> + return err;
> +
> + return tcf_action_offload_cmd(&fl_act, NULL);
> +}
> +
> static void tcf_action_cleanup(struct tc_action *p)
> {
> + tcf_action_offload_del(p);
> if (p->ops->cleanup)
> p->ops->cleanup(p);
>
> @@ -1103,6 +1265,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> sz += tcf_action_fill_size(act);
> /* Start from index 0 */
> actions[i - 1] = act;
> + if (!tc_act_bind(flags))
> + tcf_action_offload_add(act, extack);
> }
>
> /* We have to commit them all together, because if any error happened in
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d9d6ff0bf361..55fa48999d43 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats)
> return hw_stats;
> }
>
> -int tc_setup_flow_action(struct flow_action *flow_action,
> - const struct tcf_exts *exts)
> +int tc_setup_action(struct flow_action *flow_action,
> + struct tc_action *actions[])
> {
> struct tc_action *act;
> int i, j, k, err = 0;
> @@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE);
> BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED != FLOW_ACTION_HW_STATS_DELAYED);
>
> - if (!exts)
> + if (!actions)
> return 0;
>
> j = 0;
> - tcf_exts_for_each_action(i, act, exts) {
> + tcf_act_for_each_action(i, act, actions) {
> struct flow_action_entry *entry;
>
> entry = &flow_action->entries[j];
> @@ -3724,6 +3724,20 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> spin_unlock_bh(&act->tcfa_lock);
> goto err_out;
> }
> +EXPORT_SYMBOL(tc_setup_action);
> +
> +int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> + if (!exts)
> + return 0;
> +
> + return tc_setup_action(flow_action, exts->actions);
> +#else
> + return 0;
> +#endif
> +}
> EXPORT_SYMBOL(tc_setup_flow_action);
>
> unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> @@ -3742,6 +3756,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> }
> EXPORT_SYMBOL(tcf_exts_num_actions);
>
> +unsigned int tcf_act_num_actions_single(struct tc_action *act)
> +{
> + if (is_tcf_pedit(act))
> + return tcf_pedit_nkeys(act);
> + else
> + return 1;
> +}
> +EXPORT_SYMBOL(tcf_act_num_actions_single);
> +
> #ifdef CONFIG_NET_CLS_ACT
> static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
> u32 *p_block_index,
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count
2021-11-18 13:08 ` [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count Simon Horman
@ 2021-11-19 20:09 ` Vlad Buslov
2021-11-22 10:13 ` Baowen Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2021-11-19 20:09 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko,
Oz Shlomo, Roi Dayan, Baowen Zheng, Louis Peens, oss-drivers
On Thu 18 Nov 2021 at 15:08, Simon Horman <simon.horman@corigine.com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> Add reoffload process to update hw_count when driver
> is inserted or removed.
>
> When reoffloading actions, we still offload the actions
> that are added independent of filters.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
> include/net/act_api.h | 24 +++++
> net/core/flow_offload.c | 4 +
> net/sched/act_api.c | 213 ++++++++++++++++++++++++++++++++++++----
> 3 files changed, 222 insertions(+), 19 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 7900598d2dd3..e5e6e58df618 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/refcount.h>
> +#include <net/flow_offload.h>
> #include <net/sch_generic.h>
> #include <net/pkt_sched.h>
> #include <net/net_namespace.h>
> @@ -243,11 +244,26 @@ static inline void flow_action_hw_count_set(struct tc_action *act,
> act->in_hw_count = hw_count;
> }
>
> +static inline void flow_action_hw_count_inc(struct tc_action *act,
> + u32 hw_count)
> +{
> + act->in_hw_count += hw_count;
> +}
> +
> +static inline void flow_action_hw_count_dec(struct tc_action *act,
> + u32 hw_count)
> +{
> + act->in_hw_count = act->in_hw_count > hw_count ?
> + act->in_hw_count - hw_count : 0;
> +}
> +
> void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
> u64 drops, bool hw);
> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
>
> int tcf_action_update_hw_stats(struct tc_action *action);
> +int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
> + void *cb_priv, bool add);
> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> struct tcf_chain **handle,
> struct netlink_ext_ack *newchain);
> @@ -259,6 +275,14 @@ DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
> #endif
>
> int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
> +
> +#else /* !CONFIG_NET_CLS_ACT */
> +
> +static inline int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
> + void *cb_priv, bool add) {
> + return 0;
> +}
> +
> #endif /* CONFIG_NET_CLS_ACT */
>
> static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 6676431733ef..92000164ac37 100644
> --- a/net/core/flow_offload.c
> +++ b/net/core/flow_offload.c
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #include <linux/kernel.h>
> #include <linux/slab.h>
> +#include <net/act_api.h>
> #include <net/flow_offload.h>
> #include <linux/rtnetlink.h>
> #include <linux/mutex.h>
> @@ -418,6 +419,8 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
> existing_qdiscs_register(cb, cb_priv);
> mutex_unlock(&flow_indr_block_lock);
>
> + tcf_action_reoffload_cb(cb, cb_priv, true);
> +
> return 0;
> }
> EXPORT_SYMBOL(flow_indr_dev_register);
> @@ -470,6 +473,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
> mutex_unlock(&flow_indr_block_lock);
>
> + tcf_action_reoffload_cb(cb, cb_priv, false);
> flow_block_indr_notify(&cleanup_list);
> kfree(indr_dev);
> }
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index f5834d47a392..ada51b2df851 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -225,15 +225,11 @@ static int flow_action_init(struct flow_offload_action *fl_action,
> return 0;
> }
>
> -static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
> - u32 *hw_count,
> - struct netlink_ext_ack *extack)
> +static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
> + u32 *hw_count)
> {
> int err;
>
> - if (IS_ERR(fl_act))
> - return PTR_ERR(fl_act);
> -
> err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
> fl_act, NULL, NULL);
> if (err < 0)
> @@ -245,9 +241,41 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
> return 0;
> }
>
> +static int tcf_action_offload_cmd_cb_ex(struct flow_offload_action *fl_act,
> + u32 *hw_count,
> + flow_indr_block_bind_cb_t *cb,
> + void *cb_priv)
> +{
> + int err;
> +
> + err = cb(NULL, NULL, cb_priv, TC_SETUP_ACT, NULL, fl_act, NULL);
> + if (err < 0)
> + return err;
> +
> + if (hw_count)
> + *hw_count = 1;
> +
> + return 0;
> +}
> +
> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
> + u32 *hw_count,
> + flow_indr_block_bind_cb_t *cb,
> + void *cb_priv)
> +{
> + if (IS_ERR(fl_act))
> + return PTR_ERR(fl_act);
> +
> + return cb ? tcf_action_offload_cmd_cb_ex(fl_act, hw_count,
> + cb, cb_priv) :
> + tcf_action_offload_cmd_ex(fl_act, hw_count);
> +}
> +
> /* offload the tc command after inserted */
> -static int tcf_action_offload_add(struct tc_action *action,
> - struct netlink_ext_ack *extack)
> +static int tcf_action_offload_add_ex(struct tc_action *action,
> + struct netlink_ext_ack *extack,
> + flow_indr_block_bind_cb_t *cb,
> + void *cb_priv)
> {
> bool skip_sw = tc_act_skip_sw(action->tcfa_flags);
> struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
> @@ -275,9 +303,10 @@ static int tcf_action_offload_add(struct tc_action *action,
> goto fl_err;
> }
>
> - err = tcf_action_offload_cmd(fl_action, &in_hw_count, extack);
> + err = tcf_action_offload_cmd(fl_action, &in_hw_count, cb, cb_priv);
> if (!err)
> - flow_action_hw_count_set(action, in_hw_count);
> + cb ? flow_action_hw_count_inc(action, in_hw_count) :
> + flow_action_hw_count_set(action, in_hw_count);
>
> if (skip_sw && !tc_act_in_hw(action))
> err = -EINVAL;
> @@ -290,6 +319,12 @@ static int tcf_action_offload_add(struct tc_action *action,
> return err;
> }
>
> +static int tcf_action_offload_add(struct tc_action *action,
> + struct netlink_ext_ack *extack)
> +{
> + return tcf_action_offload_add_ex(action, extack, NULL, NULL);
> +}
> +
> int tcf_action_update_hw_stats(struct tc_action *action)
> {
> struct flow_offload_action fl_act = {};
> @@ -302,7 +337,7 @@ int tcf_action_update_hw_stats(struct tc_action *action)
> if (err)
> return err;
>
> - err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
> + err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
> if (!err) {
> preempt_disable();
> tcf_action_stats_update(action, fl_act.stats.bytes,
> @@ -321,7 +356,9 @@ int tcf_action_update_hw_stats(struct tc_action *action)
> }
> EXPORT_SYMBOL(tcf_action_update_hw_stats);
>
> -static int tcf_action_offload_del(struct tc_action *action)
> +static int tcf_action_offload_del_ex(struct tc_action *action,
> + flow_indr_block_bind_cb_t *cb,
> + void *cb_priv)
> {
> struct flow_offload_action fl_act;
> u32 in_hw_count = 0;
> @@ -337,16 +374,25 @@ static int tcf_action_offload_del(struct tc_action *action)
> if (err)
> return err;
>
> - err = tcf_action_offload_cmd(&fl_act, &in_hw_count, NULL);
> - if (err)
> + err = tcf_action_offload_cmd(&fl_act, &in_hw_count, cb, cb_priv);
> + if (err < 0)
> return err;
>
> - if (action->in_hw_count != in_hw_count)
> + if (!cb && action->in_hw_count != in_hw_count)
> return -EINVAL;
>
> + /* do not need to update hw state when deleting action */
> + if (cb && in_hw_count)
> + flow_action_hw_count_dec(action, in_hw_count);
> +
> return 0;
> }
>
> +static int tcf_action_offload_del(struct tc_action *action)
> +{
> + return tcf_action_offload_del_ex(action, NULL, NULL);
> +}
> +
> static void tcf_action_cleanup(struct tc_action *p)
> {
> tcf_action_offload_del(p);
> @@ -841,6 +887,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);
>
> static LIST_HEAD(act_base);
> static DEFINE_RWLOCK(act_mod_lock);
> +/* since act ops id is stored in pernet subsystem list,
> + * then there is no way to walk through only all the action
> + * subsystem, so we keep tc action pernet ops id for
> + * reoffload to walk through.
> + */
> +static LIST_HEAD(act_pernet_id_list);
> +static DEFINE_MUTEX(act_id_mutex);
> +struct tc_act_pernet_id {
> + struct list_head list;
> + unsigned int id;
> +};
> +
> +static int tcf_pernet_add_id_list(unsigned int id)
> +{
> + struct tc_act_pernet_id *id_ptr;
> + int ret = 0;
> +
> + mutex_lock(&act_id_mutex);
> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
> + if (id_ptr->id == id) {
> + ret = -EEXIST;
> + goto err_out;
> + }
> + }
> +
> + id_ptr = kzalloc(sizeof(*id_ptr), GFP_KERNEL);
> + if (!id_ptr) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + id_ptr->id = id;
> +
> + list_add_tail(&id_ptr->list, &act_pernet_id_list);
> +
> +err_out:
> + mutex_unlock(&act_id_mutex);
> + return ret;
> +}
> +
> +static void tcf_pernet_del_id_list(unsigned int id)
> +{
> + struct tc_act_pernet_id *id_ptr;
> +
> + mutex_lock(&act_id_mutex);
> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
> + if (id_ptr->id == id) {
> + list_del(&id_ptr->list);
> + kfree(id_ptr);
> + break;
> + }
> + }
> + mutex_unlock(&act_id_mutex);
> +}
>
> int tcf_register_action(struct tc_action_ops *act,
> struct pernet_operations *ops)
> @@ -859,18 +958,30 @@ int tcf_register_action(struct tc_action_ops *act,
> if (ret)
> return ret;
>
> + if (ops->id) {
> + ret = tcf_pernet_add_id_list(*ops->id);
> + if (ret)
> + goto id_err;
> + }
> +
> write_lock(&act_mod_lock);
> list_for_each_entry(a, &act_base, head) {
> if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
> - write_unlock(&act_mod_lock);
> - unregister_pernet_subsys(ops);
> - return -EEXIST;
> + ret = -EEXIST;
> + goto err_out;
> }
> }
> list_add_tail(&act->head, &act_base);
> write_unlock(&act_mod_lock);
>
> return 0;
> +
> +err_out:
> + write_unlock(&act_mod_lock);
> + tcf_pernet_del_id_list(*ops->id);
> +id_err:
Rename to 'err_id' to harmonize label naming.
> + unregister_pernet_subsys(ops);
> + return ret;
> }
> EXPORT_SYMBOL(tcf_register_action);
>
> @@ -889,12 +1000,76 @@ int tcf_unregister_action(struct tc_action_ops *act,
> }
> }
> write_unlock(&act_mod_lock);
> - if (!err)
> + if (!err) {
> unregister_pernet_subsys(ops);
> + if (ops->id)
> + tcf_pernet_del_id_list(*ops->id);
> + }
> return err;
> }
> EXPORT_SYMBOL(tcf_unregister_action);
>
> +int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
> + void *cb_priv, bool add)
> +{
> + struct tc_act_pernet_id *id_ptr;
> + struct tcf_idrinfo *idrinfo;
> + struct tc_action_net *tn;
> + struct tc_action *p;
> + unsigned int act_id;
> + unsigned long tmp;
> + unsigned long id;
> + struct idr *idr;
> + struct net *net;
> + int ret;
> +
> + if (!cb)
> + return -EINVAL;
> +
> + down_read(&net_rwsem);
> + mutex_lock(&act_id_mutex);
> +
> + for_each_net(net) {
> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
> + act_id = id_ptr->id;
> + tn = net_generic(net, act_id);
> + if (!tn)
> + continue;
> + idrinfo = tn->idrinfo;
> + if (!idrinfo)
> + continue;
> +
> + mutex_lock(&idrinfo->lock);
> + idr = &idrinfo->action_idr;
> + idr_for_each_entry_ul(idr, p, tmp, id) {
> + if (IS_ERR(p) || tc_act_bind(p->tcfa_flags))
> + continue;
> + if (add) {
> + tcf_action_offload_add_ex(p, NULL, cb,
> + cb_priv);
> + continue;
> + }
> +
> + /* cb unregister to update hw count */
> + ret = tcf_action_offload_del_ex(p, cb, cb_priv);
> + if (ret < 0)
> + continue;
> + if (tc_act_skip_sw(p->tcfa_flags) &&
> + !tc_act_in_hw(p)) {
> + ret = tcf_idr_release_unsafe(p);
Deleting skip_sw action that is no longer offloaded to any hardware
device is reasonable, but note that in this case no netlink notification
is ever generated. Don't know if it is a problem, just highlighting the
fact since the whole behavior of implicitly deleting such actions is
completely omitted from the change log.
> + if (ret == ACT_P_DELETED)
> + module_put(p->ops->owner);
> + }
> + }
> + mutex_unlock(&idrinfo->lock);
> + }
> + }
> + mutex_unlock(&act_id_mutex);
> + up_read(&net_rwsem);
> +
> + return 0;
> +}
> +
> /* lookup by name */
> static struct tc_action_ops *tc_lookup_action_n(char *kind)
> {
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-19 19:05 ` Vlad Buslov
@ 2021-11-22 2:18 ` Baowen Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Baowen Zheng @ 2021-11-22 2:18 UTC (permalink / raw)
To: Vlad Buslov, Simon Horman
Cc: netdev, Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko,
Oz Shlomo, Roi Dayan, Louis Peens, oss-drivers
On November 20, 2021 3:06 AM, Vlad Buslov wrote:
>On Thu 18 Nov 2021 at 15:07, Simon Horman <simon.horman@corigine.com>
>wrote:
>> From: Baowen Zheng <baowen.zheng@corigine.com>
>>
>> Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
>> action.
>>
>> We need to call tc_cleanup_flow_action to clean up tc action entry
>> since in tc_setup_action, some actions may hold dev refcnt, especially
>> the mirror action.
>>
>> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
>> Signed-off-by: Louis Peens <louis.peens@corigine.com>
>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>> ---
>> include/linux/netdevice.h | 1 +
>> include/net/flow_offload.h | 17 ++++
>> include/net/pkt_cls.h | 12 +++
>> net/core/flow_offload.c | 43 ++++++++--
>> net/sched/act_api.c | 164 +++++++++++++++++++++++++++++++++++++
>> net/sched/cls_api.c | 31 ++++++-
>> 6 files changed, 256 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 4f4a299e92de..ae189fcff3c6 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -916,6 +916,7 @@ enum tc_setup_type {
>> TC_SETUP_QDISC_TBF,
>> TC_SETUP_QDISC_FIFO,
>> TC_SETUP_QDISC_HTB,
>> + TC_SETUP_ACT,
>> };
>>
>> /* These structures hold the attributes of bpf state that are being
>> passed diff --git a/include/net/flow_offload.h
>> b/include/net/flow_offload.h index f6970213497a..15662cad5bca 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -551,6 +551,23 @@ struct flow_cls_offload {
>> u32 classid;
>> };
>>
>> +enum flow_act_command {
>> + FLOW_ACT_REPLACE,
>> + FLOW_ACT_DESTROY,
>> + FLOW_ACT_STATS,
>> +};
>> +
>> +struct flow_offload_action {
>> + struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS
>process*/
>> + enum flow_act_command command;
>> + enum flow_action_id id;
>> + u32 index;
>> + struct flow_stats stats;
>> + struct flow_action action;
>> +};
>> +
>> +struct flow_offload_action *flow_action_alloc(unsigned int
>> +num_actions);
>> +
>> static inline struct flow_rule *
>> flow_cls_offload_flow_rule(struct flow_cls_offload *flow_cmd) { diff
>> --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index
>> 193f88ebf629..14d098a887d0 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -258,6 +258,14 @@ static inline void tcf_exts_put_net(struct tcf_exts
>*exts)
>> for (; 0; (void)(i), (void)(a), (void)(exts)) #endif
>>
>> +#define tcf_act_for_each_action(i, a, actions) \
>> + for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
>> +
>> +static inline bool tc_act_bind(u32 flags) {
>> + return !!(flags & TCA_ACT_FLAGS_BIND); }
>> +
>> static inline void
>> tcf_exts_stats_update(const struct tcf_exts *exts,
>> u64 bytes, u64 packets, u64 drops, u64 lastuse, @@ -534,6
>> +542,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
>>
>> int tc_setup_flow_action(struct flow_action *flow_action,
>> const struct tcf_exts *exts);
>> +
>> +int tc_setup_action(struct flow_action *flow_action,
>> + struct tc_action *actions[]);
>> void tc_cleanup_flow_action(struct flow_action *flow_action);
>>
>> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type
>> type, @@ -554,6 +565,7 @@ int tc_setup_cb_reoffload(struct tcf_block
>*block, struct tcf_proto *tp,
>> enum tc_setup_type type, void *type_data,
>> void *cb_priv, u32 *flags, unsigned int
>*in_hw_count); unsigned
>> int tcf_exts_num_actions(struct tcf_exts *exts);
>> +unsigned int tcf_act_num_actions_single(struct tc_action *act);
>>
>> #ifdef CONFIG_NET_CLS_ACT
>> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch, diff
>> --git a/net/core/flow_offload.c b/net/core/flow_offload.c index
>> 6beaea13564a..6676431733ef 100644
>> --- a/net/core/flow_offload.c
>> +++ b/net/core/flow_offload.c
>> @@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int
>> num_actions) } EXPORT_SYMBOL(flow_rule_alloc);
>>
>> +struct flow_offload_action *flow_action_alloc(unsigned int
>> +num_actions) {
>> + struct flow_offload_action *fl_action;
>> + int i;
>> +
>> + fl_action = kzalloc(struct_size(fl_action, action.entries, num_actions),
>> + GFP_KERNEL);
>> + if (!fl_action)
>> + return NULL;
>> +
>> + fl_action->action.num_entries = num_actions;
>> + /* Pre-fill each action hw_stats with DONT_CARE.
>> + * Caller can override this if it wants stats for a given action.
>> + */
>> + for (i = 0; i < num_actions; i++)
>> + fl_action->action.entries[i].hw_stats =
>> +FLOW_ACTION_HW_STATS_DONT_CARE;
>> +
>> + return fl_action;
>> +}
>> +EXPORT_SYMBOL(flow_action_alloc);
>> +
>> #define FLOW_DISSECTOR_MATCH(__rule, __type, __out)
> \
>> const struct flow_match *__m = &(__rule)->match;
> \
>> struct flow_dissector *__d = (__m)->dissector;
> \
>> @@ -549,19 +570,25 @@ int flow_indr_dev_setup_offload(struct
>net_device *dev, struct Qdisc *sch,
>> void (*cleanup)(struct flow_block_cb
>*block_cb)) {
>> struct flow_indr_dev *this;
>> + u32 count = 0;
>> + int err;
>>
>> mutex_lock(&flow_indr_block_lock);
>> + if (bo) {
>> + if (bo->command == FLOW_BLOCK_BIND)
>> + indir_dev_add(data, dev, sch, type, cleanup, bo);
>> + else if (bo->command == FLOW_BLOCK_UNBIND)
>> + indir_dev_remove(data);
>> + }
>>
>> - if (bo->command == FLOW_BLOCK_BIND)
>> - indir_dev_add(data, dev, sch, type, cleanup, bo);
>> - else if (bo->command == FLOW_BLOCK_UNBIND)
>> - indir_dev_remove(data);
>> -
>> - list_for_each_entry(this, &flow_block_indr_dev_list, list)
>> - this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup);
>> + list_for_each_entry(this, &flow_block_indr_dev_list, list) {
>> + err = this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup);
>> + if (!err)
>> + count++;
>> + }
>>
>> mutex_unlock(&flow_indr_block_lock);
>>
>> - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
>> + return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count;
>> }
>> EXPORT_SYMBOL(flow_indr_dev_setup_offload);
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c index
>> 3258da3d5bed..c3d08b710661 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -21,6 +21,19 @@
>> #include <net/pkt_cls.h>
>> #include <net/act_api.h>
>> #include <net/netlink.h>
>> +#include <net/tc_act/tc_pedit.h>
>> +#include <net/tc_act/tc_mirred.h>
>> +#include <net/tc_act/tc_vlan.h>
>> +#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h>
>> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h>
>> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h>
>> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h>
>> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h>
>>
>> #ifdef CONFIG_INET
>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
>> kfree(p);
>> }
>>
>> +static int flow_action_init(struct flow_offload_action *fl_action,
>> + struct tc_action *act,
>> + enum flow_act_command cmd,
>> + struct netlink_ext_ack *extack) {
>> + if (!fl_action)
>> + return -EINVAL;
>
>Multiple functions in this patch verify action argument that seemingly can't be
>NULL neither in this change nor in the following ones in series. Any particular
>motivation for this?
>
We will make a check and delete this check if it is redundant.
>> +
>> + fl_action->extack = extack;
>> + fl_action->command = cmd;
>> + fl_action->index = act->tcfa_index;
>> +
>> + if (is_tcf_gact_ok(act)) {
>> + fl_action->id = FLOW_ACTION_ACCEPT;
>> + } else if (is_tcf_gact_shot(act)) {
>> + fl_action->id = FLOW_ACTION_DROP;
>> + } else if (is_tcf_gact_trap(act)) {
>> + fl_action->id = FLOW_ACTION_TRAP;
>> + } else if (is_tcf_gact_goto_chain(act)) {
>> + fl_action->id = FLOW_ACTION_GOTO;
>> + } else if (is_tcf_mirred_egress_redirect(act)) {
>> + fl_action->id = FLOW_ACTION_REDIRECT;
>> + } else if (is_tcf_mirred_egress_mirror(act)) {
>> + fl_action->id = FLOW_ACTION_MIRRED;
>> + } else if (is_tcf_mirred_ingress_redirect(act)) {
>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
>> + } else if (is_tcf_mirred_ingress_mirror(act)) {
>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
>> + } else if (is_tcf_vlan(act)) {
>> + switch (tcf_vlan_action(act)) {
>> + case TCA_VLAN_ACT_PUSH:
>> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
>> + break;
>> + case TCA_VLAN_ACT_POP:
>> + fl_action->id = FLOW_ACTION_VLAN_POP;
>> + break;
>> + case TCA_VLAN_ACT_MODIFY:
>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + } else if (is_tcf_tunnel_set(act)) {
>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
>> + } else if (is_tcf_tunnel_release(act)) {
>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
>> + } else if (is_tcf_csum(act)) {
>> + fl_action->id = FLOW_ACTION_CSUM;
>> + } else if (is_tcf_skbedit_mark(act)) {
>> + fl_action->id = FLOW_ACTION_MARK;
>> + } else if (is_tcf_sample(act)) {
>> + fl_action->id = FLOW_ACTION_SAMPLE;
>> + } else if (is_tcf_police(act)) {
>> + fl_action->id = FLOW_ACTION_POLICE;
>> + } else if (is_tcf_ct(act)) {
>> + fl_action->id = FLOW_ACTION_CT;
>> + } else if (is_tcf_mpls(act)) {
>> + switch (tcf_mpls_action(act)) {
>> + case TCA_MPLS_ACT_PUSH:
>> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
>> + break;
>> + case TCA_MPLS_ACT_POP:
>> + fl_action->id = FLOW_ACTION_MPLS_POP;
>> + break;
>> + case TCA_MPLS_ACT_MODIFY:
>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + } else if (is_tcf_skbedit_ptype(act)) {
>> + fl_action->id = FLOW_ACTION_PTYPE;
>> + } else if (is_tcf_skbedit_priority(act)) {
>> + fl_action->id = FLOW_ACTION_PRIORITY;
>> + } else if (is_tcf_gate(act)) {
>> + fl_action->id = FLOW_ACTION_GATE;
>> + } else {
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
>> + struct netlink_ext_ack *extack) {
>> + int err;
>> +
>> + if (IS_ERR(fl_act))
>> + return PTR_ERR(fl_act);
>
>Ditto.
>
We will make a check and delete this check if it is redundant.
>> +
>> + err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
>> + fl_act, NULL, NULL);
>> + if (err < 0)
>> + return err;
>> +
>> + return 0;
>> +}
>> +
>> +/* offload the tc command after inserted */ static int
>> +tcf_action_offload_add(struct tc_action *action,
>> + struct netlink_ext_ack *extack) {
>> + struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
>> + [0] = action,
>> + };
>> + struct flow_offload_action *fl_action;
>> + int err = 0;
>> +
>> + fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
>> + if (!fl_action)
>> + return -ENOMEM;
>> +
>> + err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack);
>> + if (err)
>> + goto fl_err;
>> +
>> + err = tc_setup_action(&fl_action->action, actions);
>> + if (err) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Failed to setup tc actions for offload\n");
>> + goto fl_err;
>> + }
>> +
>> + err = tcf_action_offload_cmd(fl_action, extack);
>> + tc_cleanup_flow_action(&fl_action->action);
>> +
>> +fl_err:
>> + kfree(fl_action);
>> +
>> + return err;
>> +}
>> +
>> +static int tcf_action_offload_del(struct tc_action *action) {
>> + struct flow_offload_action fl_act;
>
>This is the only usage of uninitialized flow_offload_action instance.
>Since there is no reference driver implementation, it is not clear to me
>whether this is intentional because flow_action_init() is guaranteed to
>initialize all data that is necessary for delete, or just an omission.
>
We will add the initialization.
>> + int err = 0;
>> +
>> + if (!action)
>> + return -EINVAL;
>
>Seemingly redundant check again.
>
Will delete the redundant check.
>> +
>> + err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
>> + if (err)
>> + return err;
>> +
>> + return tcf_action_offload_cmd(&fl_act, NULL); }
>> +
>> static void tcf_action_cleanup(struct tc_action *p) {
>> + tcf_action_offload_del(p);
>> if (p->ops->cleanup)
>> p->ops->cleanup(p);
>>
>> @@ -1103,6 +1265,8 @@ int tcf_action_init(struct net *net, struct tcf_proto
>*tp, struct nlattr *nla,
>> sz += tcf_action_fill_size(act);
>> /* Start from index 0 */
>> actions[i - 1] = act;
>> + if (!tc_act_bind(flags))
>> + tcf_action_offload_add(act, extack);
>> }
>>
>> /* We have to commit them all together, because if any error
>> happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index d9d6ff0bf361..55fa48999d43 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats
>tc_act_hw_stats(u8 hw_stats)
>> return hw_stats;
>> }
>>
>> -int tc_setup_flow_action(struct flow_action *flow_action,
>> - const struct tcf_exts *exts)
>> +int tc_setup_action(struct flow_action *flow_action,
>> + struct tc_action *actions[])
>> {
>> struct tc_action *act;
>> int i, j, k, err = 0;
>> @@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action
>*flow_action,
>> BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE !=
>FLOW_ACTION_HW_STATS_IMMEDIATE);
>> BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED !=
>> FLOW_ACTION_HW_STATS_DELAYED);
>>
>> - if (!exts)
>> + if (!actions)
>> return 0;
>>
>> j = 0;
>> - tcf_exts_for_each_action(i, act, exts) {
>> + tcf_act_for_each_action(i, act, actions) {
>> struct flow_action_entry *entry;
>>
>> entry = &flow_action->entries[j];
>> @@ -3724,6 +3724,20 @@ int tc_setup_flow_action(struct flow_action
>*flow_action,
>> spin_unlock_bh(&act->tcfa_lock);
>> goto err_out;
>> }
>> +EXPORT_SYMBOL(tc_setup_action);
>> +
>> +int tc_setup_flow_action(struct flow_action *flow_action,
>> + const struct tcf_exts *exts)
>> +{
>> +#ifdef CONFIG_NET_CLS_ACT
>> + if (!exts)
>> + return 0;
>> +
>> + return tc_setup_action(flow_action, exts->actions); #else
>> + return 0;
>> +#endif
>> +}
>> EXPORT_SYMBOL(tc_setup_flow_action);
>>
>> unsigned int tcf_exts_num_actions(struct tcf_exts *exts) @@ -3742,6
>> +3756,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
>> } EXPORT_SYMBOL(tcf_exts_num_actions);
>>
>> +unsigned int tcf_act_num_actions_single(struct tc_action *act) {
>> + if (is_tcf_pedit(act))
>> + return tcf_pedit_nkeys(act);
>> + else
>> + return 1;
>> +}
>> +EXPORT_SYMBOL(tcf_act_num_actions_single);
>> +
>> #ifdef CONFIG_NET_CLS_ACT
>> static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
>> u32 *p_block_index,
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count
2021-11-19 20:09 ` Vlad Buslov
@ 2021-11-22 10:13 ` Baowen Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Baowen Zheng @ 2021-11-22 10:13 UTC (permalink / raw)
To: Vlad Buslov, Simon Horman
Cc: netdev, Cong Wang, Ido Schimmel, Jamal Hadi Salim, Jiri Pirko,
Oz Shlomo, Roi Dayan, Louis Peens, oss-drivers
On November 20, 2021 4:09 AM, Vlad Buslov wrote:
>On Thu 18 Nov 2021 at 15:08, Simon Horman <simon.horman@corigine.com>
>wrote:
>> From: Baowen Zheng <baowen.zheng@corigine.com>
>>
>> Add reoffload process to update hw_count when driver is inserted or
>> removed.
>>
>> When reoffloading actions, we still offload the actions that are added
>> independent of filters.
>>
>> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
>> Signed-off-by: Louis Peens <louis.peens@corigine.com>
>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>> ---
>> include/net/act_api.h | 24 +++++
>> net/core/flow_offload.c | 4 +
>> net/sched/act_api.c | 213
>++++++++++++++++++++++++++++++++++++----
>> 3 files changed, 222 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h index
>> 7900598d2dd3..e5e6e58df618 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -7,6 +7,7 @@
>> */
>>
>> #include <linux/refcount.h>
>> +#include <net/flow_offload.h>
>> #include <net/sch_generic.h>
>> #include <net/pkt_sched.h>
>> #include <net/net_namespace.h>
>> @@ -243,11 +244,26 @@ static inline void flow_action_hw_count_set(struct
>tc_action *act,
>> act->in_hw_count = hw_count;
>> }
>>
>> +static inline void flow_action_hw_count_inc(struct tc_action *act,
>> + u32 hw_count)
>> +{
>> + act->in_hw_count += hw_count;
>> +}
>> +
>> +static inline void flow_action_hw_count_dec(struct tc_action *act,
>> + u32 hw_count)
>> +{
>> + act->in_hw_count = act->in_hw_count > hw_count ?
>> + act->in_hw_count - hw_count : 0; }
>> +
>> void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
>> u64 drops, bool hw);
>> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
>>
>> int tcf_action_update_hw_stats(struct tc_action *action);
>> +int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv, bool add);
>> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>> struct tcf_chain **handle,
>> struct netlink_ext_ack *newchain); @@ -259,6
>+275,14 @@
>> DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>> #endif
>>
>> int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct
>> sk_buff *skb));
>> +
>> +#else /* !CONFIG_NET_CLS_ACT */
>> +
>> +static inline int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv, bool add) {
>> + return 0;
>> +}
>> +
>> #endif /* CONFIG_NET_CLS_ACT */
>>
>> static inline void tcf_action_stats_update(struct tc_action *a, u64
>> bytes, diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
>> index 6676431733ef..92000164ac37 100644
>> --- a/net/core/flow_offload.c
>> +++ b/net/core/flow_offload.c
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0 */ #include <linux/kernel.h>
>> #include <linux/slab.h>
>> +#include <net/act_api.h>
>> #include <net/flow_offload.h>
>> #include <linux/rtnetlink.h>
>> #include <linux/mutex.h>
>> @@ -418,6 +419,8 @@ int
>flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
>> existing_qdiscs_register(cb, cb_priv);
>> mutex_unlock(&flow_indr_block_lock);
>>
>> + tcf_action_reoffload_cb(cb, cb_priv, true);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(flow_indr_dev_register);
>> @@ -470,6 +473,7 @@ void
>flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
>> __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
>> mutex_unlock(&flow_indr_block_lock);
>>
>> + tcf_action_reoffload_cb(cb, cb_priv, false);
>> flow_block_indr_notify(&cleanup_list);
>> kfree(indr_dev);
>> }
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c index
>> f5834d47a392..ada51b2df851 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -225,15 +225,11 @@ static int flow_action_init(struct
>flow_offload_action *fl_action,
>> return 0;
>> }
>>
>> -static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
>> - u32 *hw_count,
>> - struct netlink_ext_ack *extack)
>> +static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
>> + u32 *hw_count)
>> {
>> int err;
>>
>> - if (IS_ERR(fl_act))
>> - return PTR_ERR(fl_act);
>> -
>> err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
>> fl_act, NULL, NULL);
>> if (err < 0)
>> @@ -245,9 +241,41 @@ static int tcf_action_offload_cmd(struct
>flow_offload_action *fl_act,
>> return 0;
>> }
>>
>> +static int tcf_action_offload_cmd_cb_ex(struct flow_offload_action
>*fl_act,
>> + u32 *hw_count,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> +{
>> + int err;
>> +
>> + err = cb(NULL, NULL, cb_priv, TC_SETUP_ACT, NULL, fl_act, NULL);
>> + if (err < 0)
>> + return err;
>> +
>> + if (hw_count)
>> + *hw_count = 1;
>> +
>> + return 0;
>> +}
>> +
>> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
>> + u32 *hw_count,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> +{
>> + if (IS_ERR(fl_act))
>> + return PTR_ERR(fl_act);
>> +
>> + return cb ? tcf_action_offload_cmd_cb_ex(fl_act, hw_count,
>> + cb, cb_priv) :
>> + tcf_action_offload_cmd_ex(fl_act, hw_count); }
>> +
>> /* offload the tc command after inserted */ -static int
>> tcf_action_offload_add(struct tc_action *action,
>> - struct netlink_ext_ack *extack)
>> +static int tcf_action_offload_add_ex(struct tc_action *action,
>> + struct netlink_ext_ack *extack,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> {
>> bool skip_sw = tc_act_skip_sw(action->tcfa_flags);
>> struct tc_action *actions[TCA_ACT_MAX_PRIO] = { @@ -275,9 +303,10
>@@
>> static int tcf_action_offload_add(struct tc_action *action,
>> goto fl_err;
>> }
>>
>> - err = tcf_action_offload_cmd(fl_action, &in_hw_count, extack);
>> + err = tcf_action_offload_cmd(fl_action, &in_hw_count, cb, cb_priv);
>> if (!err)
>> - flow_action_hw_count_set(action, in_hw_count);
>> + cb ? flow_action_hw_count_inc(action, in_hw_count) :
>> + flow_action_hw_count_set(action, in_hw_count);
>>
>> if (skip_sw && !tc_act_in_hw(action))
>> err = -EINVAL;
>> @@ -290,6 +319,12 @@ static int tcf_action_offload_add(struct tc_action
>*action,
>> return err;
>> }
>>
>> +static int tcf_action_offload_add(struct tc_action *action,
>> + struct netlink_ext_ack *extack) {
>> + return tcf_action_offload_add_ex(action, extack, NULL, NULL); }
>> +
>> int tcf_action_update_hw_stats(struct tc_action *action) {
>> struct flow_offload_action fl_act = {}; @@ -302,7 +337,7 @@ int
>> tcf_action_update_hw_stats(struct tc_action *action)
>> if (err)
>> return err;
>>
>> - err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
>> + err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
>> if (!err) {
>> preempt_disable();
>> tcf_action_stats_update(action, fl_act.stats.bytes, @@ -321,7
>> +356,9 @@ int tcf_action_update_hw_stats(struct tc_action *action) }
>> EXPORT_SYMBOL(tcf_action_update_hw_stats);
>>
>> -static int tcf_action_offload_del(struct tc_action *action)
>> +static int tcf_action_offload_del_ex(struct tc_action *action,
>> + flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv)
>> {
>> struct flow_offload_action fl_act;
>> u32 in_hw_count = 0;
>> @@ -337,16 +374,25 @@ static int tcf_action_offload_del(struct tc_action
>*action)
>> if (err)
>> return err;
>>
>> - err = tcf_action_offload_cmd(&fl_act, &in_hw_count, NULL);
>> - if (err)
>> + err = tcf_action_offload_cmd(&fl_act, &in_hw_count, cb, cb_priv);
>> + if (err < 0)
>> return err;
>>
>> - if (action->in_hw_count != in_hw_count)
>> + if (!cb && action->in_hw_count != in_hw_count)
>> return -EINVAL;
>>
>> + /* do not need to update hw state when deleting action */
>> + if (cb && in_hw_count)
>> + flow_action_hw_count_dec(action, in_hw_count);
>> +
>> return 0;
>> }
>>
>> +static int tcf_action_offload_del(struct tc_action *action) {
>> + return tcf_action_offload_del_ex(action, NULL, NULL); }
>> +
>> static void tcf_action_cleanup(struct tc_action *p) {
>> tcf_action_offload_del(p);
>> @@ -841,6 +887,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);
>>
>> static LIST_HEAD(act_base);
>> static DEFINE_RWLOCK(act_mod_lock);
>> +/* since act ops id is stored in pernet subsystem list,
>> + * then there is no way to walk through only all the action
>> + * subsystem, so we keep tc action pernet ops id for
>> + * reoffload to walk through.
>> + */
>> +static LIST_HEAD(act_pernet_id_list); static
>> +DEFINE_MUTEX(act_id_mutex); struct tc_act_pernet_id {
>> + struct list_head list;
>> + unsigned int id;
>> +};
>> +
>> +static int tcf_pernet_add_id_list(unsigned int id) {
>> + struct tc_act_pernet_id *id_ptr;
>> + int ret = 0;
>> +
>> + mutex_lock(&act_id_mutex);
>> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
>> + if (id_ptr->id == id) {
>> + ret = -EEXIST;
>> + goto err_out;
>> + }
>> + }
>> +
>> + id_ptr = kzalloc(sizeof(*id_ptr), GFP_KERNEL);
>> + if (!id_ptr) {
>> + ret = -ENOMEM;
>> + goto err_out;
>> + }
>> + id_ptr->id = id;
>> +
>> + list_add_tail(&id_ptr->list, &act_pernet_id_list);
>> +
>> +err_out:
>> + mutex_unlock(&act_id_mutex);
>> + return ret;
>> +}
>> +
>> +static void tcf_pernet_del_id_list(unsigned int id) {
>> + struct tc_act_pernet_id *id_ptr;
>> +
>> + mutex_lock(&act_id_mutex);
>> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
>> + if (id_ptr->id == id) {
>> + list_del(&id_ptr->list);
>> + kfree(id_ptr);
>> + break;
>> + }
>> + }
>> + mutex_unlock(&act_id_mutex);
>> +}
>>
>> int tcf_register_action(struct tc_action_ops *act,
>> struct pernet_operations *ops)
>> @@ -859,18 +958,30 @@ int tcf_register_action(struct tc_action_ops *act,
>> if (ret)
>> return ret;
>>
>> + if (ops->id) {
>> + ret = tcf_pernet_add_id_list(*ops->id);
>> + if (ret)
>> + goto id_err;
>> + }
>> +
>> write_lock(&act_mod_lock);
>> list_for_each_entry(a, &act_base, head) {
>> if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
>> - write_unlock(&act_mod_lock);
>> - unregister_pernet_subsys(ops);
>> - return -EEXIST;
>> + ret = -EEXIST;
>> + goto err_out;
>> }
>> }
>> list_add_tail(&act->head, &act_base);
>> write_unlock(&act_mod_lock);
>>
>> return 0;
>> +
>> +err_out:
>> + write_unlock(&act_mod_lock);
>> + tcf_pernet_del_id_list(*ops->id);
>> +id_err:
>
>Rename to 'err_id' to harmonize label naming.
Ok, will make the change.
>
>> + unregister_pernet_subsys(ops);
>> + return ret;
>> }
>> EXPORT_SYMBOL(tcf_register_action);
>>
>> @@ -889,12 +1000,76 @@ int tcf_unregister_action(struct tc_action_ops
>*act,
>> }
>> }
>> write_unlock(&act_mod_lock);
>> - if (!err)
>> + if (!err) {
>> unregister_pernet_subsys(ops);
>> + if (ops->id)
>> + tcf_pernet_del_id_list(*ops->id);
>> + }
>> return err;
>> }
>> EXPORT_SYMBOL(tcf_unregister_action);
>>
>> +int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
>> + void *cb_priv, bool add)
>> +{
>> + struct tc_act_pernet_id *id_ptr;
>> + struct tcf_idrinfo *idrinfo;
>> + struct tc_action_net *tn;
>> + struct tc_action *p;
>> + unsigned int act_id;
>> + unsigned long tmp;
>> + unsigned long id;
>> + struct idr *idr;
>> + struct net *net;
>> + int ret;
>> +
>> + if (!cb)
>> + return -EINVAL;
>> +
>> + down_read(&net_rwsem);
>> + mutex_lock(&act_id_mutex);
>> +
>> + for_each_net(net) {
>> + list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
>> + act_id = id_ptr->id;
>> + tn = net_generic(net, act_id);
>> + if (!tn)
>> + continue;
>> + idrinfo = tn->idrinfo;
>> + if (!idrinfo)
>> + continue;
>> +
>> + mutex_lock(&idrinfo->lock);
>> + idr = &idrinfo->action_idr;
>> + idr_for_each_entry_ul(idr, p, tmp, id) {
>> + if (IS_ERR(p) || tc_act_bind(p->tcfa_flags))
>> + continue;
>> + if (add) {
>> + tcf_action_offload_add_ex(p, NULL,
>cb,
>> + cb_priv);
>> + continue;
>> + }
>> +
>> + /* cb unregister to update hw count */
>> + ret = tcf_action_offload_del_ex(p, cb,
>cb_priv);
>> + if (ret < 0)
>> + continue;
>> + if (tc_act_skip_sw(p->tcfa_flags) &&
>> + !tc_act_in_hw(p)) {
>> + ret = tcf_idr_release_unsafe(p);
>
>Deleting skip_sw action that is no longer offloaded to any hardware device is
>reasonable, but note that in this case no netlink notification is ever generated.
>Don't know if it is a problem, just highlighting the fact since the whole
>behavior of implicitly deleting such actions is completely omitted from the
>change log.
Thanks for bring this to us. We will think about to add the notification implement and also we will
change commit message to mention about this.
>
>> + if (ret == ACT_P_DELETED)
>> + module_put(p->ops->owner);
>> + }
>> + }
>> + mutex_unlock(&idrinfo->lock);
>> + }
>> + }
>> + mutex_unlock(&act_id_mutex);
>> + up_read(&net_rwsem);
>> +
>> + return 0;
>> +}
>> +
>> /* lookup by name */
>> static struct tc_action_ops *tc_lookup_action_n(char *kind) {
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 net-next 0/10] allow user to offload tc action to net device
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
` (9 preceding siblings ...)
2021-11-18 13:08 ` [PATCH v4 10/10] selftests: tc-testing: add action offload selftest for action and filter Simon Horman
@ 2021-11-22 12:17 ` Jamal Hadi Salim
2021-11-23 7:57 ` Baowen Zheng
10 siblings, 1 reply; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-22 12:17 UTC (permalink / raw)
To: Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
On 2021-11-18 08:07, Simon Horman wrote:
> Tc cli command to offload and quote an action:
>
> tc qdisc add dev $DEV ingress
> tc qdisc show dev $DEV ingress
>
> tc actions add action police rate 100mbit burst 10000k index 200 skip_sw
> tc -s -d actions list action police
>
Could you show the output above?
> tc filter add dev $DEV protocol ip parent ffff: \
> flower skip_sw ip_proto tcp action police index 200
> tc -s -d filter show dev $DEV protocol ip parent ffff:
Same here..
> tc filter add dev $DEV protocol ipv6 parent ffff: \
> flower skip_sw ip_proto tcp action police index 200
> tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
Same here..
> tc -s -d actions list action police
Same here...
cheers,
jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-18 13:07 ` [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device Simon Horman
2021-11-19 19:05 ` Vlad Buslov
@ 2021-11-22 12:24 ` Jamal Hadi Salim
2021-11-23 8:23 ` Baowen Zheng
1 sibling, 1 reply; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-22 12:24 UTC (permalink / raw)
To: Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Baowen Zheng, Louis Peens, oss-drivers
On 2021-11-18 08:07, Simon Horman wrote:
[..]
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -21,6 +21,19 @@
> +#include <net/tc_act/tc_pedit.h>
> +#include <net/tc_act/tc_mirred.h>
> +#include <net/tc_act/tc_vlan.h>
> +#include <net/tc_act/tc_tunnel_key.h>
> +#include <net/tc_act/tc_csum.h>
> +#include <net/tc_act/tc_gact.h>
> +#include <net/tc_act/tc_police.h>
> +#include <net/tc_act/tc_sample.h>
> +#include <net/tc_act/tc_skbedit.h>
> +#include <net/tc_act/tc_ct.h>
> +#include <net/tc_act/tc_mpls.h>
> +#include <net/tc_act/tc_gate.h>
> +#include <net/flow_offload.h>
>
> #ifdef CONFIG_INET
> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
> kfree(p);
> }
>
> +static int flow_action_init(struct flow_offload_action *fl_action,
> + struct tc_action *act,
> + enum flow_act_command cmd,
> + struct netlink_ext_ack *extack)
> +{
> + if (!fl_action)
> + return -EINVAL;
> +
> + fl_action->extack = extack;
> + fl_action->command = cmd;
> + fl_action->index = act->tcfa_index;
> +
> + if (is_tcf_gact_ok(act)) {
> + fl_action->id = FLOW_ACTION_ACCEPT;
> + } else if (is_tcf_gact_shot(act)) {
> + fl_action->id = FLOW_ACTION_DROP;
> + } else if (is_tcf_gact_trap(act)) {
> + fl_action->id = FLOW_ACTION_TRAP;
> + } else if (is_tcf_gact_goto_chain(act)) {
> + fl_action->id = FLOW_ACTION_GOTO;
> + } else if (is_tcf_mirred_egress_redirect(act)) {
> + fl_action->id = FLOW_ACTION_REDIRECT;
> + } else if (is_tcf_mirred_egress_mirror(act)) {
> + fl_action->id = FLOW_ACTION_MIRRED;
> + } else if (is_tcf_mirred_ingress_redirect(act)) {
> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
> + } else if (is_tcf_mirred_ingress_mirror(act)) {
> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
> + } else if (is_tcf_vlan(act)) {
> + switch (tcf_vlan_action(act)) {
> + case TCA_VLAN_ACT_PUSH:
> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
> + break;
> + case TCA_VLAN_ACT_POP:
> + fl_action->id = FLOW_ACTION_VLAN_POP;
> + break;
> + case TCA_VLAN_ACT_MODIFY:
> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + } else if (is_tcf_tunnel_set(act)) {
> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
> + } else if (is_tcf_tunnel_release(act)) {
> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
> + } else if (is_tcf_csum(act)) {
> + fl_action->id = FLOW_ACTION_CSUM;
> + } else if (is_tcf_skbedit_mark(act)) {
> + fl_action->id = FLOW_ACTION_MARK;
> + } else if (is_tcf_sample(act)) {
> + fl_action->id = FLOW_ACTION_SAMPLE;
> + } else if (is_tcf_police(act)) {
> + fl_action->id = FLOW_ACTION_POLICE;
> + } else if (is_tcf_ct(act)) {
> + fl_action->id = FLOW_ACTION_CT;
> + } else if (is_tcf_mpls(act)) {
> + switch (tcf_mpls_action(act)) {
> + case TCA_MPLS_ACT_PUSH:
> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
> + break;
> + case TCA_MPLS_ACT_POP:
> + fl_action->id = FLOW_ACTION_MPLS_POP;
> + break;
> + case TCA_MPLS_ACT_MODIFY:
> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + } else if (is_tcf_skbedit_ptype(act)) {
> + fl_action->id = FLOW_ACTION_PTYPE;
> + } else if (is_tcf_skbedit_priority(act)) {
> + fl_action->id = FLOW_ACTION_PRIORITY;
> + } else if (is_tcf_gate(act)) {
> + fl_action->id = FLOW_ACTION_GATE;
> + } else {
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
The challenge with this is now it is impossible to write an action
as a standalone module (which works today).
One resolution to this is to either reuse or introduce a new ops in
struct tc_action_ops.
Then flow_action_init() would just invoke this act->ops() which will
do action specific setup.
cheers,
jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 net-next 0/10] allow user to offload tc action to net device
2021-11-22 12:17 ` [PATCH v4 net-next 0/10] allow user to offload tc action to net device Jamal Hadi Salim
@ 2021-11-23 7:57 ` Baowen Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Baowen Zheng @ 2021-11-23 7:57 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On November 22, 2021 8:17 PM, Jamal Hadi Salim wrote:
>On 2021-11-18 08:07, Simon Horman wrote:
>> Tc cli command to offload and quote an action:
>>
>> tc qdisc add dev $DEV ingress
>> tc qdisc show dev $DEV ingress
>>
>> tc actions add action police rate 100mbit burst 10000k index 200 skip_sw
>> tc -s -d actions list action police
>>
>
>Could you show the output above?
>
>> tc filter add dev $DEV protocol ip parent ffff: \
>> flower skip_sw ip_proto tcp action police index 200
>> tc -s -d filter show dev $DEV protocol ip parent ffff:
>Same here..
>
>> tc filter add dev $DEV protocol ipv6 parent ffff: \
>> flower skip_sw ip_proto tcp action police index 200
>> tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
>
>Same here..
>
>> tc -s -d actions list action police
>
>Same here...
>
Thanks, we will add the output in the next patch for all the above command.
>cheers,
>jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-22 12:24 ` Jamal Hadi Salim
@ 2021-11-23 8:23 ` Baowen Zheng
2021-11-23 19:03 ` Jamal Hadi Salim
0 siblings, 1 reply; 30+ messages in thread
From: Baowen Zheng @ 2021-11-23 8:23 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote:
>On 2021-11-18 08:07, Simon Horman wrote:
>
>[..]
>
> > --- a/net/sched/act_api.c
> > +++ b/net/sched/act_api.c
> > @@ -21,6 +21,19 @@
>> +#include <net/tc_act/tc_pedit.h>
>> +#include <net/tc_act/tc_mirred.h>
>> +#include <net/tc_act/tc_vlan.h>
>> +#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h>
>> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h>
>> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h>
>> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h>
>> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h>
>>
>> #ifdef CONFIG_INET
>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
>> kfree(p);
>> }
>>
>> +static int flow_action_init(struct flow_offload_action *fl_action,
>> + struct tc_action *act,
>> + enum flow_act_command cmd,
>> + struct netlink_ext_ack *extack) {
>> + if (!fl_action)
>> + return -EINVAL;
>> +
>> + fl_action->extack = extack;
>> + fl_action->command = cmd;
>> + fl_action->index = act->tcfa_index;
>> +
>> + if (is_tcf_gact_ok(act)) {
>> + fl_action->id = FLOW_ACTION_ACCEPT;
>> + } else if (is_tcf_gact_shot(act)) {
>> + fl_action->id = FLOW_ACTION_DROP;
>> + } else if (is_tcf_gact_trap(act)) {
>> + fl_action->id = FLOW_ACTION_TRAP;
>> + } else if (is_tcf_gact_goto_chain(act)) {
>> + fl_action->id = FLOW_ACTION_GOTO;
>> + } else if (is_tcf_mirred_egress_redirect(act)) {
>> + fl_action->id = FLOW_ACTION_REDIRECT;
>> + } else if (is_tcf_mirred_egress_mirror(act)) {
>> + fl_action->id = FLOW_ACTION_MIRRED;
>> + } else if (is_tcf_mirred_ingress_redirect(act)) {
>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
>> + } else if (is_tcf_mirred_ingress_mirror(act)) {
>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
>> + } else if (is_tcf_vlan(act)) {
>> + switch (tcf_vlan_action(act)) {
>> + case TCA_VLAN_ACT_PUSH:
>> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
>> + break;
>> + case TCA_VLAN_ACT_POP:
>> + fl_action->id = FLOW_ACTION_VLAN_POP;
>> + break;
>> + case TCA_VLAN_ACT_MODIFY:
>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + } else if (is_tcf_tunnel_set(act)) {
>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
>> + } else if (is_tcf_tunnel_release(act)) {
>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
>> + } else if (is_tcf_csum(act)) {
>> + fl_action->id = FLOW_ACTION_CSUM;
>> + } else if (is_tcf_skbedit_mark(act)) {
>> + fl_action->id = FLOW_ACTION_MARK;
>> + } else if (is_tcf_sample(act)) {
>> + fl_action->id = FLOW_ACTION_SAMPLE;
>> + } else if (is_tcf_police(act)) {
>> + fl_action->id = FLOW_ACTION_POLICE;
>> + } else if (is_tcf_ct(act)) {
>> + fl_action->id = FLOW_ACTION_CT;
>> + } else if (is_tcf_mpls(act)) {
>> + switch (tcf_mpls_action(act)) {
>> + case TCA_MPLS_ACT_PUSH:
>> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
>> + break;
>> + case TCA_MPLS_ACT_POP:
>> + fl_action->id = FLOW_ACTION_MPLS_POP;
>> + break;
>> + case TCA_MPLS_ACT_MODIFY:
>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + } else if (is_tcf_skbedit_ptype(act)) {
>> + fl_action->id = FLOW_ACTION_PTYPE;
>> + } else if (is_tcf_skbedit_priority(act)) {
>> + fl_action->id = FLOW_ACTION_PRIORITY;
>> + } else if (is_tcf_gate(act)) {
>> + fl_action->id = FLOW_ACTION_GATE;
>> + } else {
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
>The challenge with this is now it is impossible to write an action as a
>standalone module (which works today).
>One resolution to this is to either reuse or introduce a new ops in struct
>tc_action_ops.
>Then flow_action_init() would just invoke this act->ops() which will do action
>specific setup.
>
Thanks for bringing this to us.
As my understanding, for this issue, we are facing the same fact with What we do in function tc_setup_flow_action.
If we add a filter with a new added action, we will also fail to offload the filter.
For a new added action, if we aim to offload the action to hardware, then we definitely need a
init fction and setup function for action/filter offload. We can add a ops for the new added action to init or setup the action.
Do you think it is proper to include this implement in our patch series or we can delivery a new patch for this?
>cheers,
>jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-23 8:23 ` Baowen Zheng
@ 2021-11-23 19:03 ` Jamal Hadi Salim
2021-11-24 2:11 ` Baowen Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-23 19:03 UTC (permalink / raw)
To: Baowen Zheng, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On 2021-11-23 03:23, Baowen Zheng wrote:
> On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote:
>> On 2021-11-18 08:07, Simon Horman wrote:
>>
>> [..]
>>
>>> --- a/net/sched/act_api.c
>>> +++ b/net/sched/act_api.c
>>> @@ -21,6 +21,19 @@
>>> +#include <net/tc_act/tc_pedit.h>
>>> +#include <net/tc_act/tc_mirred.h>
>>> +#include <net/tc_act/tc_vlan.h>
>>> +#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h>
>>> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h>
>>> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h>
>>> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h>
>>> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h>
>>>
>>> #ifdef CONFIG_INET
>>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
>>> kfree(p);
>>> }
>>>
>>> +static int flow_action_init(struct flow_offload_action *fl_action,
>>> + struct tc_action *act,
>>> + enum flow_act_command cmd,
>>> + struct netlink_ext_ack *extack) {
>>> + if (!fl_action)
>>> + return -EINVAL;
>>> +
>>> + fl_action->extack = extack;
>>> + fl_action->command = cmd;
>>> + fl_action->index = act->tcfa_index;
>>> +
>>> + if (is_tcf_gact_ok(act)) {
>>> + fl_action->id = FLOW_ACTION_ACCEPT;
>>> + } else if (is_tcf_gact_shot(act)) {
>>> + fl_action->id = FLOW_ACTION_DROP;
>>> + } else if (is_tcf_gact_trap(act)) {
>>> + fl_action->id = FLOW_ACTION_TRAP;
>>> + } else if (is_tcf_gact_goto_chain(act)) {
>>> + fl_action->id = FLOW_ACTION_GOTO;
>>> + } else if (is_tcf_mirred_egress_redirect(act)) {
>>> + fl_action->id = FLOW_ACTION_REDIRECT;
>>> + } else if (is_tcf_mirred_egress_mirror(act)) {
>>> + fl_action->id = FLOW_ACTION_MIRRED;
>>> + } else if (is_tcf_mirred_ingress_redirect(act)) {
>>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
>>> + } else if (is_tcf_mirred_ingress_mirror(act)) {
>>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
>>> + } else if (is_tcf_vlan(act)) {
>>> + switch (tcf_vlan_action(act)) {
>>> + case TCA_VLAN_ACT_PUSH:
>>> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
>>> + break;
>>> + case TCA_VLAN_ACT_POP:
>>> + fl_action->id = FLOW_ACTION_VLAN_POP;
>>> + break;
>>> + case TCA_VLAN_ACT_MODIFY:
>>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> + } else if (is_tcf_tunnel_set(act)) {
>>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
>>> + } else if (is_tcf_tunnel_release(act)) {
>>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
>>> + } else if (is_tcf_csum(act)) {
>>> + fl_action->id = FLOW_ACTION_CSUM;
>>> + } else if (is_tcf_skbedit_mark(act)) {
>>> + fl_action->id = FLOW_ACTION_MARK;
>>> + } else if (is_tcf_sample(act)) {
>>> + fl_action->id = FLOW_ACTION_SAMPLE;
>>> + } else if (is_tcf_police(act)) {
>>> + fl_action->id = FLOW_ACTION_POLICE;
>>> + } else if (is_tcf_ct(act)) {
>>> + fl_action->id = FLOW_ACTION_CT;
>>> + } else if (is_tcf_mpls(act)) {
>>> + switch (tcf_mpls_action(act)) {
>>> + case TCA_MPLS_ACT_PUSH:
>>> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
>>> + break;
>>> + case TCA_MPLS_ACT_POP:
>>> + fl_action->id = FLOW_ACTION_MPLS_POP;
>>> + break;
>>> + case TCA_MPLS_ACT_MODIFY:
>>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> + } else if (is_tcf_skbedit_ptype(act)) {
>>> + fl_action->id = FLOW_ACTION_PTYPE;
>>> + } else if (is_tcf_skbedit_priority(act)) {
>>> + fl_action->id = FLOW_ACTION_PRIORITY;
>>> + } else if (is_tcf_gate(act)) {
>>> + fl_action->id = FLOW_ACTION_GATE;
>>> + } else {
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> The challenge with this is now it is impossible to write an action as a
>> standalone module (which works today).
>> One resolution to this is to either reuse or introduce a new ops in struct
>> tc_action_ops.
>> Then flow_action_init() would just invoke this act->ops() which will do action
>> specific setup.
>>
> Thanks for bringing this to us.
> As my understanding, for this issue, we are facing the same fact with What we do in function tc_setup_flow_action.
> If we add a filter with a new added action, we will also fail to offload the filter.
> For a new added action, if we aim to offload the action to hardware, then we definitely need a
> init fction and setup function for action/filter offload. We can add a ops for the new added action to init or setup the action.
>
The simplest approach seems to be adding a field in ops struct and call
it hw_id (we already have id which represents the s/w side).
All your code in flow_action_init() then becomes something like:
if (!fl_action)
return -EINVAL;
fl_action->extack = extack;
fl_action->command = cmd;
fl_action->index = act->tcfa_index;
fl_action->id = act->hwid;
And modules continue to work. Did i miss something?
> Do you think it is proper to include this implement in our patch series or we can delivery a new patch for this?
Unless I am missing something basic, I dont see this as hard to do as
explained above in this patch series.
BTW: shouldnt extack be used here instead of returning just -EINVAL?
I didnt stare long enough but it seems extack is not passed when
deleting from hardware? I saw a NULL being passed in one of the patches.
cheers,
jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-23 19:03 ` Jamal Hadi Salim
@ 2021-11-24 2:11 ` Baowen Zheng
2021-11-24 2:59 ` Baowen Zheng
2021-11-24 11:10 ` Jamal Hadi Salim
0 siblings, 2 replies; 30+ messages in thread
From: Baowen Zheng @ 2021-11-24 2:11 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>On 2021-11-23 03:23, Baowen Zheng wrote:
>> On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote:
>>> On 2021-11-18 08:07, Simon Horman wrote:
>>>
>>> [..]
>>>
>>>> --- a/net/sched/act_api.c
>>>> +++ b/net/sched/act_api.c
>>>> @@ -21,6 +21,19 @@
>>>> +#include <net/tc_act/tc_pedit.h>
>>>> +#include <net/tc_act/tc_mirred.h>
>>>> +#include <net/tc_act/tc_vlan.h>
>>>> +#include <net/tc_act/tc_tunnel_key.h> #include
>>>> +<net/tc_act/tc_csum.h> #include <net/tc_act/tc_gact.h> #include
>>>> +<net/tc_act/tc_police.h> #include <net/tc_act/tc_sample.h> #include
>>>> +<net/tc_act/tc_skbedit.h> #include <net/tc_act/tc_ct.h> #include
>>>> +<net/tc_act/tc_mpls.h> #include <net/tc_act/tc_gate.h> #include
>>>> +<net/flow_offload.h>
>>>>
>>>> #ifdef CONFIG_INET
>>>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>>>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
>>>> kfree(p);
>>>> }
>>>>
>>>> +static int flow_action_init(struct flow_offload_action *fl_action,
>>>> + struct tc_action *act,
>>>> + enum flow_act_command cmd,
>>>> + struct netlink_ext_ack *extack) {
>>>> + if (!fl_action)
>>>> + return -EINVAL;
>>>> +
>>>> + fl_action->extack = extack;
>>>> + fl_action->command = cmd;
>>>> + fl_action->index = act->tcfa_index;
>>>> +
>>>> + if (is_tcf_gact_ok(act)) {
>>>> + fl_action->id = FLOW_ACTION_ACCEPT;
>>>> + } else if (is_tcf_gact_shot(act)) {
>>>> + fl_action->id = FLOW_ACTION_DROP;
>>>> + } else if (is_tcf_gact_trap(act)) {
>>>> + fl_action->id = FLOW_ACTION_TRAP;
>>>> + } else if (is_tcf_gact_goto_chain(act)) {
>>>> + fl_action->id = FLOW_ACTION_GOTO;
>>>> + } else if (is_tcf_mirred_egress_redirect(act)) {
>>>> + fl_action->id = FLOW_ACTION_REDIRECT;
>>>> + } else if (is_tcf_mirred_egress_mirror(act)) {
>>>> + fl_action->id = FLOW_ACTION_MIRRED;
>>>> + } else if (is_tcf_mirred_ingress_redirect(act)) {
>>>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
>>>> + } else if (is_tcf_mirred_ingress_mirror(act)) {
>>>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
>>>> + } else if (is_tcf_vlan(act)) {
>>>> + switch (tcf_vlan_action(act)) {
>>>> + case TCA_VLAN_ACT_PUSH:
>>>> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
>>>> + break;
>>>> + case TCA_VLAN_ACT_POP:
>>>> + fl_action->id = FLOW_ACTION_VLAN_POP;
>>>> + break;
>>>> + case TCA_VLAN_ACT_MODIFY:
>>>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
>>>> + break;
>>>> + default:
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> + } else if (is_tcf_tunnel_set(act)) {
>>>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
>>>> + } else if (is_tcf_tunnel_release(act)) {
>>>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
>>>> + } else if (is_tcf_csum(act)) {
>>>> + fl_action->id = FLOW_ACTION_CSUM;
>>>> + } else if (is_tcf_skbedit_mark(act)) {
>>>> + fl_action->id = FLOW_ACTION_MARK;
>>>> + } else if (is_tcf_sample(act)) {
>>>> + fl_action->id = FLOW_ACTION_SAMPLE;
>>>> + } else if (is_tcf_police(act)) {
>>>> + fl_action->id = FLOW_ACTION_POLICE;
>>>> + } else if (is_tcf_ct(act)) {
>>>> + fl_action->id = FLOW_ACTION_CT;
>>>> + } else if (is_tcf_mpls(act)) {
>>>> + switch (tcf_mpls_action(act)) {
>>>> + case TCA_MPLS_ACT_PUSH:
>>>> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
>>>> + break;
>>>> + case TCA_MPLS_ACT_POP:
>>>> + fl_action->id = FLOW_ACTION_MPLS_POP;
>>>> + break;
>>>> + case TCA_MPLS_ACT_MODIFY:
>>>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
>>>> + break;
>>>> + default:
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> + } else if (is_tcf_skbedit_ptype(act)) {
>>>> + fl_action->id = FLOW_ACTION_PTYPE;
>>>> + } else if (is_tcf_skbedit_priority(act)) {
>>>> + fl_action->id = FLOW_ACTION_PRIORITY;
>>>> + } else if (is_tcf_gate(act)) {
>>>> + fl_action->id = FLOW_ACTION_GATE;
>>>> + } else {
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> The challenge with this is now it is impossible to write an action as
>>> a standalone module (which works today).
>>> One resolution to this is to either reuse or introduce a new ops in
>>> struct tc_action_ops.
>>> Then flow_action_init() would just invoke this act->ops() which will
>>> do action specific setup.
>>>
>> Thanks for bringing this to us.
>> As my understanding, for this issue, we are facing the same fact with What
>we do in function tc_setup_flow_action.
>> If we add a filter with a new added action, we will also fail to offload the
>filter.
>> For a new added action, if we aim to offload the action to hardware,
>> then we definitely need a init fction and setup function for action/filter
>offload. We can add a ops for the new added action to init or setup the action.
>>
>
>The simplest approach seems to be adding a field in ops struct and call it
>hw_id (we already have id which represents the s/w side).
>All your code in flow_action_init() then becomes something like:
>
> if (!fl_action)
> return -EINVAL;
>
> fl_action->extack = extack;
> fl_action->command = cmd;
> fl_action->index = act->tcfa_index;
>
>
> fl_action->id = act->hwid;
>
>And modules continue to work. Did i miss something?
>
Hi Jamal, for your suggestion, I think it will work for most of the case. But there maybe some kind of actions
that will be assigned different hw_id in different case, such as the gact, we need to think about this case.
So I will prefer to add a callback in action ops struct to implement the flow_action_init function for the new added
Standalone action.
WDYT?
>> Do you think it is proper to include this implement in our patch series or we
>can delivery a new patch for this?
>
>Unless I am missing something basic, I dont see this as hard to do as explained
>above in this patch series.
I did not mean it is difficult.
Since as my understanding, we will have the same problem in function of tc_setup_flow_action to
Setup the actions for a to be offloaded flower. So my proposal is to add a callback in action ops to implement
Both the function of flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a distinguish.
What is your opinion?
>
>BTW: shouldnt extack be used here instead of returning just -EINVAL?
>I didnt stare long enough but it seems extack is not passed when deleting from
>hardware? I saw a NULL being passed in one of the patches.
Ok we will consider to delete the extack parameter.
>cheers,
>jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 2:11 ` Baowen Zheng
@ 2021-11-24 2:59 ` Baowen Zheng
2021-11-24 11:39 ` Jamal Hadi Salim
2021-11-24 11:10 ` Jamal Hadi Salim
1 sibling, 1 reply; 30+ messages in thread
From: Baowen Zheng @ 2021-11-24 2:59 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
Sorry for reply this message again.
On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>>On 2021-11-23 03:23, Baowen Zheng wrote:
>>> On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote:
>>>> On 2021-11-18 08:07, Simon Horman wrote:
>>>>
>>>> [..]
>>>>
>>>>> --- a/net/sched/act_api.c
>>>>> +++ b/net/sched/act_api.c
>>>>> @@ -21,6 +21,19 @@
>>>>> +#include <net/tc_act/tc_pedit.h>
>>>>> +#include <net/tc_act/tc_mirred.h>
>>>>> +#include <net/tc_act/tc_vlan.h>
>>>>> +#include <net/tc_act/tc_tunnel_key.h> #include
>>>>> +<net/tc_act/tc_csum.h> #include <net/tc_act/tc_gact.h> #include
>>>>> +<net/tc_act/tc_police.h> #include <net/tc_act/tc_sample.h>
>>>>> +#include <net/tc_act/tc_skbedit.h> #include <net/tc_act/tc_ct.h>
>>>>> +#include <net/tc_act/tc_mpls.h> #include <net/tc_act/tc_gate.h>
>>>>> +#include <net/flow_offload.h>
>>>>>
>>>>> #ifdef CONFIG_INET
>>>>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>>>>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
>>>>> kfree(p);
>>>>> }
>>>>>
>>>>> +static int flow_action_init(struct flow_offload_action *fl_action,
>>>>> + struct tc_action *act,
>>>>> + enum flow_act_command cmd,
>>>>> + struct netlink_ext_ack *extack) {
>>>>> + if (!fl_action)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + fl_action->extack = extack;
>>>>> + fl_action->command = cmd;
>>>>> + fl_action->index = act->tcfa_index;
>>>>> +
>>>>> + if (is_tcf_gact_ok(act)) {
>>>>> + fl_action->id = FLOW_ACTION_ACCEPT;
>>>>> + } else if (is_tcf_gact_shot(act)) {
>>>>> + fl_action->id = FLOW_ACTION_DROP;
>>>>> + } else if (is_tcf_gact_trap(act)) {
>>>>> + fl_action->id = FLOW_ACTION_TRAP;
>>>>> + } else if (is_tcf_gact_goto_chain(act)) {
>>>>> + fl_action->id = FLOW_ACTION_GOTO;
>>>>> + } else if (is_tcf_mirred_egress_redirect(act)) {
>>>>> + fl_action->id = FLOW_ACTION_REDIRECT;
>>>>> + } else if (is_tcf_mirred_egress_mirror(act)) {
>>>>> + fl_action->id = FLOW_ACTION_MIRRED;
>>>>> + } else if (is_tcf_mirred_ingress_redirect(act)) {
>>>>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
>>>>> + } else if (is_tcf_mirred_ingress_mirror(act)) {
>>>>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
>>>>> + } else if (is_tcf_vlan(act)) {
>>>>> + switch (tcf_vlan_action(act)) {
>>>>> + case TCA_VLAN_ACT_PUSH:
>>>>> + fl_action->id = FLOW_ACTION_VLAN_PUSH;
>>>>> + break;
>>>>> + case TCA_VLAN_ACT_POP:
>>>>> + fl_action->id = FLOW_ACTION_VLAN_POP;
>>>>> + break;
>>>>> + case TCA_VLAN_ACT_MODIFY:
>>>>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE;
>>>>> + break;
>>>>> + default:
>>>>> + return -EOPNOTSUPP;
>>>>> + }
>>>>> + } else if (is_tcf_tunnel_set(act)) {
>>>>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
>>>>> + } else if (is_tcf_tunnel_release(act)) {
>>>>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
>>>>> + } else if (is_tcf_csum(act)) {
>>>>> + fl_action->id = FLOW_ACTION_CSUM;
>>>>> + } else if (is_tcf_skbedit_mark(act)) {
>>>>> + fl_action->id = FLOW_ACTION_MARK;
>>>>> + } else if (is_tcf_sample(act)) {
>>>>> + fl_action->id = FLOW_ACTION_SAMPLE;
>>>>> + } else if (is_tcf_police(act)) {
>>>>> + fl_action->id = FLOW_ACTION_POLICE;
>>>>> + } else if (is_tcf_ct(act)) {
>>>>> + fl_action->id = FLOW_ACTION_CT;
>>>>> + } else if (is_tcf_mpls(act)) {
>>>>> + switch (tcf_mpls_action(act)) {
>>>>> + case TCA_MPLS_ACT_PUSH:
>>>>> + fl_action->id = FLOW_ACTION_MPLS_PUSH;
>>>>> + break;
>>>>> + case TCA_MPLS_ACT_POP:
>>>>> + fl_action->id = FLOW_ACTION_MPLS_POP;
>>>>> + break;
>>>>> + case TCA_MPLS_ACT_MODIFY:
>>>>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE;
>>>>> + break;
>>>>> + default:
>>>>> + return -EOPNOTSUPP;
>>>>> + }
>>>>> + } else if (is_tcf_skbedit_ptype(act)) {
>>>>> + fl_action->id = FLOW_ACTION_PTYPE;
>>>>> + } else if (is_tcf_skbedit_priority(act)) {
>>>>> + fl_action->id = FLOW_ACTION_PRIORITY;
>>>>> + } else if (is_tcf_gate(act)) {
>>>>> + fl_action->id = FLOW_ACTION_GATE;
>>>>> + } else {
>>>>> + return -EOPNOTSUPP;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>
>>>> The challenge with this is now it is impossible to write an action
>>>> as a standalone module (which works today).
>>>> One resolution to this is to either reuse or introduce a new ops in
>>>> struct tc_action_ops.
>>>> Then flow_action_init() would just invoke this act->ops() which will
>>>> do action specific setup.
>>>>
>>> Thanks for bringing this to us.
>>> As my understanding, for this issue, we are facing the same fact with
>>> What
>>we do in function tc_setup_flow_action.
>>> If we add a filter with a new added action, we will also fail to
>>> offload the
>>filter.
>>> For a new added action, if we aim to offload the action to hardware,
>>> then we definitely need a init fction and setup function for
>>> action/filter
>>offload. We can add a ops for the new added action to init or setup the
>action.
>>>
>>
>>The simplest approach seems to be adding a field in ops struct and call
>>it hw_id (we already have id which represents the s/w side).
>>All your code in flow_action_init() then becomes something like:
>>
>> if (!fl_action)
>> return -EINVAL;
>>
>> fl_action->extack = extack;
>> fl_action->command = cmd;
>> fl_action->index = act->tcfa_index;
>>
>>
>> fl_action->id = act->hwid;
>>
>>And modules continue to work. Did i miss something?
>>
>Hi Jamal, for your suggestion, I think it will work for most of the case. But
>there maybe some kind of actions that will be assigned different hw_id in
>different case, such as the gact, we need to think about this case.
>So I will prefer to add a callback in action ops struct to implement the
>flow_action_init function for the new added Standalone action.
>WDYT?
>
>>> Do you think it is proper to include this implement in our patch
>>> series or we
>>can delivery a new patch for this?
>>
>>Unless I am missing something basic, I dont see this as hard to do as
>>explained above in this patch series.
>I did not mean it is difficult.
>Since as my understanding, we will have the same problem in function of
>tc_setup_flow_action to Setup the actions for a to be offloaded flower. So my
>proposal is to add a callback in action ops to implement Both the function of
>flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a
>distinguish.
>What is your opinion?
>>
>>BTW: shouldnt extack be used here instead of returning just -EINVAL?
>>I didnt stare long enough but it seems extack is not passed when
>>deleting from hardware? I saw a NULL being passed in one of the patches.
Maybe I misunderstand what you mean previously, when I look through the implement in
flow_action_init, I did not found we use the extack to make a log before return -EINVAL.
So could you please figure it out? Maybe I miss something or misunderstand again.
>>cheers,
>>jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 2:11 ` Baowen Zheng
2021-11-24 2:59 ` Baowen Zheng
@ 2021-11-24 11:10 ` Jamal Hadi Salim
2021-11-24 11:32 ` Jamal Hadi Salim
1 sibling, 1 reply; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-24 11:10 UTC (permalink / raw)
To: Baowen Zheng, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On 2021-11-23 21:11, Baowen Zheng wrote:
> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
[..]
>> The simplest approach seems to be adding a field in ops struct and call it
>> hw_id (we already have id which represents the s/w side).
>> All your code in flow_action_init() then becomes something like:
>>
>> if (!fl_action)
>> return -EINVAL;
>>
>> fl_action->extack = extack;
>> fl_action->command = cmd;
>> fl_action->index = act->tcfa_index;
>>
>>
>> fl_action->id = act->hwid;
>>
>> And modules continue to work. Did i miss something?
>>
> Hi Jamal, for your suggestion, I think it will work for most of the case. But there maybe some kind of actions
> that will be assigned different hw_id in different case, such as the gact, we need to think about this case.
> So I will prefer to add a callback in action ops struct to implement the flow_action_init function for the new added
> Standalone action.
> WDYT?
>
Yes, the callback makes sense. I imagine this would be needed also
if you offload mirred (selecting whether to mirror or redirect).
>>> Do you think it is proper to include this implement in our patch series or we
>> can delivery a new patch for this?
>>
>> Unless I am missing something basic, I dont see this as hard to do as explained
>> above in this patch series.
> I did not mean it is difficult.
> Since as my understanding, we will have the same problem in function of tc_setup_flow_action to
> Setup the actions for a to be offloaded flower. So my proposal is to add a callback in action ops to implement
> Both the function of flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a distinguish.
> What is your opinion?
See above. I agree with your suggestion.
cheers,
jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 11:10 ` Jamal Hadi Salim
@ 2021-11-24 11:32 ` Jamal Hadi Salim
0 siblings, 0 replies; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-24 11:32 UTC (permalink / raw)
To: Baowen Zheng, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On 2021-11-24 06:10, Jamal Hadi Salim wrote:
> On 2021-11-23 21:11, Baowen Zheng wrote:
>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>
> [..]
>
>>> The simplest approach seems to be adding a field in ops struct and
>>> call it
>>> hw_id (we already have id which represents the s/w side).
>>> All your code in flow_action_init() then becomes something like:
>>>
>>> if (!fl_action)
>>> return -EINVAL;
>>>
>>> fl_action->extack = extack;
>>> fl_action->command = cmd;
>>> fl_action->index = act->tcfa_index;
>>>
>>>
>>> fl_action->id = act->hwid;
>>>
>>> And modules continue to work. Did i miss something?
>>>
>> Hi Jamal, for your suggestion, I think it will work for most of the
>> case. But there maybe some kind of actions
>> that will be assigned different hw_id in different case, such as the
>> gact, we need to think about this case.
>> So I will prefer to add a callback in action ops struct to implement
>> the flow_action_init function for the new added
>> Standalone action.
>> WDYT?
>>
>
> Yes, the callback makes sense. I imagine this would be needed also
> if you offload mirred (selecting whether to mirror or redirect).
>
BTW, I think i am able to parse your earlier message better. There is
an equivalent piece of code in cls_api.c. I didnt realize you had
cutnpasted from that code.
So this callback change has to be a separate patch. i.e
patchset 1 to
1) add the callback 2) simplify cls_api.c code
patchset 2: Your patchset that then uses the cb.
I am also wondering why that code is in the cls_api.c to begin with...
cheers,
jamal
I think if you add the action
callback then you can also simplify that.
Unfortunately that is now a separate patch given tha
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 2:59 ` Baowen Zheng
@ 2021-11-24 11:39 ` Jamal Hadi Salim
2021-11-24 13:47 ` Baowen Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-24 11:39 UTC (permalink / raw)
To: Baowen Zheng, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On 2021-11-23 21:59, Baowen Zheng wrote:
> Sorry for reply this message again.
> On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
[..]
>>>
>>> BTW: shouldnt extack be used here instead of returning just -EINVAL?
>>> I didnt stare long enough but it seems extack is not passed when
>>> deleting from hardware? I saw a NULL being passed in one of the patches.
> Maybe I misunderstand what you mean previously, when I look through the implement in
> flow_action_init, I did not found we use the extack to make a log before return -EINVAL.
> So could you please figure it out? Maybe I miss something or misunderstand again.
I mean there are maybe 1-2 places where you called that function
flow_action_init() with extack being NULL but the others with
legitimate extack.
I pointed to offload delete as an example. This may have existed
before your changes (but it is hard to tell from just eyeballing
patches); regardless it is a problem for debugging incase some
delete offload fails, no?
BTW:
now that i am looking at the patches again - small details:
struct flow_offload_action is sometimes initialized and sometimes
not (and sometimes allocated and sometimes off the stack). Maybe
to be consistent pick one style and stick with it.
cheers,
jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 11:39 ` Jamal Hadi Salim
@ 2021-11-24 13:47 ` Baowen Zheng
2021-11-24 14:58 ` Jamal Hadi Salim
0 siblings, 1 reply; 30+ messages in thread
From: Baowen Zheng @ 2021-11-24 13:47 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote:
>On 2021-11-23 21:59, Baowen Zheng wrote:
>> Sorry for reply this message again.
>> On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>
>[..]
>
>>>>
>>>> BTW: shouldnt extack be used here instead of returning just -EINVAL?
>>>> I didnt stare long enough but it seems extack is not passed when
>>>> deleting from hardware? I saw a NULL being passed in one of the patches.
>> Maybe I misunderstand what you mean previously, when I look through
>> the implement in flow_action_init, I did not found we use the extack to
>make a log before return -EINVAL.
>> So could you please figure it out? Maybe I miss something or misunderstand
>again.
>
>I mean there are maybe 1-2 places where you called that function
>flow_action_init() with extack being NULL but the others with legitimate extack.
>I pointed to offload delete as an example. This may have existed before your
>changes (but it is hard to tell from just eyeballing patches); regardless it is a
>problem for debugging incase some delete offload fails, no?
Yes, you are right, for the most of the delete scenario, the extack is NULL since
The original implement to delete the action does not include an extack, so we will
Use extack when it is available.
>
>BTW:
>now that i am looking at the patches again - small details:
>struct flow_offload_action is sometimes initialized and sometimes not (and
>sometimes allocated and sometimes off the stack). Maybe to be consistent
>pick one style and stick with it.
For this implement, it is because for the action offload process, we need items of
flow_action_entry in the flow_offload_action, then the size of flow_offload_action is
dependent on the action to be offloaded. But for delete case, we just need a pure
flow_offload_action, so we take it in stack. You can refer to the implement in cls_flower,
it is similar with our case.
Do you think if it make sense to us?
>cheers,
>jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 13:47 ` Baowen Zheng
@ 2021-11-24 14:58 ` Jamal Hadi Salim
2021-11-25 0:49 ` Baowen Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Jamal Hadi Salim @ 2021-11-24 14:58 UTC (permalink / raw)
To: Baowen Zheng, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On 2021-11-24 08:47, Baowen Zheng wrote:
> On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote:
>> On 2021-11-23 21:59, Baowen Zheng wrote:
>>> Sorry for reply this message again.
>>> On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>>>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>>
>> [..]
>>
>>>>>
>>>>> BTW: shouldnt extack be used here instead of returning just -EINVAL?
>>>>> I didnt stare long enough but it seems extack is not passed when
>>>>> deleting from hardware? I saw a NULL being passed in one of the patches.
>>> Maybe I misunderstand what you mean previously, when I look through
>>> the implement in flow_action_init, I did not found we use the extack to
>> make a log before return -EINVAL.
>>> So could you please figure it out? Maybe I miss something or misunderstand
>> again.
>>
>> I mean there are maybe 1-2 places where you called that function
>> flow_action_init() with extack being NULL but the others with legitimate extack.
>> I pointed to offload delete as an example. This may have existed before your
>> changes (but it is hard to tell from just eyeballing patches); regardless it is a
>> problem for debugging incase some delete offload fails, no?
> Yes, you are right, for the most of the delete scenario, the extack is NULL since
> The original implement to delete the action does not include an extack, so we will
> Use extack when it is available.
You may have to go deeper in the code for this to work. I think if it is
tricky to do consider doing it as a followup patch.
>>
>> BTW:
>> now that i am looking at the patches again - small details:
>> struct flow_offload_action is sometimes initialized and sometimes not (and
>> sometimes allocated and sometimes off the stack). Maybe to be consistent
>> pick one style and stick with it.
> For this implement, it is because for the action offload process, we need items of
> flow_action_entry in the flow_offload_action, then the size of flow_offload_action is
> dependent on the action to be offloaded. But for delete case, we just need a pure
> flow_offload_action, so we take it in stack. You can refer to the implement in cls_flower,
> it is similar with our case.
> Do you think if it make sense to us?
I think it does. Let me see if i can explain it in my words:
For delete you dont have any attributes to pass but for create you need
to pass attributes which may be variable sized depending on the action.
And for that reason for create you need to allocate (but for delete
you can use a variable on the stack).
If yes, then at least make sure you are consistent on the stack values;
I think i saw some cases you initialize and in some you didnt.
cheers,
jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device
2021-11-24 14:58 ` Jamal Hadi Salim
@ 2021-11-25 0:49 ` Baowen Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Baowen Zheng @ 2021-11-25 0:49 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, netdev
Cc: Cong Wang, Ido Schimmel, Jiri Pirko, Oz Shlomo, Roi Dayan,
Vlad Buslov, Louis Peens, oss-drivers
On November 24, 2021 10:59 PM, Jamal Hadi Salim wrote:
>On 2021-11-24 08:47, Baowen Zheng wrote:
>> On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote:
>>> On 2021-11-23 21:59, Baowen Zheng wrote:
>>>> Sorry for reply this message again.
>>>> On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>>>>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>>>
>>> [..]
>>>
>>>>>>
>>>>>> BTW: shouldnt extack be used here instead of returning just -EINVAL?
>>>>>> I didnt stare long enough but it seems extack is not passed when
>>>>>> deleting from hardware? I saw a NULL being passed in one of the
>patches.
>>>> Maybe I misunderstand what you mean previously, when I look through
>>>> the implement in flow_action_init, I did not found we use the extack
>>>> to
>>> make a log before return -EINVAL.
>>>> So could you please figure it out? Maybe I miss something or
>>>> misunderstand
>>> again.
>>>
>>> I mean there are maybe 1-2 places where you called that function
>>> flow_action_init() with extack being NULL but the others with legitimate
>extack.
>>> I pointed to offload delete as an example. This may have existed
>>> before your changes (but it is hard to tell from just eyeballing
>>> patches); regardless it is a problem for debugging incase some delete
>offload fails, no?
>> Yes, you are right, for the most of the delete scenario, the extack is
>> NULL since The original implement to delete the action does not
>> include an extack, so we will Use extack when it is available.
>
>You may have to go deeper in the code for this to work. I think if it is tricky to
>do consider doing it as a followup patch.
>
>>>
>>> BTW:
>>> now that i am looking at the patches again - small details:
>>> struct flow_offload_action is sometimes initialized and sometimes not
>>> (and sometimes allocated and sometimes off the stack). Maybe to be
>>> consistent pick one style and stick with it.
>> For this implement, it is because for the action offload process, we
>> need items of flow_action_entry in the flow_offload_action, then the
>> size of flow_offload_action is dependent on the action to be
>> offloaded. But for delete case, we just need a pure
>> flow_offload_action, so we take it in stack. You can refer to the implement
>in cls_flower, it is similar with our case.
>> Do you think if it make sense to us?
>
>I think it does. Let me see if i can explain it in my words:
>For delete you dont have any attributes to pass but for create you need to
>pass attributes which may be variable sized depending on the action.
>And for that reason for create you need to allocate (but for delete you can use
>a variable on the stack).
>If yes, then at least make sure you are consistent on the stack values; I think i
>saw some cases you initialize and in some you didnt.
For the initialization, Vald mentioned this in another message, we will add the
Initialization in action delete, thanks.
>cheers,
>jamal
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2021-11-25 0:51 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 13:07 [PATCH v4 net-next 0/10] allow user to offload tc action to net device Simon Horman
2021-11-18 13:07 ` [PATCH v4 01/10] flow_offload: fill flags to action structure Simon Horman
2021-11-18 13:07 ` [PATCH v4 02/10] flow_offload: reject to offload tc actions in offload drivers Simon Horman
2021-11-18 13:07 ` [PATCH v4 03/10] flow_offload: add index to flow_action_entry structure Simon Horman
2021-11-19 6:31 ` Jakub Kicinski
2021-11-19 7:03 ` Baowen Zheng
2021-11-18 13:07 ` [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device Simon Horman
2021-11-19 19:05 ` Vlad Buslov
2021-11-22 2:18 ` Baowen Zheng
2021-11-22 12:24 ` Jamal Hadi Salim
2021-11-23 8:23 ` Baowen Zheng
2021-11-23 19:03 ` Jamal Hadi Salim
2021-11-24 2:11 ` Baowen Zheng
2021-11-24 2:59 ` Baowen Zheng
2021-11-24 11:39 ` Jamal Hadi Salim
2021-11-24 13:47 ` Baowen Zheng
2021-11-24 14:58 ` Jamal Hadi Salim
2021-11-25 0:49 ` Baowen Zheng
2021-11-24 11:10 ` Jamal Hadi Salim
2021-11-24 11:32 ` Jamal Hadi Salim
2021-11-18 13:08 ` [PATCH v4 05/10] flow_offload: add skip_hw and skip_sw to control if offload the action Simon Horman
2021-11-18 13:08 ` [PATCH v4 06/10] flow_offload: add process to update action stats from hardware Simon Horman
2021-11-18 13:08 ` [PATCH v4 07/10] net: sched: save full flags for tc action Simon Horman
2021-11-18 13:08 ` [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count Simon Horman
2021-11-19 20:09 ` Vlad Buslov
2021-11-22 10:13 ` Baowen Zheng
2021-11-18 13:08 ` [PATCH v4 09/10] flow_offload: validate flags of filter and actions Simon Horman
2021-11-18 13:08 ` [PATCH v4 10/10] selftests: tc-testing: add action offload selftest for action and filter Simon Horman
2021-11-22 12:17 ` [PATCH v4 net-next 0/10] allow user to offload tc action to net device Jamal Hadi Salim
2021-11-23 7:57 ` Baowen Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.