Message-ID: <Zl4m-sAhZknHOHdb@x1n>
Date: Mon, 3 Jun 2024 16:26:34 -0400
From: Peter Xu <peterx@...hat.com>
To: David Hildenbrand <david@...hat.com>
Cc: Yang Shi <shy828301@...il.com>,
	kernel test robot <oliver.sang@...el.com>,
	Jason Gunthorpe <jgg@...dia.com>,
	Vivek Kasireddy <vivek.kasireddy@...el.com>,
	Rik van Riel <riel@...riel.com>, oe-lkp@...ts.linux.dev,
	lkp@...el.com, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Matthew Wilcox <willy@...radead.org>,
	Christopher Lameter <cl@...ux.com>, linux-mm@...ck.org
Subject: Re: [linus:master] [mm] efa7df3e3b:
 kernel_BUG_at_include/linux/page_ref.h

On Mon, Jun 03, 2024 at 05:42:44PM +0200, David Hildenbrand wrote:
> On 03.06.24 17:08, Peter Xu wrote:
> > On Sat, Jun 01, 2024 at 08:22:21AM +0200, David Hildenbrand wrote:
> > > On 01.06.24 02:59, Yang Shi wrote:
> > > > On Fri, May 31, 2024 at 5:01 PM Yang Shi <shy828301@...il.com> wrote:
> > > > > 
> > > > > On Fri, May 31, 2024 at 4:25 PM Peter Xu <peterx@...hat.com> wrote:
> > > > > > 
> > > > > > On Fri, May 31, 2024 at 07:46:41PM +0200, David Hildenbrand wrote:
> > > > > > > try_grab_folio()->try_get_folio()->folio_ref_try_add_rcu()
> > > > > > > 
> > > > > > > Is called (mm-unstable) from:
> > > > > > > 
> > > > > > > (1) gup_fast function, here IRQs are disable
> > > > > > > (2) gup_hugepte(), possibly problematic
> > > > > > > (3) memfd_pin_folios(), possibly problematic
> > > > > > > (4) __get_user_pages(), likely problematic
> > > > > > > 
> > > > > > > (1) should be fine.
> > > > > > > 
> > > > > > > (2) is possibly problematic on the !fast path. If so, due to commit
> > > > > > >       a12083d721d7 ("mm/gup: handle hugepd for follow_page()") ? CCing Peter.
> > > > > > > 
> > > > > > > (3) is possibly wrong. CCing Vivek.
> > > > > > > 
> > > > > > > (4) is what we hit here
> > > > > > 
> > > > > > I guess it was overlooked because try_grab_folio() didn't have any comment
> > > > > > or implication on RCU or IRQ internal helpers being used, hence a bit
> > > > > > confusing.  E.g. it has different context requirement on try_grab_page(),
> > > > > > even though they look like sister functions.  It might be helpful to have a
> > > > > > better name, something like try_grab_folio_rcu() in this case.
> > > > > > 
> > > > > > Btw, none of above cases (2-4) have real bug, but we're looking at some way
> > > > > > to avoid triggering the sanity check, am I right?  I hope besides the host
> > > > > > splash I didn't overlook any other side effect this issue would cause, and
> > > > > > the splash IIUC should so far be benign, as either gup slow (2,4) or the
> > > > > > newly added memfd_pin_folios() (3) look like to have the refcount stablized
> > > > > > anyway.
> > > > > > 
> > > > > > Yang's patch in the other email looks sane to me, just that then we'll add
> > > > > > quite some code just to avoid this sanity check in paths 2-4 which seems
> > > > > > like an slight overkill.
> > > > > > 
> > > > > > One thing I'm thinking is whether folio_ref_try_add_rcu() can get rid of
> > > > > > its RCU limitation. It boils down to whether we can use atomic_add_unless()
> > > > > > on TINY_RCU / UP setup too?  I mean, we do plenty of similar things
> > > > > > (get_page_unless_zero, etc.) in generic code and I don't understand why
> > > > > > here we need to treat folio_ref_try_add_rcu() specially.
> > > > > > 
> > > > > > IOW, the assertions here we added:
> > > > > > 
> > > > > >           VM_BUG_ON(!in_atomic() && !irqs_disabled());
> > > > > > 
> > > > > > Is because we need atomicity of below sequences:
> > > > > > 
> > > > > >           VM_BUG_ON_FOLIO(folio_ref_count(folio) == 0, folio);
> > > > > >           folio_ref_add(folio, count);
> > > > > > 
> > > > > > But atomic ops avoids it.
> > > > > 
> > > > > Yeah, I didn't think of why atomic can't do it either. But is it
> > > > > written in this way because we want to catch the refcount == 0 case
> > > > > since it means a severe bug? Did we miss something?
> > > > 
> > > > Thought more about it and disassembled the code. IIUC, this is an
> > > > optimization for non-SMP kernel. When in rcu critical section or irq
> > > > is disabled, we just need an atomic add instruction.
> > > > folio_ref_add_unless() would yield more instructions, including branch
> > > > instruction. But I'm wondering how useful it would be nowadays. Is it
> > > > really worth the complexity? AFAIK, for example, ARM64 has not
> > > > supported non-SMP kernel for years.
> > > > 
> > > > My patch actually replaced all folio_ref_add_unless() to
> > > > folio_ref_add() for slow paths, so it is supposed to run faster, but
> > > > we are already in slow path, it may be not measurable at all. So
> > > > having more simple and readable code may outweigh the potential slight
> > > > performance gain in this case?
> > > 
> > > Yes, we don't want to use atomic RMW that return values where we can use
> > > atomic RMW that don't return values. The former is slower and implies a
> > > memory barrier, that can be optimized out on some arcitectures (arm64 IIRC)
> > > 
> > > We should clean that up here, and make it clearer that the old function is
> > > only for grabbing a folio if it can be freed concurrently -- GUP-fast.
> > 
> > Note again that this only affects TINY_RCU, which mostly implies
> > !PREEMPTION and UP.  It's a matter of whether we prefer adding these bunch
> > of code to optimize that.
> > 
> > Also we didn't yet measure that in a real workload and see how that
> > "unless" plays when buried in other paths, but then we'll need a special
> > kernel build first, and again I'm not sure whether it'll be worthwhile.
> 
> try_get_folio() is all about grabbing a folio that might get freed
> concurrently. That's why it calls folio_ref_try_add_rcu() and does
> complicated stuff.

