[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH for-4.15] xen/sched: Add missing memory barrier in vcpu_block()
On 22/02/2021 20:09, Stefano Stabellini wrote: On Mon, 22 Feb 2021, Jan Beulich wrote:On 20.02.2021 20:47, Julien Grall wrote:From: Julien Grall <jgrall@xxxxxxxxxx> The comment in vcpu_block() states that the events should be checked /after/ blocking to avoids wakeup waiting race. However, from a generic perspective, set_bit() doesn't prevent re-ordering. So the following could happen: CPU0 (blocking vCPU A) | CPU1 ( unblock vCPU A) | A <- read local events | | set local events | test_and_clear_bit(_VPF_blocked) | -> Bail out as the bit if not set | set_bit(_VFP_blocked) | | check A | The variable A will be 0 and therefore the vCPU will be blocked when it should continue running. vcpu_block() is now gaining an smp_mb__after_atomic() to prevent the CPU to read any information about local events before the flag _VPF_blocked is set. Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>This is a follow-up of the discussion that started in 2019 (see [1]) regarding a possible race between do_poll()/vcpu_unblock() and the wake up path. I haven't yet fully thought about the potential race in do_poll(). If there is, then this would likely want to be fixed in a separate patch. On x86, the current code is safe because set_bit() is fully ordered. SO the problem is Arm (and potentially any new architectures). I couldn't convince myself whether the Arm implementation of local_events_need_delivery() contains enough barrier to prevent the re-ordering. However, I don't think we want to play with devil here as the function may be optimized in the future.In fact I think this ...--- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -1418,6 +1418,8 @@ void vcpu_block(void)set_bit(_VPF_blocked, &v->pause_flags); + smp_mb__after_atomic();+... pattern should be looked for throughout the codebase, and barriers be added unless it can be proven none is needed. >And in that case it would be best to add an in-code comment to explain why the barrier is not needed .I would rather not add comment for every *_bit() calls. It should be pretty obvious for most of them that the barrier is not necessary. We should only add comments where the barrier is necessary or it is not clear why it is not necessary. Cheers, -- Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |