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

Jiri Pirko
2023-11-27T12:51:07Z

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]