lkml.org 
[lkml]   [2022]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4] mm: fix is_pinnable_page against on cma page
    On Tue, May 17, 2022 at 01:12:02PM -0700, John Hubbard wrote:
    > On 5/17/22 12:28, Jason Gunthorpe wrote:
    > > > If you compare this to the snippet above, you'll see that there is
    > > > an extra mov statement, and that one dereferences a pointer from
    > > > %rax:
    > > >
    > > > mov (%rax),%rbx
    > >
    > > That is the same move as:
    > >
    > > mov 0x8(%rdx,%rax,8),%rbx
    > >
    > > Except that the EA calculation was done in advance and stored in rax.
    > >
    > > lea isn't a memory reference, it is just computing the pointer value
    > > that 0x8(%rdx,%rax,8) represents. ie the lea computes
    > >
    > > %rax = %rdx + %rax*8 + 6
    > >
    > > Which is then fed into the mov. Maybe it is an optimization to allow
    > > one pipe to do the shr and an other to the EA - IDK, seems like a
    > > random thing for the compiler to do.

    Maybe an optimization suppressed due to the volatile nature of the
    load? If so, perhaps it might be considered a compiler bug. Though
    it is quite difficult to get optimization bugs involving volatile
    to be taken seriously.

    > Apologies for getting that wrong, and thanks for walking me through the
    > asm.
    >
    > [...]
    > >
    > > Paul can correct me, but I understand we do not have a list of allowed
    > > operations that are exempted from the READ_ONCE() requirement. ie it
    > > is not just conditional branching that requires READ_ONCE().
    > >
    > > This is why READ_ONCE() must always be on the memory load, because the
    > > point is to sanitize away the uncertainty that comes with an unlocked
    > > read of unstable memory contents. READ_ONCE() samples the value in
    > > memory, and removes all tearing, multiload, etc "instability" that may
    > > effect down stream computations. In this way down stream compulations
    > > become reliable.
    > >
    > > Jason
    >
    > So then:

    Works for me!

    Acked-by: Paul E. McKenney <paulmck@kernel.org>

    Thanx, Paul

    > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
    > index 0e42038382c1..b404f87e2682 100644
    > --- a/mm/page_alloc.c
    > +++ b/mm/page_alloc.c
    > @@ -482,7 +482,12 @@ unsigned long __get_pfnblock_flags_mask(const struct page *page,
    > word_bitidx = bitidx / BITS_PER_LONG;
    > bitidx &= (BITS_PER_LONG-1);
    >
    > - word = bitmap[word_bitidx];
    > + /*
    > + * This races, without locks, with set_pageblock_migratetype(). Ensure
    > + * a consistent (non-tearing) read of the memory array, so that results,
    > + * even though racy, are not corrupted.
    > + */
    > + word = READ_ONCE(bitmap[word_bitidx]);
    > return (word >> bitidx) & mask;
    > }
    >
    >
    > thanks,
    > --
    > John Hubbard
    > NVIDIA

    \
     
     \ /
      Last update: 2022-05-17 22:22    [W:2.212 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site