Very nice in general, I'll apply the previous 8 but I'd like to offer
some alternatives here..
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.
> + 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;
> }
>
> if (!bulk_len)
You can invert this condition, and move all the code from here to the
out label into a small helper with just 3 params (pool, bulk, bulk_len).
Naming will be the tricky part but you can save us a bunch of gotos.
> - return;
> + goto out;
>
> /* Bulk producer into ptr_ring page_pool cache */
> in_softirq = page_pool_producer_lock(pool);
> for (i = 0; i < bulk_len; i++) {
> - if (__ptr_ring_produce(&pool->ring, (__force void *)data[i])) {
> + if (__ptr_ring_produce(&pool->ring, (__force void *)bulk[i])) {
> /* ring full */
> recycle_stat_inc(pool, ring_full);
> break;
> @@ -893,13 +915,22 @@ void page_pool_put_netmem_bulk(struct page_pool *pool, netmem_ref *data,
>
> /* Hopefully all pages was return into ptr_ring */
> if (likely(i == bulk_len))
> - return;
> + goto out;
>
> /* ptr_ring cache full, free remaining pages outside producer lock
> * since put_page() with refcnt == 1 can be an expensive operation
> */
> for (; i < bulk_len; i++)
> - page_pool_return_page(pool, data[i]);
> + page_pool_return_page(pool, bulk[i]);
> +
> +out:
> + if (!foreign)
> + return;
> +
> + count = foreign;
> + again = true;
> +
> + goto again;
> }
> EXPORT_SYMBOL(page_pool_put_netmem_bulk);
