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

[Simon Horman <horms@kernel.org>]

On Wed, Dec 06, 2023 at 01:44:15PM -0300, Pedro Tammela wrote:
> As of today tc-action events are unconditionally built and sent to
> RTNLGRP_TC. As with the introduction of rtnl_notify_needed we can check
> before-hand if they are really needed.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Hi Pedro,

a nice optimisation :)

...

> +static int tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
> +{
> +	const struct tc_action_ops *ops = action->ops;
> +	struct sk_buff *skb = NULL;
> +	int ret;
> +
> +	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC))
> +		goto skip_msg;

Is there a reason (performance?) to use a goto here
rather than putting the tcf_reoffload_del_notify_msg() call inside
an if condition?

Completely untested!

	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
		skb = NULL;
	} else {
		skb = tcf_reoffload_del_notify_msg(net, action);
		if (IS_ERR(skb))
			return PTR_ERR(skb);
	}

Or perhaps a helper, as this pattern seems to also appear in tcf_add_notify()


> +
> +	skb = tcf_reoffload_del_notify_msg(net, action);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +
> +skip_msg:
>  	ret = tcf_idr_release_unsafe(action);
>  	if (ret == ACT_P_DELETED) {
>  		module_put(ops->owner);
> -		ret = rtnetlink_send(skb, net, 0, RTNLGRP_TC, 0);
> +		ret = rtnetlink_maybe_send(skb, net, 0, RTNLGRP_TC, 0);
>  	} else {
>  		kfree_skb(skb);
>  	}

...

> +static int tcf_add_notify(struct net *net, struct nlmsghdr *n,
> +			  struct tc_action *actions[], u32 portid,
> +			  size_t attr_size, struct netlink_ext_ack *extack)
> +{
> +	struct sk_buff *skb = NULL;
> +
> +	if (!rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
> +		goto skip_msg;
> +
> +	skb = tcf_add_notify_msg(net, n, actions, portid, attr_size, extack);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +
> +skip_msg:
> +	return rtnetlink_maybe_send(skb, net, portid, RTNLGRP_TC,
> +				    n->nlmsg_flags & NLM_F_ECHO);
>  }
>  
>  static int tcf_action_add(struct net *net, struct nlattr *nla,
> -- 
> 2.40.1
>