Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts

[Jiri Pirko <jiri@resnulli.us>]

Mon, Nov 27, 2023 at 01:30:04PM CET, przemyslaw.kitszel@intel.com wrote:
>On 11/23/23 19:15, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Currently the user listening on a socket for devlink notifications
>> gets always all messages for all existing instances, even if he is
>> interested only in one of those. That may cause unnecessary overhead
>> on setups with thousands of instances present.
>> 
>> User is currently able to narrow down the devlink objects replies
>> to dump commands by specifying select attributes.
>> 
>> Allow similar approach for notifications. Introduce a new devlink
>> NOTIFY_FILTER_SET which the user passes the select attributes. Store
>> these per-socket and use them for filtering messages
>> during multicast send.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> v3->v4:
>> - rebased on top of genl_sk_priv_*() introduction
>> ---
>>   Documentation/netlink/specs/devlink.yaml | 10 ++++
>>   include/uapi/linux/devlink.h             |  2 +
>>   net/devlink/devl_internal.h              | 34 ++++++++++-
>>   net/devlink/netlink.c                    | 73 ++++++++++++++++++++++++
>>   net/devlink/netlink_gen.c                | 15 ++++-
>>   net/devlink/netlink_gen.h                |  4 +-
>>   tools/net/ynl/generated/devlink-user.c   | 31 ++++++++++
>>   tools/net/ynl/generated/devlink-user.h   | 47 +++++++++++++++
>>   8 files changed, 212 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
>> index 43067e1f63aa..6bad1d3454b7 100644
>> --- a/Documentation/netlink/specs/devlink.yaml
>> +++ b/Documentation/netlink/specs/devlink.yaml
>> @@ -2055,3 +2055,13 @@ operations:
>>               - bus-name
>>               - dev-name
>>               - selftests
>> +
>> +    -
>> +      name: notify-filter-set
>> +      doc: Set notification messages socket filter.
>> +      attribute-set: devlink
>> +      do:
>> +        request:
>> +          attributes:
>> +            - bus-name
>> +            - dev-name
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index b3c8383d342d..130cae0d3e20 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -139,6 +139,8 @@ enum devlink_command {
>>   	DEVLINK_CMD_SELFTESTS_GET,	/* can dump */
>>   	DEVLINK_CMD_SELFTESTS_RUN,
>> +	DEVLINK_CMD_NOTIFY_FILTER_SET,
>> +
>>   	/* add new commands above here */
>>   	__DEVLINK_CMD_MAX,
>>   	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> index 84dc9628d3f2..82e0fb3bbebf 100644
>> --- a/net/devlink/devl_internal.h
>> +++ b/net/devlink/devl_internal.h
>> @@ -191,11 +191,41 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink)
>>   				  DEVLINK_MCGRP_CONFIG);
>>   }
>> +struct devlink_obj_desc {
>> +	struct rcu_head rcu;
>> +	const char *bus_name;
>> +	const char *dev_name;
>> +	long data[];
>
>could you please remove that data pointer?,
>you are not using desc as flex pointer as of now

But I am. See devlink_nl_notify_filter_set_doit()


>
>> +};
>> +
>> +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc,
>
>given next patch of the series with port index, you could rename this
>function to devlink_nl_obj_desc_names_set(), and move 0-init outside.

I don't see why. This init will be called all the time, even if in
future more attrs selection is going to be used.


>
>> +					    struct devlink *devlink)
>> +{
>> +	memset(desc, 0, sizeof(*desc));
>> +	desc->bus_name = devlink->dev->bus->name;
>> +	desc->dev_name = dev_name(devlink->dev);
>> +}
>> +
>> +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data);
>> +
>> +static inline void devlink_nl_notify_send_desc(struct devlink *devlink,
>> +					       struct sk_buff *msg,
>> +					       struct devlink_obj_desc *desc)
>> +{
>> +	genlmsg_multicast_netns_filtered(&devlink_nl_family,
>> +					 devlink_net(devlink),
>> +					 msg, 0, DEVLINK_MCGRP_CONFIG,
>> +					 GFP_KERNEL,
>> +					 devlink_nl_notify_filter, desc);
>> +}
>> +
>>   static inline void devlink_nl_notify_send(struct devlink *devlink,
>>   					  struct sk_buff *msg)
>>   {
>> -	genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
>> -				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
>> +	struct devlink_obj_desc desc;
>
>`= {};` would wipe out the need for memset().

True. If there is going to be a respin, I'll change this.


>
>> +
>> +	devlink_nl_obj_desc_init(&desc, devlink);
>> +	devlink_nl_notify_send_desc(devlink, msg, &desc);
>>   }
>>   /* Notify */
>
>[snip]