Re: [PATCH v2 8/8] io_uring: drain based on allocates reqs

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, May 09, 2025 at 12:12:54PM +0100, Pavel Begunkov wrote:
> Don't rely on CQ sequence numbers for draining, as it has become messy
> and needs cq_extra adjustments. Instead, base it on the number of
> allocated requests and only allow flushing when all requests are in the
> drain list.
> 
> As a result, cq_extra is gone, no overhead for its accounting in aux cqe
> posting, less bloating as it was inlined before, and it's in general
> simpler than trying to track where we should bump it and where it should
> be put back like in cases of overflow. Also, it'll likely help with
> cleaning and unifying some of the CQ posting helpers.

This patch breaks the `make W=1` build. Please, always test your changes with
`make W=1`. See below the details.

...

>  static __cold void io_drain_req(struct io_kiocb *req)
>  	__must_hold(&ctx->uring_lock)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
>  	bool drain = req->flags & IOSQE_IO_DRAIN;
>  	struct io_defer_entry *de;
> +	struct io_kiocb *tmp;

> +	int nr = 0;

Defined and assigned...

>  
>  	de = kmalloc(sizeof(*de), GFP_KERNEL_ACCOUNT);
>  	if (!de) {
> @@ -1667,17 +1658,17 @@ static __cold void io_drain_req(struct io_kiocb *req)
>  		return;
>  	}
>  
> +	io_for_each_link(tmp, req)
> +		nr++;

...just incremented...

And that seems it. Does the above have any side-effects? Or is it just a dead
code (a.k.a. leftovers from the rebase/upcoming work)?

In any case, please make use of nr or drop it completely.

io_uring/io_uring.c:1649:6: error: variable 'nr' set but not used [-Werror,-Wunused-but-set-variable]
 1649 |         int nr = 0;
      |             ^
1 error generated.

>  	io_prep_async_link(req);
>  	trace_io_uring_defer(req);
>  	de->req = req;
> -	de->seq = io_get_sequence(req);
>  
> -	scoped_guard(spinlock, &ctx->completion_lock) {
> -		list_add_tail(&de->list, &ctx->defer_list);
> -		__io_queue_deferred(ctx);
> -		if (!drain && list_empty(&ctx->defer_list))
> -			ctx->drain_active = false;
> -	}
> +	ctx->nr_drained += io_linked_nr(req);
> +	list_add_tail(&de->list, &ctx->defer_list);
> +	io_queue_deferred(ctx);
> +	if (!drain && list_empty(&ctx->defer_list))
> +		ctx->drain_active = false;
>  }

-- 
With Best Regards,
Andy Shevchenko