Message ID | 20200506151655.26445-1-jgross@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xen/sched: always modify vcpu pause flags atomically | expand |
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@suse.com> > Reviewed-by: Dario Faggioli <dfaggioli@suse.com> > --- > 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
diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c index 93d89da278..d0aa017c64 100644 --- a/xen/common/sched/credit.c +++ b/xen/common/sched/credit.c @@ -453,7 +453,7 @@ static inline void __runq_tickle(const struct csched_unit *new) SCHED_UNIT_STAT_CRANK(cur, kicked_away); SCHED_UNIT_STAT_CRANK(cur, migrate_r); SCHED_STAT_CRANK(migrate_kicked_away); - sched_set_pause_flags_atomic(cur->unit, _VPF_migrating); + sched_set_pause_flags(cur->unit, _VPF_migrating); } /* Tickle cpu anyway, to let new preempt cur. */ SCHED_STAT_CRANK(tickled_busy_cpu); @@ -973,7 +973,7 @@ csched_unit_acct(struct csched_private *prv, unsigned int cpu) { SCHED_UNIT_STAT_CRANK(svc, migrate_r); SCHED_STAT_CRANK(migrate_running); - sched_set_pause_flags_atomic(currunit, _VPF_migrating); + sched_set_pause_flags(currunit, _VPF_migrating); /* * As we are about to tickle cpu, we should clear its bit in * idlers. But, if we are here, it means there is someone running diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h index 367811a12f..b9a5b4c01c 100644 --- a/xen/common/sched/private.h +++ b/xen/common/sched/private.h @@ -172,7 +172,7 @@ static inline void sched_set_pause_flags(struct sched_unit *unit, struct vcpu *v; for_each_sched_unit_vcpu ( unit, v ) - __set_bit(bit, &v->pause_flags); + set_bit(bit, &v->pause_flags); } /* Clear a bit in pause_flags of all vcpus of a unit. */ @@ -181,26 +181,6 @@ static inline void sched_clear_pause_flags(struct sched_unit *unit, { struct vcpu *v; - for_each_sched_unit_vcpu ( unit, v ) - __clear_bit(bit, &v->pause_flags); -} - -/* Set a bit in pause_flags of all vcpus of a unit via atomic updates. */ -static inline void sched_set_pause_flags_atomic(struct sched_unit *unit, - unsigned int bit) -{ - struct vcpu *v; - - for_each_sched_unit_vcpu ( unit, v ) - set_bit(bit, &v->pause_flags); -} - -/* Clear a bit in pause_flags of all vcpus of a unit via atomic updates. */ -static inline void sched_clear_pause_flags_atomic(struct sched_unit *unit, - unsigned int bit) -{ - struct vcpu *v; - for_each_sched_unit_vcpu ( unit, v ) clear_bit(bit, &v->pause_flags); }
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. 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@suse.com> --- 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. --- xen/common/sched/credit.c | 4 ++-- xen/common/sched/private.h | 22 +--------------------- 2 files changed, 3 insertions(+), 23 deletions(-)