Re: [PATCH 2/2] irq_work: Fix racy IRQ_WORK_BUSY flag setting

[Steven Rostedt]

On Tue, 2012-10-30 at 16:35 +0100, Frederic Weisbecker wrote:
> The IRQ_WORK_BUSY flag is set right before we execute the
> work. Once this flag value is set, the work enters a
> claimable state again.
> 
> This is necessary because if we want to enqueue a work but we
> fail the claim, we want to ensure that the CPU where that work
> is still pending will see and handle the data we expected the
> work to compute.
> 
> This might not work as expected though because IRQ_WORK_BUSY
> isn't set atomically. By the time a CPU fails a work claim,
> this work may well have been already executed by the CPU where
> it was previously pending.
> 
> Due to the lack of appropriate memory barrier, the IRQ_WORK_BUSY
> flag value may not be visible by the CPU trying to claim while
> the work is executing, and that until we clear the busy bit in
> the work flags using cmpxchg() that implies the full barrier.
> 
> One solution could involve a full barrier between setting
> IRQ_WORK_BUSY flag and the work execution. This way we
> ensure that the work execution site sees the expected data
> and the claim site sees the IRQ_WORK_BUSY:
> 
> CPU 0                                 CPU 1
> 
> data = something                     flags = IRQ_WORK_BUSY
> smp_mb() (implicit with cmpxchg      smp_mb()
>           on flags in claim)         execute_work (sees data from CPU 0)
> try to claim
> 
> As a shortcut, let's just use xchg() that implies a full memory
> barrier.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>

Reviewed-by: Steven Rostedt <rostedt@xxxxxxxxxxx>

-- Steve

> ---
>  kernel/irq_work.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> index 764240a..ea79365 100644
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -130,9 +130,12 @@ void irq_work_run(void)
>  
>  		/*
>  		 * Clear the PENDING bit, after this point the @work
> -		 * can be re-used.
> +		 * can be re-used. Use xchg to force ordering against
> +		 * data to process, such that if claiming fails on
> +		 * another CPU, we see and handle the data it wants
> +		 * us to process on the work.
>  		 */
> -		work->flags = IRQ_WORK_BUSY;
> +		xchg(&work->flags, IRQ_WORK_BUSY);
>  		work->func(work);
>  		/*
>  		 * Clear the BUSY bit and return to the free state if


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/