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

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]

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

On 2021-12-09 04:28, Simon Horman wrote:
> From: Baowen Zheng <baowen.zheng@...igine.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