On Mon, Jan 09, 2023 at 02:08:35PM -0800, Yang Shi wrote:
> On Mon, Jan 9, 2023 at 9:31 AM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 09, 2023 at 09:14:19AM -0800, Yang Shi wrote:
> > > Hi Paul,
> > >
> > > Hope this email finds you are doing well. I recently ran into a
> > > problem which might be related to control dependency of the memory
> > > model. Conceptually, the code does (from copy_present_pte()):
> > >
> > > acquire mmap_lock
> > > spin_lock
> > > ...
> > > clear bit (a bit in page flags)
> > > ...
> > > VM_BUG_ON(test bit)
> > > ...
> > > spin_unlock
> > > release mmap_lock
> > >
> > >
> > > IIUC there is control dependency between the "clear bit" and
> > > "VM_BUG_ON" since VM_BUG_ON simply tests the bit then raises the BUG.
> > > They do touch the overlapping address (the page flags from the same
> > > struct page), but they are bit field operations. Per the memory model
> > > documentation, the order is not guaranteed for bit field operations
> > > IIRC.
> > >
> > > And there are not any implicit barriers between clear bit and test
> > > bit, so the question is whether an explicit barrier, for example,
> > > smp_mb__after_atomic() is required after clear bit to guarantee it
> > > works as expected?
> >
> > I am not familiar with this code, so I will stick with LKMM
> > clarifications.
>
> Yeah, sure. This is why I tried to generalize the code.
>
> > First, please don't forget any protection and ordering that might be
> > provided by the two locks held across this code.
>
> Yes, but for this case I just care about the code between clear bit
> and VM_BUG_ON.
Fair enough!
> > Second, a control dependency extends from a READ_ONCE() or stronger
> > (clear_bit() included) to a later store. Please note "store", not
> > "load". If you need to order an earlier READ_ONCE() or clear_bit()
>
> So you mean:
>
> clear bit
> ...
> if (test bit) {
> load_1
> store_1
> load_2
> store_2
> }
>
> The dependency reaches to the first store?
It reaches both stores, but neither load.
That means that your example might well execute as if it had instead
been written as follows:
load_1
load_2
if (test bit) {
store_1
store_2
}
Assuming that you mean the test_bit() function. If you instead mean
a C-language statement that tests a bit, then the compiler can do all
sorts of things to you. The compiler can also do interesting things
to you if the stores are plain C-language stores instead of something
like WRITE_ONCE().
> > with a later load, you will need acquire semantics (smp_load_acquire(),
> > for example) or an explicit barrier such as smp_rmb(). Use of acquire
> > semantics almost always gets you code that is more readable.
>
> Does the load acquire have to pair with a smp_store_release()?
> smp_mb__after_stomic() is not needed because it is too strong and the
> weaker barrier is good enough, right?
It needs to pair with some type of applicable ordering, but yes,
smp_store_release() is a good one.
Thanx, Paul
> > Does that help?
>
> Yeah, sure. Thanks.
>
> >
> > Also CCing linux-mm@xxxxxxxxx in case someone with better understanding
> > of that code has advice.
> >
> > Thanx, Paul
