* 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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 13:57 ` kernel test robot
2021-07-22 13:29 ` Vlad Buslov
` (4 subsequent siblings)
5 siblings, 0 replies; 46+ 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] 46+ messages in thread* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
@ 2021-07-22 13:57 ` kernel test robot
0 siblings, 0 replies; 46+ messages in thread
From: kernel test robot @ 2021-07-22 13:57 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2655 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(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39738 bytes --]
^ permalink raw reply [flat|nested] 46+ 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 15:31 ` kernel test robot
2021-07-22 13:29 ` Vlad Buslov
` (4 subsequent siblings)
5 siblings, 0 replies; 46+ 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] 46+ messages in thread* Re: [PATCH net-next 1/3] flow_offload: allow user to offload tc action to net device
@ 2021-07-22 15:31 ` kernel test robot
0 siblings, 0 replies; 46+ messages in thread
From: kernel test robot @ 2021-07-22 15:31 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 9363 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(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 44151 bytes --]
^ permalink raw reply [flat|nested] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ 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; 46+ 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] 46+ messages in thread