Re: [PATCH net-next v2 4/5] net/sched: act_api: conditional notification of events

[Jiri Pirko <jiri@resnulli.us>]

Tue, Dec 05, 2023 at 03:45:52PM CET, pctammela@mojatatu.com wrote:
>On 05/12/2023 08:32, Jiri Pirko wrote:
>> Mon, Dec 04, 2023 at 09:39:06PM CET, pctammela@mojatatu.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@mojatatu.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.

Okay.

>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;
>> > +}
>> > +
>> 
>> [...]
>