On 04.10.24 19:20, Steven Sistare wrote:
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.
This should be unified/cleaned up at some point.
@Nico, interested in looking into this? Otherwise, I can add it to my
todo list.
--
Cheers,
David / dhildenb
