Re: [PATCH net-next 3/3] flow_offload: add process to update action stats from hardware

[Vlad Buslov <vladbu@nvidia.com>]

On Thu 22 Jul 2021 at 12:19, Simon Horman <simon.horman@corigine.com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>
>
> When collecting stats for actions update them using both
> both hardware and software counters.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
>  include/net/act_api.h      |  1 +
>  include/net/flow_offload.h |  2 +-
>  include/net/pkt_cls.h      |  4 ++++
>  net/sched/act_api.c        | 49 ++++++++++++++++++++++++++++++++++----
>  4 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 086b291e9530..fe8331b5efce 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -233,6 +233,7 @@ static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
>  void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
>  			     u64 drops, bool hw);
>  int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
> +int tcf_action_update_hw_stats(struct tc_action *action);
>  
>  int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>  			     struct tcf_chain **handle,
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 26644596fd54..467688fff7ce 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -560,7 +560,7 @@ enum flow_act_command {
>  };
>  
>  struct flow_offload_action {
> -	struct netlink_ext_ack *extack;
> +	struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/
>  	enum flow_act_command command;
>  	struct flow_stats stats;
>  	struct flow_action action;
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 03dae225d64f..569c9294b15b 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -282,6 +282,10 @@ tcf_exts_stats_update(const struct tcf_exts *exts,
>  	for (i = 0; i < exts->nr_actions; i++) {
>  		struct tc_action *a = exts->actions[i];
>  
> +		/* if stats from hw, just skip */
> +		if (!tcf_action_update_hw_stats(a))
> +			continue;
> +

Is it okay to call this inside preempt disable section?

>  		tcf_action_stats_update(a, bytes, packets, drops,
>  					lastuse, true);
>  		a->used_hw_stats = used_hw_stats;
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 23a4538916af..7d5535bc2c13 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1089,15 +1089,18 @@ int tcf_action_offload_cmd_pre(struct tc_action *actions[],
>  EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
>  
>  int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
> -				struct netlink_ext_ack *extack)
> +				struct netlink_ext_ack *extack,
> +				bool keep_fl_act)
>  {
>  	if (IS_ERR(fl_act))
>  		return PTR_ERR(fl_act);
>  
>  	flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
>  
> -	tc_cleanup_flow_action(&fl_act->action);
> -	kfree(fl_act);
> +	if (!keep_fl_act) {
> +		tc_cleanup_flow_action(&fl_act->action);
> +		kfree(fl_act);
> +	}
>  	return 0;
>  }
>  
> @@ -1115,10 +1118,45 @@ int tcf_action_offload_cmd(struct tc_action *actions[],
>  	if (err)
>  		return err;
>  
> -	return tcf_action_offload_cmd_post(fl_act, extack);
> +	return tcf_action_offload_cmd_post(fl_act, extack, false);
>  }
>  EXPORT_SYMBOL(tcf_action_offload_cmd);
>  
> +int tcf_action_update_hw_stats(struct tc_action *action)
> +{
> +	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
> +		[0] = action,
> +	};
> +	struct flow_offload_action *fl_act;
> +	int err = 0;
> +

Having some way to distinguish offloaded actions would also be useful
here to skip this function. I wonder how this affects dump rate when
executed for every single action, even when none of them were offloaded
through action API.

> +	err = tcf_action_offload_cmd_pre(actions,
> +					 FLOW_ACT_STATS,
> +					 NULL,
> +					 &fl_act);
> +	if (err)
> +		goto err_out;
> +
> +	err = tcf_action_offload_cmd_post(fl_act, NULL, true);
> +
> +	if (fl_act->stats.lastused) {
> +		tcf_action_stats_update(action, fl_act->stats.bytes,
> +					fl_act->stats.pkts,
> +					fl_act->stats.drops,
> +					fl_act->stats.lastused,
> +					true);
> +		err = 0;
> +	} else {
> +		err = -EOPNOTSUPP;
> +	}
> +	tc_cleanup_flow_action(&fl_act->action);
> +	kfree(fl_act);
> +
> +err_out:
> +	return err;
> +}
> +EXPORT_SYMBOL(tcf_action_update_hw_stats);
> +
>  /* offload the tc command after deleted */
>  int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
>  				struct tc_action *actions[],
> @@ -1255,6 +1293,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
>  	if (p == NULL)
>  		goto errout;
>  
> +	/* update hw stats for this action */
> +	tcf_action_update_hw_stats(p);
> +
>  	/* compat_mode being true specifies a call that is supposed
>  	 * to add additional backward compatibility statistic TLVs.
>  	 */