Re: [lkp] [ipc/sem.c] 5864a2fd30: aim9.shared_memory.ops_per_sec -13.0%
On 10/20/2016 02:21 AM, Andrew Morton wrote: On Wed, 19 Oct 2016 06:38:14 +0200 Manfred Spraul wrote: Hi, as discussed before: The root cause for the performance regression is the smp_mb() that was added into the fast path. I see two options: 1) switch to full spin_lock()/spin_unlock() for the rare codepath, then the fast path doesn't need the smp_mb() anymore. 2) confirm that no arch needs the smp_mb(), then remove it. - powerpc is ok after commit 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") - arm is ok after commit d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against concurrent lockers") - for x86 is ok after commit 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more") - for the remaining SMP architectures, I don't have a status. I would prefer the approach 1: The memory ordering provided by spin_lock()/spin_unlock() is clear. Thus: Attached are patches for approach 1: - Patch 1 replaces spin_unlock_wait() with spin_lock()/spin_unlock() and removes all memory barriers that are then unnecessary. - Patch 2 adds the hysteresis code: This makes the rare codepath extremely rare. It also corrects some wrong comments, e.g. regarding switching from global lock to per-sem lock (we "must' switch, not we "can" switch as written right now). The patches passed stress-testing. What do you think? Are you able to confirm that the performance issues are fixed? I don't know why we are now at -13%. The previous -9% were resolved by the patches: http://marc.info/?l=linux-kernel&m=147608082017551&w=2 My initial idea was to aim for 4.10, then we have more time to decide. I suppose I can slip these into -next and see what the effect is upon the Intel test results. But a) I don't know if they test linux-next(?) and b) I don't know where the test results are published, assuming they are published(?). -- Manfred
Re: [lkp] [ipc/sem.c] 5864a2fd30: aim9.shared_memory.ops_per_sec -13.0%
On Wed, 19 Oct 2016 06:38:14 +0200 Manfred Spraul wrote: > Hi, > > as discussed before: > The root cause for the performance regression is the smp_mb() that was > added into the fast path. > > I see two options: > 1) switch to full spin_lock()/spin_unlock() for the rare codepath, > then the fast path doesn't need the smp_mb() anymore. > > 2) confirm that no arch needs the smp_mb(), then remove it. > - powerpc is ok after commit > 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") > - arm is ok after commit > d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against > concurrent lockers") > - for x86 is ok after commit > 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more") > - for the remaining SMP architectures, I don't have a status. > > I would prefer the approach 1: > The memory ordering provided by spin_lock()/spin_unlock() is clear. > > Thus: > Attached are patches for approach 1: > > - Patch 1 replaces spin_unlock_wait() with spin_lock()/spin_unlock() and > removes all memory barriers that are then unnecessary. > > - Patch 2 adds the hysteresis code: This makes the rare codepath > extremely rare. > It also corrects some wrong comments, e.g. regarding switching > from global lock to per-sem lock (we "must' switch, not we "can" > switch as written right now). > > The patches passed stress-testing. > > What do you think? Are you able to confirm that the performance issues are fixed? > My initial idea was to aim for 4.10, then we have more time to decide. I suppose I can slip these into -next and see what the effect is upon the Intel test results. But a) I don't know if they test linux-next(?) and b) I don't know where the test results are published, assuming they are published(?).
Re: [lkp] [ipc/sem.c] 5864a2fd30: aim9.shared_memory.ops_per_sec -13.0%
Hi, as discussed before: The root cause for the performance regression is the smp_mb() that was added into the fast path. I see two options: 1) switch to full spin_lock()/spin_unlock() for the rare codepath, then the fast path doesn't need the smp_mb() anymore. 2) confirm that no arch needs the smp_mb(), then remove it. - powerpc is ok after commit 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") - arm is ok after commit d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against concurrent lockers") - for x86 is ok after commit 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more") - for the remaining SMP architectures, I don't have a status. I would prefer the approach 1: The memory ordering provided by spin_lock()/spin_unlock() is clear. Thus: Attached are patches for approach 1: - Patch 1 replaces spin_unlock_wait() with spin_lock()/spin_unlock() and removes all memory barriers that are then unnecessary. - Patch 2 adds the hysteresis code: This makes the rare codepath extremely rare. It also corrects some wrong comments, e.g. regarding switching from global lock to per-sem lock (we "must' switch, not we "can" switch as written right now). The patches passed stress-testing. What do you think? My initial idea was to aim for 4.10, then we have more time to decide. -- Manfred