diff mbox series

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

Message ID 20231115141724.411507-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/tree_selection success Clearly marked for net-next
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: 1136 this patch: 1136
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 1162 this patch: 1162
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: 1163 this patch: 1163
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. 15, 2023, 2:17 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

Jacob Keller Nov. 15, 2023, 8:17 p.m. UTC | #1 
On 11/15/2023 6:17 AM, Jiri Pirko wrote:
> +static inline bool devlink_nl_notify_need(struct devlink *devlink)
> +{
> +	return genl_has_listeners(&devlink_nl_family, devlink_net(devlink),
> +				  DEVLINK_MCGRP_CONFIG);
> +}
> +

I assume following changes will add more checks here to filter here so
it doesn't make sense to call this "devlink_has_listeners"? I feel like
the devlink_nl_notify_need is a bit weird of a way to phrase this.

I don't have a strong objection to the name overall, just found it a bit
odd.

>  /* 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 695df61f8ac2..93eae8b5d2d3 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;
>  

A bunch of callers are checking both if its registered and a
notification is needed, does it make sense to combine this? Or I guess
at least a few places are notifying of removal after its no longer
registered, so we can't inline the devl_is_registered into the
devlink_nl_notify_need. Probably more clear to keep it separate too.

Ok.
Jiri Pirko Nov. 16, 2023, 7:14 a.m. UTC | #2 
Wed, Nov 15, 2023 at 09:17:17PM CET, jacob.e.keller@intel.com wrote:
>
>
>On 11/15/2023 6:17 AM, Jiri Pirko wrote:
>> +static inline bool devlink_nl_notify_need(struct devlink *devlink)
>> +{
>> +	return genl_has_listeners(&devlink_nl_family, devlink_net(devlink),
>> +				  DEVLINK_MCGRP_CONFIG);
>> +}
>> +
>
>I assume following changes will add more checks here to filter here so
>it doesn't make sense to call this "devlink_has_listeners"? I feel like
>the devlink_nl_notify_need is a bit weird of a way to phrase this.
>
>I don't have a strong objection to the name overall, just found it a bit
>odd.

Right. I named it like this because eventually, this function is going
to check the filters as well return false if there is no msg passed to
any listening socket through any filter. That is the plan.


>
>>  /* 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 695df61f8ac2..93eae8b5d2d3 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;
>>  
>
>A bunch of callers are checking both if its registered and a
>notification is needed, does it make sense to combine this? Or I guess
>at least a few places are notifying of removal after its no longer
>registered, so we can't inline the devl_is_registered into the
>devlink_nl_notify_need. Probably more clear to keep it separate too.

Yeah, it is more clear to have it separate. Plus there would have to be
2 locked and unlocked versions.


>
>Ok.
diff mbox series

Patch

diff --git a/net/devlink/dev.c b/net/devlink/dev.c
index 4667ab3e9ff1..582b5177f403 100644
--- a/net/devlink/dev.c
+++ b/net/devlink/dev.c
@@ -203,6 +203,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;
@@ -977,7 +980,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 381b8e62d906..8b48a07eb7b7 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -172,6 +172,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 695df61f8ac2..93eae8b5d2d3 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 396930324da4..f1402da66277 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);