On Mon, Jan 09, 2023 at 03:44:54PM -0800, Yang Shi wrote:
> On Mon, Jan 9, 2023 at 3:11 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > 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().
>
> It is a test_bit() function. Is it possible clear_bit() is reordered
> with test_bit(), or test_bit() doesn't see the result from
> clear_bit()?
If the various calls to test_bit() and clear_bit() are to the same
location, then they will not be reordered with each other.
If they are to different locations, they can be reordered. But in that
case, they would not see each others' results anyway.
> > > > 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.
>
> So, it should look like IIUC:
>
> clear_bit()
> smp_load_acquire()
> ...
> if (test_bit()) {
> smp_store_release()
> load_1
> store_1
> load_2
> store_2
> }
Just so you know, smp_load_acquire() does a load and smp_store_release()
does a store.
Also, is this code executed by a single CPU/task? If so, you need to
also consider the corresponding code executed by some other CPU/task.
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
