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]

Date:   Sun, 12 Dec 2021 09:22:39 +0000
From:   Baowen Zheng <baowen.zheng@...igine.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <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>,
        Louis Peens <louis.peens@...igine.com>,
        oss-drivers <oss-drivers@...igine.com>
Subject: RE: [PATCH v6 net-next 06/12] flow_offload: allow user to offload tc
 action to net device

On December 12, 2021 3:42 AM, Jamal Hadi Salim wrote:
>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?
Ok, we will make the change.
maybe it is more proper for "offload_act_command " as we discussed in previous patch?
>
>
>> +
>> +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"
Thanks, we will make the change.
>
>> +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?
For this function of tcf_action_cleanup, it is not only related to offload process, it will also recycle resource for the software action. 
So it is better to keep it as it is now?
>
>
>> 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()
Thanks, we will make the change for the renaming function as your suggestion.
>
>cheers,
>jamal