* [PATCH net-next 0/3] flow_offload: hardware offload of TC actions
@ 2021-07-22 9:19 Simon Horman
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
` (2 more replies)
0 siblings, 3 replies; 42+ messages in thread
From: Simon Horman @ 2021-07-22 9:19 UTC (permalink / raw)
To: David Miller, Jakub Kicinski
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens, Simon Horman
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Changes since RFC:
- Fix robot test failure.
- Change actions offload process in action add function rather than action
init.
- Change actions offload delete process after tcf_del_notify to keep
undeleted actions.
- Add process to update actions stats from hardware.
Baowen Zheng (3):
flow_offload: allow user to offload tc action to net device
flow_offload: add process to delete offloaded actions from net device
flow_offload: add process to update action stats from hardware
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 | 1 +
include/net/flow_offload.h | 15 ++
include/net/pkt_cls.h | 20 +++
net/core/flow_offload.c | 26 ++-
net/sched/act_api.c | 162 +++++++++++++++++-
net/sched/cls_api.c | 42 ++++-
10 files changed, 264 insertions(+), 11 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 [PATCH net-next 0/3] flow_offload: hardware offload of TC actions Simon Horman
@ 2021-07-22 9:19 ` Simon Horman
2021-07-22 12:24 ` Roi Dayan
` (5 more replies)
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
2021-07-22 9:19 ` [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware Simon Horman
2 siblings, 6 replies; 42+ messages in thread
From: Simon Horman @ 2021-07-22 9:19 UTC (permalink / raw)
To: David Miller, Jakub Kicinski
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens, Simon Horman
From: Baowen Zheng <baowen.zheng@corigine.com>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We offload the tc action mainly for ovs meter configuration.
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.
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/flow_offload.h | 15 ++++++++
include/net/pkt_cls.h | 15 ++++++++
net/core/flow_offload.c | 26 +++++++++++++-
net/sched/act_api.c | 33 +++++++++++++++++
net/sched/cls_api.c | 36 ++++++++++++++++---
9 files changed, 128 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 5e4429b14b8c..edbbf7b4df77 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1951,7 +1951,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 059799e4f483..111daacc4cc3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
@@ -486,6 +486,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 2406d33356ad..88bbc86347b4 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -1869,6 +1869,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 42f6f866d5f3..b138219baf6f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -923,6 +923,7 @@ enum tc_setup_type {
TC_SETUP_QDISC_TBF,
TC_SETUP_QDISC_FIFO,
TC_SETUP_QDISC_HTB,
+ TC_SETUP_ACT,
};
/* These structures hold the attributes of bpf state that are being passed
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 69c9eabf8325..26644596fd54 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -553,6 +553,21 @@ struct flow_cls_offload {
u32 classid;
};
+enum flow_act_command {
+ FLOW_ACT_REPLACE,
+ FLOW_ACT_DESTROY,
+ FLOW_ACT_STATS,
+};
+
+struct flow_offload_action {
+ struct netlink_ext_ack *extack;
+ enum flow_act_command command;
+ 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 ec7823921bd2..cd4cf6b10f5d 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -266,6 +266,9 @@ 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 void
tcf_exts_stats_update(const struct tcf_exts *exts,
u64 bytes, u64 packets, u64 drops, u64 lastuse,
@@ -536,8 +539,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,
@@ -558,6 +572,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(struct tc_action *actions[]);
#ifdef CONFIG_NET_CLS_ACT
int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 715b67f6c62f..0fa2f75cc9b3 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; \
@@ -476,6 +497,9 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
mutex_unlock(&flow_indr_block_lock);
- return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
+ if (bo)
+ return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
+ else
+ return 0;
}
EXPORT_SYMBOL(flow_indr_dev_setup_offload);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 998a2374f7ae..185f17ea60d5 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
return ERR_PTR(err);
}
+/* offload the tc command after inserted */
+int tcf_action_offload_cmd(struct tc_action *actions[],
+ struct netlink_ext_ack *extack)
+{
+ struct flow_offload_action *fl_act;
+ int err = 0;
+
+ fl_act = flow_action_alloc(tcf_act_num_actions(actions));
+ if (!fl_act)
+ return -ENOMEM;
+
+ fl_act->extack = extack;
+ err = tc_setup_action(&fl_act->action, actions);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Failed to setup tc actions for offload\n");
+ goto err_out;
+ }
+ fl_act->command = FLOW_ACT_REPLACE;
+
+ flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
+
+ tc_cleanup_flow_action(&fl_act->action);
+
+err_out:
+ kfree(fl_act);
+ return err;
+}
+EXPORT_SYMBOL(tcf_action_offload_cmd);
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
return ret;
ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
+ /* offload actions to hardware if possible */
+ tcf_action_offload_cmd(actions, extack);
+
/* only put existing actions */
for (i = 0; i < TCA_ACT_MAX_PRIO; i++)
if (init_res[i] == ACT_P_CREATED)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c8cb59a11098..9b9770dab5e8 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,22 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
}
EXPORT_SYMBOL(tcf_exts_num_actions);
+unsigned int tcf_act_num_actions(struct tc_action *actions[])
+{
+ unsigned int num_acts = 0;
+ struct tc_action *act;
+ int i;
+
+ tcf_act_for_each_action(i, act, actions) {
+ if (is_tcf_pedit(act))
+ num_acts += tcf_pedit_nkeys(act);
+ else
+ num_acts++;
+ }
+ return num_acts;
+}
+EXPORT_SYMBOL(tcf_act_num_actions);
+
#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] 42+ messages in thread
* [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from net device
2021-07-22 9:19 [PATCH net-next 0/3] flow_offload: hardware offload of TC actions Simon Horman
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-07-22 9:19 ` Simon Horman
2021-07-22 14:25 ` Vlad Buslov
` (3 more replies)
2021-07-22 9:19 ` [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware Simon Horman
2 siblings, 4 replies; 42+ messages in thread
From: Simon Horman @ 2021-07-22 9:19 UTC (permalink / raw)
To: David Miller, Jakub Kicinski
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens, Simon Horman
From: Baowen Zheng <baowen.zheng@corigine.com>
Add a basic process to delete offloaded actions from net device.
Should not remove the offloaded action entries if the action
fails to delete in tcf_del_notify.
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 | 1 +
net/sched/act_api.c | 112 +++++++++++++++++++++++++++++++++++-------
net/sched/cls_api.c | 14 ++++--
3 files changed, 106 insertions(+), 21 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index cd4cf6b10f5d..03dae225d64f 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -573,6 +573,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp,
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(struct tc_action *actions[]);
+unsigned int tcf_act_num_actions_single(struct tc_action *act);
#ifdef CONFIG_NET_CLS_ACT
int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 185f17ea60d5..23a4538916af 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1060,36 +1060,109 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
return ERR_PTR(err);
}
-/* offload the tc command after inserted */
-int tcf_action_offload_cmd(struct tc_action *actions[],
- struct netlink_ext_ack *extack)
+int tcf_action_offload_cmd_pre(struct tc_action *actions[],
+ enum flow_act_command cmd,
+ struct netlink_ext_ack *extack,
+ struct flow_offload_action **fl_act)
{
- struct flow_offload_action *fl_act;
+ struct flow_offload_action *fl_act_p;
int err = 0;
- fl_act = flow_action_alloc(tcf_act_num_actions(actions));
- if (!fl_act)
+ fl_act_p = flow_action_alloc(tcf_act_num_actions(actions));
+ if (!fl_act_p)
return -ENOMEM;
- fl_act->extack = extack;
- err = tc_setup_action(&fl_act->action, actions);
+ fl_act_p->extack = extack;
+ fl_act_p->command = cmd;
+ err = tc_setup_action(&fl_act_p->action, actions);
if (err) {
NL_SET_ERR_MSG_MOD(extack,
"Failed to setup tc actions for offload\n");
goto err_out;
}
- fl_act->command = FLOW_ACT_REPLACE;
+ *fl_act = fl_act_p;
+ return 0;
+err_out:
+ kfree(fl_act_p);
+ return err;
+}
+EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
+
+int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
+ struct netlink_ext_ack *extack)
+{
+ if (IS_ERR(fl_act))
+ return PTR_ERR(fl_act);
flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
tc_cleanup_flow_action(&fl_act->action);
-
-err_out:
kfree(fl_act);
- return err;
+ return 0;
+}
+
+/* offload the tc command after inserted */
+int tcf_action_offload_cmd(struct tc_action *actions[],
+ struct netlink_ext_ack *extack)
+{
+ struct flow_offload_action *fl_act;
+ int err = 0;
+
+ err = tcf_action_offload_cmd_pre(actions,
+ FLOW_ACT_REPLACE,
+ extack,
+ &fl_act);
+ if (err)
+ return err;
+
+ return tcf_action_offload_cmd_post(fl_act, extack);
}
EXPORT_SYMBOL(tcf_action_offload_cmd);
+/* offload the tc command after deleted */
+int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
+ struct tc_action *actions[],
+ struct netlink_ext_ack *extack,
+ int fallback_num)
+{
+ int fallback_entries = 0;
+ struct tc_action *act;
+ int total_entries = 0;
+ int i;
+
+ if (!fl_act)
+ return -EINVAL;
+
+ if (fallback_num) {
+ /* for each the actions to fallback the action entries remain in the actions */
+ for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
+ act = actions[i];
+ if (!act)
+ continue;
+
+ fallback_entries += tcf_act_num_actions_single(act);
+ }
+ fallback_entries += fallback_num;
+ }
+ total_entries = fl_act->action.num_entries;
+ if (total_entries > fallback_entries) {
+ /* just offload the actions that is not fallback and start with the actions */
+ fl_act->action.num_entries -= fallback_entries;
+ flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
+
+ /* recovery num_entries for cleanup */
+ fl_act->action.num_entries = total_entries;
+ } else {
+ NL_SET_ERR_MSG(extack, "no entries to offload when deleting the tc actions");
+ }
+
+ tc_cleanup_flow_action(&fl_act->action);
+
+ kfree(fl_act);
+ return 0;
+}
+EXPORT_SYMBOL(tcf_action_offload_del_post);
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -1393,7 +1466,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
return err;
}
-static int tcf_action_delete(struct net *net, struct tc_action *actions[])
+static int tcf_action_delete(struct net *net, struct tc_action *actions[], int *fallbacknum)
{
int i;
@@ -1407,6 +1480,7 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
u32 act_index = a->tcfa_index;
actions[i] = NULL;
+ *fallbacknum = tcf_act_num_actions_single(a);
if (tcf_action_put(a)) {
/* last reference, action was deleted concurrently */
module_put(ops->owner);
@@ -1419,12 +1493,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
return ret;
}
}
+ *fallbacknum = 0;
return 0;
}
static int
tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
- u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
+ u32 portid, size_t attr_size, struct netlink_ext_ack *extack, int *fallbacknum)
{
int ret;
struct sk_buff *skb;
@@ -1442,7 +1517,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
}
/* now do the delete */
- ret = tcf_action_delete(net, actions);
+ ret = tcf_action_delete(net, actions, fallbacknum);
if (ret < 0) {
NL_SET_ERR_MSG(extack, "Failed to delete TC action");
kfree_skb(skb);
@@ -1458,11 +1533,12 @@ static int
tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
u32 portid, int event, struct netlink_ext_ack *extack)
{
- int i, ret;
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
struct tc_action *act;
size_t attr_size = 0;
struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
+ struct flow_offload_action *fl_act;
+ int i, ret, fallback_num;
ret = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
extack);
@@ -1492,7 +1568,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
if (event == RTM_GETACTION)
ret = tcf_get_notify(net, portid, n, actions, event, extack);
else { /* delete */
- ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
+ tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack, &fl_act);
+ ret = tcf_del_notify(net, n, actions, portid, attr_size, extack, &fallback_num);
+ tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
if (ret)
goto err;
return 0;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 9b9770dab5e8..23ce021f07f8 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3755,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);
+
unsigned int tcf_act_num_actions(struct tc_action *actions[])
{
unsigned int num_acts = 0;
@@ -3762,10 +3771,7 @@ unsigned int tcf_act_num_actions(struct tc_action *actions[])
int i;
tcf_act_for_each_action(i, act, actions) {
- if (is_tcf_pedit(act))
- num_acts += tcf_pedit_nkeys(act);
- else
- num_acts++;
+ num_acts += tcf_act_num_actions_single(act);
}
return num_acts;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware
2021-07-22 9:19 [PATCH net-next 0/3] flow_offload: hardware offload of TC actions Simon Horman
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
@ 2021-07-22 9:19 ` Simon Horman
2021-07-22 14:55 ` Vlad Buslov
2021-08-03 11:24 ` Jamal Hadi Salim
2 siblings, 2 replies; 42+ messages in thread
From: Simon Horman @ 2021-07-22 9:19 UTC (permalink / raw)
To: David Miller, Jakub Kicinski
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens, Simon Horman
From: Baowen Zheng <baowen.zheng@corigine.com>
When collecting stats for actions update them using both
both hardware and software counters.
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/flow_offload.h | 2 +-
include/net/pkt_cls.h | 4 ++++
net/sched/act_api.c | 49 ++++++++++++++++++++++++++++++++++----
4 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 086b291e9530..fe8331b5efce 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -233,6 +233,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_update_hw_stats(struct tc_action *action);
int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
struct tcf_chain **handle,
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 26644596fd54..467688fff7ce 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -560,7 +560,7 @@ enum flow_act_command {
};
struct flow_offload_action {
- struct netlink_ext_ack *extack;
+ struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/
enum flow_act_command command;
struct flow_stats stats;
struct flow_action action;
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 03dae225d64f..569c9294b15b 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -282,6 +282,10 @@ tcf_exts_stats_update(const struct tcf_exts *exts,
for (i = 0; i < exts->nr_actions; i++) {
struct tc_action *a = exts->actions[i];
+ /* if stats from hw, just skip */
+ if (!tcf_action_update_hw_stats(a))
+ continue;
+
tcf_action_stats_update(a, bytes, packets, drops,
lastuse, true);
a->used_hw_stats = used_hw_stats;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 23a4538916af..7d5535bc2c13 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1089,15 +1089,18 @@ int tcf_action_offload_cmd_pre(struct tc_action *actions[],
EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack,
+ bool keep_fl_act)
{
if (IS_ERR(fl_act))
return PTR_ERR(fl_act);
flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
- tc_cleanup_flow_action(&fl_act->action);
- kfree(fl_act);
+ if (!keep_fl_act) {
+ tc_cleanup_flow_action(&fl_act->action);
+ kfree(fl_act);
+ }
return 0;
}
@@ -1115,10 +1118,45 @@ int tcf_action_offload_cmd(struct tc_action *actions[],
if (err)
return err;
- return tcf_action_offload_cmd_post(fl_act, extack);
+ return tcf_action_offload_cmd_post(fl_act, extack, false);
}
EXPORT_SYMBOL(tcf_action_offload_cmd);
+int tcf_action_update_hw_stats(struct tc_action *action)
+{
+ struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
+ [0] = action,
+ };
+ struct flow_offload_action *fl_act;
+ int err = 0;
+
+ err = tcf_action_offload_cmd_pre(actions,
+ FLOW_ACT_STATS,
+ NULL,
+ &fl_act);
+ if (err)
+ goto err_out;
+
+ err = tcf_action_offload_cmd_post(fl_act, NULL, true);
+
+ if (fl_act->stats.lastused) {
+ tcf_action_stats_update(action, fl_act->stats.bytes,
+ fl_act->stats.pkts,
+ fl_act->stats.drops,
+ fl_act->stats.lastused,
+ true);
+ err = 0;
+ } else {
+ err = -EOPNOTSUPP;
+ }
+ tc_cleanup_flow_action(&fl_act->action);
+ kfree(fl_act);
+
+err_out:
+ return err;
+}
+EXPORT_SYMBOL(tcf_action_update_hw_stats);
+
/* offload the tc command after deleted */
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
struct tc_action *actions[],
@@ -1255,6 +1293,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] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-07-22 12:24 ` Roi Dayan
2021-07-22 13:19 ` Simon Horman
2021-07-22 13:29 ` Vlad Buslov
` (4 subsequent siblings)
5 siblings, 1 reply; 42+ messages in thread
From: Roi Dayan @ 2021-07-22 12:24 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens
On 2021-07-22 12:19 PM, Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> Use flow_indr_dev_register/flow_indr_dev_setup_offload to
> offload tc action.
>
> We offload the tc action mainly for ovs meter configuration.
> 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.
>
> 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/flow_offload.h | 15 ++++++++
> include/net/pkt_cls.h | 15 ++++++++
> net/core/flow_offload.c | 26 +++++++++++++-
> net/sched/act_api.c | 33 +++++++++++++++++
> net/sched/cls_api.c | 36 ++++++++++++++++---
> 9 files changed, 128 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> index 5e4429b14b8c..edbbf7b4df77 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> @@ -1951,7 +1951,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 059799e4f483..111daacc4cc3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> @@ -486,6 +486,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 2406d33356ad..88bbc86347b4 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -1869,6 +1869,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 42f6f866d5f3..b138219baf6f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -923,6 +923,7 @@ enum tc_setup_type {
> TC_SETUP_QDISC_TBF,
> TC_SETUP_QDISC_FIFO,
> TC_SETUP_QDISC_HTB,
> + TC_SETUP_ACT,
> };
>
> /* These structures hold the attributes of bpf state that are being passed
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 69c9eabf8325..26644596fd54 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -553,6 +553,21 @@ struct flow_cls_offload {
> u32 classid;
> };
>
> +enum flow_act_command {
> + FLOW_ACT_REPLACE,
> + FLOW_ACT_DESTROY,
> + FLOW_ACT_STATS,
> +};
> +
> +struct flow_offload_action {
> + struct netlink_ext_ack *extack;
> + enum flow_act_command command;
> + 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 ec7823921bd2..cd4cf6b10f5d 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -266,6 +266,9 @@ 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 void
> tcf_exts_stats_update(const struct tcf_exts *exts,
> u64 bytes, u64 packets, u64 drops, u64 lastuse,
> @@ -536,8 +539,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,
> @@ -558,6 +572,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(struct tc_action *actions[]);
>
> #ifdef CONFIG_NET_CLS_ACT
> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 715b67f6c62f..0fa2f75cc9b3 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;
Hi Simon,
Our automatic tests got a trace from flow_action_alloc()
introduced in this series.
I don't have specific commands right now but maybe its easy
to reproduce with option CONFIG_DEBUG_ATOMIC_SLEEP=y
fl_dump->fl_hw_update_stats->fl_hw_update_stats->tcf_exts_stats_update
->tcf_action_update_hw_stats->tcf_action_offload_cmd_pre->
->flow_action_alloc
Thanks,
Roi
[ 761.008866] BUG: sleeping function called from invalid context at
include/linux/sched/mm.h:201
[ 761.011047] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid:
20194, name: handler1
[ 761.013052] INFO: lockdep is turned off.
[ 761.014102] CPU: 1 PID: 20194 Comm: handler1 Not tainted
5.14.0-rc1_2021_07_22_09_58_22_ #1
[ 761.015891] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
[ 761.017190] Call Trace:
[ 761.017556] dump_stack_lvl+0x57/0x7d
[ 761.018038] ___might_sleep.cold+0x15a/0x1b7
[ 761.018583] __kmalloc+0x259/0x430
[ 761.019044] ? flow_action_alloc+0x28/0xd0
[ 761.019574] ? lock_release+0x460/0x750
[ 761.020068] flow_action_alloc+0x28/0xd0
[ 761.020586] tcf_action_offload_cmd_pre+0x25/0x130
[ 761.021188] tcf_action_update_hw_stats+0x9e/0x2e0
[ 761.021791] ? tcf_action_offload_cmd+0xe0/0xe0
[ 761.022358] ? is_bpf_text_address+0x73/0x110
[ 761.022918] ? mlx5e_delete_flower+0xf50/0xf50 [mlx5_core]
[ 761.023713] ? tc_setup_cb_call+0x181/0x270
[ 761.024254] fl_hw_update_stats+0x2e4/0x4e0 [cls_flower]
[ 761.024908] ? fl_hw_replace_filter+0x6a0/0x6a0 [cls_flower]
[ 761.025595] ? lock_release+0x460/0x750
[ 761.026095] ? memcpy+0x39/0x60
[ 761.026537] fl_dump+0x499/0x5f0 [cls_flower]
[ 761.027106] ? fl_tmplt_dump+0x220/0x220 [cls_flower]
[ 761.027746] ? memcpy+0x39/0x60
[ 761.028173] ? nla_put+0x15f/0x1c0
[ 761.028634] tcf_fill_node+0x50f/0x930
[ 761.029137] ? __tcf_get_next_proto+0x470/0x470
[ 761.029725] ? memset+0x20/0x40
[ 761.030167] tfilter_notify+0x15e/0x2d0
[ 761.030681] tc_new_tfilter+0x1128/0x1ea0
[ 761.031218] ? tc_del_tfilter+0x13e0/0x13e0
[ 761.031768] ? security_capable+0x51/0x90
[ 761.032280] ? tc_del_tfilter+0x13e0/0x13e0
[ 761.032826] rtnetlink_rcv_msg+0x637/0x950
> +
> + 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; \
> @@ -476,6 +497,9 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
>
> mutex_unlock(&flow_indr_block_lock);
>
> - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
> + if (bo)
> + return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
> + else
> + return 0;
> }
> EXPORT_SYMBOL(flow_indr_dev_setup_offload);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 998a2374f7ae..185f17ea60d5 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> +/* offload the tc command after inserted */
> +int tcf_action_offload_cmd(struct tc_action *actions[],
> + struct netlink_ext_ack *extack)
> +{
> + struct flow_offload_action *fl_act;
> + int err = 0;
> +
> + fl_act = flow_action_alloc(tcf_act_num_actions(actions));
> + if (!fl_act)
> + return -ENOMEM;
> +
> + fl_act->extack = extack;
> + err = tc_setup_action(&fl_act->action, actions);
> + if (err) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Failed to setup tc actions for offload\n");
> + goto err_out;
> + }
> + fl_act->command = FLOW_ACT_REPLACE;
> +
> + flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
> +
> + tc_cleanup_flow_action(&fl_act->action);
> +
> +err_out:
> + kfree(fl_act);
> + return err;
> +}
> +EXPORT_SYMBOL(tcf_action_offload_cmd);
> +
> /* Returns numbers of initialized actions or negative error. */
>
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> @@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> return ret;
> ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
>
> + /* offload actions to hardware if possible */
> + tcf_action_offload_cmd(actions, extack);
> +
> /* only put existing actions */
> for (i = 0; i < TCA_ACT_MAX_PRIO; i++)
> if (init_res[i] == ACT_P_CREATED)
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index c8cb59a11098..9b9770dab5e8 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,22 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> }
> EXPORT_SYMBOL(tcf_exts_num_actions);
>
> +unsigned int tcf_act_num_actions(struct tc_action *actions[])
> +{
> + unsigned int num_acts = 0;
> + struct tc_action *act;
> + int i;
> +
> + tcf_act_for_each_action(i, act, actions) {
> + if (is_tcf_pedit(act))
> + num_acts += tcf_pedit_nkeys(act);
> + else
> + num_acts++;
> + }
> + return num_acts;
> +}
> +EXPORT_SYMBOL(tcf_act_num_actions);
> +
> #ifdef CONFIG_NET_CLS_ACT
> static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
> u32 *p_block_index,
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 12:24 ` Roi Dayan
@ 2021-07-22 13:19 ` Simon Horman
0 siblings, 0 replies; 42+ messages in thread
From: Simon Horman @ 2021-07-22 13:19 UTC (permalink / raw)
To: Roi Dayan
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Thu, Jul 22, 2021 at 03:24:07PM +0300, Roi Dayan wrote:
>
>
> On 2021-07-22 12:19 PM, Simon Horman wrote:
> > From: Baowen Zheng <baowen.zheng@corigine.com>
> >
> > Use flow_indr_dev_register/flow_indr_dev_setup_offload to
> > offload tc action.
> >
> > We offload the tc action mainly for ovs meter configuration.
> > 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.
> >
> > 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/flow_offload.h | 15 ++++++++
> > include/net/pkt_cls.h | 15 ++++++++
> > net/core/flow_offload.c | 26 +++++++++++++-
> > net/sched/act_api.c | 33 +++++++++++++++++
> > net/sched/cls_api.c | 36 ++++++++++++++++---
> > 9 files changed, 128 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> > index 5e4429b14b8c..edbbf7b4df77 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> > @@ -1951,7 +1951,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 059799e4f483..111daacc4cc3 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> > @@ -486,6 +486,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 2406d33356ad..88bbc86347b4 100644
> > --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> > @@ -1869,6 +1869,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 42f6f866d5f3..b138219baf6f 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -923,6 +923,7 @@ enum tc_setup_type {
> > TC_SETUP_QDISC_TBF,
> > TC_SETUP_QDISC_FIFO,
> > TC_SETUP_QDISC_HTB,
> > + TC_SETUP_ACT,
> > };
> > /* These structures hold the attributes of bpf state that are being passed
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index 69c9eabf8325..26644596fd54 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -553,6 +553,21 @@ struct flow_cls_offload {
> > u32 classid;
> > };
> > +enum flow_act_command {
> > + FLOW_ACT_REPLACE,
> > + FLOW_ACT_DESTROY,
> > + FLOW_ACT_STATS,
> > +};
> > +
> > +struct flow_offload_action {
> > + struct netlink_ext_ack *extack;
> > + enum flow_act_command command;
> > + 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 ec7823921bd2..cd4cf6b10f5d 100644
> > --- a/include/net/pkt_cls.h
> > +++ b/include/net/pkt_cls.h
> > @@ -266,6 +266,9 @@ 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 void
> > tcf_exts_stats_update(const struct tcf_exts *exts,
> > u64 bytes, u64 packets, u64 drops, u64 lastuse,
> > @@ -536,8 +539,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,
> > @@ -558,6 +572,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(struct tc_action *actions[]);
> > #ifdef CONFIG_NET_CLS_ACT
> > int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
> > diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> > index 715b67f6c62f..0fa2f75cc9b3 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;
>
> Hi Simon,
>
> Our automatic tests got a trace from flow_action_alloc()
> introduced in this series.
> I don't have specific commands right now but maybe its easy
> to reproduce with option CONFIG_DEBUG_ATOMIC_SLEEP=y
>
> fl_dump->fl_hw_update_stats->fl_hw_update_stats->tcf_exts_stats_update
> ->tcf_action_update_hw_stats->tcf_action_offload_cmd_pre->
> ->flow_action_alloc
>
> Thanks,
> Roi
Thanks Roi,
we'll look into this.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
2021-07-22 12:24 ` Roi Dayan
@ 2021-07-22 13:29 ` Vlad Buslov
2021-07-22 13:33 ` Jamal Hadi Salim
2021-07-22 13:57 ` kernel test robot
` (3 subsequent siblings)
5 siblings, 1 reply; 42+ messages in thread
From: Vlad Buslov @ 2021-07-22 13:29 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
> 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.
>
> 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/flow_offload.h | 15 ++++++++
> include/net/pkt_cls.h | 15 ++++++++
> net/core/flow_offload.c | 26 +++++++++++++-
> net/sched/act_api.c | 33 +++++++++++++++++
> net/sched/cls_api.c | 36 ++++++++++++++++---
> 9 files changed, 128 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> index 5e4429b14b8c..edbbf7b4df77 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> @@ -1951,7 +1951,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 059799e4f483..111daacc4cc3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> @@ -486,6 +486,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 2406d33356ad..88bbc86347b4 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -1869,6 +1869,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 42f6f866d5f3..b138219baf6f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -923,6 +923,7 @@ enum tc_setup_type {
> TC_SETUP_QDISC_TBF,
> TC_SETUP_QDISC_FIFO,
> TC_SETUP_QDISC_HTB,
> + TC_SETUP_ACT,
> };
>
> /* These structures hold the attributes of bpf state that are being passed
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 69c9eabf8325..26644596fd54 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -553,6 +553,21 @@ struct flow_cls_offload {
> u32 classid;
> };
>
> +enum flow_act_command {
> + FLOW_ACT_REPLACE,
> + FLOW_ACT_DESTROY,
> + FLOW_ACT_STATS,
> +};
> +
> +struct flow_offload_action {
> + struct netlink_ext_ack *extack;
> + enum flow_act_command command;
> + 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 ec7823921bd2..cd4cf6b10f5d 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -266,6 +266,9 @@ 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 void
> tcf_exts_stats_update(const struct tcf_exts *exts,
> u64 bytes, u64 packets, u64 drops, u64 lastuse,
> @@ -536,8 +539,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,
> @@ -558,6 +572,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(struct tc_action *actions[]);
>
> #ifdef CONFIG_NET_CLS_ACT
> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 715b67f6c62f..0fa2f75cc9b3 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; \
> @@ -476,6 +497,9 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch,
>
> mutex_unlock(&flow_indr_block_lock);
>
> - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
> + if (bo)
> + return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0;
> + else
> + return 0;
> }
> EXPORT_SYMBOL(flow_indr_dev_setup_offload);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 998a2374f7ae..185f17ea60d5 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> +/* offload the tc command after inserted */
> +int tcf_action_offload_cmd(struct tc_action *actions[],
> + struct netlink_ext_ack *extack)
> +{
> + struct flow_offload_action *fl_act;
> + int err = 0;
> +
> + fl_act = flow_action_alloc(tcf_act_num_actions(actions));
> + if (!fl_act)
> + return -ENOMEM;
> +
> + fl_act->extack = extack;
> + err = tc_setup_action(&fl_act->action, actions);
> + if (err) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Failed to setup tc actions for offload\n");
> + goto err_out;
> + }
> + fl_act->command = FLOW_ACT_REPLACE;
> +
> + flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
> +
> + tc_cleanup_flow_action(&fl_act->action);
> +
> +err_out:
> + kfree(fl_act);
> + return err;
> +}
> +EXPORT_SYMBOL(tcf_action_offload_cmd);
> +
> /* Returns numbers of initialized actions or negative error. */
>
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> @@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> return ret;
> ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
>
> + /* offload actions to hardware if possible */
> + tcf_action_offload_cmd(actions, extack);
> +
I think this has already been suggested for RFC, but some sort of
visibility for offload status of action would be extremely welcome.
Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
filters.
> /* only put existing actions */
> for (i = 0; i < TCA_ACT_MAX_PRIO; i++)
> if (init_res[i] == ACT_P_CREATED)
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index c8cb59a11098..9b9770dab5e8 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,22 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> }
> EXPORT_SYMBOL(tcf_exts_num_actions);
>
> +unsigned int tcf_act_num_actions(struct tc_action *actions[])
> +{
> + unsigned int num_acts = 0;
> + struct tc_action *act;
> + int i;
> +
> + tcf_act_for_each_action(i, act, actions) {
> + if (is_tcf_pedit(act))
> + num_acts += tcf_pedit_nkeys(act);
> + else
> + num_acts++;
> + }
> + return num_acts;
> +}
> +EXPORT_SYMBOL(tcf_act_num_actions);
> +
> #ifdef CONFIG_NET_CLS_ACT
> static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
> u32 *p_block_index,
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 13:29 ` Vlad Buslov
@ 2021-07-22 13:33 ` Jamal Hadi Salim
2021-07-27 13:04 ` Simon Horman
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-07-22 13:33 UTC (permalink / raw)
To: Vlad Buslov, Simon Horman
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng, Louis Peens
On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
> On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
>> 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.
>>
>> 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 ++
>> 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
[..]
>>
>> + /* offload actions to hardware if possible */
>> + tcf_action_offload_cmd(actions, extack);
>> +
>
> I think this has already been suggested for RFC, but some sort of
> visibility for offload status of action would be extremely welcome.
> Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
> filters.
>
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
2021-07-22 12:24 ` Roi Dayan
2021-07-22 13:29 ` Vlad Buslov
@ 2021-07-22 13:57 ` kernel test robot
2021-07-22 15:31 ` kernel test robot
` (2 subsequent siblings)
5 siblings, 0 replies; 42+ messages in thread
From: kernel test robot @ 2021-07-22 13:57 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: kbuild-all, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
oss-drivers, Baowen Zheng, Louis Peens, Simon Horman
[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]
Hi Simon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c2255ff47768c94a0ebc3968f007928bb47ea43b
config: microblaze-randconfig-r011-20210722 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9228a8efdbf7a736354b87c0db3260dd7d2c4abd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
git checkout 9228a8efdbf7a736354b87c0db3260dd7d2c4abd
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=microblaze
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> net/sched/act_api.c:1064:5: warning: no previous prototype for 'tcf_action_offload_cmd' [-Wmissing-prototypes]
1064 | int tcf_action_offload_cmd(struct tc_action *actions[],
| ^~~~~~~~~~~~~~~~~~~~~~
vim +/tcf_action_offload_cmd +1064 net/sched/act_api.c
1062
1063 /* offload the tc command after inserted */
> 1064 int tcf_action_offload_cmd(struct tc_action *actions[],
1065 struct netlink_ext_ack *extack)
1066 {
1067 struct flow_offload_action *fl_act;
1068 int err = 0;
1069
1070 fl_act = flow_action_alloc(tcf_act_num_actions(actions));
1071 if (!fl_act)
1072 return -ENOMEM;
1073
1074 fl_act->extack = extack;
1075 err = tc_setup_action(&fl_act->action, actions);
1076 if (err) {
1077 NL_SET_ERR_MSG_MOD(extack,
1078 "Failed to setup tc actions for offload\n");
1079 goto err_out;
1080 }
1081 fl_act->command = FLOW_ACT_REPLACE;
1082
1083 flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
1084
1085 tc_cleanup_flow_action(&fl_act->action);
1086
1087 err_out:
1088 kfree(fl_act);
1089 return err;
1090 }
1091 EXPORT_SYMBOL(tcf_action_offload_cmd);
1092
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39738 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from net device
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
@ 2021-07-22 14:25 ` Vlad Buslov
2021-07-22 14:50 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: Vlad Buslov @ 2021-07-22 14:25 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Thu 22 Jul 2021 at 12:19, Simon Horman <simon.horman@corigine.com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> Add a basic process to delete offloaded actions from net device.
>
> Should not remove the offloaded action entries if the action
> fails to delete in tcf_del_notify.
>
> 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 | 1 +
> net/sched/act_api.c | 112 +++++++++++++++++++++++++++++++++++-------
> net/sched/cls_api.c | 14 ++++--
> 3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index cd4cf6b10f5d..03dae225d64f 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -573,6 +573,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp,
> 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(struct tc_action *actions[]);
> +unsigned int tcf_act_num_actions_single(struct tc_action *act);
>
> #ifdef CONFIG_NET_CLS_ACT
> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 185f17ea60d5..23a4538916af 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1060,36 +1060,109 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> -/* offload the tc command after inserted */
> -int tcf_action_offload_cmd(struct tc_action *actions[],
> - struct netlink_ext_ack *extack)
> +int tcf_action_offload_cmd_pre(struct tc_action *actions[],
> + enum flow_act_command cmd,
> + struct netlink_ext_ack *extack,
> + struct flow_offload_action **fl_act)
> {
> - struct flow_offload_action *fl_act;
> + struct flow_offload_action *fl_act_p;
> int err = 0;
>
> - fl_act = flow_action_alloc(tcf_act_num_actions(actions));
> - if (!fl_act)
> + fl_act_p = flow_action_alloc(tcf_act_num_actions(actions));
> + if (!fl_act_p)
> return -ENOMEM;
>
> - fl_act->extack = extack;
> - err = tc_setup_action(&fl_act->action, actions);
> + fl_act_p->extack = extack;
> + fl_act_p->command = cmd;
> + err = tc_setup_action(&fl_act_p->action, actions);
> if (err) {
> NL_SET_ERR_MSG_MOD(extack,
> "Failed to setup tc actions for offload\n");
> goto err_out;
> }
> - fl_act->command = FLOW_ACT_REPLACE;
> + *fl_act = fl_act_p;
> + return 0;
> +err_out:
> + kfree(fl_act_p);
> + return err;
> +}
> +EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
This doesn't seem be used anywhere outside this file.
> +
> +int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
> + struct netlink_ext_ack *extack)
> +{
> + if (IS_ERR(fl_act))
> + return PTR_ERR(fl_act);
>
> flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
>
> tc_cleanup_flow_action(&fl_act->action);
> -
> -err_out:
> kfree(fl_act);
> - return err;
> + return 0;
> +}
This one is not exported, by is non-static.
> +
> +/* offload the tc command after inserted */
> +int tcf_action_offload_cmd(struct tc_action *actions[],
> + struct netlink_ext_ack *extack)
> +{
> + struct flow_offload_action *fl_act;
> + int err = 0;
> +
> + err = tcf_action_offload_cmd_pre(actions,
> + FLOW_ACT_REPLACE,
> + extack,
> + &fl_act);
> + if (err)
> + return err;
> +
> + return tcf_action_offload_cmd_post(fl_act, extack);
> }
> EXPORT_SYMBOL(tcf_action_offload_cmd);
>
> +/* offload the tc command after deleted */
> +int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
> + struct tc_action *actions[],
> + struct netlink_ext_ack *extack,
> + int fallback_num)
> +{
> + int fallback_entries = 0;
> + struct tc_action *act;
> + int total_entries = 0;
> + int i;
> +
> + if (!fl_act)
> + return -EINVAL;
> +
> + if (fallback_num) {
> + /* for each the actions to fallback the action entries remain in the actions */
> + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
> + act = actions[i];
> + if (!act)
> + continue;
> +
> + fallback_entries += tcf_act_num_actions_single(act);
> + }
> + fallback_entries += fallback_num;
> + }
> + total_entries = fl_act->action.num_entries;
> + if (total_entries > fallback_entries) {
> + /* just offload the actions that is not fallback and start with the actions */
> + fl_act->action.num_entries -= fallback_entries;
> + flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
> +
> + /* recovery num_entries for cleanup */
> + fl_act->action.num_entries = total_entries;
> + } else {
> + NL_SET_ERR_MSG(extack, "no entries to offload when deleting the tc actions");
> + }
> +
> + tc_cleanup_flow_action(&fl_act->action);
> +
> + kfree(fl_act);
> + return 0;
> +}
> +EXPORT_SYMBOL(tcf_action_offload_del_post);
> +
> /* Returns numbers of initialized actions or negative error. */
>
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> @@ -1393,7 +1466,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
> return err;
> }
>
> -static int tcf_action_delete(struct net *net, struct tc_action *actions[])
> +static int tcf_action_delete(struct net *net, struct tc_action *actions[], int *fallbacknum)
> {
> int i;
>
> @@ -1407,6 +1480,7 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
> u32 act_index = a->tcfa_index;
>
> actions[i] = NULL;
> + *fallbacknum = tcf_act_num_actions_single(a);
> if (tcf_action_put(a)) {
> /* last reference, action was deleted concurrently */
> module_put(ops->owner);
> @@ -1419,12 +1493,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
> return ret;
> }
> }
> + *fallbacknum = 0;
> return 0;
> }
>
> static int
> tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
> - u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
> + u32 portid, size_t attr_size, struct netlink_ext_ack *extack, int *fallbacknum)
> {
> int ret;
> struct sk_buff *skb;
> @@ -1442,7 +1517,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
> }
>
> /* now do the delete */
> - ret = tcf_action_delete(net, actions);
> + ret = tcf_action_delete(net, actions, fallbacknum);
> if (ret < 0) {
> NL_SET_ERR_MSG(extack, "Failed to delete TC action");
> kfree_skb(skb);
> @@ -1458,11 +1533,12 @@ static int
> tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> u32 portid, int event, struct netlink_ext_ack *extack)
> {
> - int i, ret;
> struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
> struct tc_action *act;
> size_t attr_size = 0;
> struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
> + struct flow_offload_action *fl_act;
> + int i, ret, fallback_num;
>
> ret = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
> extack);
> @@ -1492,7 +1568,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> if (event == RTM_GETACTION)
> ret = tcf_get_notify(net, portid, n, actions, event, extack);
> else { /* delete */
> - ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
> + tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack, &fl_act);
> + ret = tcf_del_notify(net, n, actions, portid, attr_size, extack, &fallback_num);
> + tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
This tcf_action_offload_cmd_{pre|post}() approach looks slightly
complicated, especially with fallback_num calculations. I would suggest
to simplify it by only initializing action cookies in
flow_action->entries[] (I assume you don't really need all the action
data just to delete it, right?) for DEL/STATS and do one of the
following:
- Unoffload actions one-by-one after every deletion in
tcf_actions_delete(), perhaps reusing the same flow_offload_action of
size 1 by only changing the cookie on each iteration.
- If you really want to send the whole batch to the driver, save cookies
for all successfully deleted actions in an array and initialize
compound flow_offload_action from the array.
This would remove the need for whole pre/post thing, which otherwise
gets even more complicated in following patch by 'keep_fl_act' arg.
> if (ret)
> goto err;
> return 0;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 9b9770dab5e8..23ce021f07f8 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3755,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);
> +
> unsigned int tcf_act_num_actions(struct tc_action *actions[])
> {
> unsigned int num_acts = 0;
> @@ -3762,10 +3771,7 @@ unsigned int tcf_act_num_actions(struct tc_action *actions[])
> int i;
>
> tcf_act_for_each_action(i, act, actions) {
> - if (is_tcf_pedit(act))
> - num_acts += tcf_pedit_nkeys(act);
> - else
> - num_acts++;
> + num_acts += tcf_act_num_actions_single(act);
> }
> return num_acts;
> }
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from net device
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
2021-07-22 14:25 ` Vlad Buslov
@ 2021-07-22 14:50 ` kernel test robot
2021-07-22 17:07 ` kernel test robot
2021-08-03 10:59 ` Jamal Hadi Salim
3 siblings, 0 replies; 42+ messages in thread
From: kernel test robot @ 2021-07-22 14:50 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: kbuild-all, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
oss-drivers, Baowen Zheng, Louis Peens, Simon Horman
[-- Attachment #1: Type: text/plain, Size: 5698 bytes --]
Hi Simon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c2255ff47768c94a0ebc3968f007928bb47ea43b
config: microblaze-randconfig-r011-20210722 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/a8e2d0acfc98c127ab0b5189f7635049515c43f3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
git checkout a8e2d0acfc98c127ab0b5189f7635049515c43f3
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=microblaze
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> net/sched/act_api.c:1063:5: warning: no previous prototype for 'tcf_action_offload_cmd_pre' [-Wmissing-prototypes]
1063 | int tcf_action_offload_cmd_pre(struct tc_action *actions[],
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> net/sched/act_api.c:1091:5: warning: no previous prototype for 'tcf_action_offload_cmd_post' [-Wmissing-prototypes]
1091 | int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
net/sched/act_api.c:1105:5: warning: no previous prototype for 'tcf_action_offload_cmd' [-Wmissing-prototypes]
1105 | int tcf_action_offload_cmd(struct tc_action *actions[],
| ^~~~~~~~~~~~~~~~~~~~~~
>> net/sched/act_api.c:1123:5: warning: no previous prototype for 'tcf_action_offload_del_post' [-Wmissing-prototypes]
1123 | int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/tcf_action_offload_cmd_pre +1063 net/sched/act_api.c
1062
> 1063 int tcf_action_offload_cmd_pre(struct tc_action *actions[],
1064 enum flow_act_command cmd,
1065 struct netlink_ext_ack *extack,
1066 struct flow_offload_action **fl_act)
1067 {
1068 struct flow_offload_action *fl_act_p;
1069 int err = 0;
1070
1071 fl_act_p = flow_action_alloc(tcf_act_num_actions(actions));
1072 if (!fl_act_p)
1073 return -ENOMEM;
1074
1075 fl_act_p->extack = extack;
1076 fl_act_p->command = cmd;
1077 err = tc_setup_action(&fl_act_p->action, actions);
1078 if (err) {
1079 NL_SET_ERR_MSG_MOD(extack,
1080 "Failed to setup tc actions for offload\n");
1081 goto err_out;
1082 }
1083 *fl_act = fl_act_p;
1084 return 0;
1085 err_out:
1086 kfree(fl_act_p);
1087 return err;
1088 }
1089 EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
1090
> 1091 int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
1092 struct netlink_ext_ack *extack)
1093 {
1094 if (IS_ERR(fl_act))
1095 return PTR_ERR(fl_act);
1096
1097 flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
1098
1099 tc_cleanup_flow_action(&fl_act->action);
1100 kfree(fl_act);
1101 return 0;
1102 }
1103
1104 /* offload the tc command after inserted */
1105 int tcf_action_offload_cmd(struct tc_action *actions[],
1106 struct netlink_ext_ack *extack)
1107 {
1108 struct flow_offload_action *fl_act;
1109 int err = 0;
1110
1111 err = tcf_action_offload_cmd_pre(actions,
1112 FLOW_ACT_REPLACE,
1113 extack,
1114 &fl_act);
1115 if (err)
1116 return err;
1117
1118 return tcf_action_offload_cmd_post(fl_act, extack);
1119 }
1120 EXPORT_SYMBOL(tcf_action_offload_cmd);
1121
1122 /* offload the tc command after deleted */
> 1123 int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
1124 struct tc_action *actions[],
1125 struct netlink_ext_ack *extack,
1126 int fallback_num)
1127 {
1128 int fallback_entries = 0;
1129 struct tc_action *act;
1130 int total_entries = 0;
1131 int i;
1132
1133 if (!fl_act)
1134 return -EINVAL;
1135
1136 if (fallback_num) {
1137 /* for each the actions to fallback the action entries remain in the actions */
1138 for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
1139 act = actions[i];
1140 if (!act)
1141 continue;
1142
1143 fallback_entries += tcf_act_num_actions_single(act);
1144 }
1145 fallback_entries += fallback_num;
1146 }
1147 total_entries = fl_act->action.num_entries;
1148 if (total_entries > fallback_entries) {
1149 /* just offload the actions that is not fallback and start with the actions */
1150 fl_act->action.num_entries -= fallback_entries;
1151 flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
1152
1153 /* recovery num_entries for cleanup */
1154 fl_act->action.num_entries = total_entries;
1155 } else {
1156 NL_SET_ERR_MSG(extack, "no entries to offload when deleting the tc actions");
1157 }
1158
1159 tc_cleanup_flow_action(&fl_act->action);
1160
1161 kfree(fl_act);
1162 return 0;
1163 }
1164 EXPORT_SYMBOL(tcf_action_offload_del_post);
1165
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39738 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware
2021-07-22 9:19 ` [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware Simon Horman
@ 2021-07-22 14:55 ` Vlad Buslov
2021-08-03 11:24 ` Jamal Hadi Salim
1 sibling, 0 replies; 42+ messages in thread
From: Vlad Buslov @ 2021-07-22 14:55 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Thu 22 Jul 2021 at 12:19, Simon Horman <simon.horman@corigine.com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> When collecting stats for actions update them using both
> both hardware and software counters.
>
> 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/flow_offload.h | 2 +-
> include/net/pkt_cls.h | 4 ++++
> net/sched/act_api.c | 49 ++++++++++++++++++++++++++++++++++----
> 4 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 086b291e9530..fe8331b5efce 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -233,6 +233,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_update_hw_stats(struct tc_action *action);
>
> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> struct tcf_chain **handle,
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 26644596fd54..467688fff7ce 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -560,7 +560,7 @@ enum flow_act_command {
> };
>
> struct flow_offload_action {
> - struct netlink_ext_ack *extack;
> + struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/
> enum flow_act_command command;
> struct flow_stats stats;
> struct flow_action action;
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 03dae225d64f..569c9294b15b 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -282,6 +282,10 @@ tcf_exts_stats_update(const struct tcf_exts *exts,
> for (i = 0; i < exts->nr_actions; i++) {
> struct tc_action *a = exts->actions[i];
>
> + /* if stats from hw, just skip */
> + if (!tcf_action_update_hw_stats(a))
> + continue;
> +
Is it okay to call this inside preempt disable section?
> tcf_action_stats_update(a, bytes, packets, drops,
> lastuse, true);
> a->used_hw_stats = used_hw_stats;
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 23a4538916af..7d5535bc2c13 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1089,15 +1089,18 @@ int tcf_action_offload_cmd_pre(struct tc_action *actions[],
> EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
>
> int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
> - struct netlink_ext_ack *extack)
> + struct netlink_ext_ack *extack,
> + bool keep_fl_act)
> {
> if (IS_ERR(fl_act))
> return PTR_ERR(fl_act);
>
> flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
>
> - tc_cleanup_flow_action(&fl_act->action);
> - kfree(fl_act);
> + if (!keep_fl_act) {
> + tc_cleanup_flow_action(&fl_act->action);
> + kfree(fl_act);
> + }
> return 0;
> }
>
> @@ -1115,10 +1118,45 @@ int tcf_action_offload_cmd(struct tc_action *actions[],
> if (err)
> return err;
>
> - return tcf_action_offload_cmd_post(fl_act, extack);
> + return tcf_action_offload_cmd_post(fl_act, extack, false);
> }
> EXPORT_SYMBOL(tcf_action_offload_cmd);
>
> +int tcf_action_update_hw_stats(struct tc_action *action)
> +{
> + struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
> + [0] = action,
> + };
> + struct flow_offload_action *fl_act;
> + int err = 0;
> +
Having some way to distinguish offloaded actions would also be useful
here to skip this function. I wonder how this affects dump rate when
executed for every single action, even when none of them were offloaded
through action API.
> + err = tcf_action_offload_cmd_pre(actions,
> + FLOW_ACT_STATS,
> + NULL,
> + &fl_act);
> + if (err)
> + goto err_out;
> +
> + err = tcf_action_offload_cmd_post(fl_act, NULL, true);
> +
> + if (fl_act->stats.lastused) {
> + tcf_action_stats_update(action, fl_act->stats.bytes,
> + fl_act->stats.pkts,
> + fl_act->stats.drops,
> + fl_act->stats.lastused,
> + true);
> + err = 0;
> + } else {
> + err = -EOPNOTSUPP;
> + }
> + tc_cleanup_flow_action(&fl_act->action);
> + kfree(fl_act);
> +
> +err_out:
> + return err;
> +}
> +EXPORT_SYMBOL(tcf_action_update_hw_stats);
> +
> /* offload the tc command after deleted */
> int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
> struct tc_action *actions[],
> @@ -1255,6 +1293,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.
> */
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
` (2 preceding siblings ...)
2021-07-22 13:57 ` kernel test robot
@ 2021-07-22 15:31 ` kernel test robot
2021-08-03 10:50 ` Jamal Hadi Salim
2021-08-03 11:05 ` Jamal Hadi Salim
5 siblings, 0 replies; 42+ messages in thread
From: kernel test robot @ 2021-07-22 15:31 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: clang-built-linux, kbuild-all, netdev, Jamal Hadi Salim,
Cong Wang, Jiri Pirko, oss-drivers, Baowen Zheng, Louis Peens,
Simon Horman
[-- Attachment #1: Type: text/plain, Size: 9187 bytes --]
Hi Simon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c2255ff47768c94a0ebc3968f007928bb47ea43b
config: powerpc-randconfig-r016-20210722 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9625ca5b602616b2f5584e8a49ba93c52c141e40)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/9228a8efdbf7a736354b87c0db3260dd7d2c4abd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
git checkout 9228a8efdbf7a736354b87c0db3260dd7d2c4abd
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:238:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:2:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:4:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:6:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:8:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:10:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
>> net/sched/act_api.c:1064:5: warning: no previous prototype for function 'tcf_action_offload_cmd' [-Wmissing-prototypes]
int tcf_action_offload_cmd(struct tc_action *actions[],
^
net/sched/act_api.c:1064:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd(struct tc_action *actions[],
^
static
13 warnings generated.
vim +/tcf_action_offload_cmd +1064 net/sched/act_api.c
1062
1063 /* offload the tc command after inserted */
> 1064 int tcf_action_offload_cmd(struct tc_action *actions[],
1065 struct netlink_ext_ack *extack)
1066 {
1067 struct flow_offload_action *fl_act;
1068 int err = 0;
1069
1070 fl_act = flow_action_alloc(tcf_act_num_actions(actions));
1071 if (!fl_act)
1072 return -ENOMEM;
1073
1074 fl_act->extack = extack;
1075 err = tc_setup_action(&fl_act->action, actions);
1076 if (err) {
1077 NL_SET_ERR_MSG_MOD(extack,
1078 "Failed to setup tc actions for offload\n");
1079 goto err_out;
1080 }
1081 fl_act->command = FLOW_ACT_REPLACE;
1082
1083 flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
1084
1085 tc_cleanup_flow_action(&fl_act->action);
1086
1087 err_out:
1088 kfree(fl_act);
1089 return err;
1090 }
1091 EXPORT_SYMBOL(tcf_action_offload_cmd);
1092
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44151 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from net device
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
2021-07-22 14:25 ` Vlad Buslov
2021-07-22 14:50 ` kernel test robot
@ 2021-07-22 17:07 ` kernel test robot
2021-08-03 10:59 ` Jamal Hadi Salim
3 siblings, 0 replies; 42+ messages in thread
From: kernel test robot @ 2021-07-22 17:07 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: clang-built-linux, kbuild-all, netdev, Jamal Hadi Salim,
Cong Wang, Jiri Pirko, oss-drivers, Baowen Zheng, Louis Peens,
Simon Horman
[-- Attachment #1: Type: text/plain, Size: 12844 bytes --]
Hi Simon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c2255ff47768c94a0ebc3968f007928bb47ea43b
config: powerpc-randconfig-r016-20210722 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9625ca5b602616b2f5584e8a49ba93c52c141e40)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/a8e2d0acfc98c127ab0b5189f7635049515c43f3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
git checkout a8e2d0acfc98c127ab0b5189f7635049515c43f3
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:238:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:2:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:4:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:6:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:8:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:10:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
>> net/sched/act_api.c:1063:5: warning: no previous prototype for function 'tcf_action_offload_cmd_pre' [-Wmissing-prototypes]
int tcf_action_offload_cmd_pre(struct tc_action *actions[],
^
net/sched/act_api.c:1063:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd_pre(struct tc_action *actions[],
^
static
>> net/sched/act_api.c:1091:5: warning: no previous prototype for function 'tcf_action_offload_cmd_post' [-Wmissing-prototypes]
int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
^
net/sched/act_api.c:1091:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
^
static
net/sched/act_api.c:1105:5: warning: no previous prototype for function 'tcf_action_offload_cmd' [-Wmissing-prototypes]
int tcf_action_offload_cmd(struct tc_action *actions[],
^
net/sched/act_api.c:1105:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd(struct tc_action *actions[],
^
static
>> net/sched/act_api.c:1123:5: warning: no previous prototype for function 'tcf_action_offload_del_post' [-Wmissing-prototypes]
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
^
net/sched/act_api.c:1123:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
^
static
16 warnings generated.
vim +/tcf_action_offload_cmd_pre +1063 net/sched/act_api.c
1062
> 1063 int tcf_action_offload_cmd_pre(struct tc_action *actions[],
1064 enum flow_act_command cmd,
1065 struct netlink_ext_ack *extack,
1066 struct flow_offload_action **fl_act)
1067 {
1068 struct flow_offload_action *fl_act_p;
1069 int err = 0;
1070
1071 fl_act_p = flow_action_alloc(tcf_act_num_actions(actions));
1072 if (!fl_act_p)
1073 return -ENOMEM;
1074
1075 fl_act_p->extack = extack;
1076 fl_act_p->command = cmd;
1077 err = tc_setup_action(&fl_act_p->action, actions);
1078 if (err) {
1079 NL_SET_ERR_MSG_MOD(extack,
1080 "Failed to setup tc actions for offload\n");
1081 goto err_out;
1082 }
1083 *fl_act = fl_act_p;
1084 return 0;
1085 err_out:
1086 kfree(fl_act_p);
1087 return err;
1088 }
1089 EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
1090
> 1091 int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
1092 struct netlink_ext_ack *extack)
1093 {
1094 if (IS_ERR(fl_act))
1095 return PTR_ERR(fl_act);
1096
1097 flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
1098
1099 tc_cleanup_flow_action(&fl_act->action);
1100 kfree(fl_act);
1101 return 0;
1102 }
1103
1104 /* offload the tc command after inserted */
1105 int tcf_action_offload_cmd(struct tc_action *actions[],
1106 struct netlink_ext_ack *extack)
1107 {
1108 struct flow_offload_action *fl_act;
1109 int err = 0;
1110
1111 err = tcf_action_offload_cmd_pre(actions,
1112 FLOW_ACT_REPLACE,
1113 extack,
1114 &fl_act);
1115 if (err)
1116 return err;
1117
1118 return tcf_action_offload_cmd_post(fl_act, extack);
1119 }
1120 EXPORT_SYMBOL(tcf_action_offload_cmd);
1121
1122 /* offload the tc command after deleted */
> 1123 int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
1124 struct tc_action *actions[],
1125 struct netlink_ext_ack *extack,
1126 int fallback_num)
1127 {
1128 int fallback_entries = 0;
1129 struct tc_action *act;
1130 int total_entries = 0;
1131 int i;
1132
1133 if (!fl_act)
1134 return -EINVAL;
1135
1136 if (fallback_num) {
1137 /* for each the actions to fallback the action entries remain in the actions */
1138 for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
1139 act = actions[i];
1140 if (!act)
1141 continue;
1142
1143 fallback_entries += tcf_act_num_actions_single(act);
1144 }
1145 fallback_entries += fallback_num;
1146 }
1147 total_entries = fl_act->action.num_entries;
1148 if (total_entries > fallback_entries) {
1149 /* just offload the actions that is not fallback and start with the actions */
1150 fl_act->action.num_entries -= fallback_entries;
1151 flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
1152
1153 /* recovery num_entries for cleanup */
1154 fl_act->action.num_entries = total_entries;
1155 } else {
1156 NL_SET_ERR_MSG(extack, "no entries to offload when deleting the tc actions");
1157 }
1158
1159 tc_cleanup_flow_action(&fl_act->action);
1160
1161 kfree(fl_act);
1162 return 0;
1163 }
1164 EXPORT_SYMBOL(tcf_action_offload_del_post);
1165
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44151 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 13:33 ` Jamal Hadi Salim
@ 2021-07-27 13:04 ` Simon Horman
2021-07-27 14:38 ` Vlad Buslov
0 siblings, 1 reply; 42+ messages in thread
From: Simon Horman @ 2021-07-27 13:04 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens
On Thu, Jul 22, 2021 at 09:33:09AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
> > On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
> > > 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.
> > >
> > > 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 ++
>
> > > 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
>
> [..]
>
> > > + /* offload actions to hardware if possible */
> > > + tcf_action_offload_cmd(actions, extack);
> > > +
> >
> > I think this has already been suggested for RFC, but some sort of
> > visibility for offload status of action would be extremely welcome.
> > Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
> > filters.
> >
>
> Also showing a tc command line in the cover letter on how one would
> ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-27 13:04 ` Simon Horman
@ 2021-07-27 14:38 ` Vlad Buslov
2021-07-27 16:13 ` Jamal Hadi Salim
0 siblings, 1 reply; 42+ messages in thread
From: Vlad Buslov @ 2021-07-27 14:38 UTC (permalink / raw)
To: Simon Horman
Cc: Jamal Hadi Salim, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
> On Thu, Jul 22, 2021 at 09:33:09AM -0400, Jamal Hadi Salim wrote:
>> On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
>> > On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
>> > > 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.
>> > >
>> > > 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 ++
>>
>> > > 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
>>
>> [..]
>>
>> > > + /* offload actions to hardware if possible */
>> > > + tcf_action_offload_cmd(actions, extack);
>> > > +
>> >
>> > I think this has already been suggested for RFC, but some sort of
>> > visibility for offload status of action would be extremely welcome.
>> > Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
>> > filters.
>> >
>>
>> Also showing a tc command line in the cover letter on how one would
>> ask for a specific action to be offloaded.
>
> In practice actions are offloaded when a flow using them is offloaded.
> So I think we need to consider what the meaning of IN_HW is.
>
> Is it that:
>
> * The driver (and potentially hardware, though not in our current
> implementation) has accepted the action for offload;
> * That a classifier that uses the action has bee offloaded;
> * Or something else?
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
>
> With regards to a counter, I'm not quite sure what this would be:
>
> * The number of devices where the action has been offloaded (which ties
> into the question of what we mean by IN_HW)
> * The number of offloaded classifier instances using the action
> * Something else
I would prefer to have semantics similar to filters:
1. Count number of driver callbacks that returned "success".
2. If count > 0, then set in_hw flag.
3. Set in_hw_count to success count.
This would allow user to immediately determine whether action passed
driver validation.
>
> Regarding a flag to control offload:
>
> * For classifiers (at least the flower classifier) there is the skip_sw and
> skip_hw flags, which allow control of placement of a classifier in SW and
> HW.
> * We could add similar flags for actions, which at least in my
> world view would have the net-effect of controlling which classifiers can
> be added to sw and hw - f.e. a classifier that uses an action marked
> skip_hw could not be added to HW.
> * Doing so would add some extra complexity and its not immediately apparent
> to me what the use-case would be given that there are already flags for
> classifiers.
Yeah, adding such flag for action offload seems to complicate things.
Also, "skip_sw" flag doesn't even make much sense for actions. I thought
that "skip_hw" flag would be nice to have for users that would like to
avoid "spamming" their NIC drivers (potentially causing higher latency
and resource consumption) for filters/actions they have no intention to
offload to hardware, but I'm not sure how useful is that option really
is.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-27 14:38 ` Vlad Buslov
@ 2021-07-27 16:13 ` Jamal Hadi Salim
2021-07-27 16:47 ` Vlad Buslov
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-07-27 16:13 UTC (permalink / raw)
To: Vlad Buslov, Simon Horman
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng, Louis Peens
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>
> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
>>>
>>> Also showing a tc command line in the cover letter on how one would
>>> ask for a specific action to be offloaded.
>>
>> In practice actions are offloaded when a flow using them is offloaded.
>> So I think we need to consider what the meaning of IN_HW is.
>>
>> Is it that:
>>
>> * The driver (and potentially hardware, though not in our current
>> implementation) has accepted the action for offload;
>> * That a classifier that uses the action has bee offloaded;
>> * Or something else?
>
> I think we have the same issue with filters - they might not be in
> hardware after driver callback returned "success" (due to neigh state
> being invalid for tunnel_key encap, for example).
>
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
>> With regards to a counter, I'm not quite sure what this would be:
>>
>> * The number of devices where the action has been offloaded (which ties
>> into the question of what we mean by IN_HW)
>> * The number of offloaded classifier instances using the action
>> * Something else
>
> I would prefer to have semantics similar to filters:
>
> 1. Count number of driver callbacks that returned "success".
>
> 2. If count > 0, then set in_hw flag.
>
> 3. Set in_hw_count to success count.
>
> This would allow user to immediately determine whether action passed
> driver validation.
>
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
>>
>> Regarding a flag to control offload:
>>
>> * For classifiers (at least the flower classifier) there is the skip_sw and
>> skip_hw flags, which allow control of placement of a classifier in SW and
>> HW.
>> * We could add similar flags for actions, which at least in my
>> world view would have the net-effect of controlling which classifiers can
>> be added to sw and hw - f.e. a classifier that uses an action marked
>> skip_hw could not be added to HW.
I guess it depends on the hardware implementation.
In S/W we have two modes:
Approach A: create an action and then 2) bind it to a filter.
Approach B: Create a filter and then bind it to an action.
And #2A can be repeated multiple times for the same action
(would require some index as a reference for the action)
To Simon's comment above that would mean allowing
"a classifier that uses an action marked skip_hw to be added to HW"
i.e
Some hardware is capable of doing both option #A and #B.
Todays offload assumes #B - in which both filter and action are assumed
offloaded.
I am hoping whatever approach we end up agreeing on doesnt limit
either mode.
>> * Doing so would add some extra complexity and its not immediately apparent
>> to me what the use-case would be given that there are already flags for
>> classifiers.
> Yeah, adding such flag for action offload seems to complicate things.
> Also, "skip_sw" flag doesn't even make much sense for actions. I thought
> that "skip_hw" flag would be nice to have for users that would like to
> avoid "spamming" their NIC drivers (potentially causing higher latency
> and resource consumption) for filters/actions they have no intention to
> offload to hardware, but I'm not sure how useful is that option really
> is.
Hold on Vlad.
So you are looking at this mostly as an optimization to speed up h/w
control updates? ;->
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-27 16:13 ` Jamal Hadi Salim
@ 2021-07-27 16:47 ` Vlad Buslov
2021-07-28 7:46 ` Simon Horman
0 siblings, 1 reply; 42+ messages in thread
From: Vlad Buslov @ 2021-07-27 16:47 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Simon Horman, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
>
>>>>
>>>> Also showing a tc command line in the cover letter on how one would
>>>> ask for a specific action to be offloaded.
>>>
>>> In practice actions are offloaded when a flow using them is offloaded.
>>> So I think we need to consider what the meaning of IN_HW is.
>>>
>>> Is it that:
>>>
>>> * The driver (and potentially hardware, though not in our current
>>> implementation) has accepted the action for offload;
>>> * That a classifier that uses the action has bee offloaded;
>>> * Or something else?
>> I think we have the same issue with filters - they might not be in
>> hardware after driver callback returned "success" (due to neigh state
>> being invalid for tunnel_key encap, for example).
>>
>
> Sounds like we need another state for this. Otherwise, how do you debug
> that something is sitting in the driver and not in hardware after you
> issued a command to offload it? How do i tell today?
> Also knowing reason why something is sitting in the driver would be
> helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
>
>>> With regards to a counter, I'm not quite sure what this would be:
>>>
>>> * The number of devices where the action has been offloaded (which ties
>>> into the question of what we mean by IN_HW)
>>> * The number of offloaded classifier instances using the action
>>> * Something else
>> I would prefer to have semantics similar to filters:
>> 1. Count number of driver callbacks that returned "success".
>> 2. If count > 0, then set in_hw flag.
>> 3. Set in_hw_count to success count.
>> This would allow user to immediately determine whether action passed
>> driver validation.
>>
>
> I didnt follow this:
> Are we refering to the the "block" semantics (where a filter for
> example applies to multiple devices)?
This uses indirect offload infrastructure, which means all drivers
in flow_block_indr_dev_list will receive action offload requests.
>
>>>
>>> Regarding a flag to control offload:
>>>
>>> * For classifiers (at least the flower classifier) there is the skip_sw and
>>> skip_hw flags, which allow control of placement of a classifier in SW and
>>> HW.
>>> * We could add similar flags for actions, which at least in my
>>> world view would have the net-effect of controlling which classifiers can
>>> be added to sw and hw - f.e. a classifier that uses an action marked
>>> skip_hw could not be added to HW.
>
> I guess it depends on the hardware implementation.
> In S/W we have two modes:
> Approach A: create an action and then 2) bind it to a filter.
> Approach B: Create a filter and then bind it to an action.
>
> And #2A can be repeated multiple times for the same action
> (would require some index as a reference for the action)
> To Simon's comment above that would mean allowing
> "a classifier that uses an action marked skip_hw to be added to HW"
> i.e
> Some hardware is capable of doing both option #A and #B.
>
> Todays offload assumes #B - in which both filter and action are assumed
> offloaded.
>
> I am hoping whatever approach we end up agreeing on doesnt limit
> either mode.
>
>>> * Doing so would add some extra complexity and its not immediately apparent
>>> to me what the use-case would be given that there are already flags for
>>> classifiers.
>> Yeah, adding such flag for action offload seems to complicate things.
>> Also, "skip_sw" flag doesn't even make much sense for actions. I thought
>> that "skip_hw" flag would be nice to have for users that would like to
>> avoid "spamming" their NIC drivers (potentially causing higher latency
>> and resource consumption) for filters/actions they have no intention to
>> offload to hardware, but I'm not sure how useful is that option really
>> is.
>
> Hold on Vlad.
> So you are looking at this mostly as an optimization to speed up h/w
> control updates? ;->
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
>
> I was looking at it more as a (currently missing) feature improvement.
> We already have a use case that is implemented by s/w today. The feature
> mimics it in h/w.
>
> At minimal all existing NICs should be able to support the counters
> as mapped to simple actions like drop. I understand for example if some
> cant support adding separately offloading of tunnels for example.
> So the syntax is something along the lines of:
>
> tc actions add action drop index 15 skip_sw
> tc filter add dev ...parent ... protocol ip prio X ..\
> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>
> You get an error if counter index 15 is not offloaded or
> if skip_sw was left out..
>
> And then later on, if you support sharing of actions:
> tc filter add dev ...parent ... protocol ip prio X2 ..\
> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>
> cheers,
> jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-27 16:47 ` Vlad Buslov
@ 2021-07-28 7:46 ` Simon Horman
2021-07-28 8:05 ` Vlad Buslov
2021-07-28 13:51 ` Jamal Hadi Salim
0 siblings, 2 replies; 42+ messages in thread
From: Simon Horman @ 2021-07-28 7:46 UTC (permalink / raw)
To: Vlad Buslov
Cc: Jamal Hadi Salim, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
> On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
> >> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
> >
> >>>>
> >>>> Also showing a tc command line in the cover letter on how one would
> >>>> ask for a specific action to be offloaded.
> >>>
> >>> In practice actions are offloaded when a flow using them is offloaded.
> >>> So I think we need to consider what the meaning of IN_HW is.
> >>>
> >>> Is it that:
> >>>
> >>> * The driver (and potentially hardware, though not in our current
> >>> implementation) has accepted the action for offload;
> >>> * That a classifier that uses the action has bee offloaded;
> >>> * Or something else?
> >> I think we have the same issue with filters - they might not be in
> >> hardware after driver callback returned "success" (due to neigh state
> >> being invalid for tunnel_key encap, for example).
> >>
> >
> > Sounds like we need another state for this. Otherwise, how do you debug
> > that something is sitting in the driver and not in hardware after you
> > issued a command to offload it? How do i tell today?
> > Also knowing reason why something is sitting in the driver would be
> > helpful.
>
> It is not about just adding another state. The issue is that there is no
> way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
> >>> With regards to a counter, I'm not quite sure what this would be:
> >>>
> >>> * The number of devices where the action has been offloaded (which ties
> >>> into the question of what we mean by IN_HW)
> >>> * The number of offloaded classifier instances using the action
> >>> * Something else
> >> I would prefer to have semantics similar to filters:
> >> 1. Count number of driver callbacks that returned "success".
> >> 2. If count > 0, then set in_hw flag.
> >> 3. Set in_hw_count to success count.
> >> This would allow user to immediately determine whether action passed
> >> driver validation.
Thanks, that makes sense to me.
> > I didnt follow this:
> > Are we refering to the the "block" semantics (where a filter for
> > example applies to multiple devices)?
>
> This uses indirect offload infrastructure, which means all drivers
> in flow_block_indr_dev_list will receive action offload requests.
>
> >>> Regarding a flag to control offload:
> >>>
> >>> * For classifiers (at least the flower classifier) there is the skip_sw and
> >>> skip_hw flags, which allow control of placement of a classifier in SW and
> >>> HW.
> >>> * We could add similar flags for actions, which at least in my
> >>> world view would have the net-effect of controlling which classifiers can
> >>> be added to sw and hw - f.e. a classifier that uses an action marked
> >>> skip_hw could not be added to HW.
> >
> > I guess it depends on the hardware implementation.
> > In S/W we have two modes:
> > Approach A: create an action and then 2) bind it to a filter.
> > Approach B: Create a filter and then bind it to an action.
> >
> > And #2A can be repeated multiple times for the same action
> > (would require some index as a reference for the action)
> > To Simon's comment above that would mean allowing
> > "a classifier that uses an action marked skip_hw to be added to HW"
> > i.e
> > Some hardware is capable of doing both option #A and #B.
> >
> > Todays offload assumes #B - in which both filter and action are assumed
> > offloaded.
> >
> > I am hoping whatever approach we end up agreeing on doesnt limit
> > either mode.
> >
> >>> * Doing so would add some extra complexity and its not immediately apparent
> >>> to me what the use-case would be given that there are already flags for
> >>> classifiers.
> >> Yeah, adding such flag for action offload seems to complicate things.
> >> Also, "skip_sw" flag doesn't even make much sense for actions. I thought
> >> that "skip_hw" flag would be nice to have for users that would like to
> >> avoid "spamming" their NIC drivers (potentially causing higher latency
> >> and resource consumption) for filters/actions they have no intention to
> >> offload to hardware, but I'm not sure how useful is that option really
> >> is.
> >
> > Hold on Vlad.
> > So you are looking at this mostly as an optimization to speed up h/w
> > control updates? ;->
>
> No. How would adding more flags improve h/w update rate? I was just
> thinking that it is strange that users that are not interested in
> offloads would suddenly have higher memory usage for their actions just
> because they happen to have offload-capable driver loaded. But it is not
> a major concern for me.
In that case can we rely on the global tc-offload on/off flag
provided by ethtool? (I understand its not the same, but perhaps
it is sufficient in practice.)
> > I was looking at it more as a (currently missing) feature improvement.
> > We already have a use case that is implemented by s/w today. The feature
> > mimics it in h/w.
> >
> > At minimal all existing NICs should be able to support the counters
> > as mapped to simple actions like drop. I understand for example if some
> > cant support adding separately offloading of tunnels for example.
> > So the syntax is something along the lines of:
> >
> > tc actions add action drop index 15 skip_sw
> > tc filter add dev ...parent ... protocol ip prio X ..\
> > u32/flower skip_sw match ... flowid 1:10 action gact index 15
> >
> > You get an error if counter index 15 is not offloaded or
> > if skip_sw was left out..
> >
> > And then later on, if you support sharing of actions:
> > tc filter add dev ...parent ... protocol ip prio X2 ..\
> > u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-28 7:46 ` Simon Horman
@ 2021-07-28 8:05 ` Vlad Buslov
2021-07-28 13:51 ` Jamal Hadi Salim
1 sibling, 0 replies; 42+ messages in thread
From: Vlad Buslov @ 2021-07-28 8:05 UTC (permalink / raw)
To: Simon Horman
Cc: Jamal Hadi Salim, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On Wed 28 Jul 2021 at 10:46, Simon Horman <simon.horman@corigine.com> wrote:
> On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
>> On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> > On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>> >> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
>> >
>> >>>>
>> >>>> Also showing a tc command line in the cover letter on how one would
>> >>>> ask for a specific action to be offloaded.
>> >>>
>> >>> In practice actions are offloaded when a flow using them is offloaded.
>> >>> So I think we need to consider what the meaning of IN_HW is.
>> >>>
>> >>> Is it that:
>> >>>
>> >>> * The driver (and potentially hardware, though not in our current
>> >>> implementation) has accepted the action for offload;
>> >>> * That a classifier that uses the action has bee offloaded;
>> >>> * Or something else?
>> >> I think we have the same issue with filters - they might not be in
>> >> hardware after driver callback returned "success" (due to neigh state
>> >> being invalid for tunnel_key encap, for example).
>> >>
>> >
>> > Sounds like we need another state for this. Otherwise, how do you debug
>> > that something is sitting in the driver and not in hardware after you
>> > issued a command to offload it? How do i tell today?
>> > Also knowing reason why something is sitting in the driver would be
>> > helpful.
>>
>> It is not about just adding another state. The issue is that there is no
>> way for drivers to change the state of software filter dynamically.
>
> I think it might be worth considering enhancing things at some point.
> But I agree that its more than a matter of adding an extra flag. And
> I think it's reasonable to implement something similar to the classifier
> current offload handling of IN_HW now and consider enhancements separately.
>
>> >>> With regards to a counter, I'm not quite sure what this would be:
>> >>>
>> >>> * The number of devices where the action has been offloaded (which ties
>> >>> into the question of what we mean by IN_HW)
>> >>> * The number of offloaded classifier instances using the action
>> >>> * Something else
>> >> I would prefer to have semantics similar to filters:
>> >> 1. Count number of driver callbacks that returned "success".
>> >> 2. If count > 0, then set in_hw flag.
>> >> 3. Set in_hw_count to success count.
>> >> This would allow user to immediately determine whether action passed
>> >> driver validation.
>
> Thanks, that makes sense to me.
>
>> > I didnt follow this:
>> > Are we refering to the the "block" semantics (where a filter for
>> > example applies to multiple devices)?
>>
>> This uses indirect offload infrastructure, which means all drivers
>> in flow_block_indr_dev_list will receive action offload requests.
>>
>> >>> Regarding a flag to control offload:
>> >>>
>> >>> * For classifiers (at least the flower classifier) there is the skip_sw and
>> >>> skip_hw flags, which allow control of placement of a classifier in SW and
>> >>> HW.
>> >>> * We could add similar flags for actions, which at least in my
>> >>> world view would have the net-effect of controlling which classifiers can
>> >>> be added to sw and hw - f.e. a classifier that uses an action marked
>> >>> skip_hw could not be added to HW.
>> >
>> > I guess it depends on the hardware implementation.
>> > In S/W we have two modes:
>> > Approach A: create an action and then 2) bind it to a filter.
>> > Approach B: Create a filter and then bind it to an action.
>> >
>> > And #2A can be repeated multiple times for the same action
>> > (would require some index as a reference for the action)
>> > To Simon's comment above that would mean allowing
>> > "a classifier that uses an action marked skip_hw to be added to HW"
>> > i.e
>> > Some hardware is capable of doing both option #A and #B.
>> >
>> > Todays offload assumes #B - in which both filter and action are assumed
>> > offloaded.
>> >
>> > I am hoping whatever approach we end up agreeing on doesnt limit
>> > either mode.
>> >
>> >>> * Doing so would add some extra complexity and its not immediately apparent
>> >>> to me what the use-case would be given that there are already flags for
>> >>> classifiers.
>> >> Yeah, adding such flag for action offload seems to complicate things.
>> >> Also, "skip_sw" flag doesn't even make much sense for actions. I thought
>> >> that "skip_hw" flag would be nice to have for users that would like to
>> >> avoid "spamming" their NIC drivers (potentially causing higher latency
>> >> and resource consumption) for filters/actions they have no intention to
>> >> offload to hardware, but I'm not sure how useful is that option really
>> >> is.
>> >
>> > Hold on Vlad.
>> > So you are looking at this mostly as an optimization to speed up h/w
>> > control updates? ;->
>>
>> No. How would adding more flags improve h/w update rate? I was just
>> thinking that it is strange that users that are not interested in
>> offloads would suddenly have higher memory usage for their actions just
>> because they happen to have offload-capable driver loaded. But it is not
>> a major concern for me.
>
> In that case can we rely on the global tc-offload on/off flag
> provided by ethtool? (I understand its not the same, but perhaps
> it is sufficient in practice.)
Yes, the ethtool should be sufficient. Didn't think about it initially.
Thanks!
>
>> > I was looking at it more as a (currently missing) feature improvement.
>> > We already have a use case that is implemented by s/w today. The feature
>> > mimics it in h/w.
>> >
>> > At minimal all existing NICs should be able to support the counters
>> > as mapped to simple actions like drop. I understand for example if some
>> > cant support adding separately offloading of tunnels for example.
>> > So the syntax is something along the lines of:
>> >
>> > tc actions add action drop index 15 skip_sw
>> > tc filter add dev ...parent ... protocol ip prio X ..\
>> > u32/flower skip_sw match ... flowid 1:10 action gact index 15
>> >
>> > You get an error if counter index 15 is not offloaded or
>> > if skip_sw was left out..
>> >
>> > And then later on, if you support sharing of actions:
>> > tc filter add dev ...parent ... protocol ip prio X2 ..\
>> > u32/flower skip_sw match ... flowid 1:10 action gact index 15
>
> Right, I understand that makes sense and is internally consistent.
> But I think that in practice it only makes a difference "Approach B"
> implementations, none of which currently exist.
>
> I would suggest we can add this when the need arises, rather than
> speculatively without hw/driver support. Its not precluded by the current
> model AFAIK.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-28 7:46 ` Simon Horman
2021-07-28 8:05 ` Vlad Buslov
@ 2021-07-28 13:51 ` Jamal Hadi Salim
2021-07-28 14:46 ` Simon Horman
1 sibling, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-07-28 13:51 UTC (permalink / raw)
To: Simon Horman, Vlad Buslov
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel, Jiri Pirko,
Roopa Prabhu
On 2021-07-28 3:46 a.m., Simon Horman wrote:
> On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
>> On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>>>> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
[..]
>>>> I think we have the same issue with filters - they might not be in
>>>> hardware after driver callback returned "success" (due to neigh state
>>>> being invalid for tunnel_key encap, for example).
>>>>
>>>
>>> Sounds like we need another state for this. Otherwise, how do you debug
>>> that something is sitting in the driver and not in hardware after you
>>> issued a command to offload it? How do i tell today?
>>> Also knowing reason why something is sitting in the driver would be
>>> helpful.
>>
>> It is not about just adding another state. The issue is that there is no
>> way for drivers to change the state of software filter dynamically.
>
> I think it might be worth considering enhancing things at some point.
> But I agree that its more than a matter of adding an extra flag. And
> I think it's reasonable to implement something similar to the classifier
> current offload handling of IN_HW now and consider enhancements separately.
>
Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.
So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa
And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.
[..]
>
>>> I didnt follow this:
>>> Are we refering to the the "block" semantics (where a filter for
>>> example applies to multiple devices)?
>>
>> This uses indirect offload infrastructure, which means all drivers
>> in flow_block_indr_dev_list will receive action offload requests.
>>
Ok, understood.
[..]
>>
>> No. How would adding more flags improve h/w update rate? I was just
>> thinking that it is strange that users that are not interested in
>> offloads would suddenly have higher memory usage for their actions just
>> because they happen to have offload-capable driver loaded. But it is not
>> a major concern for me.
>
> In that case can we rely on the global tc-offload on/off flag
> provided by ethtool? (I understand its not the same, but perhaps
> it is sufficient in practice.)
>
ok.
So: I think i have seen this what is probably the spamming refered
with the intel (800?) driver ;-> Basically driver was reacting to
all filters regardless of need to offload or not.
I thought it was an oversight on their part and the driver needed
fixing. Are we invoking the offload regardless of whether h/w offload
is requested? In my naive view - at least when i looked at the intel
code - it didnt seem hard to avoid the spamming.
>>> I was looking at it more as a (currently missing) feature improvement.
>>> We already have a use case that is implemented by s/w today. The feature
>>> mimics it in h/w.
>>>
>>> At minimal all existing NICs should be able to support the counters
>>> as mapped to simple actions like drop. I understand for example if some
>>> cant support adding separately offloading of tunnels for example.
>>> So the syntax is something along the lines of:
>>>
>>> tc actions add action drop index 15 skip_sw
>>> tc filter add dev ...parent ... protocol ip prio X ..\
>>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>>>
>>> You get an error if counter index 15 is not offloaded or
>>> if skip_sw was left out..
>>>
>>> And then later on, if you support sharing of actions:
>>> tc filter add dev ...parent ... protocol ip prio X2 ..\
>>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>
> Right, I understand that makes sense and is internally consistent.
> But I think that in practice it only makes a difference "Approach B"
> implementations, none of which currently exist.
>
At minimal:
Shouldnt counters (easily correlated to basic actions like drop or
accept) fit the scenario of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X .. \
u32/flower skip_sw match ... flowid 1:10 action gact index 15
?
> I would suggest we can add this when the need arises, rather than
> speculatively without hw/driver support. Its not precluded by the current
> model AFAIK.
>
We are going to work on a driver that would have the "B" approach.
I am hoping - whatever the consensus here - it doesnt require a
surgery afterwards to make that work.
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-28 13:51 ` Jamal Hadi Salim
@ 2021-07-28 14:46 ` Simon Horman
2021-07-30 10:17 ` Jamal Hadi Salim
0 siblings, 1 reply; 42+ messages in thread
From: Simon Horman @ 2021-07-28 14:46 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-28 3:46 a.m., Simon Horman wrote:
> > On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
> > > On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
> > > > > On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
>
> [..]
>
> > > > > I think we have the same issue with filters - they might not be in
> > > > > hardware after driver callback returned "success" (due to neigh state
> > > > > being invalid for tunnel_key encap, for example).
> > > >
> > > > Sounds like we need another state for this. Otherwise, how do you debug
> > > > that something is sitting in the driver and not in hardware after you
> > > > issued a command to offload it? How do i tell today?
> > > > Also knowing reason why something is sitting in the driver would be
> > > > helpful.
> > >
> > > It is not about just adding another state. The issue is that there is no
> > > way for drivers to change the state of software filter dynamically.
> >
> > I think it might be worth considering enhancing things at some point.
> > But I agree that its more than a matter of adding an extra flag. And
> > I think it's reasonable to implement something similar to the classifier
> > current offload handling of IN_HW now and consider enhancements separately.
>
> Debugability is very important. If we have such gotchas we need to have
> the admin at least be able to tell if the driver returns "success"
> and the request is still sitting in the driver for whatever reason
> At minimal there needs to be some indicator somewhere which say
> "inprogress" or "waiting for resolution" etc.
> If the control plane(user space app) starts making other decisions
> based on assumptions that filter was successfully installed i.e
> packets are being treated in the hardware then there could be
> consequences when this assumption is wrong.
>
> So if i undestood the challenge correctly it is: how do you relay
> this info back so it is reflected in the filter details. Yes that
> would require some mechanism to exist and possibly mapping state
> between whats in the driver and in the cls layer.
> If i am not mistaken, the switchdev folks handle this asynchronicty?
> +Cc Ido, Jiri, Roopa
>
> And it should be noted that: Yes, the filters have this
> pre-existing condition but doesnt mean given the opportunity
> to do actions we should replicate what they do.
I'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.
If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.
> [..]
>
> >
> > > > I didnt follow this:
> > > > Are we refering to the the "block" semantics (where a filter for
> > > > example applies to multiple devices)?
> > >
> > > This uses indirect offload infrastructure, which means all drivers
> > > in flow_block_indr_dev_list will receive action offload requests.
> > >
>
> Ok, understood.
>
> [..]
>
> > >
> > > No. How would adding more flags improve h/w update rate? I was just
> > > thinking that it is strange that users that are not interested in
> > > offloads would suddenly have higher memory usage for their actions just
> > > because they happen to have offload-capable driver loaded. But it is not
> > > a major concern for me.
> >
> > In that case can we rely on the global tc-offload on/off flag
> > provided by ethtool? (I understand its not the same, but perhaps
> > it is sufficient in practice.)
> >
>
> ok.
> So: I think i have seen this what is probably the spamming refered
> with the intel (800?) driver ;-> Basically driver was reacting to
> all filters regardless of need to offload or not.
> I thought it was an oversight on their part and the driver needed
> fixing. Are we invoking the offload regardless of whether h/w offload
> is requested? In my naive view - at least when i looked at the intel
> code - it didnt seem hard to avoid the spamming.
There is a per-netdev (not global as I wrote above) flag to enable and
disable offload. And there is per-classifier skip_hw flag. I can dig
through the code as easily as you can but I'd be surprised if the
driver is seeing offload requests if either of those settings
are in effect.
> > > > I was looking at it more as a (currently missing) feature improvement.
> > > > We already have a use case that is implemented by s/w today. The feature
> > > > mimics it in h/w.
> > > >
> > > > At minimal all existing NICs should be able to support the counters
> > > > as mapped to simple actions like drop. I understand for example if some
> > > > cant support adding separately offloading of tunnels for example.
> > > > So the syntax is something along the lines of:
> > > >
> > > > tc actions add action drop index 15 skip_sw
> > > > tc filter add dev ...parent ... protocol ip prio X ..\
> > > > u32/flower skip_sw match ... flowid 1:10 action gact index 15
> > > >
> > > > You get an error if counter index 15 is not offloaded or
> > > > if skip_sw was left out..
> > > >
> > > > And then later on, if you support sharing of actions:
> > > > tc filter add dev ...parent ... protocol ip prio X2 ..\
> > > > u32/flower skip_sw match ... flowid 1:10 action gact index 15
> >
> > Right, I understand that makes sense and is internally consistent.
> > But I think that in practice it only makes a difference "Approach B"
> > implementations, none of which currently exist.
> >
>
> At minimal:
> Shouldnt counters (easily correlated to basic actions like drop or
> accept) fit the scenario of:
> tc actions add action drop index 15 skip_sw
> tc filter add dev ...parent ... protocol ip prio X .. \
> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>
> ?
>
> > I would suggest we can add this when the need arises, rather than
> > speculatively without hw/driver support. Its not precluded by the current
> > model AFAIK.
> >
>
> We are going to work on a driver that would have the "B" approach.
> I am hoping - whatever the consensus here - it doesnt require a
> surgery afterwards to make that work.
You should be able to build on the work proposed here to add what you
suggest into the framework to meet these requirements for your driver work.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-28 14:46 ` Simon Horman
@ 2021-07-30 10:17 ` Jamal Hadi Salim
2021-07-30 11:40 ` Vlad Buslov
2021-07-30 13:20 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
0 siblings, 2 replies; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-07-30 10:17 UTC (permalink / raw)
To: Simon Horman
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On 2021-07-28 10:46 a.m., Simon Horman wrote:
> On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
>> On 2021-07-28 3:46 a.m., Simon Horman wrote:
>>> On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
>>>> On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>> On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>>>>>> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
>>
>> [..]
>>
>>>>>> I think we have the same issue with filters - they might not be in
>>>>>> hardware after driver callback returned "success" (due to neigh state
>>>>>> being invalid for tunnel_key encap, for example).
>>>>>
>>>>> Sounds like we need another state for this. Otherwise, how do you debug
>>>>> that something is sitting in the driver and not in hardware after you
>>>>> issued a command to offload it? How do i tell today?
>>>>> Also knowing reason why something is sitting in the driver would be
>>>>> helpful.
>>>>
>>>> It is not about just adding another state. The issue is that there is no
>>>> way for drivers to change the state of software filter dynamically.
>>>
>>> I think it might be worth considering enhancing things at some point.
>>> But I agree that its more than a matter of adding an extra flag. And
>>> I think it's reasonable to implement something similar to the classifier
>>> current offload handling of IN_HW now and consider enhancements separately.
>>
>> Debugability is very important. If we have such gotchas we need to have
>> the admin at least be able to tell if the driver returns "success"
>> and the request is still sitting in the driver for whatever reason
>> At minimal there needs to be some indicator somewhere which say
>> "inprogress" or "waiting for resolution" etc.
>> If the control plane(user space app) starts making other decisions
>> based on assumptions that filter was successfully installed i.e
>> packets are being treated in the hardware then there could be
>> consequences when this assumption is wrong.
>>
>> So if i undestood the challenge correctly it is: how do you relay
>> this info back so it is reflected in the filter details. Yes that
>> would require some mechanism to exist and possibly mapping state
>> between whats in the driver and in the cls layer.
>> If i am not mistaken, the switchdev folks handle this asynchronicty?
>> +Cc Ido, Jiri, Roopa
>>
>> And it should be noted that: Yes, the filters have this
>> pre-existing condition but doesnt mean given the opportunity
>> to do actions we should replicate what they do.
>
> I'd prefer symmetry between the use of IN_HW for filters and actions,
> which I believe is what Vlad has suggested.
>
It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?
> If we wish to enhance things - f.e. for debugging, which I
> agree is important - then I think that is a separate topic.
>
My only concern is not to repeat mistakes that are in filters
just for the sake of symmetry. Example the fact that something
went wrong with insertion or insertion is still in progress
and you get an indication that all went well.
Looking at mlnx (NIC) ndrivers it does seem that in the normal case
the insertion into hw is synchronous (for anything that is not sw
only). I didnt quiet see what Vlad was referring to.
We have spent literally hours debugging issues where rules are being
offloaded thinking it was the driver so any extra info helps.
>>>>
>>>> No. How would adding more flags improve h/w update rate? I was just
>>>> thinking that it is strange that users that are not interested in
>>>> offloads would suddenly have higher memory usage for their actions just
>>>> because they happen to have offload-capable driver loaded. But it is not
>>>> a major concern for me.
>>>
>>> In that case can we rely on the global tc-offload on/off flag
>>> provided by ethtool? (I understand its not the same, but perhaps
>>> it is sufficient in practice.)
>>>
>>
>> ok.
>> So: I think i have seen this what is probably the spamming refered
>> with the intel (800?) driver ;-> Basically driver was reacting to
>> all filters regardless of need to offload or not.
>> I thought it was an oversight on their part and the driver needed
>> fixing. Are we invoking the offload regardless of whether h/w offload
>> is requested? In my naive view - at least when i looked at the intel
>> code - it didnt seem hard to avoid the spamming.
>
> There is a per-netdev (not global as I wrote above) flag to enable and
> disable offload. And there is per-classifier skip_hw flag. I can dig
> through the code as easily as you can but I'd be surprised if the
> driver is seeing offload requests if either of those settings
> are in effect.
>
For sure it was the 800 (Ice driver if you want to dig into the code).
I think the ethtool flag was turned on but not skip_sw in the policy. I
dont have the card installed in any board right now - either will go and
dig into the logs (because it spews the message into the logs).
Again not clear if this is what Vlad was calling spam.
>>>>> I was looking at it more as a (currently missing) feature improvement.
>>>>> We already have a use case that is implemented by s/w today. The feature
>>>>> mimics it in h/w.
>>>>>
>>>>> At minimal all existing NICs should be able to support the counters
>>>>> as mapped to simple actions like drop. I understand for example if some
>>>>> cant support adding separately offloading of tunnels for example.
>>>>> So the syntax is something along the lines of:
>>>>>
>>>>> tc actions add action drop index 15 skip_sw
>>>>> tc filter add dev ...parent ... protocol ip prio X ..\
>>>>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>>>>>
>>>>> You get an error if counter index 15 is not offloaded or
>>>>> if skip_sw was left out..
>>>>>
>>>>> And then later on, if you support sharing of actions:
>>>>> tc filter add dev ...parent ... protocol ip prio X2 ..\
>>>>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>>>
>>> Right, I understand that makes sense and is internally consistent.
>>> But I think that in practice it only makes a difference "Approach B"
>>> implementations, none of which currently exist.
>>>
>>
>> At minimal:
>> Shouldnt counters (easily correlated to basic actions like drop or
>> accept) fit the scenario of:
>> tc actions add action drop index 15 skip_sw
>> tc filter add dev ...parent ... protocol ip prio X .. \
>> u32/flower skip_sw match ... flowid 1:10 action gact index 15
>>
>> ?
>>
>>> I would suggest we can add this when the need arises, rather than
>>> speculatively without hw/driver support. Its not precluded by the current
>>> model AFAIK.
>>>
>>
>> We are going to work on a driver that would have the "B" approach.
>> I am hoping - whatever the consensus here - it doesnt require a
>> surgery afterwards to make that work.
>
> You should be able to build on the work proposed here to add what you
> suggest into the framework to meet these requirements for your driver work.
>
Then we are good. These are the same patches you have here?
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-30 10:17 ` Jamal Hadi Salim
@ 2021-07-30 11:40 ` Vlad Buslov
2021-08-03 9:57 ` Jamal Hadi Salim
2021-07-30 13:20 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
1 sibling, 1 reply; 42+ messages in thread
From: Vlad Buslov @ 2021-07-30 11:40 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Simon Horman, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens,
Ido Schimmel, Jiri Pirko, Roopa Prabhu
On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2021-07-28 10:46 a.m., Simon Horman wrote:
>> On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
>>> On 2021-07-28 3:46 a.m., Simon Horman wrote:
>>>> On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
>>>>> On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>>> On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
>>>>>>> On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
>>>
>>> [..]
>>>
>>>>>>> I think we have the same issue with filters - they might not be in
>>>>>>> hardware after driver callback returned "success" (due to neigh state
>>>>>>> being invalid for tunnel_key encap, for example).
>>>>>>
>>>>>> Sounds like we need another state for this. Otherwise, how do you debug
>>>>>> that something is sitting in the driver and not in hardware after you
>>>>>> issued a command to offload it? How do i tell today?
>>>>>> Also knowing reason why something is sitting in the driver would be
>>>>>> helpful.
>>>>>
>>>>> It is not about just adding another state. The issue is that there is no
>>>>> way for drivers to change the state of software filter dynamically.
>>>>
>>>> I think it might be worth considering enhancing things at some point.
>>>> But I agree that its more than a matter of adding an extra flag. And
>>>> I think it's reasonable to implement something similar to the classifier
>>>> current offload handling of IN_HW now and consider enhancements separately.
>>>
>>> Debugability is very important. If we have such gotchas we need to have
>>> the admin at least be able to tell if the driver returns "success"
>>> and the request is still sitting in the driver for whatever reason
>>> At minimal there needs to be some indicator somewhere which say
>>> "inprogress" or "waiting for resolution" etc.
>>> If the control plane(user space app) starts making other decisions
>>> based on assumptions that filter was successfully installed i.e
>>> packets are being treated in the hardware then there could be
>>> consequences when this assumption is wrong.
>>>
>>> So if i undestood the challenge correctly it is: how do you relay
>>> this info back so it is reflected in the filter details. Yes that
>>> would require some mechanism to exist and possibly mapping state
>>> between whats in the driver and in the cls layer.
>>> If i am not mistaken, the switchdev folks handle this asynchronicty?
>>> +Cc Ido, Jiri, Roopa
>>>
>>> And it should be noted that: Yes, the filters have this
>>> pre-existing condition but doesnt mean given the opportunity
>>> to do actions we should replicate what they do.
>> I'd prefer symmetry between the use of IN_HW for filters and actions,
>> which I believe is what Vlad has suggested.
>>
>
> It still not clear to me what it means from a command line pov.
> How do i add a rule and when i dump it what does it show?
>
>> If we wish to enhance things - f.e. for debugging, which I
>> agree is important - then I think that is a separate topic.
>>
>
> My only concern is not to repeat mistakes that are in filters
> just for the sake of symmetry. Example the fact that something
> went wrong with insertion or insertion is still in progress
> and you get an indication that all went well.
> Looking at mlnx (NIC) ndrivers it does seem that in the normal case
> the insertion into hw is synchronous (for anything that is not sw
> only). I didnt quiet see what Vlad was referring to.
Filters with tunnel_key encap actions can be offloaded/unoffloaded
dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
events (see mlx5e_tc_fib_event_work()).
[...]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-30 10:17 ` Jamal Hadi Salim
2021-07-30 11:40 ` Vlad Buslov
@ 2021-07-30 13:20 ` Simon Horman
2021-08-03 10:14 ` Jamal Hadi Salim
1 sibling, 1 reply; 42+ messages in thread
From: Simon Horman @ 2021-07-30 13:20 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-28 10:46 a.m., Simon Horman wrote:
> > On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
> > > On 2021-07-28 3:46 a.m., Simon Horman wrote:
> > > > On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
> > > > > On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > > > On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
> > > > > > > On Tue 27 Jul 2021 at 16:04, Simon Horman <simon.horman@corigine.com> wrote:
> > >
> > > [..]
> > >
> > > > > > > I think we have the same issue with filters - they might not be in
> > > > > > > hardware after driver callback returned "success" (due to neigh state
> > > > > > > being invalid for tunnel_key encap, for example).
> > > > > >
> > > > > > Sounds like we need another state for this. Otherwise, how do you debug
> > > > > > that something is sitting in the driver and not in hardware after you
> > > > > > issued a command to offload it? How do i tell today?
> > > > > > Also knowing reason why something is sitting in the driver would be
> > > > > > helpful.
> > > > >
> > > > > It is not about just adding another state. The issue is that there is no
> > > > > way for drivers to change the state of software filter dynamically.
> > > >
> > > > I think it might be worth considering enhancing things at some point.
> > > > But I agree that its more than a matter of adding an extra flag. And
> > > > I think it's reasonable to implement something similar to the classifier
> > > > current offload handling of IN_HW now and consider enhancements separately.
> > >
> > > Debugability is very important. If we have such gotchas we need to have
> > > the admin at least be able to tell if the driver returns "success"
> > > and the request is still sitting in the driver for whatever reason
> > > At minimal there needs to be some indicator somewhere which say
> > > "inprogress" or "waiting for resolution" etc.
> > > If the control plane(user space app) starts making other decisions
> > > based on assumptions that filter was successfully installed i.e
> > > packets are being treated in the hardware then there could be
> > > consequences when this assumption is wrong.
> > >
> > > So if i undestood the challenge correctly it is: how do you relay
> > > this info back so it is reflected in the filter details. Yes that
> > > would require some mechanism to exist and possibly mapping state
> > > between whats in the driver and in the cls layer.
> > > If i am not mistaken, the switchdev folks handle this asynchronicty?
> > > +Cc Ido, Jiri, Roopa
> > >
> > > And it should be noted that: Yes, the filters have this
> > > pre-existing condition but doesnt mean given the opportunity
> > > to do actions we should replicate what they do.
> >
> > I'd prefer symmetry between the use of IN_HW for filters and actions,
> > which I believe is what Vlad has suggested.
>
> It still not clear to me what it means from a command line pov.
> How do i add a rule and when i dump it what does it show?
How about we confirm that once we've implemented the feature.
But I would assume that:
* Existing methods for adding rules work as before
* When one dumps an action (in a sufficiently verbose
way) the in_hw and in_hw_counter fields are displayed as they are for
filters.
Does that help?
> > If we wish to enhance things - f.e. for debugging, which I
> > agree is important - then I think that is a separate topic.
> >
>
> My only concern is not to repeat mistakes that are in filters
> just for the sake of symmetry. Example the fact that something
> went wrong with insertion or insertion is still in progress
> and you get an indication that all went well.
> Looking at mlnx (NIC) ndrivers it does seem that in the normal case
> the insertion into hw is synchronous (for anything that is not sw
> only). I didnt quiet see what Vlad was referring to.
> We have spent literally hours debugging issues where rules are being
> offloaded thinking it was the driver so any extra info helps.
I do think there is a value to symmetry between the APIs.
And I don't think doing so moves things in a bad direction.
But rather a separate discussion is needed to discuss how to
improve debuggability.
...
> > > > > > I was looking at it more as a (currently missing) feature improvement.
> > > > > > We already have a use case that is implemented by s/w today. The feature
> > > > > > mimics it in h/w.
> > > > > >
> > > > > > At minimal all existing NICs should be able to support the counters
> > > > > > as mapped to simple actions like drop. I understand for example if some
> > > > > > cant support adding separately offloading of tunnels for example.
> > > > > > So the syntax is something along the lines of:
> > > > > >
> > > > > > tc actions add action drop index 15 skip_sw
> > > > > > tc filter add dev ...parent ... protocol ip prio X ..\
> > > > > > u32/flower skip_sw match ... flowid 1:10 action gact index 15
> > > > > >
> > > > > > You get an error if counter index 15 is not offloaded or
> > > > > > if skip_sw was left out..
> > > > > >
> > > > > > And then later on, if you support sharing of actions:
> > > > > > tc filter add dev ...parent ... protocol ip prio X2 ..\
> > > > > > u32/flower skip_sw match ... flowid 1:10 action gact index 15
> > > >
> > > > Right, I understand that makes sense and is internally consistent.
> > > > But I think that in practice it only makes a difference "Approach B"
> > > > implementations, none of which currently exist.
> > > >
> > >
> > > At minimal:
> > > Shouldnt counters (easily correlated to basic actions like drop or
> > > accept) fit the scenario of:
> > > tc actions add action drop index 15 skip_sw
> > > tc filter add dev ...parent ... protocol ip prio X .. \
> > > u32/flower skip_sw match ... flowid 1:10 action gact index 15
> > >
> > > ?
> > >
> > > > I would suggest we can add this when the need arises, rather than
> > > > speculatively without hw/driver support. Its not precluded by the current
> > > > model AFAIK.
> > > >
> > >
> > > We are going to work on a driver that would have the "B" approach.
> > > I am hoping - whatever the consensus here - it doesnt require a
> > > surgery afterwards to make that work.
> >
> > You should be able to build on the work proposed here to add what you
> > suggest into the framework to meet these requirements for your driver work.
> >
>
> Then we are good. These are the same patches you have here?
Yes.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-30 11:40 ` Vlad Buslov
@ 2021-08-03 9:57 ` Jamal Hadi Salim
2021-08-03 12:02 ` tc offload debug-ability Jamal Hadi Salim
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 9:57 UTC (permalink / raw)
To: Vlad Buslov
Cc: Simon Horman, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens,
Ido Schimmel, Jiri Pirko, Roopa Prabhu
On 2021-07-30 7:40 a.m., Vlad Buslov wrote:
> On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2021-07-28 10:46 a.m., Simon Horman wrote:
>
> Filters with tunnel_key encap actions can be offloaded/unoffloaded
> dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
> events (see mlx5e_tc_fib_event_work()).
>
Thanks. Will look and compare against the FIB case.
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-30 13:20 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-08-03 10:14 ` Jamal Hadi Salim
2021-08-03 11:36 ` Simon Horman
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 10:14 UTC (permalink / raw)
To: Simon Horman
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On 2021-07-30 9:20 a.m., Simon Horman wrote:
> On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:
>> On 2021-07-28 10:46 a.m., Simon Horman wrote:
[..]
>> It still not clear to me what it means from a command line pov.
>> How do i add a rule and when i dump it what does it show?
>
> How about we confirm that once we've implemented the feature.
>
> But I would assume that:
>
> * Existing methods for adding rules work as before
> * When one dumps an action (in a sufficiently verbose
> way) the in_hw and in_hw_counter fields are displayed as they are for
> filters.
>
> Does that help?
>
I think it would help a lot more to say explicitly what it actually
means in the cover letter from a tc cli pov since the subject
is about offloading actions _independently_ of filters.
I am assuming you have some more patches on top of these that
actually will actually work for that.
Example of something you could show was adding a policer,
like so:
tc actions add action ... skip_sw...
then show get or dump showing things in h/w.
And del..
And i certainly hope that the above works and it is
not meant just for the consumption of some OVS use
case.
>>> If we wish to enhance things - f.e. for debugging, which I
>>> agree is important - then I think that is a separate topic.
>>>
>>
>> My only concern is not to repeat mistakes that are in filters
>> just for the sake of symmetry. Example the fact that something
>> went wrong with insertion or insertion is still in progress
>> and you get an indication that all went well.
>> Looking at mlnx (NIC) ndrivers it does seem that in the normal case
>> the insertion into hw is synchronous (for anything that is not sw
>> only). I didnt quiet see what Vlad was referring to.
>> We have spent literally hours debugging issues where rules are being
>> offloaded thinking it was the driver so any extra info helps.
>
> I do think there is a value to symmetry between the APIs.
> And I don't think doing so moves things in a bad direction.
> But rather a separate discussion is needed to discuss how to
> improve debuggability.
>
Fair enough - lets have a separate discussion on debug-ability.
Maybe a new thread after reviewing Vlad's pointer.
>>> You should be able to build on the work proposed here to add what you
>>> suggest into the framework to meet these requirements for your driver work.
>>>
>>
>> Then we are good. These are the same patches you have here?
>
> Yes.
Thanks - will review with that in mind.
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
` (3 preceding siblings ...)
2021-07-22 15:31 ` kernel test robot
@ 2021-08-03 10:50 ` Jamal Hadi Salim
2021-08-03 11:05 ` Jamal Hadi Salim
5 siblings, 0 replies; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 10:50 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: Cong Wang, Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On 2021-07-22 5:19 a.m., Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> Use flow_indr_dev_register/flow_indr_dev_setup_offload to
> offload tc action.
>
> We offload the tc action mainly for ovs meter configuration.
> 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.
>
[..]
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> +/* offload the tc command after inserted */
> +int tcf_action_offload_cmd(struct tc_action *actions[],
> + struct netlink_ext_ack *extack)
> +{
> + struct flow_offload_action *fl_act;
> + int err = 0;
> +
> + fl_act = flow_action_alloc(tcf_act_num_actions(actions));
> + if (!fl_act)
> + return -ENOMEM;
> +
> + fl_act->extack = extack;
> + err = tc_setup_action(&fl_act->action, actions);
> + if (err) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Failed to setup tc actions for offload\n");
> + goto err_out;
> + }
> + fl_act->command = FLOW_ACT_REPLACE;
> +
The fn name is a bit misleading with _cmd suffix when it is
only targeting one command: REPLACE (and not the other two).
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from net device
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
` (2 preceding siblings ...)
2021-07-22 17:07 ` kernel test robot
@ 2021-08-03 10:59 ` Jamal Hadi Salim
3 siblings, 0 replies; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 10:59 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: Cong Wang, Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On 2021-07-22 5:19 a.m., Simon Horman wrote:
[..]
> tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> u32 portid, int event, struct netlink_ext_ack *extack)
> {
> - int i, ret;
> struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
> struct tc_action *act;
> size_t attr_size = 0;
> struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
> + struct flow_offload_action *fl_act;
> + int i, ret, fallback_num;
>
> ret = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
> extack);
> @@ -1492,7 +1568,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> if (event == RTM_GETACTION)
> ret = tcf_get_notify(net, portid, n, actions, event, extack);
> else { /* delete */
> - ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
> + tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack, &fl_act);
> + ret = tcf_del_notify(net, n, actions, portid, attr_size, extack, &fallback_num);
> + tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
> if (ret)
> goto err;
It is hard to read from a patch context, but iiuc:
if the hardware update fails in tcf_action_offload_del_post() then
user space would still have been notified that it succeeded via
tcf_del_notify()... and there is no remediation after the fact.
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
` (4 preceding siblings ...)
2021-08-03 10:50 ` Jamal Hadi Salim
@ 2021-08-03 11:05 ` Jamal Hadi Salim
2021-08-03 11:31 ` Simon Horman
5 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 11:05 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: Cong Wang, Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On 2021-07-22 5:19 a.m., Simon Horman wrote:
Triggered by my observation on 2/3 went back and looked at this again to
see if we have same problem with notification on REPLACE case (I think
we do) but here's another comment:
> +EXPORT_SYMBOL(tcf_action_offload_cmd);
> +
> /* Returns numbers of initialized actions or negative error. */
>
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> @@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> return ret;
> ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
>
> + /* offload actions to hardware if possible */
> + tcf_action_offload_cmd(actions, extack);
> +
Above seems to be unconditional whether hw update is requested or not?
The comment says the right thing ("if possible") but the code
should have checked some sort of skip_sw check?
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware
2021-07-22 9:19 ` [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware Simon Horman
2021-07-22 14:55 ` Vlad Buslov
@ 2021-08-03 11:24 ` Jamal Hadi Salim
2021-08-03 11:35 ` Simon Horman
1 sibling, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 11:24 UTC (permalink / raw)
To: Simon Horman, David Miller, Jakub Kicinski
Cc: Cong Wang, Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens
On 2021-07-22 5:19 a.m., Simon Horman wrote:
[..]
> /* offload the tc command after deleted */
> int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
> struct tc_action *actions[],
> @@ -1255,6 +1293,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);
This is more a curiosity than a comment on the patch: Is the
driver polling for these stats synchronously and what we get here
is the last update or do we end up invoking beyond
the driver when requesting for the stats?
Overall commentary from looking at the patch set:
I believe your patches will support the individual tc actions
add/del/get/dump command line requests.
What is missing is an example usage all the way to the driver. I am sure
you have additional patches that put this to good use. My suggestion
is to test that cli with that pov against your overall patches and
show this in your commit logs - even if those patches are to follow
later.
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-08-03 11:05 ` Jamal Hadi Salim
@ 2021-08-03 11:31 ` Simon Horman
0 siblings, 0 replies; 42+ messages in thread
From: Simon Horman @ 2021-08-03 11:31 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng, Louis Peens
On Tue, Aug 03, 2021 at 07:05:52AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-22 5:19 a.m., Simon Horman wrote:
>
> Triggered by my observation on 2/3 went back and looked at this again to
> see if we have same problem with notification on REPLACE case (I think
> we do) but here's another comment:
>
> > +EXPORT_SYMBOL(tcf_action_offload_cmd);
> > +
> > /* Returns numbers of initialized actions or negative error. */
> > int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> > @@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> > return ret;
> > ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
> > + /* offload actions to hardware if possible */
> > + tcf_action_offload_cmd(actions, extack);
> > +
>
> Above seems to be unconditional whether hw update is requested or not?
> The comment says the right thing ("if possible") but the code
> should have checked some sort of skip_sw check?
Jamal, we are going around in circles.
As we have already discussed, this patchset does not add support for
skip_sw (or skip_hw) for actions.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware
2021-08-03 11:24 ` Jamal Hadi Salim
@ 2021-08-03 11:35 ` Simon Horman
0 siblings, 0 replies; 42+ messages in thread
From: Simon Horman @ 2021-08-03 11:35 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng, Louis Peens
On Tue, Aug 03, 2021 at 07:24:47AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-22 5:19 a.m., Simon Horman wrote:
>
> [..]
>
> > /* offload the tc command after deleted */
> > int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
> > struct tc_action *actions[],
> > @@ -1255,6 +1293,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);
>
> This is more a curiosity than a comment on the patch: Is the
> driver polling for these stats synchronously and what we get here
> is the last update or do we end up invoking beyond
> the driver when requesting for the stats?
I would have to double check but I believe the driver will report
back stats already received from the HW, rather than going all the way
to HW when the above call is made.
> Overall commentary from looking at the patch set:
> I believe your patches will support the individual tc actions
> add/del/get/dump command line requests.
Yes, that is the aim of this patchset.
> What is missing is an example usage all the way to the driver. I am sure
> you have additional patches that put this to good use. My suggestion
> is to test that cli with that pov against your overall patches and
> show this in your commit logs - even if those patches are to follow
> later.
Thanks, I'll see about making that so.
Just to be clear. We do have patches for the driver. And we do plan to post
them for inclusion in mainline. But I do believe that from a review
perspective its easier if one thing follows another.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-08-03 10:14 ` Jamal Hadi Salim
@ 2021-08-03 11:36 ` Simon Horman
2021-08-03 11:45 ` Jamal Hadi Salim
0 siblings, 1 reply; 42+ messages in thread
From: Simon Horman @ 2021-08-03 11:36 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On Tue, Aug 03, 2021 at 06:14:08AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-30 9:20 a.m., Simon Horman wrote:
> > On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:
> > > On 2021-07-28 10:46 a.m., Simon Horman wrote:
>
> [..]
>
> > > It still not clear to me what it means from a command line pov.
> > > How do i add a rule and when i dump it what does it show?
> >
> > How about we confirm that once we've implemented the feature.
> >
> > But I would assume that:
> >
> > * Existing methods for adding rules work as before
> > * When one dumps an action (in a sufficiently verbose
> > way) the in_hw and in_hw_counter fields are displayed as they are for
> > filters.
> >
> > Does that help?
> >
>
> I think it would help a lot more to say explicitly what it actually
> means in the cover letter from a tc cli pov since the subject
> is about offloading actions _independently_ of filters.
> I am assuming you have some more patches on top of these that
> actually will actually work for that.
>
> Example of something you could show was adding a policer,
> like so:
>
> tc actions add action ... skip_sw...
>
> then show get or dump showing things in h/w.
> And del..
>
> And i certainly hope that the above works and it is
> not meant just for the consumption of some OVS use
> case.
I agree it would be useful to include a tc cli example in the cover letter.
I'll see about making that so in v2.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-08-03 11:36 ` Simon Horman
@ 2021-08-03 11:45 ` Jamal Hadi Salim
2021-08-03 12:31 ` Simon Horman
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 11:45 UTC (permalink / raw)
To: Simon Horman
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On 2021-08-03 7:36 a.m., Simon Horman wrote:
> On Tue, Aug 03, 2021 at 06:14:08AM -0400, Jamal Hadi Salim wrote:
>> On 2021-07-30 9:20 a.m., Simon Horman wrote:
[..]
>> Example of something you could show was adding a policer,
>> like so:
>>
>> tc actions add action ... skip_sw...
>>
>> then show get or dump showing things in h/w.
>> And del..
>>
>> And i certainly hope that the above works and it is
>> not meant just for the consumption of some OVS use
>> case.
>
> I agree it would be useful to include a tc cli example in the cover letter.
> I'll see about making that so in v2.
>
Thanks. That will remove 95% of my commentary.
Check my other comment on the user space notification.
I was just looking at the way classifiers handle things from this
perspective and they dont return to cls_api until completion so
the notification gets sent after both h/w and software succeed...
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* tc offload debug-ability
2021-08-03 9:57 ` Jamal Hadi Salim
@ 2021-08-03 12:02 ` Jamal Hadi Salim
2021-08-03 12:14 ` Vlad Buslov
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 12:02 UTC (permalink / raw)
To: Vlad Buslov
Cc: Simon Horman, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens,
Ido Schimmel, Jiri Pirko, Roopa Prabhu
I just changed the subject line..
On 2021-08-03 5:57 a.m., Jamal Hadi Salim wrote:
> On 2021-07-30 7:40 a.m., Vlad Buslov wrote:
>> On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2021-07-28 10:46 a.m., Simon Horman wrote:
>
>>
>> Filters with tunnel_key encap actions can be offloaded/unoffloaded
>> dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
>> events (see mlx5e_tc_fib_event_work()).
>>
>
> Thanks. Will look and compare against the FIB case.
>
So unless i am mistaken Vlad:
a) there is no way to reflect the details when someone dumps the rules.
b) No notifications sent to the control plane (user space) when the
neighbor updates are offloaded.
My comments earlier are inspired by debugging tc offload and by this:
https://patches.linaro.org/cover/378345/
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: tc offload debug-ability
2021-08-03 12:02 ` tc offload debug-ability Jamal Hadi Salim
@ 2021-08-03 12:14 ` Vlad Buslov
2021-08-03 12:50 ` Jamal Hadi Salim
0 siblings, 1 reply; 42+ messages in thread
From: Vlad Buslov @ 2021-08-03 12:14 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Simon Horman, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens,
Ido Schimmel, Jiri Pirko, Roopa Prabhu
On Tue 03 Aug 2021 at 15:02, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> I just changed the subject line..
>
> On 2021-08-03 5:57 a.m., Jamal Hadi Salim wrote:
>> On 2021-07-30 7:40 a.m., Vlad Buslov wrote:
>>> On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> On 2021-07-28 10:46 a.m., Simon Horman wrote:
>>
>>>
>>> Filters with tunnel_key encap actions can be offloaded/unoffloaded
>>> dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
>>> events (see mlx5e_tc_fib_event_work()).
>>>
>> Thanks. Will look and compare against the FIB case.
>>
>
> So unless i am mistaken Vlad:
> a) there is no way to reflect the details when someone dumps the rules.
> b) No notifications sent to the control plane (user space) when the
> neighbor updates are offloaded.
Correct.
>
> My comments earlier are inspired by debugging tc offload and by this:
>
> https://patches.linaro.org/cover/378345/
>
> cheers,
> jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-08-03 11:45 ` Jamal Hadi Salim
@ 2021-08-03 12:31 ` Simon Horman
2021-08-03 13:01 ` Jamal Hadi Salim
0 siblings, 1 reply; 42+ messages in thread
From: Simon Horman @ 2021-08-03 12:31 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On Tue, Aug 03, 2021 at 07:45:13AM -0400, Jamal Hadi Salim wrote:
> On 2021-08-03 7:36 a.m., Simon Horman wrote:
> > On Tue, Aug 03, 2021 at 06:14:08AM -0400, Jamal Hadi Salim wrote:
> > > On 2021-07-30 9:20 a.m., Simon Horman wrote:
>
> [..]
>
> > > Example of something you could show was adding a policer,
> > > like so:
> > >
> > > tc actions add action ... skip_sw...
> > >
> > > then show get or dump showing things in h/w.
> > > And del..
> > >
> > > And i certainly hope that the above works and it is
> > > not meant just for the consumption of some OVS use
> > > case.
> >
> > I agree it would be useful to include a tc cli example in the cover letter.
> > I'll see about making that so in v2.
> >
>
> Thanks. That will remove 95% of my commentary.
:)
> Check my other comment on the user space notification.
> I was just looking at the way classifiers handle things from this
> perspective and they dont return to cls_api until completion so
> the notification gets sent after both h/w and software succeed...
Thanks, I will look into this. But it would make my life slightly easier if
a) You could be more specific about what portion of cls_api you are
referring to.
b) Constrained comments to a topic to a single sub-thread.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: tc offload debug-ability
2021-08-03 12:14 ` Vlad Buslov
@ 2021-08-03 12:50 ` Jamal Hadi Salim
2021-08-03 13:34 ` Ido Schimmel
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 12:50 UTC (permalink / raw)
To: Vlad Buslov
Cc: Simon Horman, David Miller, Jakub Kicinski, Cong Wang,
Jiri Pirko, netdev, oss-drivers, Baowen Zheng, Louis Peens,
Ido Schimmel, Jiri Pirko, Roopa Prabhu
On 2021-08-03 8:14 a.m., Vlad Buslov wrote:
>
> On Tue 03 Aug 2021 at 15:02, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
[..]
>>
>> So unless i am mistaken Vlad:
>> a) there is no way to reflect the details when someone dumps the rules.
>> b) No notifications sent to the control plane (user space) when the
>> neighbor updates are offloaded.
>
> Correct.
>
Feels like we can adopt the same mechanics. Although, unless i am
misreading, it seems Ido's patches cover a slightly different use
case: not totally synchronous in successfully pushing the rule to
hardware i.e could be sitting somewhere in firmware on its way to
the ASIC (and at least your connectx driver seems to only be
relinquishing control after confirming the update succeeded).
Am i mistaken Ido?
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-08-03 12:31 ` Simon Horman
@ 2021-08-03 13:01 ` Jamal Hadi Salim
2021-08-03 14:46 ` Simon Horman
0 siblings, 1 reply; 42+ messages in thread
From: Jamal Hadi Salim @ 2021-08-03 13:01 UTC (permalink / raw)
To: Simon Horman
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On 2021-08-03 8:31 a.m., Simon Horman wrote:
> On Tue, Aug 03, 2021 at 07:45:13AM -0400, Jamal Hadi Salim wrote:
> Thanks, I will look into this. But it would make my life slightly easier if
>
> a) You could be more specific about what portion of cls_api you are
> referring to.
> b) Constrained comments to a topic to a single sub-thread.
>
Context, this was on the comment i made on 2/3 here:
-----
- ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
+ tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack,
&fl_act);
+ ret = tcf_del_notify(net, n, actions, portid, attr_size,
extack, &fallback_num);
+ tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
if (ret)
goto err;
----
where a notification goes to user space to say "success" but hardware
update fails.
If you look at fl_change() which does the offload you'll see that it
returns err on any of sw or hw failure (depending on request).
Notification of success is done in cls_api.c - example for
creating/replacing with this snippet:
---
err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
flags, extack);
if (err == 0) {
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false, rtnl_held);
tfilter_put(tp, fh);
/* q pointer is NULL for shared blocks */
if (q)
q->flags &= ~TCQ_F_CAN_BYPASS;
}
---
cheers,
jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: tc offload debug-ability
2021-08-03 12:50 ` Jamal Hadi Salim
@ 2021-08-03 13:34 ` Ido Schimmel
0 siblings, 0 replies; 42+ messages in thread
From: Ido Schimmel @ 2021-08-03 13:34 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, Simon Horman, David Miller, Jakub Kicinski,
Cong Wang, Jiri Pirko, netdev, oss-drivers, Baowen Zheng,
Louis Peens, Jiri Pirko, Roopa Prabhu
On Tue, Aug 03, 2021 at 08:50:27AM -0400, Jamal Hadi Salim wrote:
> On 2021-08-03 8:14 a.m., Vlad Buslov wrote:
> >
> > On Tue 03 Aug 2021 at 15:02, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> [..]
>
> > >
> > > So unless i am mistaken Vlad:
> > > a) there is no way to reflect the details when someone dumps the rules.
> > > b) No notifications sent to the control plane (user space) when the
> > > neighbor updates are offloaded.
> >
> > Correct.
> >
>
> Feels like we can adopt the same mechanics. Although, unless i am
> misreading, it seems Ido's patches cover a slightly different use
> case: not totally synchronous in successfully pushing the rule to
> hardware i.e could be sitting somewhere in firmware on its way to
> the ASIC (and at least your connectx driver seems to only be
> relinquishing control after confirming the update succeeded).
> Am i mistaken Ido?
It is simply that all routes are notified from an atomic context, which
means installation to hardware needs to be deferred. But even if we
solve this case, there are other cases that cannot be solved.
For example, in IPv6 routes can be installed in response to RA packets
from softIRQ context. As another example, you can have several routes
with the same key already installed in the kernel, but only the one with
the lowest metric will be installed in hardware. Once it is deleted (for
example, in response to a netdev event), the kernel will try to install
the route with the higher metric. As a user, you want to get a
notification if this operation failed.
The same problem exists with tc actions, not sure about classifiers. For
example, we can mirror to a gretap to emulate ERSPAN. The hardware
expects all the headers to be specified, which means the path towards
the remote IP must be resolved in the kernel. Due to FIB/neigh/FDB
events it can obviously change, which means that the validity of the
action in hardware (not the classifier) changes over time with zero
visibility to user space.
So I believe that we need to be able to notify user space about the
state of the action. Whether it is in hardware / not in hardware /
failed to be installed to hardware.
>
>
> cheers,
> jamal
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
2021-08-03 13:01 ` Jamal Hadi Salim
@ 2021-08-03 14:46 ` Simon Horman
0 siblings, 0 replies; 42+ messages in thread
From: Simon Horman @ 2021-08-03 14:46 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Vlad Buslov, David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko,
netdev, oss-drivers, Baowen Zheng, Louis Peens, Ido Schimmel,
Jiri Pirko, Roopa Prabhu
On Tue, Aug 03, 2021 at 09:01:45AM -0400, Jamal Hadi Salim wrote:
> On 2021-08-03 8:31 a.m., Simon Horman wrote:
> > On Tue, Aug 03, 2021 at 07:45:13AM -0400, Jamal Hadi Salim wrote:
>
>
> > Thanks, I will look into this. But it would make my life slightly easier if
> >
> > a) You could be more specific about what portion of cls_api you are
> > referring to.
> > b) Constrained comments to a topic to a single sub-thread.
> >
>
> Context, this was on the comment i made on 2/3 here:
>
> -----
> - ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
> + tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack,
> &fl_act);
> + ret = tcf_del_notify(net, n, actions, portid, attr_size, extack,
> &fallback_num);
> + tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
> if (ret)
> goto err;
> ----
>
> where a notification goes to user space to say "success" but hardware
> update fails.
>
> If you look at fl_change() which does the offload you'll see that it
> returns err on any of sw or hw failure (depending on request).
> Notification of success is done in cls_api.c - example for
> creating/replacing with this snippet:
>
> ---
> err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
> flags, extack);
> if (err == 0) {
> tfilter_notify(net, skb, n, tp, block, q, parent, fh,
> RTM_NEWTFILTER, false, rtnl_held);
> tfilter_put(tp, fh);
> /* q pointer is NULL for shared blocks */
> if (q)
> q->flags &= ~TCQ_F_CAN_BYPASS;
> }
> ---
Thanks for the context Jamal, much appreciated.
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2021-08-03 14:47 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 9:19 [PATCH net-next 0/3] flow_offload: hardware offload of TC actions Simon Horman
2021-07-22 9:19 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
2021-07-22 12:24 ` Roi Dayan
2021-07-22 13:19 ` Simon Horman
2021-07-22 13:29 ` Vlad Buslov
2021-07-22 13:33 ` Jamal Hadi Salim
2021-07-27 13:04 ` Simon Horman
2021-07-27 14:38 ` Vlad Buslov
2021-07-27 16:13 ` Jamal Hadi Salim
2021-07-27 16:47 ` Vlad Buslov
2021-07-28 7:46 ` Simon Horman
2021-07-28 8:05 ` Vlad Buslov
2021-07-28 13:51 ` Jamal Hadi Salim
2021-07-28 14:46 ` Simon Horman
2021-07-30 10:17 ` Jamal Hadi Salim
2021-07-30 11:40 ` Vlad Buslov
2021-08-03 9:57 ` Jamal Hadi Salim
2021-08-03 12:02 ` tc offload debug-ability Jamal Hadi Salim
2021-08-03 12:14 ` Vlad Buslov
2021-08-03 12:50 ` Jamal Hadi Salim
2021-08-03 13:34 ` Ido Schimmel
2021-07-30 13:20 ` [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device Simon Horman
2021-08-03 10:14 ` Jamal Hadi Salim
2021-08-03 11:36 ` Simon Horman
2021-08-03 11:45 ` Jamal Hadi Salim
2021-08-03 12:31 ` Simon Horman
2021-08-03 13:01 ` Jamal Hadi Salim
2021-08-03 14:46 ` Simon Horman
2021-07-22 13:57 ` kernel test robot
2021-07-22 15:31 ` kernel test robot
2021-08-03 10:50 ` Jamal Hadi Salim
2021-08-03 11:05 ` Jamal Hadi Salim
2021-08-03 11:31 ` Simon Horman
2021-07-22 9:19 ` [PATCH net-next 2/3] flow_offload: add process to delete offloaded actions from " Simon Horman
2021-07-22 14:25 ` Vlad Buslov
2021-07-22 14:50 ` kernel test robot
2021-07-22 17:07 ` kernel test robot
2021-08-03 10:59 ` Jamal Hadi Salim
2021-07-22 9:19 ` [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware Simon Horman
2021-07-22 14:55 ` Vlad Buslov
2021-08-03 11:24 ` Jamal Hadi Salim
2021-08-03 11:35 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).