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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <91870cef611bf924ab36dab5d26abecb4b673b76.camel@redhat.com>
Date: Mon, 27 Nov 2023 12:01:10 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com, 
 jacob.e.keller@...el.com, jhs@...atatu.com, johannes@...solutions.net, 
 andriy.shevchenko@...ux.intel.com, amritha.nambiar@...el.com,
 sdf@...gle.com,  horms@...nel.org
Subject: Re: [patch net-next v4 3/9] devlink: send notifications only if
 there are listeners

On Thu, 2023-11-23 at 19:15 +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@...dia.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@...dia.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