[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9c6e3d86-a283-4c88-9293-613bd4de3d11@mojatatu.com>
Date: Fri, 8 Dec 2023 11:21:43 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, jhs@...atatu.com,
xiyou.wangcong@...il.com, jiri@...nulli.us, marcelo.leitner@...il.com,
vladbu@...dia.com
Subject: Re: [PATCH net-next v3 4/5] net/sched: act_api: conditional
notification of events
On 07/12/2023 17:56, Simon Horman wrote:
> 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@...atatu.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?
Not really, seems like the goto mosquito bit me on this one.
I will move to your suggestion and do the change Jiri asked which I
totally forgot in v3.
>
> 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
>>
Powered by blists - more mailing lists