On Thu, Jun 27, 2024 at 4:32 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 27 Jun 2024 19:19:40 -0400 Peter Xu peterx@redhat.com 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+.