Re: [PATCH v6 net-next 06/12] flow_offload: allow user to offload tc action to net device

From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Simon Horman <simon.horman@corigine.com>, netdev@vger.kernel.org
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Ido Schimmel <idosch@nvidia.com>, Jiri Pirko <jiri@resnulli.us>,
	Oz Shlomo <ozsh@nvidia.com>, Roi Dayan <roid@nvidia.com>,
	Vlad Buslov <vladbu@nvidia.com>,
	Baowen Zheng <baowen.zheng@corigine.com>,
	Louis Peens <louis.peens@corigine.com>,
	oss-drivers@corigine.com
Subject: Re: [PATCH v6 net-next 06/12] flow_offload: allow user to offload tc action to net device
Date: Sat, 11 Dec 2021 14:42:25 -0500	[thread overview]
Message-ID: <a262c1ba-27e4-25cb-460c-c168a938f70e@mojatatu.com> (raw)
In-Reply-To: <20211209092806.12336-7-simon.horman@corigine.com>

On 2021-12-09 04:28, Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@corigine.com>

>   
>   /* 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 f6970213497a..15662cad5bca 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -551,6 +551,23 @@ struct flow_cls_offload {
>   	u32 classid;
>   };
>   
> +enum flow_act_command {

Readability:
flow_offload_act_command?

> +
> +struct flow_offload_action *flow_action_alloc(unsigned int num_actions);

Same here:
s/flow_action_alloc/offload_action_alloc

> +struct flow_offload_action *flow_action_alloc(unsigned int num_actions)
> +{

>   
> +static unsigned int tcf_act_num_actions_single(struct tc_action *act)
> +{
> +	if (is_tcf_pedit(act))
> +		return tcf_pedit_nkeys(act);
> +	else
> +		return 1;
> +}

Again - above only seems needed for offload. Could we name this
appropriately?

> +
> +static int flow_action_init(struct flow_offload_action *fl_action,

I think i mentioned this earlier:

> +
> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
> +				  struct netlink_ext_ack *extack)\

nice

> +
> +/* offload the tc command after inserted */

"after it is inserted"

> +static int tcf_action_offload_add(struct tc_action *action,

nice.

\
> +	err = tcf_action_offload_cmd(fl_action, extack);
> +	tc_cleanup_flow_action(&fl_action->action);

tc_cleanup_offload_action()?

> +static int tcf_action_offload_del(struct tc_action *action)

nice.

> +{
> +	struct flow_offload_action fl_act = {};
> +	int err = 0;
> +
> +	err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
> +	if (err)
> +		return err;
> +
> +	return tcf_action_offload_cmd(&fl_act, NULL);
> +}
> +
>   static void tcf_action_cleanup(struct tc_action *p)
>   {

mention offload somewhere there?

> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 33b81c867ac0..2a1cc7fe2dd9 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3488,8 +3488,8 @@ static int tc_setup_flow_act(struct tc_action *act,
>   #endif
>   }
>   
> -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[])
>   {
>   	int i, j, index, err = 0;
>   	struct tc_action *act;
> @@ -3498,11 +3498,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];
> @@ -3531,6 +3531,19 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>   	spin_unlock_bh(&act->tcfa_lock);
>   	goto err_out;
>   }
> +
> +int tc_setup_flow_action(struct flow_action *flow_action,
> +			 const struct tcf_exts *exts)
> +{

I think i mentioned this one earlier:
tc_setup_offload_action()

cheers,
jamal