Re: [net-next,V5,06/15] devlink: Decouple rate storage from associated devlink object

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
devlink: Decouple rate storage from associated devlink object

This patch introduces the concept of a "rate node devlink" to allow rates
from multiple devlink instances to be stored together. It updates all
rate node manipulation code to filter by devlink during iteration and
adds lock/unlock helpers (currently no-ops) for future locking support.

> diff --git a/net/devlink/rate.c b/net/devlink/rate.c
> index 0d68b5c477dc..ddbd0beec4b9 100644
> --- a/net/devlink/rate.c
> +++ b/net/devlink/rate.c

[ ... ]

> @@ -209,10 +235,12 @@ devlink_nl_rate_get_dump_one(struct sk_buff *msg, struct devlink *devlink,
>  {
>  	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
>  	struct devlink_rate *devlink_rate;
> +	struct devlink *rate_devlink;
>  	int idx = 0;
>  	int err = 0;
>
> -	list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
> +	rate_devlink = devl_rate_lock(devlink);
> +	list_for_each_entry(devlink_rate, &rate_devlink->rate_list, list) {
>  		enum devlink_command cmd = DEVLINK_CMD_RATE_NEW;
>  		u32 id = NETLINK_CB(cb->skb).portid;
>
> @@ -220,6 +248,9 @@ devlink_nl_rate_get_dump_one(struct sk_buff *msg, struct devlink *devlink,
>  			idx++;
>  			continue;
>  		}
> +		if (devlink_rate->devlink != devlink)
> +			continue;

When this filter skips a non-matching entry, idx is not incremented.
However, state->idx is used to track position across netlink dump
resumptions and expects idx to reflect the actual position in the list.

Once cross-device rate nodes are enabled (later in this series), the list
can contain entries from multiple devlinks. If the dump requires multiple
netlink messages and non-matching entries exist between matching ones,
the idx tracking becomes inconsistent with the list position. This can
cause entries to be reported multiple times on dump resumption.

Should this continue statement also increment idx to maintain correct
position tracking?

> +
>  		err = devlink_nl_rate_fill(msg, devlink_rate, cmd, id,
>  					   cb->nlh->nlmsg_seq, flags, NULL);
>  		if (err) {
> @@ -228,6 +259,7 @@ devlink_nl_rate_get_dump_one(struct sk_buff *msg, struct devlink *devlink,
>  		}
>  		idx++;
>  	}
> +	devl_rate_unlock(devlink);

[ ... ]
-- 
pw-bot: cr