svn commit: r271604 - head/sys/kern

[Konstantin Belousov]

On Mon, Sep 15, 2014 at 12:04:51PM +0300, Alexander Motin wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 15.09.2014 11:51, Konstantin Belousov wrote:
> > On Sun, Sep 14, 2014 at 10:13:19PM +0000, Alexander Motin wrote:
> >> Author: mav Date: Sun Sep 14 22:13:19 2014 New Revision: 271604 
> >> URL: http://svnweb.freebsd.org/changeset/base/271604
> >> 
> >> Log: Add couple memory barries to serialize tdq_cpu_idle and
> >> tdq_load accesses.
> > Serialize what against what ?
> > 
> > It seems what you do is just ensuring that the write to
> > tdq_cpu_idle in sched_idletd() become visible faster than it was
> > before your patch. Memory barrier after the assignment of
> > dq->tdq_cpu_idle = 1 flushes write buffers, which makes the
> > tdq_notify() to see the write immediately. Am I right ?
> 
> Right. My understanding of the problem is that tdq_notify() does not
> see updated tdq_cpu_idle in time, while cpu_idle() does not see
> updated tdq_load in time. From my experiments any one of those two
> barriers appear enough to fix the problem, but I tried to be safe.
> 
> > If true, this must be commented to explain the stray barriers
> > appearing in the code.
> > 
> >> 
> >> This change fixes transient performance drops in some of my
> >> benchmarks, vanishing as soon as I am trying to collect any stats
> >> from the scheduler. It looks like reordered access to those
> >> variables sometimes caused loss of IPI_PREEMPT, that delayed
> >> thread execution until some later interrupt.
> >> 
> >> MFC after:	3 days
> >> 
> >> Modified: head/sys/kern/sched_ule.c
> >> 
> >> Modified: head/sys/kern/sched_ule.c 
> >> ==============================================================================
> >>
> >> 
> - --- head/sys/kern/sched_ule.c	Sun Sep 14 22:10:35 2014	(r271603)
> >> +++ head/sys/kern/sched_ule.c	Sun Sep 14 22:13:19 2014	(r271604) 
> >> @@ -1037,6 +1037,7 @@ tdq_notify(struct tdq *tdq, struct threa 
> >> ctd = pcpu_find(cpu)->pc_curthread; if (!sched_shouldpreempt(pri,
> >> ctd->td_priority, 1)) return; +	mb(); if (TD_IS_IDLETHREAD(ctd))
> >> { /* * If the MD code has an idle wakeup routine try that before 
> >> @@ -2640,6 +2641,7 @@ sched_idletd(void *dummy)
> >> 
> >> /* Run main MD idle handler. */ tdq->tdq_cpu_idle = 1; +		mb(); 
> >> cpu_idle(switchcnt * 4 > sched_idlespinthresh); tdq->tdq_cpu_idle
> >> = 0;
> >> 
> > I suspect that what you are trying to do could be achieved by
> > using the FreeBSD API, instead of compat shims, in the following
> > way.
> 
> I was really trying to do it native way originally, but haven't found
> how. If I understand semantics right, I would need atomic_load_rel()
> and atomic_store_acq(), but there is no such ones. Second one you
> successfully replaced by atomic_add_rel(). But I am not sure about the
> first -- on x86 your idea should work since uses LOCK CMPXCHG, but on
> other platforms barrier is provided after the load, not before.
No, neither _acq/_rel, nor mb() do not provide formal semantic that
you want.  The write buffers (on x86), or read/write buffers on
other arches are the implementation detail.  The API of barriers
guarantees the visibility ordering of operations on other cores,
and is completely silent about visibility timing.

I.e., either your use of mb(), or store_rel() (it uses add_rel()
since store_rel() is without lock prefix due to TSO+ on x86) rely on
implementation details for x86.

This is why I do not see an advantage of using alien API of mb(),
and why I said that comment is required to explain what is going on.

> 
> > diff --git a/sys/kern/sched_ule.c b/sys/kern/sched_ule.c index
> > 0a63c01..1ffac22 100644 --- a/sys/kern/sched_ule.c +++
> > b/sys/kern/sched_ule.c @@ -1042,7 +1042,8 @@ tdq_notify(struct tdq
> > *tdq, struct thread *td) * If the MD code has an idle wakeup
> > routine try that before * falling back to IPI. */ -		if
> > (!tdq->tdq_cpu_idle || cpu_idle_wakeup(cpu)) +		if
> > (!atomic_load_acq_int(&tdq->tdq_cpu_idle) || +
> > cpu_idle_wakeup(cpu)) return; } tdq->tdq_ipipending = 1; @@ -2639,7
> > +2640,7 @@ sched_idletd(void *dummy) continue;
> > 
> > /* Run main MD idle handler. */ -		tdq->tdq_cpu_idle = 1; +
> > atomic_add_rel_int(&tdq->tdq_cpu_idle, 1); cpu_idle(switchcnt * 4 >
> > sched_idlespinthresh); tdq->tdq_cpu_idle = 0;
> > 
> > 
> 
> 
> - -- 
> Alexander Motin
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQF8BAEBCgBmBQJUFquzXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
> ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRFOThDRjNDNEU2OUNDM0NEMEU1NzlENTU4
> MzE4QzM5NTVCQUIyMjdGAAoJEIMYw5VbqyJ/3LUIAMizmXrjgfoQBLhD25fKhP0w
> XulDxS4L0xGE17xDW0MTL7BJ8A/xz/nolY8SQFbHe5/7jlueqqG2PoSokrQMHpYw
> IZ0aYy5rQw+ZgR40zRFbTY1iNdZ7K9QsB6qweWQztSnBvLvlQ0Nx+/YgeDNuHNe1
> jWfJjmJLp3gz+/VQOUnHIBdslPS6YplOc5vQmc4NqFGhMplnhzWARJbLqxmYu7JK
> HmeIP1uVxDEiG82TU+x4n21HehUi2POXAnygLrv0B4xqN4n8pYNyEtknesJiQO0t
> lyrcXE2vWsDrLMrqCsaZc6pZEXqk8PJ4Og/v6S/XztV/FtrI5Ilqjz/i6e45b8Y=
> =tfah
> -----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140915/c99394b9/attachment.sig>