[cross posting this to ppc@] On Wed, May 19, 2021 at 12:27:51AM -0400, George Koehler wrote: > On Thu, 13 May 2021 02:20:45 -0400 > George Koehler <kern...@gmail.com> wrote: > > > My last diff (11 May 2021) still has a potential problem with memory > > barriers. I will mail a new diff if I think of a fix. > > Here's my new diff. It is a copy of what I sent to ppc@ a moment ago. > I would ask, "Is it ok to commit?", but while writing this mail, I > found a 2nd potential problem with memory barriers, so I will need to > edit this diff yet again.
Hi George, I tested this patch on sources from April 27th. The patch applied fine and the kernel booted fine on this hardware which I'll describe to you: hw.machine=macppc hw.model=970FX (Revision 0x300) hw.ncpu=2 hw.byteorder=4321 ... hw.cpuspeed=1800 hw.vendor=Apple Computer, Inc. hw.product=PowerMac7,3 hw.physmem=2147483648 hw.usermem=2147467264 hw.ncpufound=2 hw.allowpowerdown=1 hw.ncpuonline=2 I realise I'm a little out of sync between kernel and userland now. I'll try everything to better this once I figure out git to bring my tree up to -current: https://github.com/pbug44/openbsd , without affecting what I have for my macppc64 personal effort to bring this hardware to 64-bit OS. Best Regards, -peter > I add a 2nd membar_enter() to __ppc_lock() to avoid my 1st "potential > problem". I also simplify the code by changing __ppc_lock_spin to > check the owning cpu, not the count. > > A moment ago, I sent a copy of this diff to ppc@, in reply to another > report of bsd.mp hanging at "using parent ..." on a G5. My own hang > happened when the function __ppc_lock(), in a randomly reordered > bsd.mp, crossed a page boundary and caused a page fault with a > recursive call to __ppc_lock(). The goal of my diff is to prevent > such hangs, by allowing such recursive calls to work. > > I acquire the lock with an atomic 32-bit write, so the lock is always > in a valid state before or after the write. The old code did spin > until mpl_count == 0. In my earlier diffs, I acquired the lock by > setting both the owning cpu and a nonzero count in one write, so I > needed to pack both owner and count in a 32-bit integer. > > This diff spins until mpl_cpu == NULL. I acquire the lock by setting > only mpl_cpu and leaving mpl_count at 0. So mpl_cpu is the spinlock, > and mpl_count is a variable protected by the lock. I no longer need > to pack both fields in a 32-bit integer, so I get rid of my BOLT* > macros for packing and unpacking the fields. > > I need 2 memory barriers: > 1. after acquiring the lock, before accessing the locked data. > 2. after accessing the locked data, before releasing the lock. > > I added the 2nd membar_enter(), because I feared that a cpu would > acquire the lock, but get a page fault (and recursive call) before > it would reach the memory barrier. I didn't think of the opposite > case, where a cpu would do membar_exit(), but gets a page fault > before it would release the lock. This is the 2nd potential problem > that I didn't fix. I didn't observe an actual problem after running > this diff for more than 24 hours. > > --George [..]