IMHO we can define it.. e.g. try_get_page() wasn't defined as so.

If we want to be crystal clear on that and if we think that's what we want,
again I would suggest we rename it differently from try_get_page() to avoid
future misuses, then add documents. We may want to also even assert the
rcu/irq implications in try_get_folio() at entrance, then that'll be
detected even without TINY_RCU config.

> 
> On !CONFIG_TINY_RCU, it performs a folio_ref_add_unless(). That's
> essentially a atomic_add_unless(), which in the worst case ends up being a
> cmpxchg loop.
> 
> 
> Stating that we should be using try_get_folio() in paths where we are sure
> the folio refcount is not 0 is the same as using folio_try_get() where
> folio_get() would be sufficient.
> 
> The VM_BUG_ON in folio_ref_try_add_rcu() really tells us here that we are
> using a function in the wrong context, although in our case, it is safe to
> use (there is now BUG). Which is true, because we know we have a folio
> reference and can simply use a simple folio_ref_add().
> 
> Again, just like we have folio_get() and folio_try_get(), we should
> distinguish in GUP whether we are adding more reference to a folio (and
> effectively do what folio_get() would), or whether we are actually grabbing
> a folio that could be freed concurrently (what folio_try_get() would do).

Yes we can.  Again, IMHO it's a matter of whether it will worth it.

Note that even with SMP and even if we keep this code, the
atomic_add_unless only affects gup slow on THP only, and even with that
overhead it is much faster than before when that path was introduced.. and
per my previous experience we don't care too much there, really.

So it's literally only three paths that are relevant here on the "unless"
overhead:

  - gup slow on THP (I just added it; used to be even slower..)

  - vivik's new path

  - hugepd (which may be gone for good in a few months..)
  
IMHO none of them has perf concerns.  The real perf concern paths is
gup-fast when pgtable entry existed, but that must use atomic_add_unless()
anyway.  Even gup-slow !thp case won't regress as that uses try_get_page().

So again, IMHO the easist way to fix this WARN is we drop the TINY_RCU bit,
if nobody worries on UP perf.

I don't have a strong opinion, if any of us really worry about above three
use cases on "unless" overhead, and think it worthwhile to add the code to
support it, I won't object. But to me it's adding pain with no real benefit
we could ever measure, and adding complexity to backport too since we'll
need a fix for as old as 6.6.

Thanks,

-- 
Peter Xu

Powered by blists - more mailing lists