Re: [PATCH V2] mm/gup: folio_split_user_page_pin

[David Hildenbrand <david@redhat.com>]

On 01.10.24 19:17, Steven Sistare wrote:
> On 9/27/2024 11:58 AM, Jason Gunthorpe wrote:
>> On Fri, Sep 27, 2024 at 05:44:52PM +0200, David Hildenbrand wrote:
>>> /**
>>>    * folio_try_add_pins() - add pins to an already-pinned folio
>>>    * @folio: the folio to add more pins to
>>>    *
>>>    * Try to add more pins to an already-pinned folio. The semantics
>>>    * of the pin (e.g., FOLL_WRITE) follow any existing pin and cannot
>>>    * be changed.
>>>    *
>>>    * This function is helpful when having obtained a pin on a large folio
>>>    * using memfd_pin_folios(), but wanting to logically unpin parts
>>>    * (e.g., individual pages) of the folio later, for example, using
>>>    * unpin_user_page_range_dirty_lock().
>>>    *
>>>    * This is not the right interface to initially pin a folio.
>>>    */
>>> int folio_try_add_pins(struct folio *folio, unsigned int pins)
>>> {
>>> 	VM_WARN_ON_ONCE(!folio_maybe_dma_pinned(folio));
>>>
>>> 	return try_grab_folio(folio, pins, FOLL_PIN);
>>> }
>>
>> That looks pretty good to me too
> 
> Looks good and passes my tests, I will add this version in V3 of the patch series.
> 
> Are you sure you want "try" in the name folio_try_add_pins?  Usually try means
> that any failure is transient and a future call may succeed

And now I took another look at the codebase and we already do have 
folio_add_pin() that adds a single pin, but continues on overflows (not 
sure I like that, but at least it can be caught and debugged).

So yes, we could simply turn folio_add_pin() into a wrapper around a 
folio_add_pins() that adds multiple pins.


Looking at folio_add_pin() vs. try_grab_folio() I am not sure if the 
open-coding the logic in folio_add_pin() got the NR_FOLL_PIN_ACQUIRED 
accounting correct.

-- 
Cheers,

David / dhildenb