Re: [QUESTION] Linux memory model: control dependency with bitfield

[Yang Shi <shy828301@gmail.com>]

On Mon, Jan 9, 2023 at 3:11 PM Paul E. McKenney <paulmck@kernel.org> 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@kernel.org> 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()?

>
> > > 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
}

>
>                                                         Thanx, Paul
>
> > > Does that help?
> >
> > Yeah, sure. Thanks.
> >
> > >
> > > Also CCing linux-mm@kvack.org in case someone with better understanding
> > > of that code has advice.
> > >
> > >                                                         Thanx, Paul