On Sun, 16 Jun 2024 18:43:13 +0100, Pavel Begunkov wrote:
> On 6/17/24 04:12, Chenliang Li wrote:
>> On Sun, 16 Jun 2024 19:04:38 +0100, Pavel Begunkov wrote:
>>> On 5/14/24 08:54, Chenliang Li wrote:
>>>> Introduce helper functions to check whether a buffer can
>>>> be coalesced or not, and gather folio data for later use.
>>>>
>>>> The coalescing optimizes time and space consumption caused
>>>> by mapping and storing multi-hugepage fixed buffers.
>>>>
>>>> A coalescable multi-hugepage buffer should fully cover its folios
>>>> (except potentially the first and last one), and these folios should
>>>> have the same size. These requirements are for easier later process,
>>>> also we need same size'd chunks in io_import_fixed for fast iov_iter
>>>> adjust.
>>>>
>>>> Signed-off-by: Chenliang Li <cliang01.li@xxxxxxxxxxx>
>>>> ---
>>>> io_uring/rsrc.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>> io_uring/rsrc.h | 10 +++++++
>>>> 2 files changed, 88 insertions(+)
>>>>
>>>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>>>> index 65417c9553b1..d08224c0c5b0 100644
>>>> --- a/io_uring/rsrc.c
>>>> +++ b/io_uring/rsrc.c
>>>> @@ -871,6 +871,84 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
>>>> return ret;
>>>> }
>>>>
>>>> +static bool __io_sqe_buffer_try_coalesce(struct page **pages, int nr_pages,
>>>> + struct io_imu_folio_data *data)
>>> io_can_coalesce_buffer(), you're not actually trying to
>>> do it here.
>>
>> Will change it.
>>
>>>> +static bool io_sqe_buffer_try_coalesce(struct page **pages, int nr_pages,
>>>> + struct io_imu_folio_data *data)
>>>> +{
>>>> + int i, j;
>>>> +
>>>> + if (nr_pages <= 1 ||
>>>> + !__io_sqe_buffer_try_coalesce(pages, nr_pages, data))
>>>> + return false;
>>>> +
>>>> + /*
>>>> + * The pages are bound to the folio, it doesn't
>>>> + * actually unpin them but drops all but one reference,
>>>> + * which is usually put down by io_buffer_unmap().
>>>> + * Note, needs a better helper.
>>>> + */
>>>> + if (data->nr_pages_head > 1)
>>>> + unpin_user_pages(&pages[1], data->nr_pages_head - 1);
>>> Should be pages[0]. page[1] can be in another folio, and even
>>> though data->nr_pages_head > 1 protects against touching it,
>>> it's still flimsy.
>>
>> But here it is unpinning the tail pages inside those coalesceable folios,
>> I think we only unpin pages[0] when failure, am I right? And in
>> __io_sqe_buffer_try_coalesce we have ensured that pages[1:nr_head_pages] are
>> in same folio and contiguous.
>
> We want the entire folio to still be pinned, but don't want to
> leave just one reference and not care down the line how many
> refcounts / etc. you have to put down.
>
> void unpin_user_page(struct page *page)
> {
> sanity_check_pinned_pages(&page, 1);
> gup_put_folio(page_folio(page), 1, FOLL_PIN);
> }
>
> And all that goes to the folio as a single object, so doesn't
> really matter which page you pass. Anyway, let's then leave it
> as is then, I wish there would be unpin_folio_nr(), but there
> is unpin_user_page_range_dirty_lock() resembling it.
I see. Thanks for the explanation.
>>>> +
>>>> + j = data->nr_pages_head;
>>>> + nr_pages -= data->nr_pages_head;
>>>> + for (i = 1; i < data->nr_folios; i++) {
>>>> + unsigned int nr_unpin;
>>>> +
>>>> + nr_unpin = min_t(unsigned int, nr_pages - 1,
>>>> + data->nr_pages_mid - 1);
>>>> + if (nr_unpin == 0)
>>>> + break;
>>>> + unpin_user_pages(&pages[j+1], nr_unpin);
>>> same
>>>> + j += data->nr_pages_mid;
>>> And instead of duplicating this voodoo iteration later,
>>> please just assemble a new compacted ->nr_folios sized
>>> page array.
>>
>> Indeed, a new page array would make things a lot easier.
>> If alloc overhead is not a concern here, then yeah I'll change it.
>
> It's not, and the upside is reducing memory footprint,
> which would be noticeable with huge pages. It's also
> kvmalloc'ed, so compacting also improves the TLB situation.
OK, will use a new page array.
Thanks,
Chenliang Li
