Re: [PATCH] xen/sched: always modify vcpu pause flags atomically

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Wed, 2020-05-06 at 17:16 +0200, Juergen Gross wrote:
> credit2 is currently modifying the pause flags of vcpus non-
> atomically
> via sched_set_pause_flags() and sched_clear_pause_flags(). This is
> dangerous as there are cases where the paus flags are modified
> without
> any lock held.
> 
Right.

> So drop the non-atomic pause flag modification functions and rename
> the
> atomic ones dropping the _atomic suffix.
> 
> Fixes: a76255b4266516 ("xen/sched: make credit2 scheduler vcpu
> agnostic.")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>
Reviewed-by: Dario Faggioli <dfaggioli@xxxxxxxx>

> ---
> It should be noted that this issue wasn't introduced by core
> scheduling
> as even before credit2 was using the non-atomic __set_bit() and
> __clear_bit() variants.
>
Yes. I can see that in 222234f2ad17185 ("xen: credit2: use non-atomic
cpumask and bit operations"), where the svc->flags are switched to non-
atomic updates (as, for them, it is true that they're always accessed
while holding locks), switching of setting the _VPF_migrating
pause->flags non atomically also slipped in, but that was clearly a
mistake. :-(

I believe (but I haven't checked this part too thoroughly) that it was
the only one back then. Afterwords, when another instance was added, in
__runq_tickle(), we found the already existing one and followed suit.

Good catch, and thanks. :-)

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part