Re: [PATCH net-next v3 08/12] net: ethernet: cortina: Rotate free queue page allocation

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit "net: ethernet: cortina: Rotate free queue page
  allocation" carries `Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver…
- [Low] The commit message carries a Fixes tag and `Assisted-by: LLM`, so
  it presents itself as a netdev bug fix.
- [Low] `geth_cleanup_freeq()` and the error labels in
  `geth_setup_freeq()` (`err_freeq_bitmap` / `err_freeq_pages`) free the…

--

net: ethernet: cortina: Rotate free queue page allocation

This patch changes geth_freeq_alloc_slot() to start each bitmap search
just after the most recently allocated slot, wrapping at the end, instead
of always starting from zero. A new freeq_page_cursor field holds the
position, and geth_setup_freeq() resets it to 0.

> The free queue metadata bitmap currently searches for a free slot from
> zero for every page allocation. Under sustained traffic this repeatedly
> scans occupied low-numbered slots.

[ ... ]

> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

[Severity: Medium]
Is this Fixes tag pointing at the right commit? The freeq_page_bitmap,
geth_freeq_alloc_slot() and its find_first_zero_bit() search are not in
4d5ae32f5e1e. They were added earlier in this series by "net: ethernet:
cortina: Preserve in-flight free queue pages".

The change also doesn't seem to alter behavior beyond where the search
starts. The old code returned -ENOSPC only when there was no zero bit in
[0, num_freeq_pages). The new code returns -ENOSPC only when both searches
fail, which is the same condition. A free slot is still found whenever one
exists.

This looks like an optimization of code added in the same unmerged series.
Could it be folded into "Preserve in-flight free queue pages", or sent
without a Fixes tag?

> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
The patch carries a Fixes tag, but the commit message doesn't say how the
scan cost was found or measured, or how the change was tested.
Documentation/process/maintainer-netdev.rst asks for this:

  All bug fixes must state how the issue was discovered and validated,
  and how the fix was tested.

For hardware drivers, it also asks that the testing statement name the
hardware the fix was tested on. Could that be added, including which
Gemini board was used?

Also, the message says the bitmap "currently searches for a free slot from
zero", but that behavior was added two patches earlier in this series.
Could the message say so?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2345d582cb597..87c537ae83e7a 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -743,12 +744,20 @@ static int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
>  
>  	lockdep_assert_held(&geth->freeq_lock);
>  
> -	slot = find_first_zero_bit(geth->freeq_page_bitmap,
> -				   geth->num_freeq_pages);
> -	if (slot == geth->num_freeq_pages)
> -		return -ENOSPC;
> +	slot = find_next_zero_bit(geth->freeq_page_bitmap,
> +				  geth->num_freeq_pages,
> +				  geth->freeq_page_cursor);
> +	if (slot == geth->num_freeq_pages) {
> +		slot = find_first_zero_bit(geth->freeq_page_bitmap,
> +					   geth->freeq_page_cursor);

[Severity: Low]
As far as I can tell this can't be reached today, but the wrap search
now passes freeq_page_cursor as the size argument. That means it relies
on the cursor matching the current bitmap.

geth_cleanup_freeq() and the error unwind in geth_setup_freeq() free the
bitmap and clear the count, but leave the cursor at its last value:

geth_cleanup_freeq() {
    ...
	bitmap_free(geth->freeq_page_bitmap);
	geth->freeq_page_bitmap = NULL;
	kfree(geth->freeq_pages);
	geth->freeq_pages = NULL;
	geth->num_freeq_pages = 0;
    ...
}

If geth_freeq_alloc_slot() ran in that state,
find_next_zero_bit(NULL, 0, cursor) would return 0. Then
find_first_zero_bit(NULL, cursor) with a non-zero cursor would read
through the NULL bitmap. The old find_first_zero_bit(bitmap, 0) returned
-ENOSPC without touching memory.

The current callers of geth_fill_freeq() appear to be guarded:

- geth_setup_freeq() resets the cursor after bitmap_zalloc() succeeds.
- NAPI only runs after a successful geth_resize_freeq().
- The SWFQ_EMPTY source stays masked after a failed resize.

Would it make sense to reset freeq_page_cursor to 0 next to the
num_freeq_pages = 0 assignments? Then the allocator wouldn't depend on
those outside guards.

> +		if (slot == geth->freeq_page_cursor)
> +			return -ENOSPC;
> +	}
>  
>  	__set_bit(slot, geth->freeq_page_bitmap);
> +	geth->freeq_page_cursor = slot + 1;
> +	if (geth->freeq_page_cursor == geth->num_freeq_pages)
> +		geth->freeq_page_cursor = 0;
>  
>  	return slot;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-gemini-ethernet-fixes-3-v3-0-3e1f25890ae8%40kernel.org