Re: [PATCH V2] mm/gup: folio_split_user_page_pin

[Steven Sistare <steven.sistare@oracle.com>]

On 10/4/2024 6:04 AM, David Hildenbrand wrote:
> 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.

To be clear, I am only suggesting that I use your folio_try_add_pins implementation, but
rename it to folio_add_pins.  And not touch the existing folio_add_pin.

I am ready to send V3 of the iommu_ioas_map_file series, and I would like to add this
patch back to the series as Jason requested.

- Steve