Re: [PATCH net-next v3 1/7] net: ethtool: netlink: Allow per-netdevice DUMP operations
From: Paolo Abeni <pabeni@redhat.com>
Date: 2025-03-21 16:31:23
Also in:
linux-arm-kernel, lkml
On 3/13/25 7:26 PM, Maxime Chevallier wrote:
quoted hunk ↗ jump to hunk
We have a number of netlink commands in the ethnl family that may have multiple objects to dump even for a single net_device, including : - PLCA, PSE-PD, phy: one message per PHY device - tsinfo: one message per timestamp source (netdev + phys) - rss: One per RSS context To get this behaviour, these netlink commands need to roll a custom ->dumpit(). To prepare making per-netdev DUMP more generic in ethnl, introduce a member in the ethnl ops to indicate if a given command may allow pernetdev DUMPs (also referred to as filtered DUMPs). Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- net/ethtool/netlink.c | 45 +++++++++++++++++++++++++++++-------------- net/ethtool/netlink.h | 2 ++ 2 files changed, 33 insertions(+), 14 deletions(-)diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index a163d40c6431..7adede5e4ff1 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c@@ -587,21 +587,38 @@ static int ethnl_default_dumpit(struct sk_buff *skb, int ret = 0; rcu_read_lock();
Maintain the RCU read lock here is IMHO confusing...
- for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
- dev_hold(dev);
+ if (ctx->req_info->dev) {
+ dev = ctx->req_info->dev;.. as this is refcounted. I suggest to move the rcu_read_lock inside the if.
rcu_read_unlock();
+ /* Filtered DUMP request targeted to a single netdev. We already
+ * hold a ref to the netdev from ->start()
+ */
+ ret = ethnl_default_dump_one(skb, dev, ctx,
+ genl_info_dump(cb));
+ rcu_read_lock();
+ netdev_put(ctx->req_info->dev, &ctx->req_info->dev_tracker);
- ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
+ if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
+ ret = skb->len;
- rcu_read_lock();
- dev_put(dev);
+ } else {
+ for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
+ dev_hold(dev);
+ rcu_read_unlock();
+
+ ret = ethnl_default_dump_one(skb, dev, ctx,
+ genl_info_dump(cb));
+
+ rcu_read_lock();
+ dev_put(dev);
- if (ret < 0 && ret != -EOPNOTSUPP) {
- if (likely(skb->len))
- ret = skb->len;
- break;
+ if (ret < 0 && ret != -EOPNOTSUPP) {
+ if (likely(skb->len))
+ ret = skb->len;IMHO a bit too many levels of indentation. It's possibly better to move this code in a separate helper. Thanks, Paolo