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@xxxxxxxxxx> 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
