| Message ID | 146620512051.29766.574756341169521466.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.06.16 at 01:12, <dario.faggioli@citrix.com> wrote: > Yet another situation very similar to 779511f4bf5ae > ("sched: avoid races on time values read from NOW()"). > > In fact, when more than one runqueue is involved, we need > to make sure that the following does not happen: > 1. take the lock of 1st runq > 2. now = NOW() > 3. take the lock of 2nd runq > 4. use now > > as, if we have to wait at step 3, the value in now may > be stale when we get to use it at step 4. Is this really meaningful here? We're talking of trylocks, which don't incur any delay other than the latency of the LOCKed (on x86) instruction to determine lock availability. Jan
On Mon, Jun 20, 2016 at 8:56 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 18.06.16 at 01:12, <dario.faggioli@citrix.com> wrote: >> Yet another situation very similar to 779511f4bf5ae >> ("sched: avoid races on time values read from NOW()"). >> >> In fact, when more than one runqueue is involved, we need >> to make sure that the following does not happen: >> 1. take the lock of 1st runq >> 2. now = NOW() >> 3. take the lock of 2nd runq >> 4. use now >> >> as, if we have to wait at step 3, the value in now may >> be stale when we get to use it at step 4. > > Is this really meaningful here? We're talking of trylocks, which don't > incur any delay other than the latency of the LOCKed (on x86) > instruction to determine lock availability. This makes sense to me -- Dario? -George
On Wed, 2016-07-06 at 17:10 +0100, George Dunlap wrote: > On Mon, Jun 20, 2016 at 8:56 AM, Jan Beulich <JBeulich@suse.com> > wrote: > > > > > > > On 18.06.16 at 01:12, <dario.faggioli@citrix.com> wrote: > > > Yet another situation very similar to 779511f4bf5ae > > > ("sched: avoid races on time values read from NOW()"). > > > > > > In fact, when more than one runqueue is involved, we need > > > to make sure that the following does not happen: > > > 1. take the lock of 1st runq > > > 2. now = NOW() > > > 3. take the lock of 2nd runq > > > 4. use now > > > > > > as, if we have to wait at step 3, the value in now may > > > be stale when we get to use it at step 4. > > Is this really meaningful here? We're talking of trylocks, which > > don't > > incur any delay other than the latency of the LOCKed (on x86) > > instruction to determine lock availability. > This makes sense to me -- Dario? > Yes, I think this patch is, after all, not really necessary. Thanks and Regards, Dario
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 9e8e561..50f8dfd 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -1361,7 +1361,7 @@ static void balance_load(const struct scheduler *ops, int cpu, s_time_t now) __update_runq_load(ops, st.lrqd, 0, now); -retry: + retry: if ( !spin_trylock(&prv->lock) ) return; @@ -1377,7 +1377,8 @@ retry: || !spin_trylock(&st.orqd->lock) ) continue; - __update_runq_load(ops, st.orqd, 0, now); + /* Use a value of NOW() sampled after taking orqd's lock. */ + __update_runq_load(ops, st.orqd, 0, NOW()); delta = st.lrqd->b_avgload - st.orqd->b_avgload; if ( delta < 0 ) @@ -1435,6 +1436,8 @@ retry: if ( unlikely(st.orqd->id < 0) ) goto out_up; + now = NOW(); + /* Look for "swap" which gives the best load average * FIXME: O(n^2)! */
Yet another situation very similar to 779511f4bf5ae ("sched: avoid races on time values read from NOW()"). In fact, when more than one runqueue is involved, we need to make sure that the following does not happen: 1. take the lock of 1st runq 2. now = NOW() 3. take the lock of 2nd runq 4. use now as, if we have to wait at step 3, the value in now may be stale when we get to use it at step 4. While there, fix the style of a label. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@citrix.com> Cc: Anshul Makkar <anshul.makkar@citrix.com> Cc: David Vrabel <david.vrabel@citrix.com> --- xen/common/sched_credit2.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)