From: Jakub Kicinski <kuba@xxxxxxxxxx>
Date: Thu, 5 Dec 2024 18:40:16 -0800
> Very nice in general, I'll apply the previous 8 but I'd like to offer
> some alternatives here..
Great suggestions, I addressed most of them already and the function
looks much better now.
One note below.
>
> On Tue, 3 Dec 2024 18:37:32 +0100 Alexander Lobakin wrote:
>> +void page_pool_put_netmem_bulk(netmem_ref *data, u32 count)
>> {
>> - int i, bulk_len = 0;
>> - bool allow_direct;
>> - bool in_softirq;
>> + bool allow_direct, in_softirq, again = false;
>> + netmem_ref bulk[XDP_BULK_QUEUE_SIZE];
>> + u32 i, bulk_len, foreign;
>> + struct page_pool *pool;
>>
>> - allow_direct = page_pool_napi_local(pool);
>> +again:
>> + pool = NULL;
>> + bulk_len = 0;
>> + foreign = 0;
>>
>> for (i = 0; i < count; i++) {
>> - netmem_ref netmem = netmem_compound_head(data[i]);
>> + struct page_pool *netmem_pp;
>> + netmem_ref netmem;
>> +
>> + if (!again) {
>> + netmem = netmem_compound_head(data[i]);
>>
>> - /* It is not the last user for the page frag case */
>> - if (!page_pool_is_last_ref(netmem))
>> + /* It is not the last user for the page frag case */
>> + if (!page_pool_is_last_ref(netmem))
>> + continue;
>
> We check the "again" condition potentially n^2 times, is it written
> this way because we expect no mixing? Would it not be fewer cycles
> to do a first pass, convert all buffers to heads, filter out all
> non-last refs, and delete the "again" check?
>
> Minor benefit is that it removes a few of the long lines so it'd be
> feasible to drop the "goto again" as well and just turn this function
> into a while (count) loop.
>
>> + } else {
>> + netmem = data[i];
>> + }
>> +
>> + netmem_pp = netmem_get_pp(netmem);
>
> nit: netmem_pp is not a great name. Ain't nothing especially netmem
> about it, it's just the _current_ page pool.
It's the page_pool of the @netmem we're processing on this iteration.
"This netmem's PP" => netmem_pp.
Current page_pool which we'll use for recycling is @pool.
>
>> + if (unlikely(!pool)) {
>> + pool = netmem_pp;
>> + allow_direct = page_pool_napi_local(pool);
>> + } else if (netmem_pp != pool) {
>> + /*
>> + * If the netmem belongs to a different page_pool, save
>> + * it for another round after the main loop.
>> + */
>> + data[foreign++] = netmem;
>> continue;
>> + }
>>
>> netmem = __page_pool_put_page(pool, netmem, -1, allow_direct);
>> /* Approved for bulk recycling in ptr_ring cache */
>> if (netmem)
>> - data[bulk_len++] = netmem;
>> + bulk[bulk_len++] = netmem;
>> }
Thanks,
Olek
