Re: [patch net-next v4 3/9] devlink: send notifications only if there are listeners

[Jiri Pirko <jiri@resnulli.us>]

Mon, Nov 27, 2023 at 12:01:10PM CET, pabeni@redhat.com wrote:
>On Thu, 2023-11-23 at 19:15 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Introduce devlink_nl_notify_need() helper and using it to check at the
>> beginning of notification functions to avoid overhead of composing
>> notification messages in case nobody listens.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  net/devlink/dev.c           | 5 ++++-
>>  net/devlink/devl_internal.h | 6 ++++++
>>  net/devlink/health.c        | 3 +++
>>  net/devlink/linecard.c      | 2 +-
>>  net/devlink/param.c         | 2 +-
>>  net/devlink/port.c          | 2 +-
>>  net/devlink/rate.c          | 2 +-
>>  net/devlink/region.c        | 2 +-
>>  net/devlink/trap.c          | 6 +++---
>>  9 files changed, 21 insertions(+), 9 deletions(-)
>> 
>> diff --git a/net/devlink/dev.c b/net/devlink/dev.c
>> index 7c7517e26862..46407689ef70 100644
>> --- a/net/devlink/dev.c
>> +++ b/net/devlink/dev.c
>> @@ -204,6 +204,9 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
>>  	WARN_ON(cmd != DEVLINK_CMD_NEW && cmd != DEVLINK_CMD_DEL);
>>  	WARN_ON(!devl_is_registered(devlink));
>
>minor nit: possibly use ASSERT_DEVLINK_REGISTERED(devlink) above?

Sure, but unrelated to this patch.


>
>>  
>> +	if (!devlink_nl_notify_need(devlink))
>> +		return;
>> +
>>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>  	if (!msg)
>>  		return;
>> @@ -985,7 +988,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>>  		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
>>  		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
>>  
>> -	if (!devl_is_registered(devlink))
>> +	if (!devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
>>  		return;
>>  
>>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> index 59ae4761d10a..510990de094e 100644
>> --- a/net/devlink/devl_internal.h
>> +++ b/net/devlink/devl_internal.h
>> @@ -185,6 +185,12 @@ int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net,
>>  				 struct devlink *devlink, int attrtype);
>>  int devlink_nl_msg_reply_and_new(struct sk_buff **msg, struct genl_info *info);
>>  
>> +static inline bool devlink_nl_notify_need(struct devlink *devlink)
>> +{
>> +	return genl_has_listeners(&devlink_nl_family, devlink_net(devlink),
>> +				  DEVLINK_MCGRP_CONFIG);
>> +}
>> +
>>  /* Notify */
>>  void devlink_notify_register(struct devlink *devlink);
>>  void devlink_notify_unregister(struct devlink *devlink);
>> diff --git a/net/devlink/health.c b/net/devlink/health.c
>> index 71ae121dc739..0795dcf22ca8 100644
>> --- a/net/devlink/health.c
>> +++ b/net/devlink/health.c
>> @@ -496,6 +496,9 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
>>  	WARN_ON(cmd != DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
>>  	ASSERT_DEVLINK_REGISTERED(devlink);
>>  
>> +	if (!devlink_nl_notify_need(devlink))
>> +		return;
>> +
>>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>  	if (!msg)
>>  		return;
>> diff --git a/net/devlink/linecard.c b/net/devlink/linecard.c
>> index 9d080ac1734b..45b36975ee6f 100644
>> --- a/net/devlink/linecard.c
>> +++ b/net/devlink/linecard.c
>> @@ -136,7 +136,7 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
>>  	WARN_ON(cmd != DEVLINK_CMD_LINECARD_NEW &&
>>  		cmd != DEVLINK_CMD_LINECARD_DEL);
>>  
>> -	if (!__devl_is_registered(devlink))
>> +	if (!__devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
>>  		return;
>>  
>>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> diff --git a/net/devlink/param.c b/net/devlink/param.c
>> index d74df09311a9..6bb6aee5d937 100644
>> --- a/net/devlink/param.c
>> +++ b/net/devlink/param.c
>> @@ -343,7 +343,7 @@ static void devlink_param_notify(struct devlink *devlink,
>>  	 * will replay the notifications if the params are added/removed
>>  	 * outside of the lifetime of the instance.
>>  	 */
>> -	if (!devl_is_registered(devlink))
>> +	if (!devlink_nl_notify_need(devlink) || !devl_is_registered(devlink))
>
>Minor nit: this is the only statement using this order, perhaps swap
>the tests for consistency?

Right. If respin is needed, I'll swap.


>
>Also possibly add the devlink_nl_notify_need() check in
>devl_is_registered to reduce code duplication? plus a

It would be odd to have devlink_nl_notify_need() called from
devl_is_registered(). Also, it is non only used on notification paths.
I thought about putting the checks in one function, but those are 2
separate and unrelated checks, so better to keep them separate.


>__devl_is_registered() variant for the 2 cases above not requiring the
>additional check.
>
>No need to repost for the above.
>
>Cheers,
>
>Paolo
>