On Thu, Jun 27, 2024 at 4:32 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 27 Jun 2024 19:19:40 -0400 Peter Xu <peterx@xxxxxxxxxx> wrote: > > > Yang, > > > > On Thu, Jun 27, 2024 at 03:14:13PM -0700, Yang Shi wrote: > > > The try_grab_folio() is supposed to be used in fast path and it elevates > > > folio refcount by using add ref unless zero. We are guaranteed to have > > > at least one stable reference in slow path, so the simple atomic add > > > could be used. The performance difference should be trivial, but the > > > misuse may be confusing and misleading. > > > > This first paragraph is IMHO misleading itself.. > > > > I think we should mention upfront the important bit, on the user impact. > > > > Here IMO the user impact should be: Linux may fail longterm pin in some > > releavnt paths when applied over CMA reserved blocks. And if to extend a > > bit, that include not only slow-gup but also the new memfd pinning, because > > both of them used try_grab_folio() which used to be only for fast-gup. > > It's still unclear how users will be affected. What do the *users* > see? If it's a slight slowdown, do we need to backport this at all? I think Peter meant the warning reported by yangge? Peter also mentioned the patch subject is misleading. I agree. So how's about "mm: gup: stop abusing try_grab_folio()"? > > > > > The patch itself looks mostly ok to me. > > > > There's still some "cleanup" part mangled together, e.g., the real meat > > should be avoiding the folio_is_longterm_pinnable() check in relevant > > paths. The rest (e.g. switch slow-gup / memfd pin to use folio_ref_add() > > not try_get_folio(), and renames) could be good cleanups. > > > > So a smaller fix might be doable, but again I don't have a strong opinion > > here. > > The smaller the better for backporting, of course. I view the fix to the warning as just by-product of the clean up. The whole patch is naturally integral IMHO. We can generate a smaller fix if it is too hard to backport. However, it should be ok since we just need to backport to 6.6+. >
