Date: Mon, 15 Sep 2014 12:34:33 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Alexander Motin <mav@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r271604 - head/sys/kern Message-ID: <20140915093433.GI2737@kib.kiev.ua> In-Reply-To: <5416ABB3.50808@FreeBSD.org> References: <201409142213.s8EMDJdM065051@svn.freebsd.org> <20140915085122.GF2737@kib.kiev.ua> <5416ABB3.50808@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--3j/5DwRpO0pwQS5+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 15, 2014 at 12:04:51PM +0300, Alexander Motin wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA512 >=20 > 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=20 > >> URL: http://svnweb.freebsd.org/changeset/base/271604 > >>=20 > >> Log: Add couple memory barries to serialize tdq_cpu_idle and > >> tdq_load accesses. > > Serialize what against what ? > >=20 > > 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 =3D 1 flushes write buffers, which makes the > > tdq_notify() to see the write immediately. Am I right ? >=20 > 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. >=20 > > If true, this must be commented to explain the stray barriers > > appearing in the code. > >=20 > >>=20 > >> 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. > >>=20 > >> MFC after: 3 days > >>=20 > >> Modified: head/sys/kern/sched_ule.c > >>=20 > >> Modified: head/sys/kern/sched_ule.c=20 > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > >> > >>=20 > - --- 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)=20 > >> @@ -1037,6 +1037,7 @@ tdq_notify(struct tdq *tdq, struct threa=20 > >> ctd =3D 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=20 > >> @@ -2640,6 +2641,7 @@ sched_idletd(void *dummy) > >>=20 > >> /* Run main MD idle handler. */ tdq->tdq_cpu_idle =3D 1; + mb();=20 > >> cpu_idle(switchcnt * 4 > sched_idlespinthresh); tdq->tdq_cpu_idle > >> =3D 0; > >>=20 > > 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. >=20 > 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. >=20 > > 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 =3D 1; @@ -2639,7 > > +2640,7 @@ sched_idletd(void *dummy) continue; > >=20 > > /* Run main MD idle handler. */ - tdq->tdq_cpu_idle =3D 1; + > > atomic_add_rel_int(&tdq->tdq_cpu_idle, 1); cpu_idle(switchcnt * 4 > > > sched_idlespinthresh); tdq->tdq_cpu_idle =3D 0; > >=20 > >=20 >=20 >=20 > - --=20 > Alexander Motin > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2 > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ >=20 > iQF8BAEBCgBmBQJUFquzXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w > ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRFOThDRjNDNEU2OUNDM0NEMEU1NzlENTU4 > MzE4QzM5NTVCQUIyMjdGAAoJEIMYw5VbqyJ/3LUIAMizmXrjgfoQBLhD25fKhP0w > XulDxS4L0xGE17xDW0MTL7BJ8A/xz/nolY8SQFbHe5/7jlueqqG2PoSokrQMHpYw > IZ0aYy5rQw+ZgR40zRFbTY1iNdZ7K9QsB6qweWQztSnBvLvlQ0Nx+/YgeDNuHNe1 > jWfJjmJLp3gz+/VQOUnHIBdslPS6YplOc5vQmc4NqFGhMplnhzWARJbLqxmYu7JK > HmeIP1uVxDEiG82TU+x4n21HehUi2POXAnygLrv0B4xqN4n8pYNyEtknesJiQO0t > lyrcXE2vWsDrLMrqCsaZc6pZEXqk8PJ4Og/v6S/XztV/FtrI5Ilqjz/i6e45b8Y=3D > =3Dtfah > -----END PGP SIGNATURE----- --3j/5DwRpO0pwQS5+ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUFrKpAAoJEJDCuSvBvK1BaMkP/RgRgf+3v0NQ0h/nbZsiSAoP kxBs01szunp8iJoECeKW/ARt/saMeykSMNs5LBs+R87pf3Q/jobSHTNADwdVfjAD 293e8AcA27IBk2B/Zkzb+4KkXE72xSpqsyhfNyCBVNq41u7VX+q86DfQ+KNjS5C3 fhOv84luFzqTnXjTyz7YyxUO1y0uiTz+BaYwR9t6Xu7Cf9mTIwlGO8Im70EeIaAW IQ+ChJDYa9eyP+b//QKiB7fGHqfP1pfZ2ldvtDur12Thi1nNkWrI6+dEXbJ/8yK3 xNNfIoR8DZO97CwgKAfqr1Sxja43AkqRls5erU4/KPX8A+BdhHiiOBkwfK0At40X qv5B2/dkBmWd1KBKZB2hn8pjDrnU6ba6B6CY2iL/xPMI/5AMpcg4KY+gujdy1TK7 ohzTQ5kaY+QBVY7fPEGQnmZuLGQ2wwS/L1AQWfulRjxDjLWBy0hZ+A/0+ZPoZbqx tOkrOckAxEPyWJUQHKM9Ebek5I1w31SKIQAWYUQhUZ5bsUcj15jW1v17WZDwnG1X 18OtW+UBePPGnGforSVHGtk2y01/ceYxo0Htc6qKhSy7OQwIQ7SsjBXRVq3n9E0i XtRi7PwOA5v8LbevbAsS5FiPMgeevx+RRN/MGzi77DHS4WOWdpQtZ4DNHwirqsv/ 5YusJmUFcXhlnvd4TpwS =nNOH -----END PGP SIGNATURE----- --3j/5DwRpO0pwQS5+--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140915093433.GI2737>