* [patch net-next v4 0/9] devlink: introduce notifications filtering
@ 2023-11-23 18:15 Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 1/9] devlink: use devl_is_registered() helper instead xa_get_mark() Jiri Pirko
` (8 more replies)
0 siblings, 9 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Currently the user listening on a socket for devlink notifications
gets always all messages for all existing devlink instances and objects,
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 providing user a new
notify-filter-set command to select attributes with values
the notification message has to match. In that case, it is delivered
to the socket.
Note that the filtering is done per-socket, so multiple users may
specify different selection of attributes with values.
This patchset initially introduces support for following attributes:
DEVLINK_ATTR_BUS_NAME
DEVLINK_ATTR_DEV_NAME
DEVLINK_ATTR_PORT_INDEX
Patches #1 - #4 are preparations in devlink code, patch #3 is
an optimization done on the way.
Patches #5 - #7 are preparations in netlink and generic netlink code.
Patch #8 is the main one in this set implementing of
the notify-filter-set command and the actual
per-socket filtering.
Patch #9 extends the infrastructure allowing to filter according
to a port index.
Example:
$ devlink mon port pci/0000:08:00.0/32768
[port,new] pci/0000:08:00.0/32768: type notset flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
[port,new] pci/0000:08:00.0/32768: type eth flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
[port,new] pci/0000:08:00.0/32768: type eth netdev eth3 flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
[port,new] pci/0000:08:00.0/32768: type eth netdev eth3 flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
[port,new] pci/0000:08:00.0/32768: type eth flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
[port,new] pci/0000:08:00.0/32768: type notset flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
[port,del] pci/0000:08:00.0/32768: type notset flavour pcisf controller 0 pfnum 0 sfnum 107 splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
---
v3->v4:
- converted from sk_user_data pointer use to nlk(sk)->priv pointer and
allow priv to be stored for multiple generic netlink families, see
patch #5 for more details
v2->v3:
- small cosmetical fixes in patch #6
v1->v2:
- added patch #6, fixed generated docs
- see individual patches for details
Jiri Pirko (9):
devlink: use devl_is_registered() helper instead xa_get_mark()
devlink: introduce __devl_is_registered() helper and use it instead of
xa_get_mark()
devlink: send notifications only if there are listeners
devlink: introduce a helper for netlink multicast send
genetlink: introduce per-sock family private pointer storage
netlink: introduce typedef for filter function
genetlink: introduce helpers to do filtered multicast
devlink: add a command to set notification filter and use it for
multicasts
devlink: extend multicast filtering by port index
Documentation/netlink/specs/devlink.yaml | 11 +++
drivers/connector/connector.c | 5 +-
include/linux/connector.h | 3 +-
include/linux/netlink.h | 6 +-
include/net/genetlink.h | 38 ++++++++-
include/net/netlink.h | 31 +++++++-
include/uapi/linux/devlink.h | 2 +
net/devlink/dev.c | 13 ++--
net/devlink/devl_internal.h | 59 +++++++++++++-
net/devlink/health.c | 10 ++-
net/devlink/linecard.c | 5 +-
net/devlink/netlink.c | 81 ++++++++++++++++++++
net/devlink/netlink_gen.c | 16 +++-
net/devlink/netlink_gen.h | 4 +-
net/devlink/param.c | 5 +-
net/devlink/port.c | 8 +-
net/devlink/rate.c | 5 +-
net/devlink/region.c | 6 +-
net/devlink/trap.c | 18 ++---
net/netlink/af_netlink.c | 3 +-
net/netlink/af_netlink.h | 1 +
net/netlink/genetlink.c | 98 ++++++++++++++++++++++++
tools/net/ynl/generated/devlink-user.c | 33 ++++++++
tools/net/ynl/generated/devlink-user.h | 56 ++++++++++++++
24 files changed, 465 insertions(+), 52 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 40+ messages in thread
* [patch net-next v4 1/9] devlink: use devl_is_registered() helper instead xa_get_mark()
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 2/9] devlink: introduce __devl_is_registered() helper and use it instead of xa_get_mark() Jiri Pirko
` (7 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Instead of checking the xarray mark directly using xa_get_mark() helper
use devl_is_registered() helper which wraps it up. Note that there are
couple more users of xa_get_mark() left which are going to be handled
by the next patch.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
net/devlink/dev.c | 4 ++--
net/devlink/rate.c | 2 +-
net/devlink/trap.c | 9 ++++++---
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/net/devlink/dev.c b/net/devlink/dev.c
index ea6a92f2e6a2..7c7517e26862 100644
--- a/net/devlink/dev.c
+++ b/net/devlink/dev.c
@@ -202,7 +202,7 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
int err;
WARN_ON(cmd != DEVLINK_CMD_NEW && cmd != DEVLINK_CMD_DEL);
- WARN_ON(!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED));
+ WARN_ON(!devl_is_registered(devlink));
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
@@ -985,7 +985,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
- if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+ if (!devl_is_registered(devlink))
return;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/devlink/rate.c b/net/devlink/rate.c
index 94b289b93ff2..e2190cf22beb 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 (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+ if (!devl_is_registered(devlink))
return;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/devlink/trap.c b/net/devlink/trap.c
index c26313e7ca08..908085e2c990 100644
--- a/net/devlink/trap.c
+++ b/net/devlink/trap.c
@@ -1173,7 +1173,8 @@ devlink_trap_group_notify(struct devlink *devlink,
WARN_ON_ONCE(cmd != DEVLINK_CMD_TRAP_GROUP_NEW &&
cmd != DEVLINK_CMD_TRAP_GROUP_DEL);
- if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+
+ if (!devl_is_registered(devlink))
return;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -1234,7 +1235,8 @@ static void devlink_trap_notify(struct devlink *devlink,
WARN_ON_ONCE(cmd != DEVLINK_CMD_TRAP_NEW &&
cmd != DEVLINK_CMD_TRAP_DEL);
- if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+
+ if (!devl_is_registered(devlink))
return;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -1710,7 +1712,8 @@ devlink_trap_policer_notify(struct devlink *devlink,
WARN_ON_ONCE(cmd != DEVLINK_CMD_TRAP_POLICER_NEW &&
cmd != DEVLINK_CMD_TRAP_POLICER_DEL);
- if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+
+ if (!devl_is_registered(devlink))
return;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 2/9] devlink: introduce __devl_is_registered() helper and use it instead of xa_get_mark()
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 1/9] devlink: use devl_is_registered() helper instead xa_get_mark() Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 3/9] devlink: send notifications only if there are listeners Jiri Pirko
` (6 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Introduce __devl_is_registered() which does not assert on devlink
instance lock and use it in notifications which may be called
without devlink instance lock held.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
net/devlink/devl_internal.h | 7 ++++++-
net/devlink/linecard.c | 2 +-
net/devlink/port.c | 2 +-
net/devlink/region.c | 3 ++-
4 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 5ea2e2012e93..59ae4761d10a 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -91,10 +91,15 @@ extern struct genl_family devlink_nl_family;
struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp);
+static inline bool __devl_is_registered(struct devlink *devlink)
+{
+ return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+}
+
static inline bool devl_is_registered(struct devlink *devlink)
{
devl_assert_locked(devlink);
- return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+ return __devl_is_registered(devlink);
}
static inline void devl_dev_lock(struct devlink *devlink, bool dev_lock)
diff --git a/net/devlink/linecard.c b/net/devlink/linecard.c
index 2f1c317b64cd..9d080ac1734b 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 (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+ if (!__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 7634f187fa50..f229a8699214 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 (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+ if (!__devl_is_registered(devlink))
return;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/devlink/region.c b/net/devlink/region.c
index e3bab458db94..b65181aa269a 100644
--- a/net/devlink/region.c
+++ b/net/devlink/region.c
@@ -234,7 +234,8 @@ static void devlink_nl_region_notify(struct devlink_region *region,
struct sk_buff *msg;
WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
- if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
+
+ if (!__devl_is_registered(devlink))
return;
msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0);
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 3/9] devlink: send notifications only if there are listeners
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 1/9] devlink: use devl_is_registered() helper instead xa_get_mark() Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 2/9] devlink: introduce __devl_is_registered() helper and use it instead of xa_get_mark() Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-27 11:01 ` Paolo Abeni
2023-11-23 18:15 ` [patch net-next v4 4/9] devlink: introduce a helper for netlink multicast send Jiri Pirko
` (5 subsequent siblings)
8 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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));
+ 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);
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 4/9] devlink: introduce a helper for netlink multicast send
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
` (2 preceding siblings ...)
2023-11-23 18:15 ` [patch net-next v4 3/9] devlink: send notifications only if there are listeners Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage Jiri Pirko
` (4 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Introduce a helper devlink_nl_notify_send() so each object notification
function does not have to call genlmsg_multicast_netns() with the same
arguments.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
net/devlink/dev.c | 6 ++----
net/devlink/devl_internal.h | 7 +++++++
net/devlink/health.c | 3 +--
net/devlink/linecard.c | 3 +--
net/devlink/param.c | 3 +--
net/devlink/port.c | 3 +--
net/devlink/rate.c | 3 +--
net/devlink/region.c | 3 +--
net/devlink/trap.c | 9 +++------
9 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/net/devlink/dev.c b/net/devlink/dev.c
index 46407689ef70..4e54700409d1 100644
--- a/net/devlink/dev.c
+++ b/net/devlink/dev.c
@@ -217,8 +217,7 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
int devlink_nl_get_doit(struct sk_buff *skb, struct genl_info *info)
@@ -999,8 +998,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
if (err)
goto out_free_msg;
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
return;
out_free_msg:
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 510990de094e..84dc9628d3f2 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -191,6 +191,13 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink)
DEVLINK_MCGRP_CONFIG);
}
+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);
+}
+
/* 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 0795dcf22ca8..1d59ec0202f6 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -509,8 +509,7 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), msg,
- 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void
diff --git a/net/devlink/linecard.c b/net/devlink/linecard.c
index 45b36975ee6f..67f70a621d27 100644
--- a/net/devlink/linecard.c
+++ b/net/devlink/linecard.c
@@ -150,8 +150,7 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void devlink_linecards_notify_register(struct devlink *devlink)
diff --git a/net/devlink/param.c b/net/devlink/param.c
index 6bb6aee5d937..854a3af65db9 100644
--- a/net/devlink/param.c
+++ b/net/devlink/param.c
@@ -356,8 +356,7 @@ static void devlink_param_notify(struct devlink *devlink,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
static void devlink_params_notify(struct devlink *devlink,
diff --git a/net/devlink/port.c b/net/devlink/port.c
index 32f4d0331e63..758df3000a1b 100644
--- a/net/devlink/port.c
+++ b/net/devlink/port.c
@@ -525,8 +525,7 @@ static void devlink_port_notify(struct devlink_port *devlink_port,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), msg,
- 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
static void devlink_ports_notify(struct devlink *devlink,
diff --git a/net/devlink/rate.c b/net/devlink/rate.c
index 0371a2dd3e0a..7139e67e93ae 100644
--- a/net/devlink/rate.c
+++ b/net/devlink/rate.c
@@ -159,8 +159,7 @@ static void devlink_rate_notify(struct devlink_rate *devlink_rate,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), msg,
- 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void devlink_rates_notify_register(struct devlink *devlink)
diff --git a/net/devlink/region.c b/net/devlink/region.c
index bf61312f64bd..7319127c5913 100644
--- a/net/devlink/region.c
+++ b/net/devlink/region.c
@@ -242,8 +242,7 @@ static void devlink_nl_region_notify(struct devlink_region *region,
if (IS_ERR(msg))
return;
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), msg,
- 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void devlink_regions_notify_register(struct devlink *devlink)
diff --git a/net/devlink/trap.c b/net/devlink/trap.c
index 3ca1ca7e2e64..5d18c7424df1 100644
--- a/net/devlink/trap.c
+++ b/net/devlink/trap.c
@@ -1188,8 +1188,7 @@ devlink_trap_group_notify(struct devlink *devlink,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void devlink_trap_groups_notify_register(struct devlink *devlink)
@@ -1249,8 +1248,7 @@ static void devlink_trap_notify(struct devlink *devlink,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void devlink_traps_notify_register(struct devlink *devlink)
@@ -1727,8 +1725,7 @@ devlink_trap_policer_notify(struct devlink *devlink,
return;
}
- genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
- msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
+ devlink_nl_notify_send(devlink, msg);
}
void devlink_trap_policers_notify_register(struct devlink *devlink)
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
` (3 preceding siblings ...)
2023-11-23 18:15 ` [patch net-next v4 4/9] devlink: introduce a helper for netlink multicast send Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-27 11:13 ` Paolo Abeni
` (2 more replies)
2023-11-23 18:15 ` [patch net-next v4 6/9] netlink: introduce typedef for filter function Jiri Pirko
` (3 subsequent siblings)
8 siblings, 3 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Introduce a priv pointer into struct netlink_sock. Use it to store a per
socket xarray that contains family->id indexed priv pointer storage.
Note I used xarray instead of suggested linked list as it is more
convenient, without need to have a container struct that would
contain struct list_head item.
Introduce genl_sk_priv_store() to store the priv pointer.
Introduce genl_sk_priv_get() to obtain the priv pointer under RCU
read lock.
Assume that kfree() is good for free of privs for now, as the only user
introduced by the follow-up patch (devlink) will use kzalloc() for the
allocation of the memory of the stored pointer. If later on
this needs to be made custom, a callback is going to be needed.
Until then (if ever), do this in a simple way.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v3->v4:
- new patch
---
include/net/genetlink.h | 3 ++
net/netlink/af_netlink.h | 1 +
net/netlink/genetlink.c | 98 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index e18a4c0d69ee..66c1e50415e0 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -300,6 +300,9 @@ int genl_register_family(struct genl_family *family);
int genl_unregister_family(const struct genl_family *family);
void genl_notify(const struct genl_family *family, struct sk_buff *skb,
struct genl_info *info, u32 group, gfp_t flags);
+void *genl_sk_priv_get(struct sock *sk, struct genl_family *family);
+void *genl_sk_priv_store(struct sock *sk, struct genl_family *family,
+ void *priv);
void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
const struct genl_family *family, int flags, u8 cmd);
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 2145979b9986..5d96135a4cf3 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -51,6 +51,7 @@ struct netlink_sock {
struct rhash_head node;
struct rcu_head rcu;
struct work_struct work;
+ void __rcu *priv;
};
static inline struct netlink_sock *nlk_sk(struct sock *sk)
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 92ef5ed2e7b0..aae5e63fa50b 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -21,6 +21,7 @@
#include <linux/idr.h>
#include <net/sock.h>
#include <net/genetlink.h>
+#include "af_netlink.h"
static DEFINE_MUTEX(genl_mutex); /* serialization of message processing */
static DECLARE_RWSEM(cb_lock);
@@ -1699,12 +1700,109 @@ static int genl_bind(struct net *net, int group)
return ret;
}
+struct genl_sk_ctx {
+ struct xarray family_privs;
+};
+
+static struct genl_sk_ctx *genl_sk_ctx_alloc(void)
+{
+ struct genl_sk_ctx *ctx;
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return NULL;
+ xa_init_flags(&ctx->family_privs, XA_FLAGS_ALLOC);
+ return ctx;
+}
+
+static void genl_sk_ctx_free(struct genl_sk_ctx *ctx)
+{
+ unsigned long family_id;
+ void *priv;
+
+ xa_for_each(&ctx->family_privs, family_id, priv) {
+ xa_erase(&ctx->family_privs, family_id);
+ kfree(priv);
+ }
+ xa_destroy(&ctx->family_privs);
+ kfree(ctx);
+}
+
+/**
+ * genl_sk_priv_get - Get per-socket private pointer for family
+ *
+ * @sk: socket
+ * @family: family
+ *
+ * Lookup a private pointer stored per-socket by a specified
+ * Generic netlink family.
+ *
+ * Caller should make sure this is called in RCU read locked section.
+ *
+ * Returns: valid pointer on success, otherwise NULL.
+ */
+void *genl_sk_priv_get(struct sock *sk, struct genl_family *family)
+{
+ struct genl_sk_ctx *ctx;
+
+ ctx = rcu_dereference(nlk_sk(sk)->priv);
+ if (!ctx)
+ return NULL;
+ return xa_load(&ctx->family_privs, family->id);
+}
+
+/**
+ * genl_sk_priv_store - Store per-socket private pointer for family
+ *
+ * @sk: socket
+ * @family: family
+ * @priv: private pointer
+ *
+ * Store a private pointer per-socket for a specified
+ * Generic netlink family.
+ *
+ * Caller has to make sure this is not called in parallel multiple times
+ * for the same sock and also in parallel to genl_release() for the same sock.
+ *
+ * Returns: previously stored private pointer for the family (could be NULL)
+ * on success, otherwise negative error value encoded by ERR_PTR().
+ */
+void *genl_sk_priv_store(struct sock *sk, struct genl_family *family,
+ void *priv)
+{
+ struct genl_sk_ctx *ctx;
+ void *old_priv;
+
+ ctx = rcu_dereference_raw(nlk_sk(sk)->priv);
+ if (!ctx) {
+ ctx = genl_sk_ctx_alloc();
+ if (!ctx)
+ return ERR_PTR(-ENOMEM);
+ rcu_assign_pointer(nlk_sk(sk)->priv, ctx);
+ }
+
+ old_priv = xa_store(&ctx->family_privs, family->id, priv, GFP_KERNEL);
+ if (xa_is_err(old_priv))
+ return ERR_PTR(xa_err(old_priv));
+ return old_priv;
+}
+
+static void genl_release(struct sock *sk, unsigned long *groups)
+{
+ struct genl_sk_ctx *ctx;
+
+ ctx = rcu_dereference_raw(nlk_sk(sk)->priv);
+ if (ctx)
+ genl_sk_ctx_free(ctx);
+}
+
static int __net_init genl_pernet_init(struct net *net)
{
struct netlink_kernel_cfg cfg = {
.input = genl_rcv,
.flags = NL_CFG_F_NONROOT_RECV,
.bind = genl_bind,
+ .release = genl_release,
};
/* we'll bump the group number right afterwards */
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 6/9] netlink: introduce typedef for filter function
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
` (4 preceding siblings ...)
2023-11-23 18:15 ` [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 7/9] genetlink: introduce helpers to do filtered multicast Jiri Pirko
` (2 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Make the code using filter function a bit nicer by consolidating the
filter function arguments using typedef.
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- left the original .c and .h arg names and types
inconsistencies for cn_netlink_send_mult() and
netlink_broadcast_filtered()
v1->v2:
- new patch
---
drivers/connector/connector.c | 5 ++---
include/linux/connector.h | 3 +--
include/linux/netlink.h | 6 ++++--
net/netlink/af_netlink.c | 3 +--
4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 7f7b94f616a6..4028e8eeba82 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -59,9 +59,8 @@ static int cn_already_initialized;
* both, or if both are zero then the group is looked up and sent there.
*/
int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid, u32 __group,
- gfp_t gfp_mask,
- int (*filter)(struct sock *dsk, struct sk_buff *skb, void *data),
- void *filter_data)
+ gfp_t gfp_mask, netlink_filter_fn filter,
+ void *filter_data)
{
struct cn_callback_entry *__cbq;
unsigned int size;
diff --git a/include/linux/connector.h b/include/linux/connector.h
index cec2d99ae902..70bc1160f3d8 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -100,8 +100,7 @@ void cn_del_callback(const struct cb_id *id);
*/
int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid,
u32 group, gfp_t gfp_mask,
- int (*filter)(struct sock *dsk, struct sk_buff *skb,
- void *data),
+ netlink_filter_fn filter,
void *filter_data);
/**
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index abe91ed6b9aa..1a4445bf2ab9 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -228,10 +228,12 @@ bool netlink_strict_get_check(struct sk_buff *skb);
int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 portid,
__u32 group, gfp_t allocation);
+
+typedef int (*netlink_filter_fn)(struct sock *dsk, struct sk_buff *skb, void *data);
+
int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb,
__u32 portid, __u32 group, gfp_t allocation,
- int (*filter)(struct sock *dsk,
- struct sk_buff *skb, void *data),
+ netlink_filter_fn filter,
void *filter_data);
int netlink_set_err(struct sock *ssk, __u32 portid, __u32 group, int code);
int netlink_register_notifier(struct notifier_block *nb);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 177126fb0484..4ed8ffd58ff3 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1519,8 +1519,7 @@ static void do_one_broadcast(struct sock *sk,
int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb,
u32 portid,
u32 group, gfp_t allocation,
- int (*filter)(struct sock *dsk,
- struct sk_buff *skb, void *data),
+ netlink_filter_fn filter,
void *filter_data)
{
struct net *net = sock_net(ssk);
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 7/9] genetlink: introduce helpers to do filtered multicast
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
` (5 preceding siblings ...)
2023-11-23 18:15 ` [patch net-next v4 6/9] netlink: introduce typedef for filter function Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 9/9] devlink: extend multicast filtering by port index Jiri Pirko
8 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Currently it is possible for netlink kernel user to pass custom
filter function to broadcast send function netlink_broadcast_filtered().
However, this is not exposed to multicast send and to generic
netlink users.
Extend the api and introduce a netlink helper nlmsg_multicast_filtered()
and a generic netlink helper genlmsg_multicast_netns_filtered()
to allow generic netlink families to specify filter function
while sending multicast messages.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- used netlink_filter_fn introduce by the previous patch
- added return comments to silence scripts/kernel-doc warnings
---
include/net/genetlink.h | 35 +++++++++++++++++++++++++++++++----
include/net/netlink.h | 31 +++++++++++++++++++++++++++----
2 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 66c1e50415e0..f1173e9ea50b 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -438,6 +438,35 @@ static inline void genlmsg_cancel(struct sk_buff *skb, void *hdr)
nlmsg_cancel(skb, hdr - GENL_HDRLEN - NLMSG_HDRLEN);
}
+/**
+ * genlmsg_multicast_netns_filtered - multicast a netlink message
+ * to a specific netns with filter
+ * function
+ * @family: the generic netlink family
+ * @net: the net namespace
+ * @skb: netlink message as socket buffer
+ * @portid: own netlink portid to avoid sending to yourself
+ * @group: offset of multicast group in groups array
+ * @flags: allocation flags
+ * @filter: filter function
+ * @filter_data: filter function private data
+ *
+ * Return: 0 on success, negative error code for failure.
+ */
+static inline int
+genlmsg_multicast_netns_filtered(const struct genl_family *family,
+ struct net *net, struct sk_buff *skb,
+ u32 portid, unsigned int group, gfp_t flags,
+ netlink_filter_fn filter,
+ void *filter_data)
+{
+ if (WARN_ON_ONCE(group >= family->n_mcgrps))
+ return -EINVAL;
+ group = family->mcgrp_offset + group;
+ return nlmsg_multicast_filtered(net->genl_sock, skb, portid, group,
+ flags, filter, filter_data);
+}
+
/**
* genlmsg_multicast_netns - multicast a netlink message to a specific netns
* @family: the generic netlink family
@@ -451,10 +480,8 @@ static inline int genlmsg_multicast_netns(const struct genl_family *family,
struct net *net, struct sk_buff *skb,
u32 portid, unsigned int group, gfp_t flags)
{
- if (WARN_ON_ONCE(group >= family->n_mcgrps))
- return -EINVAL;
- group = family->mcgrp_offset + group;
- return nlmsg_multicast(net->genl_sock, skb, portid, group, flags);
+ return genlmsg_multicast_netns_filtered(family, net, skb, portid,
+ group, flags, NULL, NULL);
}
/**
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 167b91348e57..2ba1438b7066 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1087,27 +1087,50 @@ static inline void nlmsg_free(struct sk_buff *skb)
}
/**
- * nlmsg_multicast - multicast a netlink message
+ * nlmsg_multicast_filtered - multicast a netlink message with filter function
* @sk: netlink socket to spread messages to
* @skb: netlink message as socket buffer
* @portid: own netlink portid to avoid sending to yourself
* @group: multicast group id
* @flags: allocation flags
+ * @filter: filter function
+ * @filter_data: filter function private data
+ *
+ * Return: 0 on success, negative error code for failure.
*/
-static inline int nlmsg_multicast(struct sock *sk, struct sk_buff *skb,
- u32 portid, unsigned int group, gfp_t flags)
+static inline int nlmsg_multicast_filtered(struct sock *sk, struct sk_buff *skb,
+ u32 portid, unsigned int group,
+ gfp_t flags,
+ netlink_filter_fn filter,
+ void *filter_data)
{
int err;
NETLINK_CB(skb).dst_group = group;
- err = netlink_broadcast(sk, skb, portid, group, flags);
+ err = netlink_broadcast_filtered(sk, skb, portid, group, flags,
+ filter, filter_data);
if (err > 0)
err = 0;
return err;
}
+/**
+ * nlmsg_multicast - multicast a netlink message
+ * @sk: netlink socket to spread messages to
+ * @skb: netlink message as socket buffer
+ * @portid: own netlink portid to avoid sending to yourself
+ * @group: multicast group id
+ * @flags: allocation flags
+ */
+static inline int nlmsg_multicast(struct sock *sk, struct sk_buff *skb,
+ u32 portid, unsigned int group, gfp_t flags)
+{
+ return nlmsg_multicast_filtered(sk, skb, portid, group, flags,
+ NULL, NULL);
+}
+
/**
* nlmsg_unicast - unicast a netlink message
* @sk: netlink socket to spread message to
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
` (6 preceding siblings ...)
2023-11-23 18:15 ` [patch net-next v4 7/9] genetlink: introduce helpers to do filtered multicast Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
2023-11-27 12:30 ` Przemek Kitszel
2023-11-27 15:40 ` Przemek Kitszel
2023-11-23 18:15 ` [patch net-next v4 9/9] devlink: extend multicast filtering by port index Jiri Pirko
8 siblings, 2 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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[];
+};
+
+static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc,
+ 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;
+
+ devlink_nl_obj_desc_init(&desc, devlink);
+ devlink_nl_notify_send_desc(devlink, msg, &desc);
}
/* Notify */
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index fa9afe3e6d9b..33a8e51dea68 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = {
[DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME },
};
+int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct nlattr **attrs = info->attrs;
+ struct devlink_obj_desc *flt;
+ size_t data_offset = 0;
+ size_t data_size = 0;
+ char *pos;
+
+ if (attrs[DEVLINK_ATTR_BUS_NAME])
+ data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
+ if (attrs[DEVLINK_ATTR_DEV_NAME])
+ data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
+
+ flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
+ if (!flt)
+ return -ENOMEM;
+
+ pos = (char *) flt->data;
+ if (attrs[DEVLINK_ATTR_BUS_NAME]) {
+ data_offset += nla_strscpy(pos,
+ attrs[DEVLINK_ATTR_BUS_NAME],
+ data_size) + 1;
+ flt->bus_name = pos;
+ pos += data_offset;
+ }
+ if (attrs[DEVLINK_ATTR_DEV_NAME]) {
+ nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME],
+ data_size - data_offset);
+ flt->dev_name = pos;
+ }
+
+ /* Don't attach empty filter. */
+ if (!flt->bus_name && !flt->dev_name) {
+ kfree(flt);
+ flt = NULL;
+ }
+
+ flt = genl_sk_priv_store(NETLINK_CB(skb).sk, &devlink_nl_family, flt);
+ if (IS_ERR(flt))
+ return PTR_ERR(flt);
+ else if (flt)
+ kfree_rcu(flt, rcu);
+
+ return 0;
+}
+
+static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc,
+ const struct devlink_obj_desc *flt)
+{
+ if (desc->bus_name && flt->bus_name &&
+ strcmp(desc->bus_name, flt->bus_name))
+ return false;
+ if (desc->dev_name && flt->dev_name &&
+ strcmp(desc->dev_name, flt->dev_name))
+ return false;
+ return true;
+}
+
+int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data)
+{
+ struct devlink_obj_desc *desc = data;
+ struct devlink_obj_desc *flt;
+ int ret = 0;
+
+ rcu_read_lock();
+ flt = genl_sk_priv_get(dsk, &devlink_nl_family);
+ if (flt)
+ ret = !devlink_obj_desc_match(desc, flt);
+ rcu_read_unlock();
+ return ret;
+}
+
int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net,
struct devlink *devlink, int attrtype)
{
diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
index 95f9b4350ab7..1cb0e05305d2 100644
--- a/net/devlink/netlink_gen.c
+++ b/net/devlink/netlink_gen.c
@@ -560,8 +560,14 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF
[DEVLINK_ATTR_SELFTESTS] = NLA_POLICY_NESTED(devlink_dl_selftest_id_nl_policy),
};
+/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
+static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = {
+ [DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
+ [DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
+};
+
/* Ops table for devlink */
-const struct genl_split_ops devlink_nl_ops[73] = {
+const struct genl_split_ops devlink_nl_ops[74] = {
{
.cmd = DEVLINK_CMD_GET,
.validate = GENL_DONT_VALIDATE_STRICT,
@@ -1233,4 +1239,11 @@ const struct genl_split_ops devlink_nl_ops[73] = {
.maxattr = DEVLINK_ATTR_SELFTESTS,
.flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
},
+ {
+ .cmd = DEVLINK_CMD_NOTIFY_FILTER_SET,
+ .doit = devlink_nl_notify_filter_set_doit,
+ .policy = devlink_notify_filter_set_nl_policy,
+ .maxattr = DEVLINK_ATTR_DEV_NAME,
+ .flags = GENL_CMD_CAP_DO,
+ },
};
diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h
index 02f3c0bfae0e..8f2bd50ddf5e 100644
--- a/net/devlink/netlink_gen.h
+++ b/net/devlink/netlink_gen.h
@@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F
extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1];
/* Ops table for devlink */
-extern const struct genl_split_ops devlink_nl_ops[73];
+extern const struct genl_split_ops devlink_nl_ops[74];
int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
struct genl_info *info);
@@ -142,5 +142,7 @@ int devlink_nl_selftests_get_doit(struct sk_buff *skb, struct genl_info *info);
int devlink_nl_selftests_get_dumpit(struct sk_buff *skb,
struct netlink_callback *cb);
int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info);
+int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
+ struct genl_info *info);
#endif /* _LINUX_DEVLINK_GEN_H */
diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c
index bc5065bd99b2..cd5f70eadf5b 100644
--- a/tools/net/ynl/generated/devlink-user.c
+++ b/tools/net/ynl/generated/devlink-user.c
@@ -6830,6 +6830,37 @@ int devlink_selftests_run(struct ynl_sock *ys,
return 0;
}
+/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */
+/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
+void
+devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req)
+{
+ free(req->bus_name);
+ free(req->dev_name);
+ free(req);
+}
+
+int devlink_notify_filter_set(struct ynl_sock *ys,
+ struct devlink_notify_filter_set_req *req)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = ynl_gemsg_start_req(ys, ys->family_id, DEVLINK_CMD_NOTIFY_FILTER_SET, 1);
+ ys->req_policy = &devlink_nest;
+
+ if (req->_present.bus_name_len)
+ mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name);
+ if (req->_present.dev_name_len)
+ mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name);
+
+ err = ynl_exec(ys, nlh, NULL);
+ if (err < 0)
+ return -1;
+
+ return 0;
+}
+
const struct ynl_family ynl_devlink_family = {
.name = "devlink",
};
diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h
index 1db4edc36eaa..e5d79b824a67 100644
--- a/tools/net/ynl/generated/devlink-user.h
+++ b/tools/net/ynl/generated/devlink-user.h
@@ -5252,4 +5252,51 @@ devlink_selftests_run_req_set_selftests_flash(struct devlink_selftests_run_req *
int devlink_selftests_run(struct ynl_sock *ys,
struct devlink_selftests_run_req *req);
+/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */
+/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
+struct devlink_notify_filter_set_req {
+ struct {
+ __u32 bus_name_len;
+ __u32 dev_name_len;
+ } _present;
+
+ char *bus_name;
+ char *dev_name;
+};
+
+static inline struct devlink_notify_filter_set_req *
+devlink_notify_filter_set_req_alloc(void)
+{
+ return calloc(1, sizeof(struct devlink_notify_filter_set_req));
+}
+void
+devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req);
+
+static inline void
+devlink_notify_filter_set_req_set_bus_name(struct devlink_notify_filter_set_req *req,
+ const char *bus_name)
+{
+ free(req->bus_name);
+ req->_present.bus_name_len = strlen(bus_name);
+ req->bus_name = malloc(req->_present.bus_name_len + 1);
+ memcpy(req->bus_name, bus_name, req->_present.bus_name_len);
+ req->bus_name[req->_present.bus_name_len] = 0;
+}
+static inline void
+devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req *req,
+ const char *dev_name)
+{
+ free(req->dev_name);
+ req->_present.dev_name_len = strlen(dev_name);
+ req->dev_name = malloc(req->_present.dev_name_len + 1);
+ memcpy(req->dev_name, dev_name, req->_present.dev_name_len);
+ req->dev_name[req->_present.dev_name_len] = 0;
+}
+
+/*
+ * Set notification messages socket filter.
+ */
+int devlink_notify_filter_set(struct ynl_sock *ys,
+ struct devlink_notify_filter_set_req *req);
+
#endif /* _LINUX_DEVLINK_GEN_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [patch net-next v4 9/9] devlink: extend multicast filtering by port index
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
` (7 preceding siblings ...)
2023-11-23 18:15 ` [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts Jiri Pirko
@ 2023-11-23 18:15 ` Jiri Pirko
8 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-23 18:15 UTC (permalink / raw)
To: netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
From: Jiri Pirko <jiri@nvidia.com>
Expose the previously introduced notification multicast messages
filtering infrastructure and allow the user to select messages using
port index.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v3->v4:
- rebased on top of genl_sk_priv_*() introduction
---
Documentation/netlink/specs/devlink.yaml | 1 +
net/devlink/devl_internal.h | 9 +++++++++
net/devlink/health.c | 6 +++++-
net/devlink/netlink.c | 10 +++++++++-
net/devlink/netlink_gen.c | 5 +++--
net/devlink/port.c | 5 ++++-
tools/net/ynl/generated/devlink-user.c | 2 ++
tools/net/ynl/generated/devlink-user.h | 9 +++++++++
8 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
index 6bad1d3454b7..4996ff7e09b6 100644
--- a/Documentation/netlink/specs/devlink.yaml
+++ b/Documentation/netlink/specs/devlink.yaml
@@ -2065,3 +2065,4 @@ operations:
attributes:
- bus-name
- dev-name
+ - port-index
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 82e0fb3bbebf..c7a8e13f917c 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -195,6 +195,8 @@ struct devlink_obj_desc {
struct rcu_head rcu;
const char *bus_name;
const char *dev_name;
+ unsigned int port_index;
+ bool port_index_valid;
long data[];
};
@@ -206,6 +208,13 @@ static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc,
desc->dev_name = dev_name(devlink->dev);
}
+static inline void devlink_nl_obj_desc_port_set(struct devlink_obj_desc *desc,
+ struct devlink_port *devlink_port)
+{
+ desc->port_index = devlink_port->index;
+ desc->port_index_valid = true;
+}
+
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,
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 1d59ec0202f6..acb8c0e174bb 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -490,6 +490,7 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
enum devlink_command cmd)
{
struct devlink *devlink = reporter->devlink;
+ struct devlink_obj_desc desc;
struct sk_buff *msg;
int err;
@@ -509,7 +510,10 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
return;
}
- devlink_nl_notify_send(devlink, msg);
+ devlink_nl_obj_desc_init(&desc, devlink);
+ if (reporter->devlink_port)
+ devlink_nl_obj_desc_port_set(&desc, reporter->devlink_port);
+ devlink_nl_notify_send_desc(devlink, msg, &desc);
}
void
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 33a8e51dea68..7d3635b30725 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -49,8 +49,13 @@ int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
flt->dev_name = pos;
}
+ if (attrs[DEVLINK_ATTR_PORT_INDEX]) {
+ flt->port_index = nla_get_u32(attrs[DEVLINK_ATTR_PORT_INDEX]);
+ flt->port_index_valid = true;
+ }
+
/* Don't attach empty filter. */
- if (!flt->bus_name && !flt->dev_name) {
+ if (!flt->bus_name && !flt->dev_name && !flt->port_index_valid) {
kfree(flt);
flt = NULL;
}
@@ -73,6 +78,9 @@ static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc,
if (desc->dev_name && flt->dev_name &&
strcmp(desc->dev_name, flt->dev_name))
return false;
+ if (desc->port_index_valid && flt->port_index_valid &&
+ desc->port_index != flt->port_index)
+ return false;
return true;
}
diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
index 1cb0e05305d2..c81cf2dd154f 100644
--- a/net/devlink/netlink_gen.c
+++ b/net/devlink/netlink_gen.c
@@ -561,9 +561,10 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF
};
/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
-static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = {
+static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_PORT_INDEX + 1] = {
[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
+ [DEVLINK_ATTR_PORT_INDEX] = { .type = NLA_U32, },
};
/* Ops table for devlink */
@@ -1243,7 +1244,7 @@ const struct genl_split_ops devlink_nl_ops[74] = {
.cmd = DEVLINK_CMD_NOTIFY_FILTER_SET,
.doit = devlink_nl_notify_filter_set_doit,
.policy = devlink_notify_filter_set_nl_policy,
- .maxattr = DEVLINK_ATTR_DEV_NAME,
+ .maxattr = DEVLINK_ATTR_PORT_INDEX,
.flags = GENL_CMD_CAP_DO,
},
};
diff --git a/net/devlink/port.c b/net/devlink/port.c
index 758df3000a1b..62e54e152ecf 100644
--- a/net/devlink/port.c
+++ b/net/devlink/port.c
@@ -507,6 +507,7 @@ static void devlink_port_notify(struct devlink_port *devlink_port,
enum devlink_command cmd)
{
struct devlink *devlink = devlink_port->devlink;
+ struct devlink_obj_desc desc;
struct sk_buff *msg;
int err;
@@ -525,7 +526,9 @@ static void devlink_port_notify(struct devlink_port *devlink_port,
return;
}
- devlink_nl_notify_send(devlink, msg);
+ devlink_nl_obj_desc_init(&desc, devlink);
+ devlink_nl_obj_desc_port_set(&desc, devlink_port);
+ devlink_nl_notify_send_desc(devlink, msg, &desc);
}
static void devlink_ports_notify(struct devlink *devlink,
diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c
index cd5f70eadf5b..86392da0b52c 100644
--- a/tools/net/ynl/generated/devlink-user.c
+++ b/tools/net/ynl/generated/devlink-user.c
@@ -6853,6 +6853,8 @@ int devlink_notify_filter_set(struct ynl_sock *ys,
mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name);
if (req->_present.dev_name_len)
mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name);
+ if (req->_present.port_index)
+ mnl_attr_put_u32(nlh, DEVLINK_ATTR_PORT_INDEX, req->port_index);
err = ynl_exec(ys, nlh, NULL);
if (err < 0)
diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h
index e5d79b824a67..b96837663e6e 100644
--- a/tools/net/ynl/generated/devlink-user.h
+++ b/tools/net/ynl/generated/devlink-user.h
@@ -5258,10 +5258,12 @@ struct devlink_notify_filter_set_req {
struct {
__u32 bus_name_len;
__u32 dev_name_len;
+ __u32 port_index:1;
} _present;
char *bus_name;
char *dev_name;
+ __u32 port_index;
};
static inline struct devlink_notify_filter_set_req *
@@ -5292,6 +5294,13 @@ devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req
memcpy(req->dev_name, dev_name, req->_present.dev_name_len);
req->dev_name[req->_present.dev_name_len] = 0;
}
+static inline void
+devlink_notify_filter_set_req_set_port_index(struct devlink_notify_filter_set_req *req,
+ __u32 port_index)
+{
+ req->_present.port_index = 1;
+ req->port_index = port_index;
+}
/*
* Set notification messages socket filter.
--
2.41.0
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 3/9] devlink: send notifications only if there are listeners
2023-11-23 18:15 ` [patch net-next v4 3/9] devlink: send notifications only if there are listeners Jiri Pirko
@ 2023-11-27 11:01 ` Paolo Abeni
2023-11-27 12:04 ` Jiri Pirko
0 siblings, 1 reply; 40+ messages in thread
From: Paolo Abeni @ 2023-11-27 11:01 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: kuba, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-23 18:15 ` [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage Jiri Pirko
@ 2023-11-27 11:13 ` Paolo Abeni
2023-11-27 12:00 ` Jiri Pirko
2023-11-27 22:46 ` Jakub Kicinski
2023-11-28 12:30 ` Przemek Kitszel
2 siblings, 1 reply; 40+ messages in thread
From: Paolo Abeni @ 2023-11-27 11:13 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: kuba, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
On Thu, 2023-11-23 at 19:15 +0100, Jiri Pirko wrote:
[...]
> +/**
> + * genl_sk_priv_store - Store per-socket private pointer for family
> + *
> + * @sk: socket
> + * @family: family
> + * @priv: private pointer
> + *
> + * Store a private pointer per-socket for a specified
> + * Generic netlink family.
> + *
> + * Caller has to make sure this is not called in parallel multiple times
> + * for the same sock and also in parallel to genl_release() for the same sock.
> + *
> + * Returns: previously stored private pointer for the family (could be NULL)
> + * on success, otherwise negative error value encoded by ERR_PTR().
> + */
> +void *genl_sk_priv_store(struct sock *sk, struct genl_family *family,
> + void *priv)
> +{
> + struct genl_sk_ctx *ctx;
> + void *old_priv;
> +
> + ctx = rcu_dereference_raw(nlk_sk(sk)->priv);
Minor nit: Looking at the following patch, this will be called under
the rtnl lock. Since a look is needed to ensure the priv ptr
consistency, what about adding the relevant lockdep annotation here?
No need to repost for the above.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-27 11:13 ` Paolo Abeni
@ 2023-11-27 12:00 ` Jiri Pirko
0 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-27 12:00 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, kuba, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
Mon, Nov 27, 2023 at 12:13:09PM CET, pabeni@redhat.com wrote:
>On Thu, 2023-11-23 at 19:15 +0100, Jiri Pirko wrote:
>[...]
>> +/**
>> + * genl_sk_priv_store - Store per-socket private pointer for family
>> + *
>> + * @sk: socket
>> + * @family: family
>> + * @priv: private pointer
>> + *
>> + * Store a private pointer per-socket for a specified
>> + * Generic netlink family.
>> + *
>> + * Caller has to make sure this is not called in parallel multiple times
>> + * for the same sock and also in parallel to genl_release() for the same sock.
>> + *
>> + * Returns: previously stored private pointer for the family (could be NULL)
>> + * on success, otherwise negative error value encoded by ERR_PTR().
>> + */
>> +void *genl_sk_priv_store(struct sock *sk, struct genl_family *family,
>> + void *priv)
>> +{
>> + struct genl_sk_ctx *ctx;
>> + void *old_priv;
>> +
>> + ctx = rcu_dereference_raw(nlk_sk(sk)->priv);
>
>Minor nit: Looking at the following patch, this will be called under
>the rtnl lock. Since a look is needed to ensure the priv ptr
Well, depends on the user (as the comment says). Devlink does not call
it with rtnl lock. Relies on the socket skb processing and cleanup
flows.
>consistency, what about adding the relevant lockdep annotation here?
>
>No need to repost for the above.
>
>Cheers,
>
>Paolo
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 3/9] devlink: send notifications only if there are listeners
2023-11-27 11:01 ` Paolo Abeni
@ 2023-11-27 12:04 ` Jiri Pirko
2023-11-27 15:00 ` Paolo Abeni
0 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-11-27 12:04 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, kuba, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-23 18:15 ` [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts Jiri Pirko
@ 2023-11-27 12:30 ` Przemek Kitszel
2023-11-27 12:51 ` Jiri Pirko
2023-11-27 15:40 ` Przemek Kitszel
1 sibling, 1 reply; 40+ messages in thread
From: Przemek Kitszel @ 2023-11-27 12:30 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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
> +};
> +
> +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.
> + 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().
> +
> + devlink_nl_obj_desc_init(&desc, devlink);
> + devlink_nl_notify_send_desc(devlink, msg, &desc);
> }
>
> /* Notify */
[snip]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-27 12:30 ` Przemek Kitszel
@ 2023-11-27 12:51 ` Jiri Pirko
2023-11-27 12:56 ` Jiri Pirko
0 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-11-27 12:51 UTC (permalink / raw)
To: Przemek Kitszel
Cc: netdev, kuba, pabeni, davem, edumazet, jacob.e.keller, jhs,
johannes, andriy.shevchenko, amritha.nambiar, sdf, horms
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]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-27 12:51 ` Jiri Pirko
@ 2023-11-27 12:56 ` Jiri Pirko
0 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-27 12:56 UTC (permalink / raw)
To: Przemek Kitszel
Cc: netdev, kuba, pabeni, davem, edumazet, jacob.e.keller, jhs,
johannes, andriy.shevchenko, amritha.nambiar, sdf, horms
Mon, Nov 27, 2023 at 01:51:03PM CET, jiri@resnulli.us wrote:
>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>
[...]
>>> 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.
On a second thought, since the next patch adds couple more users of
devlink_nl_obj_desc_init(), I prefer to leave the zeroing there.
>
>
>>
>>> +
>>> + devlink_nl_obj_desc_init(&desc, devlink);
>>> + devlink_nl_notify_send_desc(devlink, msg, &desc);
>>> }
>>> /* Notify */
>>
>>[snip]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 3/9] devlink: send notifications only if there are listeners
2023-11-27 12:04 ` Jiri Pirko
@ 2023-11-27 15:00 ` Paolo Abeni
2023-11-28 7:39 ` Jiri Pirko
0 siblings, 1 reply; 40+ messages in thread
From: Paolo Abeni @ 2023-11-27 15:00 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, kuba, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-23 18:15 ` [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts Jiri Pirko
2023-11-27 12:30 ` Przemek Kitszel
@ 2023-11-27 15:40 ` Przemek Kitszel
2023-11-28 8:26 ` Jiri Pirko
2023-12-04 16:24 ` Jiri Pirko
1 sibling, 2 replies; 40+ messages in thread
From: Przemek Kitszel @ 2023-11-27 15:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms, netdev
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[];
> +};
> +
> +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc,
> + 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;
> +
> + devlink_nl_obj_desc_init(&desc, devlink);
> + devlink_nl_notify_send_desc(devlink, msg, &desc);
> }
>
> /* Notify */
> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
> index fa9afe3e6d9b..33a8e51dea68 100644
> --- a/net/devlink/netlink.c
> +++ b/net/devlink/netlink.c
> @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = {
> [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME },
> };
>
> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct nlattr **attrs = info->attrs;
> + struct devlink_obj_desc *flt;
> + size_t data_offset = 0;
> + size_t data_size = 0;
> + char *pos;
> +
> + if (attrs[DEVLINK_ATTR_BUS_NAME])
> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
> + if (attrs[DEVLINK_ATTR_DEV_NAME])
> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
> +
> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
instead of arithmetic here, you could use struct_size()
> + if (!flt)
> + return -ENOMEM;
> +
> + pos = (char *) flt->data;
> + if (attrs[DEVLINK_ATTR_BUS_NAME]) {
> + data_offset += nla_strscpy(pos,
> + attrs[DEVLINK_ATTR_BUS_NAME],
> + data_size) + 1;
> + flt->bus_name = pos;
> + pos += data_offset;
> + }
> + if (attrs[DEVLINK_ATTR_DEV_NAME]) {
> + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME],
> + data_size - data_offset);
> + flt->dev_name = pos;
> + }
> +
> + /* Don't attach empty filter. */
> + if (!flt->bus_name && !flt->dev_name) {
> + kfree(flt);
> + flt = NULL;
> + }
> +
(Thanks for pointing out to this place in the other sub-thread)
[here1] Assume that @flt is fine here.
> + flt = genl_sk_priv_store(NETLINK_CB(skb).sk, &devlink_nl_family, flt);
> + if (IS_ERR(flt))
> + return PTR_ERR(flt);
and now you got an error from genl_sk_priv_store(),
which means that you leak old flt as of [here1].
I am correct? (sorry it's kinda late :/)
> + else if (flt)
> + kfree_rcu(flt, rcu);
> +
> + return 0;
> +}
> +
> +static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc,
> + const struct devlink_obj_desc *flt)
> +{
> + if (desc->bus_name && flt->bus_name &&
> + strcmp(desc->bus_name, flt->bus_name))
> + return false;
> + if (desc->dev_name && flt->dev_name &&
> + strcmp(desc->dev_name, flt->dev_name))
> + return false;
> + return true;
> +}
> +
> +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data)
> +{
> + struct devlink_obj_desc *desc = data;
> + struct devlink_obj_desc *flt;
> + int ret = 0;
> +
> + rcu_read_lock();
> + flt = genl_sk_priv_get(dsk, &devlink_nl_family);
> + if (flt)
> + ret = !devlink_obj_desc_match(desc, flt);
> + rcu_read_unlock();
> + return ret;
> +}
> +
> int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net,
> struct devlink *devlink, int attrtype)
> {
> diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
> index 95f9b4350ab7..1cb0e05305d2 100644
> --- a/net/devlink/netlink_gen.c
> +++ b/net/devlink/netlink_gen.c
> @@ -560,8 +560,14 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF
> [DEVLINK_ATTR_SELFTESTS] = NLA_POLICY_NESTED(devlink_dl_selftest_id_nl_policy),
> };
>
> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
> +static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = {
> + [DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
> + [DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
> +};
> +
> /* Ops table for devlink */
> -const struct genl_split_ops devlink_nl_ops[73] = {
> +const struct genl_split_ops devlink_nl_ops[74] = {
> {
> .cmd = DEVLINK_CMD_GET,
> .validate = GENL_DONT_VALIDATE_STRICT,
> @@ -1233,4 +1239,11 @@ const struct genl_split_ops devlink_nl_ops[73] = {
> .maxattr = DEVLINK_ATTR_SELFTESTS,
> .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> },
> + {
> + .cmd = DEVLINK_CMD_NOTIFY_FILTER_SET,
> + .doit = devlink_nl_notify_filter_set_doit,
> + .policy = devlink_notify_filter_set_nl_policy,
> + .maxattr = DEVLINK_ATTR_DEV_NAME,
> + .flags = GENL_CMD_CAP_DO,
> + },
> };
> diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h
> index 02f3c0bfae0e..8f2bd50ddf5e 100644
> --- a/net/devlink/netlink_gen.h
> +++ b/net/devlink/netlink_gen.h
> @@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F
> extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1];
>
> /* Ops table for devlink */
> -extern const struct genl_split_ops devlink_nl_ops[73];
> +extern const struct genl_split_ops devlink_nl_ops[74];
>
> int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
> struct genl_info *info);
> @@ -142,5 +142,7 @@ int devlink_nl_selftests_get_doit(struct sk_buff *skb, struct genl_info *info);
> int devlink_nl_selftests_get_dumpit(struct sk_buff *skb,
> struct netlink_callback *cb);
> int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info);
> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
> + struct genl_info *info);
>
> #endif /* _LINUX_DEVLINK_GEN_H */
> diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c
> index bc5065bd99b2..cd5f70eadf5b 100644
> --- a/tools/net/ynl/generated/devlink-user.c
> +++ b/tools/net/ynl/generated/devlink-user.c
> @@ -6830,6 +6830,37 @@ int devlink_selftests_run(struct ynl_sock *ys,
> return 0;
> }
>
> +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */
> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
> +void
> +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req)
> +{
> + free(req->bus_name);
> + free(req->dev_name);
> + free(req);
> +}
> +
> +int devlink_notify_filter_set(struct ynl_sock *ys,
> + struct devlink_notify_filter_set_req *req)
> +{
> + struct nlmsghdr *nlh;
> + int err;
> +
> + nlh = ynl_gemsg_start_req(ys, ys->family_id, DEVLINK_CMD_NOTIFY_FILTER_SET, 1);
> + ys->req_policy = &devlink_nest;
> +
> + if (req->_present.bus_name_len)
> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name);
> + if (req->_present.dev_name_len)
> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name);
> +
> + err = ynl_exec(ys, nlh, NULL);
> + if (err < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> const struct ynl_family ynl_devlink_family = {
> .name = "devlink",
> };
> diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h
> index 1db4edc36eaa..e5d79b824a67 100644
> --- a/tools/net/ynl/generated/devlink-user.h
> +++ b/tools/net/ynl/generated/devlink-user.h
> @@ -5252,4 +5252,51 @@ devlink_selftests_run_req_set_selftests_flash(struct devlink_selftests_run_req *
> int devlink_selftests_run(struct ynl_sock *ys,
> struct devlink_selftests_run_req *req);
>
> +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */
> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
> +struct devlink_notify_filter_set_req {
> + struct {
> + __u32 bus_name_len;
> + __u32 dev_name_len;
> + } _present;
> +
> + char *bus_name;
> + char *dev_name;
> +};
> +
> +static inline struct devlink_notify_filter_set_req *
> +devlink_notify_filter_set_req_alloc(void)
> +{
> + return calloc(1, sizeof(struct devlink_notify_filter_set_req));
> +}
> +void
> +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req);
> +
> +static inline void
> +devlink_notify_filter_set_req_set_bus_name(struct devlink_notify_filter_set_req *req,
> + const char *bus_name)
> +{
> + free(req->bus_name);
> + req->_present.bus_name_len = strlen(bus_name);
> + req->bus_name = malloc(req->_present.bus_name_len + 1);
> + memcpy(req->bus_name, bus_name, req->_present.bus_name_len);
> + req->bus_name[req->_present.bus_name_len] = 0;
> +}
> +static inline void
> +devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req *req,
> + const char *dev_name)
> +{
> + free(req->dev_name);
> + req->_present.dev_name_len = strlen(dev_name);
> + req->dev_name = malloc(req->_present.dev_name_len + 1);
> + memcpy(req->dev_name, dev_name, req->_present.dev_name_len);
> + req->dev_name[req->_present.dev_name_len] = 0;
> +}
> +
> +/*
> + * Set notification messages socket filter.
> + */
> +int devlink_notify_filter_set(struct ynl_sock *ys,
> + struct devlink_notify_filter_set_req *req);
> +
> #endif /* _LINUX_DEVLINK_GEN_H */
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-23 18:15 ` [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage Jiri Pirko
2023-11-27 11:13 ` Paolo Abeni
@ 2023-11-27 22:46 ` Jakub Kicinski
2023-11-28 8:25 ` Jiri Pirko
2023-11-28 12:30 ` Przemek Kitszel
2 siblings, 1 reply; 40+ messages in thread
From: Jakub Kicinski @ 2023-11-27 22:46 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
On Thu, 23 Nov 2023 19:15:42 +0100 Jiri Pirko wrote:
> + void __rcu *priv;
How many times did I say "typed struct" in the feedback to the broken
v3 of this series? You complain so much about people not addressing
feedback you've given them, it's really hypocritical.
Put the xarray pointer here directly. Plus a lock to protect the init.
The size of the per-family struct should be in family definition,
allocation should happen on first get automatically. Family definition
should also hold a callback to how the data is going to be freed.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 3/9] devlink: send notifications only if there are listeners
2023-11-27 15:00 ` Paolo Abeni
@ 2023-11-28 7:39 ` Jiri Pirko
0 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-28 7:39 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, kuba, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
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
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-27 22:46 ` Jakub Kicinski
@ 2023-11-28 8:25 ` Jiri Pirko
2023-11-28 15:11 ` Jakub Kicinski
0 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-11-28 8:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
Mon, Nov 27, 2023 at 11:46:26PM CET, kuba@kernel.org wrote:
>On Thu, 23 Nov 2023 19:15:42 +0100 Jiri Pirko wrote:
>> + void __rcu *priv;
>
>How many times did I say "typed struct" in the feedback to the broken
>v3 of this series? You complain so much about people not addressing
>feedback you've given them, it's really hypocritical.
I did what I thought is best. Since this is struct netlink_sock, it is
not related only to genetlink. So other implementations may in theory
use this pointer for something else. Looked a bit odd to put
genetlink-specific pointer here, but as you wish.
>
>Put the xarray pointer here directly. Plus a lock to protect the init.
Okay, just to make this clear. You want me to have:
struct xarray __rcu *family_privs;
in struct netlink_sock, correct?
Why I need a lock? If I read things correctly, skbs are processed in
serial over one sock. Since this is per-sock, should be safe.
>
>The size of the per-family struct should be in family definition,
>allocation should happen on first get automatically. Family definition
Yes, I thought about that. But I decided to do this lockless, allocating
new priv every time the user sets the filter and replace the xarray item
so it could be accessed in rcu read section during notification
processing.
What you suggest requires some lock to protect the memory being changed
during filter set and suring accessing in in notify. But okay,
if you insist.
>should also hold a callback to how the data is going to be freed.
If it is alloceted automatically, why is it needed?
>--
>pw-bot: cr
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-27 15:40 ` Przemek Kitszel
@ 2023-11-28 8:26 ` Jiri Pirko
2023-12-04 16:24 ` Jiri Pirko
1 sibling, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-28 8:26 UTC (permalink / raw)
To: Przemek Kitszel
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms, netdev
Mon, Nov 27, 2023 at 04:40:22PM 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[];
>> +};
>> +
>> +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc,
>> + 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;
>> +
>> + devlink_nl_obj_desc_init(&desc, devlink);
>> + devlink_nl_notify_send_desc(devlink, msg, &desc);
>> }
>> /* Notify */
>> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>> index fa9afe3e6d9b..33a8e51dea68 100644
>> --- a/net/devlink/netlink.c
>> +++ b/net/devlink/netlink.c
>> @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = {
>> [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME },
>> };
>> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
>> + struct genl_info *info)
>> +{
>> + struct nlattr **attrs = info->attrs;
>> + struct devlink_obj_desc *flt;
>> + size_t data_offset = 0;
>> + size_t data_size = 0;
>> + char *pos;
>> +
>> + if (attrs[DEVLINK_ATTR_BUS_NAME])
>> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
>> + if (attrs[DEVLINK_ATTR_DEV_NAME])
>> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
>> +
>> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
>
>instead of arithmetic here, you could use struct_size()
>
>> + if (!flt)
>> + return -ENOMEM;
>> +
>> + pos = (char *) flt->data;
>> + if (attrs[DEVLINK_ATTR_BUS_NAME]) {
>> + data_offset += nla_strscpy(pos,
>> + attrs[DEVLINK_ATTR_BUS_NAME],
>> + data_size) + 1;
>> + flt->bus_name = pos;
>> + pos += data_offset;
>> + }
>> + if (attrs[DEVLINK_ATTR_DEV_NAME]) {
>> + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME],
>> + data_size - data_offset);
>> + flt->dev_name = pos;
>> + }
>> +
>> + /* Don't attach empty filter. */
>> + if (!flt->bus_name && !flt->dev_name) {
>> + kfree(flt);
>> + flt = NULL;
>> + }
>> +
>
>(Thanks for pointing out to this place in the other sub-thread)
>
>[here1] Assume that @flt is fine here.
>
>> + flt = genl_sk_priv_store(NETLINK_CB(skb).sk, &devlink_nl_family, flt);
>> + if (IS_ERR(flt))
>> + return PTR_ERR(flt);
>
>and now you got an error from genl_sk_priv_store(),
>which means that you leak old flt as of [here1].
>I am correct? (sorry it's kinda late :/)
You are correct. Thanks. Looks like I'll be rewriting this entirely
anyway.
>
>> + else if (flt)
>> + kfree_rcu(flt, rcu);
>> +
>> + return 0;
>> +}
>> +
>> +static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc,
>> + const struct devlink_obj_desc *flt)
>> +{
>> + if (desc->bus_name && flt->bus_name &&
>> + strcmp(desc->bus_name, flt->bus_name))
>> + return false;
>> + if (desc->dev_name && flt->dev_name &&
>> + strcmp(desc->dev_name, flt->dev_name))
>> + return false;
>> + return true;
>> +}
>> +
>> +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data)
>> +{
>> + struct devlink_obj_desc *desc = data;
>> + struct devlink_obj_desc *flt;
>> + int ret = 0;
>> +
>> + rcu_read_lock();
>> + flt = genl_sk_priv_get(dsk, &devlink_nl_family);
>> + if (flt)
>> + ret = !devlink_obj_desc_match(desc, flt);
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +
>> int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net,
>> struct devlink *devlink, int attrtype)
>> {
>> diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
>> index 95f9b4350ab7..1cb0e05305d2 100644
>> --- a/net/devlink/netlink_gen.c
>> +++ b/net/devlink/netlink_gen.c
>> @@ -560,8 +560,14 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF
>> [DEVLINK_ATTR_SELFTESTS] = NLA_POLICY_NESTED(devlink_dl_selftest_id_nl_policy),
>> };
>> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
>> +static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = {
>> + [DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
>> + [DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
>> +};
>> +
>> /* Ops table for devlink */
>> -const struct genl_split_ops devlink_nl_ops[73] = {
>> +const struct genl_split_ops devlink_nl_ops[74] = {
>> {
>> .cmd = DEVLINK_CMD_GET,
>> .validate = GENL_DONT_VALIDATE_STRICT,
>> @@ -1233,4 +1239,11 @@ const struct genl_split_ops devlink_nl_ops[73] = {
>> .maxattr = DEVLINK_ATTR_SELFTESTS,
>> .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>> },
>> + {
>> + .cmd = DEVLINK_CMD_NOTIFY_FILTER_SET,
>> + .doit = devlink_nl_notify_filter_set_doit,
>> + .policy = devlink_notify_filter_set_nl_policy,
>> + .maxattr = DEVLINK_ATTR_DEV_NAME,
>> + .flags = GENL_CMD_CAP_DO,
>> + },
>> };
>> diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h
>> index 02f3c0bfae0e..8f2bd50ddf5e 100644
>> --- a/net/devlink/netlink_gen.h
>> +++ b/net/devlink/netlink_gen.h
>> @@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F
>> extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1];
>> /* Ops table for devlink */
>> -extern const struct genl_split_ops devlink_nl_ops[73];
>> +extern const struct genl_split_ops devlink_nl_ops[74];
>> int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>> struct genl_info *info);
>> @@ -142,5 +142,7 @@ int devlink_nl_selftests_get_doit(struct sk_buff *skb, struct genl_info *info);
>> int devlink_nl_selftests_get_dumpit(struct sk_buff *skb,
>> struct netlink_callback *cb);
>> int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info);
>> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
>> + struct genl_info *info);
>> #endif /* _LINUX_DEVLINK_GEN_H */
>> diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c
>> index bc5065bd99b2..cd5f70eadf5b 100644
>> --- a/tools/net/ynl/generated/devlink-user.c
>> +++ b/tools/net/ynl/generated/devlink-user.c
>> @@ -6830,6 +6830,37 @@ int devlink_selftests_run(struct ynl_sock *ys,
>> return 0;
>> }
>> +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */
>> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
>> +void
>> +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req)
>> +{
>> + free(req->bus_name);
>> + free(req->dev_name);
>> + free(req);
>> +}
>> +
>> +int devlink_notify_filter_set(struct ynl_sock *ys,
>> + struct devlink_notify_filter_set_req *req)
>> +{
>> + struct nlmsghdr *nlh;
>> + int err;
>> +
>> + nlh = ynl_gemsg_start_req(ys, ys->family_id, DEVLINK_CMD_NOTIFY_FILTER_SET, 1);
>> + ys->req_policy = &devlink_nest;
>> +
>> + if (req->_present.bus_name_len)
>> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name);
>> + if (req->_present.dev_name_len)
>> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name);
>> +
>> + err = ynl_exec(ys, nlh, NULL);
>> + if (err < 0)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> const struct ynl_family ynl_devlink_family = {
>> .name = "devlink",
>> };
>> diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h
>> index 1db4edc36eaa..e5d79b824a67 100644
>> --- a/tools/net/ynl/generated/devlink-user.h
>> +++ b/tools/net/ynl/generated/devlink-user.h
>> @@ -5252,4 +5252,51 @@ devlink_selftests_run_req_set_selftests_flash(struct devlink_selftests_run_req *
>> int devlink_selftests_run(struct ynl_sock *ys,
>> struct devlink_selftests_run_req *req);
>> +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */
>> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */
>> +struct devlink_notify_filter_set_req {
>> + struct {
>> + __u32 bus_name_len;
>> + __u32 dev_name_len;
>> + } _present;
>> +
>> + char *bus_name;
>> + char *dev_name;
>> +};
>> +
>> +static inline struct devlink_notify_filter_set_req *
>> +devlink_notify_filter_set_req_alloc(void)
>> +{
>> + return calloc(1, sizeof(struct devlink_notify_filter_set_req));
>> +}
>> +void
>> +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req);
>> +
>> +static inline void
>> +devlink_notify_filter_set_req_set_bus_name(struct devlink_notify_filter_set_req *req,
>> + const char *bus_name)
>> +{
>> + free(req->bus_name);
>> + req->_present.bus_name_len = strlen(bus_name);
>> + req->bus_name = malloc(req->_present.bus_name_len + 1);
>> + memcpy(req->bus_name, bus_name, req->_present.bus_name_len);
>> + req->bus_name[req->_present.bus_name_len] = 0;
>> +}
>> +static inline void
>> +devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req *req,
>> + const char *dev_name)
>> +{
>> + free(req->dev_name);
>> + req->_present.dev_name_len = strlen(dev_name);
>> + req->dev_name = malloc(req->_present.dev_name_len + 1);
>> + memcpy(req->dev_name, dev_name, req->_present.dev_name_len);
>> + req->dev_name[req->_present.dev_name_len] = 0;
>> +}
>> +
>> +/*
>> + * Set notification messages socket filter.
>> + */
>> +int devlink_notify_filter_set(struct ynl_sock *ys,
>> + struct devlink_notify_filter_set_req *req);
>> +
>> #endif /* _LINUX_DEVLINK_GEN_H */
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-23 18:15 ` [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage Jiri Pirko
2023-11-27 11:13 ` Paolo Abeni
2023-11-27 22:46 ` Jakub Kicinski
@ 2023-11-28 12:30 ` Przemek Kitszel
2023-11-28 15:05 ` Jiri Pirko
2023-11-28 16:18 ` Andy Shevchenko
2 siblings, 2 replies; 40+ messages in thread
From: Przemek Kitszel @ 2023-11-28 12:30 UTC (permalink / raw)
To: Jiri Pirko, kuba, pabeni
Cc: davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms, netdev
On 11/23/23 19:15, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
>
> Introduce a priv pointer into struct netlink_sock. Use it to store a per
> socket xarray that contains family->id indexed priv pointer storage.
> Note I used xarray instead of suggested linked list as it is more
> convenient, without need to have a container struct that would
> contain struct list_head item.
>
> Introduce genl_sk_priv_store() to store the priv pointer.
> Introduce genl_sk_priv_get() to obtain the priv pointer under RCU
> read lock.
>
> Assume that kfree() is good for free of privs for now, as the only user
> introduced by the follow-up patch (devlink) will use kzalloc() for the
> allocation of the memory of the stored pointer. If later on
> this needs to be made custom, a callback is going to be needed.
> Until then (if ever), do this in a simple way.
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v3->v4:
> - new patch
> ---
> include/net/genetlink.h | 3 ++
> net/netlink/af_netlink.h | 1 +
> net/netlink/genetlink.c | 98 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 102 insertions(+)
>
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index e18a4c0d69ee..66c1e50415e0 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -300,6 +300,9 @@ int genl_register_family(struct genl_family *family);
> int genl_unregister_family(const struct genl_family *family);
> void genl_notify(const struct genl_family *family, struct sk_buff *skb,
> struct genl_info *info, u32 group, gfp_t flags);
> +void *genl_sk_priv_get(struct sock *sk, struct genl_family *family);
> +void *genl_sk_priv_store(struct sock *sk, struct genl_family *family,
> + void *priv);
>
> void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
> const struct genl_family *family, int flags, u8 cmd);
> diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
> index 2145979b9986..5d96135a4cf3 100644
> --- a/net/netlink/af_netlink.h
> +++ b/net/netlink/af_netlink.h
> @@ -51,6 +51,7 @@ struct netlink_sock {
> struct rhash_head node;
> struct rcu_head rcu;
> struct work_struct work;
> + void __rcu *priv;
> };
>
> static inline struct netlink_sock *nlk_sk(struct sock *sk)
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 92ef5ed2e7b0..aae5e63fa50b 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -21,6 +21,7 @@
> #include <linux/idr.h>
> #include <net/sock.h>
> #include <net/genetlink.h>
> +#include "af_netlink.h"
>
> static DEFINE_MUTEX(genl_mutex); /* serialization of message processing */
> static DECLARE_RWSEM(cb_lock);
> @@ -1699,12 +1700,109 @@ static int genl_bind(struct net *net, int group)
> return ret;
> }
>
> +struct genl_sk_ctx {
> + struct xarray family_privs;
> +};
> +
> +static struct genl_sk_ctx *genl_sk_ctx_alloc(void)
> +{
> + struct genl_sk_ctx *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return NULL;
> + xa_init_flags(&ctx->family_privs, XA_FLAGS_ALLOC);
> + return ctx;
> +}
> +
> +static void genl_sk_ctx_free(struct genl_sk_ctx *ctx)
> +{
> + unsigned long family_id;
> + void *priv;
> +
> + xa_for_each(&ctx->family_privs, family_id, priv) {
> + xa_erase(&ctx->family_privs, family_id);
> + kfree(priv);
> + }
> + xa_destroy(&ctx->family_privs);
> + kfree(ctx);
> +}
> +
> +/**
> + * genl_sk_priv_get - Get per-socket private pointer for family
> + *
> + * @sk: socket
> + * @family: family
> + *
> + * Lookup a private pointer stored per-socket by a specified
> + * Generic netlink family.
> + *
> + * Caller should make sure this is called in RCU read locked section.
> + *
> + * Returns: valid pointer on success, otherwise NULL.
since you are going to post next revision,
kernel-doc requires "Return:" section (singular form)
https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
for new code we should strive to fulfil the requirement
(or piss-off someone powerful enough to change the requirement ;))
[snip]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 12:30 ` Przemek Kitszel
@ 2023-11-28 15:05 ` Jiri Pirko
2023-11-28 16:18 ` Andy Shevchenko
1 sibling, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-28 15:05 UTC (permalink / raw)
To: Przemek Kitszel
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms, netdev
Tue, Nov 28, 2023 at 01:30:51PM CET, przemyslaw.kitszel@intel.com wrote:
>On 11/23/23 19:15, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Introduce a priv pointer into struct netlink_sock. Use it to store a per
>> socket xarray that contains family->id indexed priv pointer storage.
>> Note I used xarray instead of suggested linked list as it is more
>> convenient, without need to have a container struct that would
>> contain struct list_head item.
>>
>> Introduce genl_sk_priv_store() to store the priv pointer.
>> Introduce genl_sk_priv_get() to obtain the priv pointer under RCU
>> read lock.
>>
>> Assume that kfree() is good for free of privs for now, as the only user
>> introduced by the follow-up patch (devlink) will use kzalloc() for the
>> allocation of the memory of the stored pointer. If later on
>> this needs to be made custom, a callback is going to be needed.
>> Until then (if ever), do this in a simple way.
>>
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> v3->v4:
>> - new patch
>> ---
>> include/net/genetlink.h | 3 ++
>> net/netlink/af_netlink.h | 1 +
>> net/netlink/genetlink.c | 98 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 102 insertions(+)
>>
>> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
>> index e18a4c0d69ee..66c1e50415e0 100644
>> --- a/include/net/genetlink.h
>> +++ b/include/net/genetlink.h
>> @@ -300,6 +300,9 @@ int genl_register_family(struct genl_family *family);
>> int genl_unregister_family(const struct genl_family *family);
>> void genl_notify(const struct genl_family *family, struct sk_buff *skb,
>> struct genl_info *info, u32 group, gfp_t flags);
>> +void *genl_sk_priv_get(struct sock *sk, struct genl_family *family);
>> +void *genl_sk_priv_store(struct sock *sk, struct genl_family *family,
>> + void *priv);
>> void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
>> const struct genl_family *family, int flags, u8 cmd);
>> diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
>> index 2145979b9986..5d96135a4cf3 100644
>> --- a/net/netlink/af_netlink.h
>> +++ b/net/netlink/af_netlink.h
>> @@ -51,6 +51,7 @@ struct netlink_sock {
>> struct rhash_head node;
>> struct rcu_head rcu;
>> struct work_struct work;
>> + void __rcu *priv;
>> };
>> static inline struct netlink_sock *nlk_sk(struct sock *sk)
>> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
>> index 92ef5ed2e7b0..aae5e63fa50b 100644
>> --- a/net/netlink/genetlink.c
>> +++ b/net/netlink/genetlink.c
>> @@ -21,6 +21,7 @@
>> #include <linux/idr.h>
>> #include <net/sock.h>
>> #include <net/genetlink.h>
>> +#include "af_netlink.h"
>> static DEFINE_MUTEX(genl_mutex); /* serialization of message processing */
>> static DECLARE_RWSEM(cb_lock);
>> @@ -1699,12 +1700,109 @@ static int genl_bind(struct net *net, int group)
>> return ret;
>> }
>> +struct genl_sk_ctx {
>> + struct xarray family_privs;
>> +};
>> +
>> +static struct genl_sk_ctx *genl_sk_ctx_alloc(void)
>> +{
>> + struct genl_sk_ctx *ctx;
>> +
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return NULL;
>> + xa_init_flags(&ctx->family_privs, XA_FLAGS_ALLOC);
>> + return ctx;
>> +}
>> +
>> +static void genl_sk_ctx_free(struct genl_sk_ctx *ctx)
>> +{
>> + unsigned long family_id;
>> + void *priv;
>> +
>> + xa_for_each(&ctx->family_privs, family_id, priv) {
>> + xa_erase(&ctx->family_privs, family_id);
>> + kfree(priv);
>> + }
>> + xa_destroy(&ctx->family_privs);
>> + kfree(ctx);
>> +}
>> +
>> +/**
>> + * genl_sk_priv_get - Get per-socket private pointer for family
>> + *
>> + * @sk: socket
>> + * @family: family
>> + *
>> + * Lookup a private pointer stored per-socket by a specified
>> + * Generic netlink family.
>> + *
>> + * Caller should make sure this is called in RCU read locked section.
>> + *
>> + * Returns: valid pointer on success, otherwise NULL.
>
>since you are going to post next revision,
>
>kernel-doc requires "Return:" section (singular form)
>https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>
>for new code we should strive to fulfil the requirement
>(or piss-off someone powerful enough to change the requirement ;))
Okay, will fix. I just thought this is okay when scripts/kernel-doc is
happy.
>
>
>
>[snip]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 8:25 ` Jiri Pirko
@ 2023-11-28 15:11 ` Jakub Kicinski
2023-11-28 16:05 ` Jiri Pirko
0 siblings, 1 reply; 40+ messages in thread
From: Jakub Kicinski @ 2023-11-28 15:11 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
On Tue, 28 Nov 2023 09:25:21 +0100 Jiri Pirko wrote:
> >Put the xarray pointer here directly. Plus a lock to protect the init.
>
> Okay, just to make this clear. You want me to have:
> struct xarray __rcu *family_privs;
>
> in struct netlink_sock, correct?
>
>
> Why I need a lock? If I read things correctly, skbs are processed in
> serial over one sock. Since this is per-sock, should be safe.
Okay, then add an assertion that the socket lock is held, at least.
Also, is the socket lock not held yet when the filtering happens?
Maybe the __rcu annotation isn't necessary then either?
> >The size of the per-family struct should be in family definition,
> >allocation should happen on first get automatically. Family definition
>
> Yes, I thought about that. But I decided to do this lockless, allocating
> new priv every time the user sets the filter and replace the xarray item
> so it could be accessed in rcu read section during notification
> processing.
>
> What you suggest requires some lock to protect the memory being changed
> during filter set and suring accessing in in notify. But okay,
> if you insist.
Not necessarily, you can have a helper which doesn't allocate, too.
What I'm saying is that the common case for ops will be to access
the state and allocate if it doesn't exist.
How about genl_sk_family_priv() and genl_sk_has_family_priv() ?
> >should also hold a callback to how the data is going to be freed.
>
> If it is alloceted automatically, why is it needed?
Because priv may be a complex type which has member that need
individual fields to be destroyed (in fullness of time we also
need a constructor which can init things like list_head, but
we can defer that).
I'm guessing in your case the priv will look like this:
struct devlink_sk_priv {
const char *nft_fltr_instance_name;
};
static void devlink_sk_priv_free(void *ptr)
{
struct devlink_sk_priv *priv = ptr;
kfree(priv->nft_fltr_instance_name);
}
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 15:11 ` Jakub Kicinski
@ 2023-11-28 16:05 ` Jiri Pirko
2023-11-28 16:36 ` Jakub Kicinski
0 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-11-28 16:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
Tue, Nov 28, 2023 at 04:11:16PM CET, kuba@kernel.org wrote:
>On Tue, 28 Nov 2023 09:25:21 +0100 Jiri Pirko wrote:
>> >Put the xarray pointer here directly. Plus a lock to protect the init.
>>
>> Okay, just to make this clear. You want me to have:
>> struct xarray __rcu *family_privs;
>>
>> in struct netlink_sock, correct?
>>
>>
>> Why I need a lock? If I read things correctly, skbs are processed in
>> serial over one sock. Since this is per-sock, should be safe.
>
>Okay, then add an assertion that the socket lock is held, at least.
No socket lock, but I assumed revcmsg could be called only in parallel.
But I guess that with multiple threads, this assumption is broken.
Okay, will add a spin lock.
>Also, is the socket lock not held yet when the filtering happens?
Nope.
>Maybe the __rcu annotation isn't necessary then either?
>
>> >The size of the per-family struct should be in family definition,
>> >allocation should happen on first get automatically. Family definition
>>
>> Yes, I thought about that. But I decided to do this lockless, allocating
>> new priv every time the user sets the filter and replace the xarray item
>> so it could be accessed in rcu read section during notification
>> processing.
>>
>> What you suggest requires some lock to protect the memory being changed
>> during filter set and suring accessing in in notify. But okay,
>> if you insist.
>
>Not necessarily, you can have a helper which doesn't allocate, too.
>What I'm saying is that the common case for ops will be to access
>the state and allocate if it doesn't exist.
>
>How about genl_sk_family_priv() and genl_sk_has_family_priv() ?
My point is, with what you suggest, it will look something like this:
1) user does DEVLINK_CMD_NOTIFY_FILTER_SET
2) devlink calls into genetlink code for a sk_priv and inserts in xa_array
3) genetlink allocates sk_priv and returns back
4) devlink fills-up the sk_priv
5) user does DEVLINK_CMD_NOTIFY_FILTER_SET, again
6) devlink calls into genetlink code for a sk_priv
7) genetlink returns already exising sk_priv found in xa_array
8) devlink fills-up the sk_priv
Now the notification thread, sees sk_priv zeroed between 3) and 4)
and inconsistent during 4) and 8)
I originally solved that by rcu, DEVLINK_CMD_NOTIFY_FILTER_SET
code always allocates and flips the rcu pointer. Notification thread
always sees sk_priv consistent.
If you want to allocate sk_priv in genetlink code once, hard to use
the rcu mechanism and have to protect the sk_priv memory by a lock.
What am I missing?
>
>> >should also hold a callback to how the data is going to be freed.
>>
>> If it is alloceted automatically, why is it needed?
>
>Because priv may be a complex type which has member that need
>individual fields to be destroyed (in fullness of time we also
>need a constructor which can init things like list_head, but
>we can defer that).
>
>I'm guessing in your case the priv will look like this:
>
>struct devlink_sk_priv {
> const char *nft_fltr_instance_name;
>};
>
>static void devlink_sk_priv_free(void *ptr)
>{
> struct devlink_sk_priv *priv = ptr;
>
> kfree(priv->nft_fltr_instance_name);
>}
If genetlink code does the allocation, it should know how to free.
Does not make sense to pass destructor to genetlink code to free memory
it actually allocated :/
If devlink does the allocation, this callback makes sense. I was
thinking about having it, but decided kfree is okay for now and
destructor could be always introduced if needed.
Quoting the patch description:
Assume that kfree() is good for free of privs for now, as the only user
introduced by the follow-up patch (devlink) will use kzalloc() for the
allocation of the memory of the stored pointer. If later on
this needs to be made custom, a callback is going to be needed.
Until then (if ever), do this in a simple way.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 12:30 ` Przemek Kitszel
2023-11-28 15:05 ` Jiri Pirko
@ 2023-11-28 16:18 ` Andy Shevchenko
2023-11-28 19:59 ` Jacob Keller
1 sibling, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2023-11-28 16:18 UTC (permalink / raw)
To: Przemek Kitszel
Cc: Jiri Pirko, kuba, pabeni, davem, edumazet, jacob.e.keller, jhs,
johannes, amritha.nambiar, sdf, horms, netdev
On Tue, Nov 28, 2023 at 01:30:51PM +0100, Przemek Kitszel wrote:
> On 11/23/23 19:15, Jiri Pirko wrote:
...
> > + * Returns: valid pointer on success, otherwise NULL.
>
> since you are going to post next revision,
>
> kernel-doc requires "Return:" section (singular form)
> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>
> for new code we should strive to fulfil the requirement
> (or piss-off someone powerful enough to change the requirement ;))
Interestingly that the script accepts plural for a few keywords.
Is it documented somewhere as deprecated?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 16:05 ` Jiri Pirko
@ 2023-11-28 16:36 ` Jakub Kicinski
2023-11-29 13:59 ` Jiri Pirko
0 siblings, 1 reply; 40+ messages in thread
From: Jakub Kicinski @ 2023-11-28 16:36 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
On Tue, 28 Nov 2023 17:05:48 +0100 Jiri Pirko wrote:
> >Not necessarily, you can have a helper which doesn't allocate, too.
> >What I'm saying is that the common case for ops will be to access
> >the state and allocate if it doesn't exist.
> >
> >How about genl_sk_family_priv() and genl_sk_has_family_priv() ?
>
> My point is, with what you suggest, it will look something like this:
>
> 1) user does DEVLINK_CMD_NOTIFY_FILTER_SET
> 2) devlink calls into genetlink code for a sk_priv and inserts in xa_array
> 3) genetlink allocates sk_priv and returns back
> 4) devlink fills-up the sk_priv
>
> 5) user does DEVLINK_CMD_NOTIFY_FILTER_SET, again
> 6) devlink calls into genetlink code for a sk_priv
> 7) genetlink returns already exising sk_priv found in xa_array
> 8) devlink fills-up the sk_priv
>
> Now the notification thread, sees sk_priv zeroed between 3) and 4)
> and inconsistent during 4) and 8)
>
> I originally solved that by rcu, DEVLINK_CMD_NOTIFY_FILTER_SET
> code always allocates and flips the rcu pointer. Notification thread
> always sees sk_priv consistent.
>
> If you want to allocate sk_priv in genetlink code once, hard to use
> the rcu mechanism and have to protect the sk_priv memory by a lock.
No, you can do exact same thing, just instead of putting the string
directly into the xarray you put a struct which points to the string.
> What am I missing?
The fact that someone in the future may want to add another devlink
priv field, and if the state is basically a pointer to a string,
with complicated lifetime, they will have to suffer undoing that.
> >> If it is alloceted automatically, why is it needed?
> >
> >Because priv may be a complex type which has member that need
> >individual fields to be destroyed (in fullness of time we also
> >need a constructor which can init things like list_head, but
> >we can defer that).
> >
> >I'm guessing in your case the priv will look like this:
> >
> >struct devlink_sk_priv {
> > const char *nft_fltr_instance_name;
> >};
> >
> >static void devlink_sk_priv_free(void *ptr)
> >{
> > struct devlink_sk_priv *priv = ptr;
> >
> > kfree(priv->nft_fltr_instance_name);
> >}
>
> If genetlink code does the allocation, it should know how to free.
> Does not make sense to pass destructor to genetlink code to free memory
> it actually allocated :/
>
> If devlink does the allocation, this callback makes sense. I was
> thinking about having it, but decided kfree is okay for now and
> destructor could be always introduced if needed.
Did you read the code snippet above?
Core still does the kfree of the container (struct devlink_sk_priv).
But what's inside the container struct (string pointer) has to be
handled by the destructor.
Feels like you focus on how to prove me wrong more than on
understanding what I'm saying :|
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 16:18 ` Andy Shevchenko
@ 2023-11-28 19:59 ` Jacob Keller
2023-11-28 20:06 ` Andy Shevchenko
0 siblings, 1 reply; 40+ messages in thread
From: Jacob Keller @ 2023-11-28 19:59 UTC (permalink / raw)
To: Andy Shevchenko, Przemek Kitszel
Cc: Jiri Pirko, kuba, pabeni, davem, edumazet, jhs, johannes,
amritha.nambiar, sdf, horms, netdev
On 11/28/2023 8:18 AM, Andy Shevchenko wrote:
> On Tue, Nov 28, 2023 at 01:30:51PM +0100, Przemek Kitszel wrote:
>> On 11/23/23 19:15, Jiri Pirko wrote:
>
> ...
>
>>> + * Returns: valid pointer on success, otherwise NULL.
>>
>> since you are going to post next revision,
>>
>> kernel-doc requires "Return:" section (singular form)
>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>>
>> for new code we should strive to fulfil the requirement
>> (or piss-off someone powerful enough to change the requirement ;))
>
> Interestingly that the script accepts plural for a few keywords.
> Is it documented somewhere as deprecated?
>
I also checked the source:
$git grep --count -h 'Returns:' | awk '{ sum += $1 } END { print sum }'3646
$git grep --count -h 'Return:' | awk '{ sum += $1 } END { print sum }'
10907
So there is a big favor towards using 'Return:', but there are still
about 1/3 as many uses of 'Returns:'.
I dug into kernel-doc and it looks like it has accepted both "Return"
and "Returns" since the first time that section headers were limited:
f624adef3d0b ("kernel-doc: limit the "section header:" detection to a
select few")
I don't see any documentation on 'Returns;' being deprecated, but the
documentation does only call out 'Return:'.
Thanks,
Jake
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 19:59 ` Jacob Keller
@ 2023-11-28 20:06 ` Andy Shevchenko
2023-11-29 23:29 ` Jacob Keller
0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2023-11-28 20:06 UTC (permalink / raw)
To: Jacob Keller
Cc: Przemek Kitszel, Jiri Pirko, kuba, pabeni, davem, edumazet, jhs,
johannes, amritha.nambiar, sdf, horms, netdev
On Tue, Nov 28, 2023 at 11:59:05AM -0800, Jacob Keller wrote:
> On 11/28/2023 8:18 AM, Andy Shevchenko wrote:
> > On Tue, Nov 28, 2023 at 01:30:51PM +0100, Przemek Kitszel wrote:
> >> On 11/23/23 19:15, Jiri Pirko wrote:
...
> >>> + * Returns: valid pointer on success, otherwise NULL.
> >>
> >> since you are going to post next revision,
> >>
> >> kernel-doc requires "Return:" section (singular form)
> >> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> >>
> >> for new code we should strive to fulfil the requirement
> >> (or piss-off someone powerful enough to change the requirement ;))
> >
> > Interestingly that the script accepts plural for a few keywords.
> > Is it documented somewhere as deprecated?
>
> I also checked the source:
>
> $git grep --count -h 'Returns:' | awk '{ sum += $1 } END { print sum }'
> 3646
> $git grep --count -h 'Return:' | awk '{ sum += $1 } END { print sum }'
> 10907
>
> So there is a big favor towards using 'Return:', but there are still
> about 1/3 as many uses of 'Returns:'.
>
> I dug into kernel-doc and it looks like it has accepted both "Return"
> and "Returns" since the first time that section headers were limited:
> f624adef3d0b ("kernel-doc: limit the "section header:" detection to a
> select few")
>
> I don't see any documentation on 'Returns;' being deprecated, but the
> documentation does only call out 'Return:'.
Then I would amend documentation followed by amending scripts, etc.
Before that it's unclear to me that contributor must use Return:. It
sounds like similar collision to 80 vs. 100 (former in documentation,
latter in the checkpatch).
Of course, there might be sunsystem rules, but again, has to be documented.
Right?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 16:36 ` Jakub Kicinski
@ 2023-11-29 13:59 ` Jiri Pirko
2023-11-29 15:01 ` Jakub Kicinski
0 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-11-29 13:59 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
Tue, Nov 28, 2023 at 05:36:05PM CET, kuba@kernel.org wrote:
>On Tue, 28 Nov 2023 17:05:48 +0100 Jiri Pirko wrote:
>> >Not necessarily, you can have a helper which doesn't allocate, too.
>> >What I'm saying is that the common case for ops will be to access
>> >the state and allocate if it doesn't exist.
>> >
>> >How about genl_sk_family_priv() and genl_sk_has_family_priv() ?
>>
>> My point is, with what you suggest, it will look something like this:
>>
>> 1) user does DEVLINK_CMD_NOTIFY_FILTER_SET
>> 2) devlink calls into genetlink code for a sk_priv and inserts in xa_array
>> 3) genetlink allocates sk_priv and returns back
>> 4) devlink fills-up the sk_priv
>>
>> 5) user does DEVLINK_CMD_NOTIFY_FILTER_SET, again
>> 6) devlink calls into genetlink code for a sk_priv
>> 7) genetlink returns already exising sk_priv found in xa_array
>> 8) devlink fills-up the sk_priv
>>
>> Now the notification thread, sees sk_priv zeroed between 3) and 4)
>> and inconsistent during 4) and 8)
>>
>> I originally solved that by rcu, DEVLINK_CMD_NOTIFY_FILTER_SET
>> code always allocates and flips the rcu pointer. Notification thread
>> always sees sk_priv consistent.
>>
>> If you want to allocate sk_priv in genetlink code once, hard to use
>> the rcu mechanism and have to protect the sk_priv memory by a lock.
>
>No, you can do exact same thing, just instead of putting the string
>directly into the xarray you put a struct which points to the string.
I'm lost. What "string" are you talking about exactly? I'm not putting
any string to xarray.
In the existing implementation, I have following struct:
struct devlink_obj_desc {
struct rcu_head rcu;
const char *bus_name;
const char *dev_name;
unsigned int port_index;
bool port_index_valid;
long data[];
};
This is the struct put pointer to into the xarray. Pointer to this
struct is dereferenced under rcu in notification code and the struct
is freed by kfree_rcu().
>
>> What am I missing?
>
>The fact that someone in the future may want to add another devlink
>priv field, and if the state is basically a pointer to a string,
"the state is basically a pointer to a string". I don't follow what you
mean by this :/
>with complicated lifetime, they will have to suffer undoing that.
>
>> >> If it is alloceted automatically, why is it needed?
>> >
>> >Because priv may be a complex type which has member that need
>> >individual fields to be destroyed (in fullness of time we also
>> >need a constructor which can init things like list_head, but
>> >we can defer that).
>> >
>> >I'm guessing in your case the priv will look like this:
>> >
>> >struct devlink_sk_priv {
>> > const char *nft_fltr_instance_name;
>> >};
>> >
>> >static void devlink_sk_priv_free(void *ptr)
>> >{
>> > struct devlink_sk_priv *priv = ptr;
>> >
>> > kfree(priv->nft_fltr_instance_name);
>> >}
>>
>> If genetlink code does the allocation, it should know how to free.
>> Does not make sense to pass destructor to genetlink code to free memory
>> it actually allocated :/
>>
>> If devlink does the allocation, this callback makes sense. I was
>> thinking about having it, but decided kfree is okay for now and
>> destructor could be always introduced if needed.
>
>Did you read the code snippet above?
Sure.
>
>Core still does the kfree of the container (struct devlink_sk_priv).
>But what's inside the container struct (string pointer) has to be
>handled by the destructor.
>
>Feels like you focus on how to prove me wrong more than on
>understanding what I'm saying :|
Not at all, I have no reason for it. I just want to get my job done
and I am having very hard time to understand what you want exactly.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-29 13:59 ` Jiri Pirko
@ 2023-11-29 15:01 ` Jakub Kicinski
2023-11-29 15:25 ` Jiri Pirko
0 siblings, 1 reply; 40+ messages in thread
From: Jakub Kicinski @ 2023-11-29 15:01 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
On Wed, 29 Nov 2023 14:59:31 +0100 Jiri Pirko wrote:
> Tue, Nov 28, 2023 at 05:36:05PM CET, kuba@kernel.org wrote:
> >No, you can do exact same thing, just instead of putting the string
> >directly into the xarray you put a struct which points to the string.
>
> I'm lost. What "string" are you talking about exactly? I'm not putting
> any string to xarray.
>
> In the existing implementation, I have following struct:
> struct devlink_obj_desc {
> struct rcu_head rcu;
> const char *bus_name;
> const char *dev_name;
> unsigned int port_index;
> bool port_index_valid;
> long data[];
> };
>
> This is the struct put pointer to into the xarray. Pointer to this
> struct is dereferenced under rcu in notification code and the struct
> is freed by kfree_rcu().
Sorry I was looking at patch 8 which has only:
+struct devlink_obj_desc {
+ struct rcu_head rcu;
+ const char *bus_name;
+ const char *dev_name;
+ long data[];
+};
that's basically a string and an rcu_head, that's what I meant.
> >Core still does the kfree of the container (struct devlink_sk_priv).
> >But what's inside the container struct (string pointer) has to be
> >handled by the destructor.
> >
> >Feels like you focus on how to prove me wrong more than on
> >understanding what I'm saying :|
>
> Not at all, I have no reason for it. I just want to get my job done
> and I am having very hard time to understand what you want exactly.
Sockets may want to hold state for more than filtering.
Try to look past your singular use case.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-29 15:01 ` Jakub Kicinski
@ 2023-11-29 15:25 ` Jiri Pirko
0 siblings, 0 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-11-29 15:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms
Wed, Nov 29, 2023 at 04:01:57PM CET, kuba@kernel.org wrote:
>On Wed, 29 Nov 2023 14:59:31 +0100 Jiri Pirko wrote:
>> Tue, Nov 28, 2023 at 05:36:05PM CET, kuba@kernel.org wrote:
>> >No, you can do exact same thing, just instead of putting the string
>> >directly into the xarray you put a struct which points to the string.
>>
>> I'm lost. What "string" are you talking about exactly? I'm not putting
>> any string to xarray.
>>
>> In the existing implementation, I have following struct:
>> struct devlink_obj_desc {
>> struct rcu_head rcu;
>> const char *bus_name;
>> const char *dev_name;
>> unsigned int port_index;
>> bool port_index_valid;
>> long data[];
>> };
>>
>> This is the struct put pointer to into the xarray. Pointer to this
>> struct is dereferenced under rcu in notification code and the struct
>> is freed by kfree_rcu().
>
>Sorry I was looking at patch 8 which has only:
>
>+struct devlink_obj_desc {
>+ struct rcu_head rcu;
>+ const char *bus_name;
>+ const char *dev_name;
>+ long data[];
>+};
>
>that's basically a string and an rcu_head, that's what I meant.
>
>> >Core still does the kfree of the container (struct devlink_sk_priv).
>> >But what's inside the container struct (string pointer) has to be
>> >handled by the destructor.
>> >
>> >Feels like you focus on how to prove me wrong more than on
>> >understanding what I'm saying :|
>>
>> Not at all, I have no reason for it. I just want to get my job done
>> and I am having very hard time to understand what you want exactly.
>
>Sockets may want to hold state for more than filtering.
>Try to look past your singular use case.
Okay, I'll try to make another V. But please don't be mad I
misunderstood something if I do :)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage
2023-11-28 20:06 ` Andy Shevchenko
@ 2023-11-29 23:29 ` Jacob Keller
0 siblings, 0 replies; 40+ messages in thread
From: Jacob Keller @ 2023-11-29 23:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Przemek Kitszel, Jiri Pirko, kuba, pabeni, davem, edumazet, jhs,
johannes, amritha.nambiar, sdf, horms, netdev
On 11/28/2023 12:06 PM, Andy Shevchenko wrote:
> On Tue, Nov 28, 2023 at 11:59:05AM -0800, Jacob Keller wrote:
>> On 11/28/2023 8:18 AM, Andy Shevchenko wrote:
>>> On Tue, Nov 28, 2023 at 01:30:51PM +0100, Przemek Kitszel wrote:
>>>> On 11/23/23 19:15, Jiri Pirko wrote:
>
> ...
>
>>>>> + * Returns: valid pointer on success, otherwise NULL.
>>>>
>>>> since you are going to post next revision,
>>>>
>>>> kernel-doc requires "Return:" section (singular form)
>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>>>>
>>>> for new code we should strive to fulfil the requirement
>>>> (or piss-off someone powerful enough to change the requirement ;))
>>>
>>> Interestingly that the script accepts plural for a few keywords.
>>> Is it documented somewhere as deprecated?
>>
>> I also checked the source:
>>
>> $git grep --count -h 'Returns:' | awk '{ sum += $1 } END { print sum }'
>> 3646
>> $git grep --count -h 'Return:' | awk '{ sum += $1 } END { print sum }'
>> 10907
>>
>> So there is a big favor towards using 'Return:', but there are still
>> about 1/3 as many uses of 'Returns:'.
>>
>> I dug into kernel-doc and it looks like it has accepted both "Return"
>> and "Returns" since the first time that section headers were limited:
>> f624adef3d0b ("kernel-doc: limit the "section header:" detection to a
>> select few")
>>
>> I don't see any documentation on 'Returns;' being deprecated, but the
>> documentation does only call out 'Return:'.
>
> Then I would amend documentation followed by amending scripts, etc.
> Before that it's unclear to me that contributor must use Return:. It
> sounds like similar collision to 80 vs. 100 (former in documentation,
> latter in the checkpatch).
>
> Of course, there might be sunsystem rules, but again, has to be documented.
> Right?
>
Documenting it seems reasonable to me.
Thanks,
Jake
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-11-27 15:40 ` Przemek Kitszel
2023-11-28 8:26 ` Jiri Pirko
@ 2023-12-04 16:24 ` Jiri Pirko
2023-12-04 16:47 ` Andy Shevchenko
2023-12-04 19:17 ` Keller, Jacob E
1 sibling, 2 replies; 40+ messages in thread
From: Jiri Pirko @ 2023-12-04 16:24 UTC (permalink / raw)
To: Przemek Kitszel
Cc: kuba, pabeni, davem, edumazet, jacob.e.keller, jhs, johannes,
andriy.shevchenko, amritha.nambiar, sdf, horms, netdev
Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote:
>On 11/23/23 19:15, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
[...]
>> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>> index fa9afe3e6d9b..33a8e51dea68 100644
>> --- a/net/devlink/netlink.c
>> +++ b/net/devlink/netlink.c
>> @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = {
>> [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME },
>> };
>> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
>> + struct genl_info *info)
>> +{
>> + struct nlattr **attrs = info->attrs;
>> + struct devlink_obj_desc *flt;
>> + size_t data_offset = 0;
>> + size_t data_size = 0;
>> + char *pos;
>> +
>> + if (attrs[DEVLINK_ATTR_BUS_NAME])
>> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
>> + if (attrs[DEVLINK_ATTR_DEV_NAME])
>> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
>> +
>> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
>
>instead of arithmetic here, you could use struct_size()
That is used for flex array, yet I have no flex array here.
>
>> + if (!flt)
>> + return -ENOMEM;
>> +
>> + pos = (char *) flt->data;
>> + if (attrs[DEVLINK_ATTR_BUS_NAME]) {
>> + data_offset += nla_strscpy(pos,
>> + attrs[DEVLINK_ATTR_BUS_NAME],
>> + data_size) + 1;
>> + flt->bus_name = pos;
>> + pos += data_offset;
>> + }
>> + if (attrs[DEVLINK_ATTR_DEV_NAME]) {
>> + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME],
>> + data_size - data_offset);
>> + flt->dev_name = pos;
>> + }
>> +
>> + /* Don't attach empty filter. */
>> + if (!flt->bus_name && !flt->dev_name) {
>> + kfree(flt);
>> + flt = NULL;
>> + }
>> +
>
[...]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-12-04 16:24 ` Jiri Pirko
@ 2023-12-04 16:47 ` Andy Shevchenko
2023-12-04 19:17 ` Keller, Jacob E
1 sibling, 0 replies; 40+ messages in thread
From: Andy Shevchenko @ 2023-12-04 16:47 UTC (permalink / raw)
To: Jiri Pirko
Cc: Przemek Kitszel, kuba, pabeni, davem, edumazet, jacob.e.keller,
jhs, johannes, amritha.nambiar, sdf, horms, netdev
On Mon, Dec 04, 2023 at 05:24:34PM +0100, Jiri Pirko wrote:
> Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote:
> >On 11/23/23 19:15, Jiri Pirko wrote:
[...]
> >> + size_t data_size = 0;
> >> + char *pos;
> >> +
> >> + if (attrs[DEVLINK_ATTR_BUS_NAME])
> >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
> >> + if (attrs[DEVLINK_ATTR_DEV_NAME])
> >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
> >> +
> >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
> >
> >instead of arithmetic here, you could use struct_size()
>
> That is used for flex array, yet I have no flex array here.
It feels like you use flexible array even if you have the top limit of
the size. But yeah, the attributes seem out of the variadic length and
struct_size() here won't help anyway.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-12-04 16:24 ` Jiri Pirko
2023-12-04 16:47 ` Andy Shevchenko
@ 2023-12-04 19:17 ` Keller, Jacob E
2023-12-05 7:47 ` Jiri Pirko
1 sibling, 1 reply; 40+ messages in thread
From: Keller, Jacob E @ 2023-12-04 19:17 UTC (permalink / raw)
To: Jiri Pirko, Kitszel, Przemyslaw
Cc: kuba, pabeni, davem, edumazet, Hadi Salim, Jamal, johannes,
andriy.shevchenko, Nambiar, Amritha, sdf, horms, netdev
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Monday, December 4, 2023 8:25 AM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net;
> edumazet@google.com; Keller, Jacob E <jacob.e.keller@intel.com>; Hadi Salim,
> Jamal <jhs@mojatatu.com>; johannes@sipsolutions.net;
> andriy.shevchenko@linux.intel.com; Nambiar, Amritha
> <amritha.nambiar@intel.com>; sdf@google.com; horms@kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [patch net-next v4 8/9] devlink: add a command to set notification
> filter and use it for multicasts
>
> Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote:
> >On 11/23/23 19:15, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >>
>
> [...]
>
>
> >> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
> >> index fa9afe3e6d9b..33a8e51dea68 100644
> >> --- a/net/devlink/netlink.c
> >> +++ b/net/devlink/netlink.c
> >> @@ -17,6 +17,79 @@ static const struct genl_multicast_group
> devlink_nl_mcgrps[] = {
> >> [DEVLINK_MCGRP_CONFIG] = { .name =
> DEVLINK_GENL_MCGRP_CONFIG_NAME },
> >> };
> >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
> >> + struct genl_info *info)
> >> +{
> >> + struct nlattr **attrs = info->attrs;
> >> + struct devlink_obj_desc *flt;
> >> + size_t data_offset = 0;
> >> + size_t data_size = 0;
> >> + char *pos;
> >> +
> >> + if (attrs[DEVLINK_ATTR_BUS_NAME])
> >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
> >> + if (attrs[DEVLINK_ATTR_DEV_NAME])
> >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
> >> +
> >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
> >
> >instead of arithmetic here, you could use struct_size()
>
> That is used for flex array, yet I have no flex array here.
>
Yea this isn't a flexible array. You could use size_add to ensure that this can't overflow. I don't know what the bound on the attribute sizes is.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-12-04 19:17 ` Keller, Jacob E
@ 2023-12-05 7:47 ` Jiri Pirko
2023-12-05 15:58 ` andriy.shevchenko
0 siblings, 1 reply; 40+ messages in thread
From: Jiri Pirko @ 2023-12-05 7:47 UTC (permalink / raw)
To: Keller, Jacob E
Cc: Kitszel, Przemyslaw, kuba, pabeni, davem, edumazet, Hadi Salim,
Jamal, johannes, andriy.shevchenko, Nambiar, Amritha, sdf, horms,
netdev
Mon, Dec 04, 2023 at 08:17:24PM CET, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Monday, December 4, 2023 8:25 AM
>> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
>> Cc: kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net;
>> edumazet@google.com; Keller, Jacob E <jacob.e.keller@intel.com>; Hadi Salim,
>> Jamal <jhs@mojatatu.com>; johannes@sipsolutions.net;
>> andriy.shevchenko@linux.intel.com; Nambiar, Amritha
>> <amritha.nambiar@intel.com>; sdf@google.com; horms@kernel.org;
>> netdev@vger.kernel.org
>> Subject: Re: [patch net-next v4 8/9] devlink: add a command to set notification
>> filter and use it for multicasts
>>
>> Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote:
>> >On 11/23/23 19:15, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@nvidia.com>
>> >>
>>
>> [...]
>>
>>
>> >> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>> >> index fa9afe3e6d9b..33a8e51dea68 100644
>> >> --- a/net/devlink/netlink.c
>> >> +++ b/net/devlink/netlink.c
>> >> @@ -17,6 +17,79 @@ static const struct genl_multicast_group
>> devlink_nl_mcgrps[] = {
>> >> [DEVLINK_MCGRP_CONFIG] = { .name =
>> DEVLINK_GENL_MCGRP_CONFIG_NAME },
>> >> };
>> >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb,
>> >> + struct genl_info *info)
>> >> +{
>> >> + struct nlattr **attrs = info->attrs;
>> >> + struct devlink_obj_desc *flt;
>> >> + size_t data_offset = 0;
>> >> + size_t data_size = 0;
>> >> + char *pos;
>> >> +
>> >> + if (attrs[DEVLINK_ATTR_BUS_NAME])
>> >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
>> >> + if (attrs[DEVLINK_ATTR_DEV_NAME])
>> >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
>> >> +
>> >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
>> >
>> >instead of arithmetic here, you could use struct_size()
>>
>> That is used for flex array, yet I have no flex array here.
>>
>
>Yea this isn't a flexible array. You could use size_add to ensure that this can't overflow. I don't know what the bound on the attribute sizes is.
Okay, will do that to be on a safe side.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts
2023-12-05 7:47 ` Jiri Pirko
@ 2023-12-05 15:58 ` andriy.shevchenko
0 siblings, 0 replies; 40+ messages in thread
From: andriy.shevchenko @ 2023-12-05 15:58 UTC (permalink / raw)
To: Jiri Pirko
Cc: Keller, Jacob E, Kitszel, Przemyslaw, kuba, pabeni, davem,
edumazet, Hadi Salim, Jamal, johannes, Nambiar, Amritha, sdf,
horms, netdev
On Tue, Dec 05, 2023 at 08:47:43AM +0100, Jiri Pirko wrote:
> Mon, Dec 04, 2023 at 08:17:24PM CET, jacob.e.keller@intel.com wrote:
> >> -----Original Message-----
> >> From: Jiri Pirko <jiri@resnulli.us>
> >> Sent: Monday, December 4, 2023 8:25 AM
> >> Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote:
> >> >On 11/23/23 19:15, Jiri Pirko wrote:
...
> >> >> + if (attrs[DEVLINK_ATTR_BUS_NAME])
> >> >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1;
> >> >> + if (attrs[DEVLINK_ATTR_DEV_NAME])
> >> >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1;
> >> >> +
> >> >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL);
> >> >
> >> >instead of arithmetic here, you could use struct_size()
> >>
> >> That is used for flex array, yet I have no flex array here.
> >
> >Yea this isn't a flexible array. You could use size_add to ensure that this
> >can't overflow. I don't know what the bound on the attribute sizes is.
>
> Okay, will do that to be on a safe side.
If we go this direction it may be makes sense to have done inside nla_len():ish
type of helper, so it will be once for everyone. But I haven't checked the code
on how many cases we have when we need to count the size depending on the present
attributes.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2023-12-05 15:58 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-23 18:15 [patch net-next v4 0/9] devlink: introduce notifications filtering Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 1/9] devlink: use devl_is_registered() helper instead xa_get_mark() Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 2/9] devlink: introduce __devl_is_registered() helper and use it instead of xa_get_mark() Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 3/9] devlink: send notifications only if there are listeners Jiri Pirko
2023-11-27 11:01 ` Paolo Abeni
2023-11-27 12:04 ` Jiri Pirko
2023-11-27 15:00 ` Paolo Abeni
2023-11-28 7:39 ` Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 4/9] devlink: introduce a helper for netlink multicast send Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage Jiri Pirko
2023-11-27 11:13 ` Paolo Abeni
2023-11-27 12:00 ` Jiri Pirko
2023-11-27 22:46 ` Jakub Kicinski
2023-11-28 8:25 ` Jiri Pirko
2023-11-28 15:11 ` Jakub Kicinski
2023-11-28 16:05 ` Jiri Pirko
2023-11-28 16:36 ` Jakub Kicinski
2023-11-29 13:59 ` Jiri Pirko
2023-11-29 15:01 ` Jakub Kicinski
2023-11-29 15:25 ` Jiri Pirko
2023-11-28 12:30 ` Przemek Kitszel
2023-11-28 15:05 ` Jiri Pirko
2023-11-28 16:18 ` Andy Shevchenko
2023-11-28 19:59 ` Jacob Keller
2023-11-28 20:06 ` Andy Shevchenko
2023-11-29 23:29 ` Jacob Keller
2023-11-23 18:15 ` [patch net-next v4 6/9] netlink: introduce typedef for filter function Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 7/9] genetlink: introduce helpers to do filtered multicast Jiri Pirko
2023-11-23 18:15 ` [patch net-next v4 8/9] devlink: add a command to set notification filter and use it for multicasts Jiri Pirko
2023-11-27 12:30 ` Przemek Kitszel
2023-11-27 12:51 ` Jiri Pirko
2023-11-27 12:56 ` Jiri Pirko
2023-11-27 15:40 ` Przemek Kitszel
2023-11-28 8:26 ` Jiri Pirko
2023-12-04 16:24 ` Jiri Pirko
2023-12-04 16:47 ` Andy Shevchenko
2023-12-04 19:17 ` Keller, Jacob E
2023-12-05 7:47 ` Jiri Pirko
2023-12-05 15:58 ` andriy.shevchenko
2023-11-23 18:15 ` [patch net-next v4 9/9] devlink: extend multicast filtering by port index Jiri Pirko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.