diff mbox series

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

Message ID 20231123181546.521488-4-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series devlink: introduce notifications filtering | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1129 this patch: 1129
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 1155 this patch: 1155
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1156 this patch: 1156
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 102 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Nov. 23, 2023, 6:15 p.m. UTC 
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(-)

Comments

Paolo Abeni Nov. 27, 2023, 11:01 a.m. UTC | #1 
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?

>  
> +	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?

Also possibly add the devlink_nl_notify_need() check in
devl_is_registered to reduce code duplication? plus a
__devl_is_registered() variant for the 2 cases above not requiring the
additional check.

No need to repost for the above.

Cheers,

Paolo
Jiri Pirko Nov. 27, 2023, 12:04 p.m. UTC | #2 
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
>
Paolo Abeni Nov. 27, 2023, 3 p.m. UTC | #3 
On Mon, 2023-11-27 at 13:04 +0100, Jiri Pirko wrote:
> 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(). 

Sorry for the confusion, out-of-order on my side. What I really mean
is: add __devl_is_registered() in devlink_nl_notify_need(). 

> 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.

It looks like devlink_nl_notify_need() implies/requires
__devl_is_registered() ?

Cheers,

Paolo
Jiri Pirko Nov. 28, 2023, 7:39 a.m. UTC | #4 
Mon, Nov 27, 2023 at 04:00:42PM CET, pabeni@redhat.com wrote:
>On Mon, 2023-11-27 at 13:04 +0100, Jiri Pirko wrote:
>> 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(). 
>
>Sorry for the confusion, out-of-order on my side. What I really mean
>is: add __devl_is_registered() in devlink_nl_notify_need(). 
>
>> 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.
>
>It looks like devlink_nl_notify_need() implies/requires
>__devl_is_registered() ?
>

__devl_is_registered() and devl_is_registered(), depends on the case.
That's why I prefer to have the check outside devlink_nl_notify_need()


>Cheers,
>
>Paolo
>
diff mbox series

Patch

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));
 
+	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))
 		return;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/devlink/port.c b/net/devlink/port.c
index f229a8699214..32f4d0331e63 100644
--- a/net/devlink/port.c
+++ b/net/devlink/port.c
@@ -512,7 +512,7 @@  static void devlink_port_notify(struct devlink_port *devlink_port,
 
 	WARN_ON(cmd != DEVLINK_CMD_PORT_NEW && cmd != DEVLINK_CMD_PORT_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/rate.c b/net/devlink/rate.c
index e2190cf22beb..0371a2dd3e0a 100644
--- a/net/devlink/rate.c
+++ b/net/devlink/rate.c
@@ -146,7 +146,7 @@  static void devlink_rate_notify(struct devlink_rate *devlink_rate,
 
 	WARN_ON(cmd != DEVLINK_CMD_RATE_NEW && cmd != DEVLINK_CMD_RATE_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/region.c b/net/devlink/region.c
index b65181aa269a..bf61312f64bd 100644
--- a/net/devlink/region.c
+++ b/net/devlink/region.c
@@ -235,7 +235,7 @@  static void devlink_nl_region_notify(struct devlink_region *region,
 
 	WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
 
-	if (!__devl_is_registered(devlink))
+	if (!__devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
 		return;
 
 	msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0);
diff --git a/net/devlink/trap.c b/net/devlink/trap.c
index 908085e2c990..3ca1ca7e2e64 100644
--- a/net/devlink/trap.c
+++ b/net/devlink/trap.c
@@ -1174,7 +1174,7 @@  devlink_trap_group_notify(struct devlink *devlink,
 	WARN_ON_ONCE(cmd != DEVLINK_CMD_TRAP_GROUP_NEW &&
 		     cmd != DEVLINK_CMD_TRAP_GROUP_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);
@@ -1236,7 +1236,7 @@  static void devlink_trap_notify(struct devlink *devlink,
 	WARN_ON_ONCE(cmd != DEVLINK_CMD_TRAP_NEW &&
 		     cmd != DEVLINK_CMD_TRAP_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);
@@ -1713,7 +1713,7 @@  devlink_trap_policer_notify(struct devlink *devlink,
 	WARN_ON_ONCE(cmd != DEVLINK_CMD_TRAP_POLICER_NEW &&
 		     cmd != DEVLINK_CMD_TRAP_POLICER_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);