[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6a70805b-953d-494e-a9b9-a2cb0e0aff18@mojatatu.com>
Date: Tue, 5 Dec 2023 11:45:52 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, jhs@...atatu.com,
xiyou.wangcong@...il.com, marcelo.leitner@...il.com, vladbu@...dia.com
Subject: Re: [PATCH net-next v2 4/5] net/sched: act_api: conditional
notification of events
On 05/12/2023 08:32, Jiri Pirko wrote:
> Mon, Dec 04, 2023 at 09:39:06PM CET, pctammela@...atatu.com wrote:
>> As of today tc-action events are unconditionally built and sent to
>> RTNLGRP_TC. As with the introduction of tc_should_notify we can check
>> before-hand if they are really needed.
>>
>> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
>> ---
>> net/sched/act_api.c | 105 ++++++++++++++++++++++++++++++++------------
>> 1 file changed, 76 insertions(+), 29 deletions(-)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index c39252d61ebb..55c62a8e8803 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -1780,31 +1780,45 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
>> return 0;
>> }
>>
>> -static int
>> -tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
>> +static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net,
>
> I wonder, why this new function is needed? If I'm reading things
> correctly, tcf_reoffload_del_notify() with added check would be just ok,
> woundn't it?
>
> Same for others.
In V1 we had it like what you are suggesting[1].
Jakub suggested to refactor the functions a bit more. The main argument
was the code duplication introduced for the delete routines.
Note that for the case that no notification is needed, we still need to
do the action delete etc...
I agree that code duplication is bad in the long term, so I did the
changes, but I don't have a strong opinion here (either way is fine for me).
Let's see what he has to say, perhaps I overdid what he was suggesting :)
[1]
https://lore.kernel.org/all/20231201204314.220543-4-pctammela@mojatatu.com/
>
>> + struct tc_action *action)
>> {
>> size_t attr_size = tcf_action_fill_size(action);
>> struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
>> [0] = action,
>> };
>> - const struct tc_action_ops *ops = action->ops;
>> struct sk_buff *skb;
>> - int ret;
>>
>> - skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
>> - GFP_KERNEL);
>> + skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL);
>
> I don't see how this is related to this patch. Can't you do it in separate
> patch?
>
> Same for others.
Sure, will split it out.
>
>> if (!skb)
>> - return -ENOBUFS;
>> + return ERR_PTR(-ENOBUFS);
>>
>> if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) {
>> kfree_skb(skb);
>> - return -EINVAL;
>> + return ERR_PTR(-EINVAL);
>> }
>>
>> + return skb;
>> +}
>> +
>
> [...]
Powered by blists - more mailing lists