Re: Memory ordering question in the shutdown deferral code

[Julien Grall <julien@xen.org>]

(+ Xen-devel)

Sorry I forgot to CC xen-devel.

On 21/09/2020 12:38, Julien Grall wrote:
> Hi all,
> 
> I have started to look at the deferral code (see 
> vcpu_start_shutdown_deferral()) because we need it for LiveUpdate and 
> Arm will soon use it.
> 
> The current implementation is using an smp_mb() to ensure ordering 
> between a write then a read. The code looks roughly (I have slightly 
> adapted it to make my question more obvious):
> 
> domain_shutdown()
>      d->is_shutting_down = 1;
>      smp_mb();
>      if ( !vcpu0->defer_shutdown )
>      {
>        vcpu_pause_nosync(v);
>        v->paused_for_shutdown = 1;
>      }
> 
> vcpu_start_shutdown_deferral()
>      vcpu0->defer_shutdown = 1;
>      smp_mb();
>      if ( unlikely(d->is_shutting_down) )
>        vcpu_check_shutdown(v);
> 
>      return vcpu0->defer_shutdown;
> 
> smp_mb() should only guarantee ordering (this may be stronger on some 
> arch), so I think there is a race between the two functions.
> 
> It would be possible to pause the vCPU in domain_shutdown() because 
> vcpu0->defer_shutdown wasn't yet seen.
> 
> Equally, vcpu_start_shutdown_deferral() may not see d->is_shutting_down 
> and therefore Xen may continue to send the I/O. Yet the vCPU will be 
> paused so the I/O will never complete.
> 
> I am not fully familiar with the IOREQ code, but it sounds to me this is 
> not the behavior that was intended. Can someone more familiar with the 
> code confirm it?
> 
> Cheers,
> 

-- 
Julien Grall