* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-04-29 8:10 [RFC net-next] net/flow_offload: allow user to offload tc action to net device Simon Horman
@ 2021-04-29 16:49 ` Ido Schimmel
2021-04-29 18:20 ` kernel test robot
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ido Schimmel @ 2021-04-29 16:49 UTC (permalink / raw)
To: Simon Horman
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens
On Thu, Apr 29, 2021 at 10:10:14AM +0200, Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> 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 my multiple flows and whose lifecycle is
s/my/by/
> independent of any flows that use them.
Can you share an example of the tc commands? You might be able to
achieve what you want by patching NFP to take into account the policer
index in 'struct flow_action_entry'. Seems that it currently assumes
that each policer is a new policer.
FTR, I do support the overall plan to offload actions independent of
flows and to associate stats with actions.
>
> This patch includes basic changes to offload drivers to return EOPNOTSUPP
> if this feature is used - it is not yet supported by any driver.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
[...]
> diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
> index 72649512dcdd..6309519bf9d4 100644
> --- a/include/net/tc_act/tc_police.h
> +++ b/include/net/tc_act/tc_police.h
> @@ -53,6 +53,11 @@ static inline bool is_tcf_police(const struct tc_action *act)
> return false;
> }
>
> +static inline u32 tcf_police_index(const struct tc_action *act)
> +{
> + return act->tcfa_index;
> +}
Any reason this is part of the patch? Looks like nobody is using it.
> +
> static inline u64 tcf_police_rate_bytes_ps(const struct tc_action *act)
> {
> struct tcf_police *police = to_police(act);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-04-29 8:10 [RFC net-next] net/flow_offload: allow user to offload tc action to net device Simon Horman
2021-04-29 16:49 ` Ido Schimmel
@ 2021-04-29 18:20 ` kernel test robot
2021-04-30 2:47 ` Marcelo Ricardo Leitner
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-04-29 18:20 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]
Hi Simon,
[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/net-flow_offload-allow-user-to-offload-tc-action-to-net-device/20210429-161217
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a52dd8fefb45626dace70a63c0738dbd83b7edb
config: h8300-randconfig-r024-20210429 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.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/d691d7b74a9b53774b586c7f8d71059be9dc731c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/net-flow_offload-allow-user-to-offload-tc-action-to-net-device/20210429-161217
git checkout d691d7b74a9b53774b586c7f8d71059be9dc731c
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=h8300
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
net/sched/cls_api.c: In function 'tc_setup_flow_action':
>> net/sched/cls_api.c:3743:44: error: 'const struct tcf_exts' has no member named 'actions'; did you mean 'action'?
3743 | return tc_setup_action(flow_action, exts->actions);
| ^~~~~~~
| action
net/sched/cls_api.c:3744:1: error: control reaches end of non-void function [-Werror=return-type]
3744 | }
| ^
cc1: some warnings being treated as errors
vim +3743 net/sched/cls_api.c
3736
3737 int tc_setup_flow_action(struct flow_action *flow_action,
3738 const struct tcf_exts *exts)
3739 {
3740 if (!exts)
3741 return 0;
3742
> 3743 return tc_setup_action(flow_action, exts->actions);
3744 }
3745 EXPORT_SYMBOL(tc_setup_flow_action);
3746
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 21145 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-04-29 8:10 [RFC net-next] net/flow_offload: allow user to offload tc action to net device Simon Horman
2021-04-29 16:49 ` Ido Schimmel
2021-04-29 18:20 ` kernel test robot
@ 2021-04-30 2:47 ` Marcelo Ricardo Leitner
2021-04-30 11:11 ` Vlad Buslov
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Marcelo Ricardo Leitner @ 2021-04-30 2:47 UTC (permalink / raw)
To: Simon Horman
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens, dcaratti, ozsh
On Thu, Apr 29, 2021 at 10:10:14AM +0200, Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> 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 my multiple flows and whose lifecycle is
> independent of any flows that use them.
Makes sense to me.
>
> This patch includes basic changes to offload drivers to return EOPNOTSUPP
> if this feature is used - it is not yet supported by any driver.
I miss indications on whether the action is offloaded or not. That's
really useful for debugging. extacks are nice but they may not be
logged and are not present in dumps later on.
As the offloading is optional here (it will fill in the extack but not
error out), seems that it's up to the driver then to ensure that the
action is offloaded when offloading a flow that uses such action,
right? With that, all the skip_sw/skip_hw dealing is left to the
flow/classifier.
Marcelo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-04-29 8:10 [RFC net-next] net/flow_offload: allow user to offload tc action to net device Simon Horman
` (2 preceding siblings ...)
2021-04-30 2:47 ` Marcelo Ricardo Leitner
@ 2021-04-30 11:11 ` Vlad Buslov
2021-05-10 22:37 ` Jamal Hadi Salim
2021-05-21 7:33 ` Jianbo Liu
5 siblings, 0 replies; 9+ messages in thread
From: Vlad Buslov @ 2021-04-30 11:11 UTC (permalink / raw)
To: Simon Horman
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens
On Thu 29 Apr 2021 at 11:10, Simon Horman <simon.horman@netronome.com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> 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 my 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.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.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 | 6 ++++
> include/net/tc_act/tc_police.h | 5 +++
> net/core/flow_offload.c | 26 +++++++++++++-
> net/sched/act_api.c | 30 ++++++++++++++++
> net/sched/cls_api.c | 34 ++++++++++++++++---
> 10 files changed, 119 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 6cdc52d50a48..c8dde4bc3961 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> @@ -490,6 +490,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 e95969c462e4..f6c665051173 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -1839,6 +1839,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 5cbc950b34df..2f8cb65d85d1 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 dc5c1e69cd9f..5fd8b5600b4a 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -551,6 +551,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 255e4f4b521f..5c6549ae0cc7 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,
> @@ -538,6 +541,8 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
>
> int tc_setup_flow_action(struct flow_action *flow_action,
> const struct tcf_exts *exts);
> +int tc_setup_action(struct flow_action *flow_action,
> + struct tc_action *actions[]);
> void tc_cleanup_flow_action(struct flow_action *flow_action);
>
> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
> @@ -558,6 +563,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/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
> index 72649512dcdd..6309519bf9d4 100644
> --- a/include/net/tc_act/tc_police.h
> +++ b/include/net/tc_act/tc_police.h
> @@ -53,6 +53,11 @@ static inline bool is_tcf_police(const struct tc_action *act)
> return false;
> }
>
> +static inline u32 tcf_police_index(const struct tc_action *act)
> +{
> + return act->tcfa_index;
> +}
> +
> static inline u64 tcf_police_rate_bytes_ps(const struct tc_action *act)
> {
> struct tcf_police *police = to_police(act);
> 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 f6d5755d669e..dab24688d9c7 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1059,6 +1059,34 @@ 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[], bool add, struct netlink_ext_ack *extack)
> +{
> + int err = 0;
> + struct flow_offload_action *fl_act;
> +
> + 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 = add ? FLOW_ACT_REPLACE : FLOW_ACT_DESTROY;
> +
> + flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
So many NULL arguments to this function, which requires adding more
!=NULL checks down the stack. I would like to suggest some better
approach but couldn't come up with anything that doesn't require big
refactoring in flow_offload infra and/or drivers that rely on it :(
> +
> + 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,
> @@ -1107,6 +1135,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> */
> tcf_idr_insert_many(actions);
>
> + tcf_action_offload_cmd(actions, true, extack);
Marking the actions as in_hw/not_in_hw would simplify debugging problems
with offloads IMO.
> *attr_size = tcf_action_full_attrs_size(sz);
> err = i - 1;
> goto err_mod;
> @@ -1465,6 +1494,7 @@ 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 */
> + tcf_action_offload_cmd(actions, false, extack);
I wonder how this would interact with concurrent filter creation.
Consider the following example sequence:
+-------+ +-------+
| | | |
| Task1 | | Task2 |
| | | |
+---+---+ +---+---+
| |
+-------------v---------------+ |
| | |
| Init action id=1 | |
| refcnt=1 bindcnt=0 | |
| | |
+-------------+---------------+ |
| |
+-------------v---------------+ |
| | |
| tcf_action_offload_cmd(id=1)| |
| | |
+-------------+---------------+ |
| |
+-------------v---------------+ |
| | |
| Driver creates act id=1 | |
| | |
+-------------+---------------+ |
| +-------------v---------------+
v | |
... | Init filter with act id=1 |
+ | refcnt=2 bindcnt=1 |
| | |
+-------------v---------------+ +-------------+---------------+
| | |
| Del action id=1 | |
| refcnt=2 bindcnt=1 | |
| | |
+-------------+---------------+ |
| |
+-------------v---------------+ |
| | |
| tcf_action_offload_cmd(id=1)| |
| | |
+-------------+---------------+ |
| |
+-------------v---------------+ |
| | |
| Driver deletes act id=1 | |
| | |
+-------------+---------------+ |
| |
+-------------v---------------+ |
| | |
| tcf_action_delete()==-EPERM | |
| refcnt=2 bindcnt=1 | |
| | |
+-----------------------------+ |
|
+-------------v---------------+
| |
| tc_setup_cb_add() |
| |
+-------------+---------------+
|
+-------------v---------------+
| |
| Driver can't find act id=1 |
| Create new one? Return err? |
| |
+-----------------------------+
I guess one could argue that users shouldn't do such "stupid" things but
I would prefer this new action offload API to be robust from the start.
Maybe it is better to only notify the driver if tcf_del_notify()
succeeded?
> ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
> if (ret)
> goto err;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 40fbea626dfd..995024c20df6 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3551,8 +3551,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;
> @@ -3561,11 +3561,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];
> @@ -3732,6 +3732,16 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> spin_unlock_bh(&act->tcfa_lock);
> goto err_out;
> }
> +EXPORT_SYMBOL(tc_setup_action);
> +
> +int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts)
> +{
> + if (!exts)
> + return 0;
> +
> + return tc_setup_action(flow_action, exts->actions);
> +}
> EXPORT_SYMBOL(tc_setup_flow_action);
>
> unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> @@ -3750,6 +3760,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] 9+ messages in thread* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-04-29 8:10 [RFC net-next] net/flow_offload: allow user to offload tc action to net device Simon Horman
` (3 preceding siblings ...)
2021-04-30 11:11 ` Vlad Buslov
@ 2021-05-10 22:37 ` Jamal Hadi Salim
2021-05-10 22:47 ` Jamal Hadi Salim
2021-05-21 7:33 ` Jianbo Liu
5 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2021-05-10 22:37 UTC (permalink / raw)
To: Simon Horman, Cong Wang, Jiri Pirko
Cc: netdev, oss-drivers, Baowen Zheng, Louis Peens
On 2021-04-29 4:10 a.m., Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> 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 my multiple flows and whose lifecycle is
> independent of any flows that use them.
>
Finally! ;->
Happy to see this effort.
> /* 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 dc5c1e69cd9f..5fd8b5600b4a 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -551,6 +551,21 @@ struct flow_cls_offload {
> u32 classid;
> };
>
> +enum flow_act_command {
> + FLOW_ACT_REPLACE,
> + FLOW_ACT_DESTROY,
> + FLOW_ACT_STATS,
> +};
> +
Should that be CREATE as well as REPLACE/UPDATE?
Missing GET?
> +struct flow_offload_action {
> + struct netlink_ext_ack *extack;
> + enum flow_act_command command;
> + struct flow_stats stats;
> + struct flow_action action;
> +};
On this:
On 2021-04-29 12:49 p.m., Ido Schimmel wrote:
> Can you share an example of the tc commands? You might be able to
> achieve what you want by patching NFP to take into account the policer
> index in 'struct flow_action_entry'. Seems that it currently assumes
> that each policer is a new policer.
>
> FTR, I do support the overall plan to offload actions independent of
> flows and to associate stats with actions.
I think Ido may be saying the same thing, but let me provide
my $0.02 Canadiana:
We generally need to identify the instance of a specific action i.e
its general attributes. In tc currently this maps to an index;
I would categorize two types of actions:
1) Stateless, example:
A lot of actions currently fall under this category.
--
sudo tc actions add action drop index 10
--
And later bound to say two flow entries by just specifying
the action {type id,index}:
---
sudo tc filter add dev XX parent ffff: protocol ip prio 8 \
u32 match ip dst 19.0.0.8/32 flowid 1:10 action gact index 10
#
sudo tc filter add dev XX parent ffff: protocol ip prio 7 \
u32 match ip src 10.0.0.0/24 flowid 1:11 action gact index 10
--
In such a case, in hardware this would typically map the specified
index to some hardware counter table index with the same index (10).
If user doesnt specify the index then either the hardware or the kernel
provides one. And user should be able to retrieve it with a GET.
[I have some patches that would actually return the index in the
extack somewhere]
2) stateful
There are other attributes other than counters that are updated
maintained. In this case i think the attribute id aka "index"
may be attribute specific:
Only example i can think of right now is policer (meters).
in hardware such an index will likely map to a meter index.
So for stateless actions i when i enter two commands
Same deal with config, i can create the policer, get,
update, delete it etc independently and maintain the binding
connection separately.
In both cases, dumping just the actions reduces the amount
of data crossing from hw->kernel->userspace.
If you have a lot of flows this becomes extremely valuable.
cheers,
jamal
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-05-10 22:37 ` Jamal Hadi Salim
@ 2021-05-10 22:47 ` Jamal Hadi Salim
0 siblings, 0 replies; 9+ messages in thread
From: Jamal Hadi Salim @ 2021-05-10 22:47 UTC (permalink / raw)
To: Simon Horman, Cong Wang, Jiri Pirko
Cc: netdev, oss-drivers, Baowen Zheng, Louis Peens
On 2021-05-10 6:37 p.m., Jamal Hadi Salim wrote:
> On 2021-04-29 4:10 a.m., Simon Horman wrote:
>> From: Baowen Zheng <baowen.zheng@corigine.com>
>>
>
> In both cases, dumping just the actions reduces the amount
> of data crossing from hw->kernel->userspace.
> If you have a lot of flows this becomes extremely valuable.
Bah. I meant dumping either the meters or counters should
be sufficient - each has an index which can be mapped to
a specific flow.
cheers,
jamal
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC net-next] net/flow_offload: allow user to offload tc action to net device
2021-04-29 8:10 [RFC net-next] net/flow_offload: allow user to offload tc action to net device Simon Horman
` (4 preceding siblings ...)
2021-05-10 22:37 ` Jamal Hadi Salim
@ 2021-05-21 7:33 ` Jianbo Liu
5 siblings, 0 replies; 9+ messages in thread
From: Jianbo Liu @ 2021-05-21 7:33 UTC (permalink / raw)
To: Simon Horman
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, oss-drivers,
Baowen Zheng, Louis Peens
The 04/29/2021 10:10, Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> 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 my 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.
Not sure if you consider the case when there are multiple netdevs in a
system which all support action offloading (for example, police), how
does driver know which device should this software meter be offloaded
to, and create hardware meter for it?
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.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 | 6 ++++
> include/net/tc_act/tc_police.h | 5 +++
> net/core/flow_offload.c | 26 +++++++++++++-
> net/sched/act_api.c | 30 ++++++++++++++++
> net/sched/cls_api.c | 34 ++++++++++++++++---
> 10 files changed, 119 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 6cdc52d50a48..c8dde4bc3961 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
> @@ -490,6 +490,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 e95969c462e4..f6c665051173 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -1839,6 +1839,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 5cbc950b34df..2f8cb65d85d1 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 dc5c1e69cd9f..5fd8b5600b4a 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -551,6 +551,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 255e4f4b521f..5c6549ae0cc7 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,
> @@ -538,6 +541,8 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
>
> int tc_setup_flow_action(struct flow_action *flow_action,
> const struct tcf_exts *exts);
> +int tc_setup_action(struct flow_action *flow_action,
> + struct tc_action *actions[]);
> void tc_cleanup_flow_action(struct flow_action *flow_action);
>
> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
> @@ -558,6 +563,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/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
> index 72649512dcdd..6309519bf9d4 100644
> --- a/include/net/tc_act/tc_police.h
> +++ b/include/net/tc_act/tc_police.h
> @@ -53,6 +53,11 @@ static inline bool is_tcf_police(const struct tc_action *act)
> return false;
> }
>
> +static inline u32 tcf_police_index(const struct tc_action *act)
> +{
> + return act->tcfa_index;
> +}
> +
Any action should have an index, maybe we can have a general function
to get index for all actions.
> static inline u64 tcf_police_rate_bytes_ps(const struct tc_action *act)
> {
> struct tcf_police *police = to_police(act);
> 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 f6d5755d669e..dab24688d9c7 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1059,6 +1059,34 @@ 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[], bool add, struct netlink_ext_ack *extack)
> +{
> + int err = 0;
> + struct flow_offload_action *fl_act;
> +
> + 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 = add ? FLOW_ACT_REPLACE : FLOW_ACT_DESTROY;
> +
> + 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,
> @@ -1107,6 +1135,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> */
> tcf_idr_insert_many(actions);
>
> + tcf_action_offload_cmd(actions, true, extack);
Maybe it should be placed it inside tcf_action_add, so only
independently defined actions (by tc-actions commands) are offloaded.
Otherwise, new hardware meter is created for each rule with police
action, and it's hard to share hardware meter among rules, right?
> *attr_size = tcf_action_full_attrs_size(sz);
> err = i - 1;
> goto err_mod;
> @@ -1465,6 +1494,7 @@ 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 */
> + tcf_action_offload_cmd(actions, false, extack);
> ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
> if (ret)
> goto err;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 40fbea626dfd..995024c20df6 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3551,8 +3551,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;
> @@ -3561,11 +3561,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];
> @@ -3732,6 +3732,16 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> spin_unlock_bh(&act->tcfa_lock);
> goto err_out;
> }
> +EXPORT_SYMBOL(tc_setup_action);
> +
> +int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts)
> +{
> + if (!exts)
> + return 0;
> +
> + return tc_setup_action(flow_action, exts->actions);
> +}
> EXPORT_SYMBOL(tc_setup_flow_action);
>
> unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> @@ -3750,6 +3760,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 [flat|nested] 9+ messages in thread