Re: [PATCH] irq: simplify irq_im_handle_irq()

[Thomas Gleixner]

Yury!

On Sat, Jul 19 2025 at 17:18, Yury Norov wrote:

'irq:' is not the correct prefix here. See:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

Also irq_im_handle_irq() is not a known function name.

> From: Yury Norov (NVIDIA) <yury.norov@xxxxxxxxx>
>
> Hi Thomas,

Since when is a greeting part of the changelog?

> The function calls bitmap_empty() for potentially every bit in
> work_ctx->pending, which makes a simple bitmap traverse O(N^2).
> Fix it by switching to the dedicated for_each_set_bit().
>
> While there, fix using atomic clear_bit() in a context where atomicity
> cannot be guaranteed.

Seriously? See below.

>  static void irq_sim_handle_irq(struct irq_work *work)
>  {
>  	struct irq_sim_work_ctx *work_ctx;
> -	unsigned int offset = 0;
> +	unsigned int offset;
>  	int irqnum;
>  
>  	work_ctx = container_of(work, struct irq_sim_work_ctx, work);
>  
> -	while (!bitmap_empty(work_ctx->pending, work_ctx->irq_count)) {
> -		offset = find_next_bit(work_ctx->pending,
> -				       work_ctx->irq_count, offset);
> -		clear_bit(offset, work_ctx->pending);
> +	for_each_set_bit(offset, work_ctx->pending, work_ctx->irq_count) {
> +		__clear_bit(offset, work_ctx->pending);

This is just wrong.

__clear_bit() can only be used when there is _NO_ concurrency
possible. But this has concurrency:

irq_sim_set_irqchip_state()
...
        assign_bit(hwirq, irq_ctx->work_ctx->pending, state);

That function can be executed on a different CPU concurrently while the
other CPU walks the bitmap and tries to clear a bit. The function
documentation of __clear_bit() has this documented very clearly:

 * Unlike clear_bit(), this function is non-atomic. If it is called on the same
 * region of memory concurrently, the effect may be that only one operation                                                                                    * succeeds.

No?

Thanks,

        tglx