* [RFC/PATCH net-next v2 1/5] flow_offload: fill flags to action structure
2021-10-01 11:32 [RFC/PATCH net-next v2 0/5] allow user to offload tc action to net device Simon Horman
@ 2021-10-01 11:32 ` Simon Horman
2021-10-01 11:32 ` [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload tc action to net device Simon Horman
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2021-10-01 11:32 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Roi Dayan, Vlad Buslov, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng,
Simon Horman
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 5c36013339e1..2a05bad56ef3 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 7064a365a1a9..ec987ec75807 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 e4529b428cf4..4eb8751e1ad7 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 832157a840fc..d95addd8dec5 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 230501eb9e06..ab4ae24ab886 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 cbbe1861d3a2..788527154025 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -128,7 +128,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 605418538347..1bb0266e62c2 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 ecb9ee666095..ee9cc0abf9e1 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] 12+ messages in thread* [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload tc action to net device
2021-10-01 11:32 [RFC/PATCH net-next v2 0/5] allow user to offload tc action to net device Simon Horman
2021-10-01 11:32 ` [RFC/PATCH net-next v2 1/5] flow_offload: fill flags to action structure Simon Horman
@ 2021-10-01 11:32 ` Simon Horman
2021-10-01 16:20 ` Vlad Buslov
2021-10-01 11:32 ` [RFC/PATCH net-next v2 3/5] flow_offload: add process to update action stats from hardware Simon Horman
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Simon Horman @ 2021-10-01 11:32 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Roi Dayan, Vlad Buslov, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng,
Simon Horman
From: Baowen Zheng <baowen.zheng@corigine.com>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We add skip_hw and skip_sw for user to control if offload the action
to hardware.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
drivers should update the hw_counter in flow action to indicate it
accepts to offload the action.
Add a basic process to delete offloaded actions from net device.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +
.../ethernet/netronome/nfp/flower/offload.c | 3 +
include/linux/netdevice.h | 1 +
include/net/act_api.h | 3 +-
include/net/flow_offload.h | 27 ++
include/net/pkt_cls.h | 40 +++
include/uapi/linux/pkt_cls.h | 12 +-
net/core/flow_offload.c | 43 ++-
net/sched/act_api.c | 251 +++++++++++++++++-
net/sched/cls_api.c | 29 +-
11 files changed, 395 insertions(+), 19 deletions(-)
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 398c6761eeb3..5e69357df295 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
@@ -497,6 +497,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 64c0ef57ad42..17190fe17a82 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;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d79163208dfd..a5fa6fa91772 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/act_api.h b/include/net/act_api.h
index f19f7f4a463c..656a74002a98 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
@@ -239,7 +240,7 @@ static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
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_offload_del(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/flow_offload.h b/include/net/flow_offload.h
index 3961461d9c8b..bf76baca579d 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -148,6 +148,10 @@ enum flow_action_id {
FLOW_ACTION_MPLS_MANGLE,
FLOW_ACTION_GATE,
FLOW_ACTION_PPPOE_PUSH,
+ FLOW_ACTION_PEDIT, /* generic action type of pedit action for action
+ * offload, it will be different type when adding
+ * tc actions
+ */
NUM_FLOW_ACTIONS,
};
@@ -552,6 +556,29 @@ struct flow_cls_offload {
u32 classid;
};
+enum flow_act_command {
+ FLOW_ACT_REPLACE,
+ FLOW_ACT_DESTROY,
+ FLOW_ACT_STATS,
+};
+
+enum flow_act_hw_oper {
+ FLOW_ACT_HW_ADD,
+ FLOW_ACT_HW_UPDATE,
+ FLOW_ACT_HW_DEL,
+};
+
+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 83a6d0792180..3bb4e6f45038 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -258,6 +258,34 @@ 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_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(u32 flags)
+{
+ return (flags & TCA_ACT_FLAGS_IN_HW) ? true : false;
+}
+
+/* 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;
+ if (!(flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW)))
+ return false;
+
+ return true;
+}
+
static inline void
tcf_exts_stats_update(const struct tcf_exts *exts,
u64 bytes, u64 packets, u64 drops, u64 lastuse,
@@ -532,8 +560,19 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
return ifindex == skb->skb_iif;
}
+#ifdef CONFIG_NET_CLS_ACT
int tc_setup_flow_action(struct flow_action *flow_action,
const struct tcf_exts *exts);
+#else
+static inline int tc_setup_flow_action(struct flow_action *flow_action,
+ const struct tcf_exts *exts)
+{
+ return 0;
+}
+#endif
+
+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 +593,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/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 6836ccb9c45d..616e78cde822 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -19,13 +19,19 @@ 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 */
+#define TCA_ACT_FLAGS_IN_HW (1 << 3) /* action is offloaded to HW */
+#define TCA_ACT_FLAGS_NOT_IN_HW (1 << 4) /* action isn't offloaded to HW */
+#define TCA_ACT_FLAGS_VERBOSE (1 << 5) /* verbose logging */
/* tca HW stats type
* When user does not pass the attribute, he does not care.
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 7dd3a2dc5fa4..3e18f3456afa 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);
@@ -145,6 +158,7 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
if (bind)
atomic_dec(&p->tcfa_bindcnt);
+ tcf_action_offload_del(p);
idr_remove(&idrinfo->action_idr, p->tcfa_index);
mutex_unlock(&idrinfo->lock);
@@ -341,6 +355,7 @@ static int tcf_idr_release_unsafe(struct tc_action *p)
return -EPERM;
if (refcount_dec_and_test(&p->tcfa_refcnt)) {
+ tcf_action_offload_del(p);
idr_remove(&p->idrinfo->action_idr, p->tcfa_index);
tcf_action_cleanup(p);
return ACT_P_DELETED;
@@ -448,6 +463,7 @@ static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
if (refcount_dec_and_test(&p->tcfa_refcnt)) {
struct module *owner = p->ops->owner;
+ tcf_action_offload_del(p);
WARN_ON(p != idr_remove(&idrinfo->action_idr,
p->tcfa_index));
mutex_unlock(&idrinfo->lock);
@@ -733,6 +749,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)
@@ -838,6 +857,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;
@@ -917,7 +939,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),
};
@@ -1030,8 +1054,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);
@@ -1059,6 +1088,219 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
return ERR_PTR(err);
}
+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)
+{
+ int err;
+
+ 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:
+ err = -EOPNOTSUPP;
+ goto err_out;
+ }
+ } 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_pedit(act)) {
+ fl_action->id = FLOW_ACTION_PEDIT;
+ } 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:
+ err = -EOPNOTSUPP;
+ goto err_out;
+ }
+ } 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 {
+ goto err_out;
+ }
+ return 0;
+
+err_out:
+ return err;
+}
+
+static void flow_action_update_hw(struct tc_action *act,
+ u32 hw_count,
+ enum flow_act_hw_oper cmd)
+{
+ if (!act)
+ return;
+
+ switch (cmd) {
+ case FLOW_ACT_HW_ADD:
+ act->in_hw_count = hw_count;
+ break;
+ case FLOW_ACT_HW_UPDATE:
+ act->in_hw_count += hw_count;
+ break;
+ case FLOW_ACT_HW_DEL:
+ act->in_hw_count = act->in_hw_count > hw_count ?
+ act->in_hw_count - hw_count : 0;
+ break;
+ default:
+ return;
+ }
+
+ if (act->in_hw_count) {
+ act->tcfa_flags &= ~TCA_ACT_FLAGS_NOT_IN_HW;
+ act->tcfa_flags |= TCA_ACT_FLAGS_IN_HW;
+ } else {
+ act->tcfa_flags &= ~TCA_ACT_FLAGS_IN_HW;
+ act->tcfa_flags |= TCA_ACT_FLAGS_NOT_IN_HW;
+ }
+}
+
+static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
+ u32 *hw_count,
+ 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;
+
+ if (hw_count)
+ *hw_count = err;
+
+ return 0;
+}
+
+/* offload the tc command after inserted */
+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 -EINVAL;
+
+ 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, &in_hw_count, extack);
+ if (!err)
+ flow_action_update_hw(action, in_hw_count, FLOW_ACT_HW_ADD);
+
+ if (skip_sw && !tc_act_in_hw(action->tcfa_flags))
+ err = -EINVAL;
+
+ tc_cleanup_flow_action(&fl_action->action);
+
+fl_err:
+ kfree(fl_action);
+
+ return err;
+}
+
+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->tcfa_flags))
+ return 0;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
+ if (err)
+ return err;
+
+ 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;
+}
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -1101,6 +1343,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 (!(flags & TCA_ACT_FLAGS_BIND)) {
+ 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
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 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];
@@ -3725,7 +3725,19 @@ int tc_setup_flow_action(struct flow_action *flow_action,
spin_unlock_bh(&act->tcfa_lock);
goto err_out;
}
+EXPORT_SYMBOL(tc_setup_action);
+
+#ifdef CONFIG_NET_CLS_ACT
+int tc_setup_flow_action(struct flow_action *flow_action,
+ const struct tcf_exts *exts)
+{
+ if (!exts)
+ return 0;
+
+ return tc_setup_action(flow_action, exts->actions);
+}
EXPORT_SYMBOL(tc_setup_flow_action);
+#endif
unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
{
@@ -3743,6 +3755,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] 12+ messages in thread* Re: [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload tc action to net device
2021-10-01 11:32 ` [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-10-01 16:20 ` Vlad Buslov
2021-10-27 14:42 ` Simon Horman
0 siblings, 1 reply; 12+ messages in thread
From: Vlad Buslov @ 2021-10-01 16:20 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, Jamal Hadi Salim, Roi Dayan, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng
On Fri 01 Oct 2021 at 14:32, 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 add skip_hw and skip_sw for user to control if offload the action
> to hardware.
>
> Make some basic changes for different vendors to return EOPNOTSUPP.
>
> 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.
>
> drivers should update the hw_counter in flow action to indicate it
> accepts to offload the action.
>
> Add a basic process to delete offloaded actions from net device.
>
> As per review from the RFC, the kernel test robot will fail to run, so
> we add CONFIG_NET_CLS_ACT control for the action offload.
>
> 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>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
> .../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +
> .../ethernet/netronome/nfp/flower/offload.c | 3 +
> include/linux/netdevice.h | 1 +
> include/net/act_api.h | 3 +-
> include/net/flow_offload.h | 27 ++
> include/net/pkt_cls.h | 40 +++
> include/uapi/linux/pkt_cls.h | 12 +-
> net/core/flow_offload.c | 43 ++-
> net/sched/act_api.c | 251 +++++++++++++++++-
> net/sched/cls_api.c | 29 +-
> 11 files changed, 395 insertions(+), 19 deletions(-)
>
> 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 398c6761eeb3..5e69357df295 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> @@ -497,6 +497,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 64c0ef57ad42..17190fe17a82 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;
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d79163208dfd..a5fa6fa91772 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/act_api.h b/include/net/act_api.h
> index f19f7f4a463c..656a74002a98 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
> @@ -239,7 +240,7 @@ static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
> 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_offload_del(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/flow_offload.h b/include/net/flow_offload.h
> index 3961461d9c8b..bf76baca579d 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -148,6 +148,10 @@ enum flow_action_id {
> FLOW_ACTION_MPLS_MANGLE,
> FLOW_ACTION_GATE,
> FLOW_ACTION_PPPOE_PUSH,
> + FLOW_ACTION_PEDIT, /* generic action type of pedit action for action
> + * offload, it will be different type when adding
> + * tc actions
> + */
This doesn't seem to be used anywhere in the series (it is set by
flow_action_init() but never read). It is also confusing to add another
id for pedit when FLOW_ACTION_{ADD|MANGLE} already exists in same enum.
> NUM_FLOW_ACTIONS,
> };
>
> @@ -552,6 +556,29 @@ struct flow_cls_offload {
> u32 classid;
> };
>
> +enum flow_act_command {
> + FLOW_ACT_REPLACE,
> + FLOW_ACT_DESTROY,
> + FLOW_ACT_STATS,
> +};
> +
> +enum flow_act_hw_oper {
> + FLOW_ACT_HW_ADD,
> + FLOW_ACT_HW_UPDATE,
> + FLOW_ACT_HW_DEL,
> +};
> +
> +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 83a6d0792180..3bb4e6f45038 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -258,6 +258,34 @@ 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_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(u32 flags)
> +{
> + return (flags & TCA_ACT_FLAGS_IN_HW) ? true : false;
> +}
> +
> +/* 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;
> + if (!(flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW)))
> + return false;
Can be simplified to just:
return flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW);
> +
> + return true;
> +}
> +
> static inline void
> tcf_exts_stats_update(const struct tcf_exts *exts,
> u64 bytes, u64 packets, u64 drops, u64 lastuse,
> @@ -532,8 +560,19 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
> return ifindex == skb->skb_iif;
> }
>
> +#ifdef CONFIG_NET_CLS_ACT
> int tc_setup_flow_action(struct flow_action *flow_action,
> const struct tcf_exts *exts);
> +#else
> +static inline int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts)
> +{
> + return 0;
> +}
> +#endif
> +
> +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 +593,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/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 6836ccb9c45d..616e78cde822 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -19,13 +19,19 @@ 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 */
> +#define TCA_ACT_FLAGS_IN_HW (1 << 3) /* action is offloaded to HW */
> +#define TCA_ACT_FLAGS_NOT_IN_HW (1 << 4) /* action isn't offloaded to HW */
> +#define TCA_ACT_FLAGS_VERBOSE (1 << 5) /* verbose logging */
Doesn't seem to be used anywhere.
>
> /* tca HW stats type
> * When user does not pass the attribute, he does not care.
> 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 7dd3a2dc5fa4..3e18f3456afa 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);
> @@ -145,6 +158,7 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
> if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
> if (bind)
> atomic_dec(&p->tcfa_bindcnt);
> + tcf_action_offload_del(p);
> idr_remove(&idrinfo->action_idr, p->tcfa_index);
> mutex_unlock(&idrinfo->lock);
I'm curious whether it is required to call tcf_action_offload_del()
inside idrinfo->lock critical section here.
>
> @@ -341,6 +355,7 @@ static int tcf_idr_release_unsafe(struct tc_action *p)
> return -EPERM;
>
> if (refcount_dec_and_test(&p->tcfa_refcnt)) {
> + tcf_action_offload_del(p);
> idr_remove(&p->idrinfo->action_idr, p->tcfa_index);
> tcf_action_cleanup(p);
> return ACT_P_DELETED;
> @@ -448,6 +463,7 @@ static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
> if (refcount_dec_and_test(&p->tcfa_refcnt)) {
> struct module *owner = p->ops->owner;
>
> + tcf_action_offload_del(p);
> WARN_ON(p != idr_remove(&idrinfo->action_idr,
> p->tcfa_index));
> mutex_unlock(&idrinfo->lock);
> @@ -733,6 +749,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)
> @@ -838,6 +857,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;
> @@ -917,7 +939,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),
> };
>
> @@ -1030,8 +1054,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);
> @@ -1059,6 +1088,219 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> +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)
> +{
> + int err;
> +
> + 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:
> + err = -EOPNOTSUPP;
> + goto err_out;
> + }
> + } 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_pedit(act)) {
> + fl_action->id = FLOW_ACTION_PEDIT;
> + } 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:
> + err = -EOPNOTSUPP;
> + goto err_out;
> + }
> + } 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 {
> + goto err_out;
> + }
> + return 0;
> +
> +err_out:
> + return err;
> +}
Why is this function needed? It sets flow_offload_action->id, but
similar value is also set in flow_offload_action->entries[]->id.
> +
> +static void flow_action_update_hw(struct tc_action *act,
> + u32 hw_count,
> + enum flow_act_hw_oper cmd)
Enum flow_act_hw_oper is defined together with similar-ish enum
flow_act_command and only instance of flow_act_hw_oper is called "cmd"
which is confusing. More thoughts in next comment.
> +{
> + if (!act)
> + return;
> +
> + switch (cmd) {
> + case FLOW_ACT_HW_ADD:
> + act->in_hw_count = hw_count;
> + break;
> + case FLOW_ACT_HW_UPDATE:
> + act->in_hw_count += hw_count;
> + break;
> + case FLOW_ACT_HW_DEL:
> + act->in_hw_count = act->in_hw_count > hw_count ?
> + act->in_hw_count - hw_count : 0;
> + break;
> + default:
> + return;
> + }
> +
> + if (act->in_hw_count) {
> + act->tcfa_flags &= ~TCA_ACT_FLAGS_NOT_IN_HW;
> + act->tcfa_flags |= TCA_ACT_FLAGS_IN_HW;
> + } else {
> + act->tcfa_flags &= ~TCA_ACT_FLAGS_IN_HW;
> + act->tcfa_flags |= TCA_ACT_FLAGS_NOT_IN_HW;
> + }
I guess this could be just split to three one-liners for add/update/del,
if not for common code to update flags. Such simplification would also
probably remove the need for flow_act_hw_oper enum. I know I suggested
mimicking similar classifier infrastructure, but after thinking about it
some more maybe it would be better to parse in_hw_count to determine
whether action is in hardware (and either only set {not_}in_hw flags
before dumping to user space or get rid of them altogether)? After all,
it seems that having both in classifier API is just due to historic
reasons - in_hw flags were added first and couldn't be removed in order
not to break userland after in_hw_count was implemented. WDYT?
> +}
> +
> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
> + u32 *hw_count,
> + 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;
> +
> + if (hw_count)
> + *hw_count = err;
> +
> + return 0;
> +}
> +
> +/* offload the tc command after inserted */
> +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 -EINVAL;
> +
> + 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, &in_hw_count, extack);
> + if (!err)
> + flow_action_update_hw(action, in_hw_count, FLOW_ACT_HW_ADD);
> +
> + if (skip_sw && !tc_act_in_hw(action->tcfa_flags))
> + err = -EINVAL;
> +
> + tc_cleanup_flow_action(&fl_action->action);
> +
> +fl_err:
> + kfree(fl_action);
> +
> + return err;
> +}
> +
> +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->tcfa_flags))
> + return 0;
> +
> + err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
> + if (err)
> + return err;
> +
> + 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;
> +}
> +
> /* Returns numbers of initialized actions or negative error. */
>
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> @@ -1101,6 +1343,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 (!(flags & TCA_ACT_FLAGS_BIND)) {
> + 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
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 2ef8f5a6205a..351d93988b8b 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];
> @@ -3725,7 +3725,19 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> spin_unlock_bh(&act->tcfa_lock);
> goto err_out;
> }
> +EXPORT_SYMBOL(tc_setup_action);
> +
> +#ifdef CONFIG_NET_CLS_ACT
> +int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts)
> +{
> + if (!exts)
> + return 0;
> +
> + return tc_setup_action(flow_action, exts->actions);
> +}
> EXPORT_SYMBOL(tc_setup_flow_action);
> +#endif
>
> unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> {
> @@ -3743,6 +3755,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,
This ~400 lines patch is hard to properly review. I would suggest
splitting it.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload tc action to net device
2021-10-01 16:20 ` Vlad Buslov
@ 2021-10-27 14:42 ` Simon Horman
0 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2021-10-27 14:42 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, Jamal Hadi Salim, Roi Dayan, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng
Hi Vlad,
On Fri, Oct 01, 2021 at 07:20:52PM +0300, Vlad Buslov wrote:
>
> On Fri 01 Oct 2021 at 14:32, 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.
...
Thanks for your review and sorry for the delay in responding.
I believe that at this point we have addressed most of the points
your raised and plan to post a v3 shortly.
At this point I'd like to relay some responses from Baowen who
has been working on addressing your review.
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index 3961461d9c8b..bf76baca579d 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -148,6 +148,10 @@ enum flow_action_id {
> > FLOW_ACTION_MPLS_MANGLE,
> > FLOW_ACTION_GATE,
> > FLOW_ACTION_PPPOE_PUSH,
> > + FLOW_ACTION_PEDIT, /* generic action type of pedit action for action
> > + * offload, it will be different type when adding
> > + * tc actions
> > + */
>
> This doesn't seem to be used anywhere in the series (it is set by
> flow_action_init() but never read). It is also confusing to add another
> id for pedit when FLOW_ACTION_{ADD|MANGLE} already exists in same enum.
Yes, agreed. It is to be used in driver, but since we do not use it by now
we will drop it from this patch.
...
> > +/* 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;
> > + if (!(flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW)))
> > + return false;
>
> Can be simplified to just:
>
> return flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW);
Thanks, we will include that in v3.
...
> > +#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 */
> > +#define TCA_ACT_FLAGS_IN_HW (1 << 3) /* action is offloaded to HW */
> > +#define TCA_ACT_FLAGS_NOT_IN_HW (1 << 4) /* action isn't offloaded to HW */
> > +#define TCA_ACT_FLAGS_VERBOSE (1 << 5) /* verbose logging */
>
> Doesn't seem to be used anywhere.
Thanks, we will drop this from v3.
...
> > @@ -145,6 +158,7 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
> > if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
> > if (bind)
> > atomic_dec(&p->tcfa_bindcnt);
> > + tcf_action_offload_del(p);
> > idr_remove(&idrinfo->action_idr, p->tcfa_index);
> > mutex_unlock(&idrinfo->lock);
>
> I'm curious whether it is required to call tcf_action_offload_del()
> inside idrinfo->lock critical section here.
Thanks for bringing this to our attention.
We'll move action offload delete out of the critical section in v3.
...
> > @@ -1059,6 +1088,219 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> > return ERR_PTR(err);
> > }
> >
> > +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)
> > +{
> > + int err;
> > +
> > + 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;
...
> Why is this function needed? It sets flow_offload_action->id, but
> similar value is also set in flow_offload_action->entries[]->id.
We set the flow_offload_action->entries only in the action add/replace
case. So for action delete/stats we need the id in the flow_offload_action.
This is similar to how classifier offload is handled.
> > +
> > +static void flow_action_update_hw(struct tc_action *act,
> > + u32 hw_count,
> > + enum flow_act_hw_oper cmd)
>
> Enum flow_act_hw_oper is defined together with similar-ish enum
> flow_act_command and only instance of flow_act_hw_oper is called "cmd"
> which is confusing. More thoughts in next comment.
>
> > +{
> > + if (!act)
> > + return;
> > +
> > + switch (cmd) {
> > + case FLOW_ACT_HW_ADD:
> > + act->in_hw_count = hw_count;
> > + break;
> > + case FLOW_ACT_HW_UPDATE:
> > + act->in_hw_count += hw_count;
> > + break;
> > + case FLOW_ACT_HW_DEL:
> > + act->in_hw_count = act->in_hw_count > hw_count ?
> > + act->in_hw_count - hw_count : 0;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + if (act->in_hw_count) {
> > + act->tcfa_flags &= ~TCA_ACT_FLAGS_NOT_IN_HW;
> > + act->tcfa_flags |= TCA_ACT_FLAGS_IN_HW;
> > + } else {
> > + act->tcfa_flags &= ~TCA_ACT_FLAGS_IN_HW;
> > + act->tcfa_flags |= TCA_ACT_FLAGS_NOT_IN_HW;
> > + }
>
> I guess this could be just split to three one-liners for add/update/del,
> if not for common code to update flags. Such simplification would also
> probably remove the need for flow_act_hw_oper enum. I know I suggested
> mimicking similar classifier infrastructure, but after thinking about it
> some more maybe it would be better to parse in_hw_count to determine
> whether action is in hardware (and either only set {not_}in_hw flags
> before dumping to user space or get rid of them altogether)? After all,
> it seems that having both in classifier API is just due to historic
> reasons - in_hw flags were added first and couldn't be removed in order
> not to break userland after in_hw_count was implemented. WDYT?
Thanks, this is a good point.
In v3 we will drop in_hw and not_in_hw, and allow user-space to
derive them, if needed, from in_hw_count.
...
> This ~400 lines patch is hard to properly review. I would suggest
> splitting it.
Thanks, we've split this into three patches in v3.
Hopefully the result is a bit easier on the eyes.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC/PATCH net-next v2 3/5] flow_offload: add process to update action stats from hardware
2021-10-01 11:32 [RFC/PATCH net-next v2 0/5] allow user to offload tc action to net device Simon Horman
2021-10-01 11:32 ` [RFC/PATCH net-next v2 1/5] flow_offload: fill flags to action structure Simon Horman
2021-10-01 11:32 ` [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-10-01 11:32 ` Simon Horman
2021-10-01 11:32 ` [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw_count Simon Horman
2021-10-01 11:32 ` [RFC/PATCH net-next v2 5/5] flow_offload: validate flags of filter and actions Simon Horman
4 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2021-10-01 11:32 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Roi Dayan, Vlad Buslov, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng,
Simon Horman
From: Baowen Zheng <baowen.zheng@corigine.com>
When collecting stats for actions update them using both
both hardware and software counters.
Stats update process should not 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 | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 656a74002a98..1209444ac369 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -241,6 +241,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_offload_del(struct tc_action *action);
+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 3bb4e6f45038..5c394f04feb5 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -294,18 +294,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 3e18f3456afa..c98048832c80 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1275,6 +1275,40 @@ 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 = 0;
+
+ if (!tc_act_in_hw(action->tcfa_flags))
+ return -EOPNOTSUPP;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
+ if (err)
+ goto err_out;
+
+ err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
+
+ if (!err && fl_act.stats.lastused) {
+ 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;
+ err = 0;
+ } else {
+ err = -EOPNOTSUPP;
+ }
+
+err_out:
+ return err;
+}
+EXPORT_SYMBOL(tcf_action_update_hw_stats);
+
int tcf_action_offload_del(struct tc_action *action)
{
struct flow_offload_action fl_act;
@@ -1399,6 +1433,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] 12+ messages in thread* [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw_count
2021-10-01 11:32 [RFC/PATCH net-next v2 0/5] allow user to offload tc action to net device Simon Horman
` (2 preceding siblings ...)
2021-10-01 11:32 ` [RFC/PATCH net-next v2 3/5] flow_offload: add process to update action stats from hardware Simon Horman
@ 2021-10-01 11:32 ` Simon Horman
2021-10-01 17:30 ` Vlad Buslov
2021-10-01 11:32 ` [RFC/PATCH net-next v2 5/5] flow_offload: validate flags of filter and actions Simon Horman
4 siblings, 1 reply; 12+ messages in thread
From: Simon Horman @ 2021-10-01 11:32 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Roi Dayan, Vlad Buslov, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng,
Simon Horman
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.
Change the lock usage to fix sleeping in invalid context.
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 | 14 +++
net/core/flow_offload.c | 5 +
net/sched/act_api.c | 215 ++++++++++++++++++++++++++++++++++++----
3 files changed, 214 insertions(+), 20 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1209444ac369..df64489d1013 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>
@@ -44,6 +45,9 @@ struct tc_action {
u8 hw_stats;
u8 used_hw_stats;
bool used_hw_stats_valid;
+ bool add_separate; /* indicate if the action is created
+ * independent of any flow
+ */
u32 in_hw_count;
};
#define tcf_index common.tcfa_index
@@ -242,6 +246,8 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
int tcf_action_offload_del(struct tc_action *action);
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);
@@ -253,6 +259,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..d591204af6e0 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);
@@ -472,6 +475,8 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
flow_block_indr_notify(&cleanup_list);
kfree(indr_dev);
+
+ tcf_action_reoffload_cb(cb, cb_priv, false);
}
EXPORT_SYMBOL(flow_indr_dev_unregister);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index c98048832c80..7bb84d5001b6 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -512,6 +512,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
p->tcfa_tm.lastuse = jiffies;
p->tcfa_tm.firstuse = 0;
p->tcfa_flags = flags & TCA_ACT_FLAGS_USER_MASK;
+ p->add_separate = !bind;
if (est) {
err = gen_new_estimator(&p->tcfa_bstats, p->cpu_bstats,
&p->tcfa_rate_est,
@@ -636,6 +637,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)
@@ -654,18 +708,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);
@@ -684,8 +750,11 @@ 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);
@@ -1210,29 +1279,57 @@ static void flow_action_update_hw(struct tc_action *act,
}
}
-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)
+ return err;
+
+ if (hw_count)
+ *hw_count = err;
- err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
+ 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 = err;
+ *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] = {
@@ -1260,9 +1357,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_update_hw(action, in_hw_count, FLOW_ACT_HW_ADD);
+ flow_action_update_hw(action, in_hw_count,
+ cb ? FLOW_ACT_HW_UPDATE : FLOW_ACT_HW_ADD);
if (skip_sw && !tc_act_in_hw(action->tcfa_flags))
err = -EINVAL;
@@ -1275,6 +1373,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 = {};
@@ -1287,7 +1391,7 @@ int tcf_action_update_hw_stats(struct tc_action *action)
if (err)
goto err_out;
- err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
+ err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
if (!err && fl_act.stats.lastused) {
preempt_disable();
@@ -1309,7 +1413,8 @@ int tcf_action_update_hw_stats(struct tc_action *action)
}
EXPORT_SYMBOL(tcf_action_update_hw_stats);
-int tcf_action_offload_del(struct tc_action *action)
+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;
@@ -1325,13 +1430,83 @@ 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)
+ /* do not need to update hw state when deleting action */
+ if (cb && in_hw_count)
+ flow_action_update_hw(action, in_hw_count, FLOW_ACT_HW_DEL);
+
+ if (!cb && action->in_hw_count != in_hw_count)
+ return -EINVAL;
+
+ return 0;
+}
+
+int tcf_action_offload_del(struct tc_action *action)
+{
+ return tcf_action_offload_del_ex(action, NULL, NULL);
+}
+
+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) || !p->add_separate)
+ 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->tcfa_flags)) {
+ 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;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw_count
2021-10-01 11:32 ` [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw_count Simon Horman
@ 2021-10-01 17:30 ` Vlad Buslov
2021-10-27 14:42 ` Simon Horman
0 siblings, 1 reply; 12+ messages in thread
From: Vlad Buslov @ 2021-10-01 17:30 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, Jamal Hadi Salim, Roi Dayan, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng
On Fri 01 Oct 2021 at 14:32, 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.
>
> Change the lock usage to fix sleeping in invalid context.
What does this refer to? Looking at the code it is not clear to me which
lock usage is changed. Or is it just a change log from v1?
>
> 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 | 14 +++
> net/core/flow_offload.c | 5 +
> net/sched/act_api.c | 215 ++++++++++++++++++++++++++++++++++++----
> 3 files changed, 214 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 1209444ac369..df64489d1013 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>
> @@ -44,6 +45,9 @@ struct tc_action {
> u8 hw_stats;
> u8 used_hw_stats;
> bool used_hw_stats_valid;
> + bool add_separate; /* indicate if the action is created
> + * independent of any flow
> + */
This looks like a duplication of flags since this value is derived from
BIND flag. I understand that you need this because currently all flags
that are not visible to the userspace are cleared after action is
created, but maybe it would be better to refactor the code to preserve
all flags and to only apply TCA_ACT_FLAGS_USER_MASK when dumping to user
space.
> u32 in_hw_count;
> };
> #define tcf_index common.tcfa_index
> @@ -242,6 +246,8 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
> int tcf_action_offload_del(struct tc_action *action);
> 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);
> @@ -253,6 +259,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..d591204af6e0 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);
> @@ -472,6 +475,8 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
>
> flow_block_indr_notify(&cleanup_list);
> kfree(indr_dev);
> +
> + tcf_action_reoffload_cb(cb, cb_priv, false);
> }
> EXPORT_SYMBOL(flow_indr_dev_unregister);
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index c98048832c80..7bb84d5001b6 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -512,6 +512,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
> p->tcfa_tm.lastuse = jiffies;
> p->tcfa_tm.firstuse = 0;
> p->tcfa_flags = flags & TCA_ACT_FLAGS_USER_MASK;
> + p->add_separate = !bind;
> if (est) {
> err = gen_new_estimator(&p->tcfa_bstats, p->cpu_bstats,
> &p->tcfa_rate_est,
> @@ -636,6 +637,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)
> @@ -654,18 +708,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);
>
> @@ -684,8 +750,11 @@ 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);
> @@ -1210,29 +1279,57 @@ static void flow_action_update_hw(struct tc_action *act,
> }
> }
>
> -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)
> + return err;
> +
> + if (hw_count)
> + *hw_count = err;
>
> - err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
> + 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 = err;
> + *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] = {
> @@ -1260,9 +1357,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_update_hw(action, in_hw_count, FLOW_ACT_HW_ADD);
> + flow_action_update_hw(action, in_hw_count,
> + cb ? FLOW_ACT_HW_UPDATE : FLOW_ACT_HW_ADD);
>
> if (skip_sw && !tc_act_in_hw(action->tcfa_flags))
> err = -EINVAL;
> @@ -1275,6 +1373,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 = {};
> @@ -1287,7 +1391,7 @@ int tcf_action_update_hw_stats(struct tc_action *action)
> if (err)
> goto err_out;
>
> - err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
> + err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
>
> if (!err && fl_act.stats.lastused) {
> preempt_disable();
> @@ -1309,7 +1413,8 @@ int tcf_action_update_hw_stats(struct tc_action *action)
> }
> EXPORT_SYMBOL(tcf_action_update_hw_stats);
>
> -int tcf_action_offload_del(struct tc_action *action)
> +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;
> @@ -1325,13 +1430,83 @@ 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)
> + /* do not need to update hw state when deleting action */
> + if (cb && in_hw_count)
> + flow_action_update_hw(action, in_hw_count, FLOW_ACT_HW_DEL);
> +
> + if (!cb && action->in_hw_count != in_hw_count)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +int tcf_action_offload_del(struct tc_action *action)
> +{
> + return tcf_action_offload_del_ex(action, NULL, NULL);
> +}
> +
> +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) || !p->add_separate)
> + 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->tcfa_flags)) {
> + 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;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw_count
2021-10-01 17:30 ` Vlad Buslov
@ 2021-10-27 14:42 ` Simon Horman
0 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2021-10-27 14:42 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, Jamal Hadi Salim, Roi Dayan, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng
Hi Vlad,
On Fri, Oct 01, 2021 at 08:30:38PM +0300, Vlad Buslov wrote:
>
> On Fri 01 Oct 2021 at 14:32, 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.
As per comment on 2/4.
Thanks for your review and sorry for the delay in responding.
I believe that at this point we have addressed most of the points
your raised and plan to post a v3 shortly.
At this point I'd like to relay some responses from Baowen who
has been working on addressing your review.
> > Change the lock usage to fix sleeping in invalid context.
>
> What does this refer to? Looking at the code it is not clear to me which
> lock usage is changed. Or is it just a change log from v1?
Sorry, this is an artifact of our development process that shouldn't have
been left here. Please ignore.
...
> > @@ -44,6 +45,9 @@ struct tc_action {
> > u8 hw_stats;
> > u8 used_hw_stats;
> > bool used_hw_stats_valid;
> > + bool add_separate; /* indicate if the action is created
> > + * independent of any flow
> > + */
>
> This looks like a duplication of flags since this value is derived from
> BIND flag. I understand that you need this because currently all flags
> that are not visible to the userspace are cleared after action is
> created, but maybe it would be better to refactor the code to preserve
> all flags and to only apply TCA_ACT_FLAGS_USER_MASK when dumping to user
> space.
Thanks, we've cleaned things up as you suggest for v3.
...
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC/PATCH net-next v2 5/5] flow_offload: validate flags of filter and actions
2021-10-01 11:32 [RFC/PATCH net-next v2 0/5] allow user to offload tc action to net device Simon Horman
` (3 preceding siblings ...)
2021-10-01 11:32 ` [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw_count Simon Horman
@ 2021-10-01 11:32 ` Simon Horman
2021-10-01 17:45 ` Vlad Buslov
4 siblings, 1 reply; 12+ messages in thread
From: Simon Horman @ 2021-10-01 11:32 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Roi Dayan, Vlad Buslov, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng,
Simon Horman
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/pkt_cls.h | 32 ++++++++++++++++++++++++++++++++
net/sched/cls_flower.c | 5 +++++
net/sched/cls_matchall.c | 6 ++++++
net/sched/cls_u32.c | 11 +++++++++++
4 files changed, 54 insertions(+)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 5c394f04feb5..2d51bed434d2 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -735,6 +735,38 @@ static inline bool tc_in_hw(u32 flags)
return (flags & TCA_CLS_FLAGS_IN_HW) ? true : false;
}
+/**
+ * tcf_exts_validate_actions - check if exts actions flags are compatible with
+ * tc filter flags
+ * @exts: tc filter extensions handle
+ * @flags: tc filter flags
+ *
+ * Returns true if exts actions flags are compatible with tc filter flags
+ */
+static inline bool
+tcf_exts_validate_actions(const struct tcf_exts *exts, u32 flags)
+{
+#ifdef CONFIG_NET_CLS_ACT
+ bool skip_sw = tc_skip_sw(flags);
+ bool skip_hw = tc_skip_hw(flags);
+ int i;
+
+ if (!(skip_sw | skip_hw))
+ return true;
+
+ for (i = 0; i < exts->nr_actions; i++) {
+ struct tc_action *a = exts->actions[i];
+
+ if ((skip_sw && tc_act_skip_hw(a->tcfa_flags)) ||
+ (skip_hw && tc_act_skip_sw(a->tcfa_flags)))
+ return false;
+ }
+ return true;
+#else
+ return true;
+#endif
+}
+
static inline void
tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common,
const struct tcf_proto *tp, u32 flags,
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eb6345a027e1..15e439349124 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2039,6 +2039,11 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
if (err)
goto errout;
+ if (!tcf_exts_validate_actions(&fnew->exts, fnew->flags)) {
+ err = -EINVAL;
+ goto errout;
+ }
+
err = fl_check_assign_mask(head, fnew, fold, mask);
if (err)
goto errout;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 24f0046ce0b3..89dbbb9f31e8 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -231,6 +231,11 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
if (err)
goto err_set_parms;
+ if (!tcf_exts_validate_actions(&new->exts, new->flags)) {
+ err = -EINVAL;
+ goto err_validate;
+ }
+
if (!tc_skip_hw(new->flags)) {
err = mall_replace_hw_filter(tp, new, (unsigned long)new,
extack);
@@ -246,6 +251,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
return 0;
err_replace_hw_filter:
+err_validate:
err_set_parms:
free_percpu(new->pf);
err_alloc_percpu:
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4272814487f0..8bc19af18e4a 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -902,6 +902,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return err;
}
+ if (!tcf_exts_validate_actions(&new->exts, flags)) {
+ u32_destroy_key(new, false);
+ return -EINVAL;
+ }
+
err = u32_replace_hw_knode(tp, new, flags, extack);
if (err) {
u32_destroy_key(new, false);
@@ -1066,6 +1071,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
+ if (!tcf_exts_validate_actions(&n->exts, n->flags)) {
+ err = -EINVAL;
+ goto err_validate;
+ }
+
err = u32_replace_hw_knode(tp, n, flags, extack);
if (err)
goto errhw;
@@ -1086,6 +1096,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return 0;
}
+err_validate:
errhw:
#ifdef CONFIG_CLS_U32_MARK
free_percpu(n->pcpu_success);
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC/PATCH net-next v2 5/5] flow_offload: validate flags of filter and actions
2021-10-01 11:32 ` [RFC/PATCH net-next v2 5/5] flow_offload: validate flags of filter and actions Simon Horman
@ 2021-10-01 17:45 ` Vlad Buslov
2021-10-27 14:42 ` Simon Horman
0 siblings, 1 reply; 12+ messages in thread
From: Vlad Buslov @ 2021-10-01 17:45 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, Jamal Hadi Salim, Roi Dayan, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng
On Fri 01 Oct 2021 at 14:32, Simon Horman <simon.horman@corigine.com> wrote:
> 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/pkt_cls.h | 32 ++++++++++++++++++++++++++++++++
> net/sched/cls_flower.c | 5 +++++
> net/sched/cls_matchall.c | 6 ++++++
> net/sched/cls_u32.c | 11 +++++++++++
> 4 files changed, 54 insertions(+)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 5c394f04feb5..2d51bed434d2 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -735,6 +735,38 @@ static inline bool tc_in_hw(u32 flags)
> return (flags & TCA_CLS_FLAGS_IN_HW) ? true : false;
> }
>
> +/**
> + * tcf_exts_validate_actions - check if exts actions flags are compatible with
> + * tc filter flags
> + * @exts: tc filter extensions handle
> + * @flags: tc filter flags
> + *
> + * Returns true if exts actions flags are compatible with tc filter flags
> + */
> +static inline bool
> +tcf_exts_validate_actions(const struct tcf_exts *exts, u32 flags)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> + bool skip_sw = tc_skip_sw(flags);
> + bool skip_hw = tc_skip_hw(flags);
> + int i;
> +
> + if (!(skip_sw | skip_hw))
> + return true;
> +
> + for (i = 0; i < exts->nr_actions; i++) {
> + struct tc_action *a = exts->actions[i];
> +
> + if ((skip_sw && tc_act_skip_hw(a->tcfa_flags)) ||
> + (skip_hw && tc_act_skip_sw(a->tcfa_flags)))
> + return false;
> + }
> + return true;
> +#else
> + return true;
> +#endif
> +}
> +
There is already a function named tcf_exts_validate() that is called by
classifiers before this new one and is responsible for action validation
and initialization. Having two similarly-named functions is confusing
and additional call complicates classifier init implementations, which
are already quite complex as they are. Could you perform the necessary
validation inside existing exts initialization call chain?
> static inline void
> tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common,
> const struct tcf_proto *tp, u32 flags,
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index eb6345a027e1..15e439349124 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -2039,6 +2039,11 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
> if (err)
> goto errout;
>
> + if (!tcf_exts_validate_actions(&fnew->exts, fnew->flags)) {
> + err = -EINVAL;
> + goto errout;
> + }
> +
> err = fl_check_assign_mask(head, fnew, fold, mask);
> if (err)
> goto errout;
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 24f0046ce0b3..89dbbb9f31e8 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -231,6 +231,11 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
> if (err)
> goto err_set_parms;
>
> + if (!tcf_exts_validate_actions(&new->exts, new->flags)) {
> + err = -EINVAL;
> + goto err_validate;
> + }
> +
> if (!tc_skip_hw(new->flags)) {
> err = mall_replace_hw_filter(tp, new, (unsigned long)new,
> extack);
> @@ -246,6 +251,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
> return 0;
>
> err_replace_hw_filter:
> +err_validate:
> err_set_parms:
> free_percpu(new->pf);
> err_alloc_percpu:
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 4272814487f0..8bc19af18e4a 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -902,6 +902,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> return err;
> }
>
> + if (!tcf_exts_validate_actions(&new->exts, flags)) {
> + u32_destroy_key(new, false);
> + return -EINVAL;
> + }
> +
> err = u32_replace_hw_knode(tp, new, flags, extack);
> if (err) {
> u32_destroy_key(new, false);
> @@ -1066,6 +1071,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> struct tc_u_knode __rcu **ins;
> struct tc_u_knode *pins;
>
> + if (!tcf_exts_validate_actions(&n->exts, n->flags)) {
> + err = -EINVAL;
> + goto err_validate;
> + }
> +
> err = u32_replace_hw_knode(tp, n, flags, extack);
> if (err)
> goto errhw;
> @@ -1086,6 +1096,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> return 0;
> }
>
> +err_validate:
> errhw:
> #ifdef CONFIG_CLS_U32_MARK
> free_percpu(n->pcpu_success);
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC/PATCH net-next v2 5/5] flow_offload: validate flags of filter and actions
2021-10-01 17:45 ` Vlad Buslov
@ 2021-10-27 14:42 ` Simon Horman
0 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2021-10-27 14:42 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, Jamal Hadi Salim, Roi Dayan, Cong Wang, Jiri Pirko,
Baowen Zheng, Louis Peens, oss-drivers, Baowen Zheng
Hi Vlad,
On Fri, Oct 01, 2021 at 08:45:18PM +0300, Vlad Buslov wrote:
>
> On Fri 01 Oct 2021 at 14:32, Simon Horman <simon.horman@corigine.com> wrote:
> > From: Baowen Zheng <baowen.zheng@corigine.com>
> >
> > Add process to validate flags of filter and actions when adding
> > a tc filter.
As per comment on 2/4.
Thanks for your review and sorry for the delay in responding.
I believe that at this point we have addressed most of the points
your raised and plan to post a v3 shortly.
At this point I'd like to relay some responses from Baowen who
has been working on addressing your review.
...
> > +/**
> > + * tcf_exts_validate_actions - check if exts actions flags are compatible with
> > + * tc filter flags
> > + * @exts: tc filter extensions handle
> > + * @flags: tc filter flags
> > + *
> > + * Returns true if exts actions flags are compatible with tc filter flags
> > + */
> > +static inline bool
> > +tcf_exts_validate_actions(const struct tcf_exts *exts, u32 flags)
...
> There is already a function named tcf_exts_validate() that is called by
> classifiers before this new one and is responsible for action validation
> and initialization. Having two similarly-named functions is confusing
> and additional call complicates classifier init implementations, which
> are already quite complex as they are. Could you perform the necessary
> validation inside existing exts initialization call chain?
Thanks, updated v3 to address this as per your suggestion.
...
^ permalink raw reply [flat|nested] 12+ messages in thread