netdev - Re: [ RFC net-next v2 2/2] net: flow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 4 Oct 2022 11:22:40 +0300
From:   Oz Shlomo <ozsh@...dia.com>
To:     netdev@...r.kernel.org
Cc:     Jiri Pirko <jiri@...dia.com>, Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        Baowen Zheng <baowen.zheng@...igine.com>,
        Vlad Buslov <vladbu@...dia.com>,
        Ido Schimmel <idosch@...dia.com>, Roi Dayan <roid@...dia.com>,
        Edward Cree <ecree.xilinx@...il.com>
Subject: Re: [ RFC net-next v2 2/2] net: flow_offload: add action stats api



On 10/3/2022 9:17 AM, Oz Shlomo wrote:
> The current offload api provides visibility to flow hw stats.
> This works as long as the flow stats values apply to all the flow's
> actions. However, this assumption breaks when an action, such as police,
> drops or jumps over other actions.
> 
> Extend the flow_offload api to return stat record per action instance.
> Use the specific action, identified with an action cookie, stats value
> when updating the action's hardware stats.
> 
> Signed-off-by: Oz Shlomo <ozsh@...dia.com>
> ---
>   include/net/flow_offload.h | 10 ++++++++++
>   include/net/pkt_cls.h      | 18 ++++++++++++++++--
>   net/sched/cls_api.c        |  1 +
>   net/sched/cls_flower.c     |  3 ++-
>   net/sched/cls_matchall.c   |  2 +-
>   5 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index e343f9f8363e..1c88ca113544 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -213,6 +213,8 @@ struct flow_action_cookie {
>   	u8 cookie[];
>   };
>   
> +#define FLOW_OFFLOAD_MAX_ACT_STATS 32
> +
>   struct flow_action_cookie *flow_action_cookie_create(void *data,
>   						     unsigned int len,
>   						     gfp_t gfp);
> @@ -221,6 +223,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data,
>   struct flow_action_entry {
>   	enum flow_action_id		id;
>   	u32				hw_index;
> +	unsigned long			act_cookie;
>   	enum flow_action_hw_stats	hw_stats;
>   	action_destr			destructor;
>   	void				*destructor_priv;
> @@ -442,6 +445,11 @@ struct flow_stats {
>   	bool used_hw_stats_valid;
>   };
>   
> +struct flow_act_stat {
> +	unsigned long		act_cookie;
> +	struct flow_stats	stats;
> +};
> +
>   static inline void flow_stats_update(struct flow_stats *flow_stats,
>   				     u64 bytes, u64 pkts,
>   				     u64 drops, u64 lastused,
> @@ -588,6 +596,8 @@ struct flow_cls_offload {
>   	unsigned long cookie;
>   	struct flow_rule *rule;
>   	struct flow_stats stats;
> +	struct flow_act_stat act_stats[FLOW_OFFLOAD_MAX_ACT_STATS];

Apparently this array can exceed the stack frame size for several archs 
(reported by the kernel test bot).
I will change this array to be dynamically allocated.

> +	bool use_act_stats;
>   	u32 classid;
>   };
>   
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index d5b8fa01da87..642e6f07cbf0 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -282,13 +282,27 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>   
>   static inline void
>   tcf_exts_hw_stats_update(const struct tcf_exts *exts,
> -			 struct flow_stats *stats)
> +			 struct flow_stats *flow_stats,
> +			 struct flow_act_stat *act_stats,
> +			 bool use_act_stats)
>   {
>   #ifdef CONFIG_NET_CLS_ACT
> +	int nr_actions = exts->nr_actions;
>   	int i;
>   
> -	for (i = 0; i < exts->nr_actions; i++) {
> +	if (use_act_stats)
> +		nr_actions = FLOW_OFFLOAD_MAX_ACT_STATS;
> +
> +	for (i = 0; i < nr_actions; i++) {
>   		struct tc_action *a = exts->actions[i];
> +		struct flow_stats *stats = flow_stats;
> +
> +		if (use_act_stats) {
> +			a = (struct tc_action *)act_stats[i].act_cookie;
> +			if (!a)
> +				break;
> +			stats = &act_stats[i].stats;
> +		}
>   
>   		/* if stats from hw, just skip */
>   		if (tcf_action_update_hw_stats(a)) {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 50566db45949..c5a6a0d7f7a1 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3553,6 +3553,7 @@ int tc_setup_action(struct flow_action *flow_action,
>   		for (k = 0; k < index ; k++) {
>   			entry[k].hw_stats = tc_act_hw_stats(act->hw_stats);
>   			entry[k].hw_index = act->tcfa_index;
> +			entry[k].act_cookie = (unsigned long)act;
>   		}
>   
>   		j += index;
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 82b3e8ff656c..ff004f13d0c9 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -500,7 +500,8 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
>   	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
>   			 rtnl_held);
>   
> -	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
> +	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats,
> +				 cls_flower.act_stats, cls_flower.use_act_stats);
>   }
>   
>   static void __fl_put(struct cls_fl_filter *f)
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 225e87740ec5..3d441063113d 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -329,7 +329,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
>   
>   	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
>   
> -	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
> +	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, NULL, false);
>   }
>   
>   static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,