Message-ID: <eb0b843e-6da3-46a6-bf51-1c56f5919933@gmail.com>
Date: Thu, 9 Jan 2025 16:35:22 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Mina Almasry <almasrymina@...gle.com>, David Wei <dw@...idwei.uk>
Cc: io-uring@...r.kernel.org, netdev@...r.kernel.org,
 Jens Axboe <axboe@...nel.dk>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jesper Dangaard Brouer
 <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Stanislav Fomichev <stfomichev@...il.com>, Joe Damato <jdamato@...tly.com>,
 Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net-next v9 14/20] io_uring/zcrx: dma-map area for the
 device

On 1/7/25 20:23, Mina Almasry wrote:
> On Tue, Dec 17, 2024 at 4:38 PM David Wei <dw@...idwei.uk> wrote:
...
>> +
>> +               if (unlikely(niov_idx >= area->nia.num_niovs))
>> +                       continue;
>> +               niov_idx = array_index_nospec(niov_idx, area->nia.num_niovs);
>> +
>> +               niov = &area->nia.niovs[niov_idx];
>> +               if (!io_zcrx_put_niov_uref(niov))
>> +                       continue;
> 
> I have a suspicion that uref is now redundant in this series, although

It's not. You can't lose track of buffers given to the user. It plays
a similar role to devmem's ->sk_user_frags, think what happens if you
don't have it and don't track buffers in any other way.

> I'm not 100% sure. You seem to acquire a uref and pp_ref in tandem in
> io_zcrx_recv_frag and drop both in tandem in this function, which
> makes me think the uref maybe is redundant now.
> 
> io_zcrx_copy_chunk acquires a uref but not a pp_ref. I wonder if
> copy_chunk can do a page_pool_ref_netmem() instead of a uref, maybe

It takes both references.

> you would be able to make do without urefs at all. I have not looked
> at the copy fallback code closely.

If we're talking about optimisations, there is a way I described
and going to pursue, but that's not for the initial set.

>> +
>> +               netmem = net_iov_to_netmem(niov);
>> +               if (page_pool_unref_netmem(netmem, 1) != 0)
>> +                       continue;
>> +
>> +               if (unlikely(niov->pp != pp)) {
> 
>  From niov->pp != pp I surmise in this iteration one io_zcrx_area can
> serve niovs to multiple RX queues?

It should, but the main goal was rather to support multiple pools
per queue because of queue api shortcomings, even if it almost
never happens.

> The last 5 lines or so is basically doing  what page_pool_put_netmem()
> does, except there is a pp != niov->pp check in the middle. Can we
> call page_pool_put_netmem() directly if pp != niov->pp? It would just
> reduce the code duplication a bit and reduce the amount of custom
> reffing code we need to add for this mp.

Right, that sub path is basically page_pool_put_netmem(). Can be
replaced, but it's not going to de-duplicate code as the path is
shared with page_pool_mp_return_in_cache(). And it'd likely bloat
the binary a bit, though it's not that important.

>> +                       continue;
>> +               }
>> +
>> +               page_pool_mp_return_in_cache(pp, netmem);
> 
> So if niov->pp != pp, we end up basically doing a
> page_pool_put_netmem(), which is the 'correct' way to return a netmem
> to the page_pool, or at least is the way to return a netmem that all
> the other devmem/pages memory types uses. However if niov->pp == pp,
> we end up page_pool_mp_return_in_cache(), which is basically the same
> as page_pool_put_unrefed_netmem but skips the ptr_ring, so it's
> slightly faster and less overhead.

Jumping through the loops is surely not great, but there are bigger
semantical reasons. page_pool_put_netmem() has always been called by
users from the outside, this one is off the page pool allocation path.
For example, it'd nest io_uring with ptr_ring, which is not a bug and
not so bad, but that's something you'd need to always consider while
patching generic page pool. On top with the ptr_ring path in there,
either providers would need to make implicit assumptions that it'd
never happen, which is shabby, or the code should be prepared to that.
I'd say it should be more convenient to have a separate and simple
for all that.

We can play with the API more, hopefully after it's merged, but
just replacing it with page_pool_put_netmem() would do a disservice.

> I would honestly elect to page_pool_put_netmem() regardless of
> niov->pp/pp check. Sure it would be a bit more overhead than the code
> here, but it would reduce the custom pp refing code we need to add for
> this mp and it will replenish the ptr_ring in both cases, which may be
> even faster by reducing the number of times we need to replenish. We
> can always add micro optimizations like skipping the ptr_ring for
> slightly faster code if there is evidence there is significant perf
> improvement.
> 
>> +       } while (--entries);
>> +
>> +       smp_store_release(&ifq->rq_ring->head, ifq->cached_rq_head);
>> +       spin_unlock_bh(&ifq->rq_lock);
>> +}
>> +
...
>> +static bool io_pp_zc_release_netmem(struct page_pool *pp, netmem_ref netmem)
>> +{
>> +       if (WARN_ON_ONCE(!netmem_is_net_iov(netmem)))
>> +               return false;
>> +
>> +       if (page_pool_unref_netmem(netmem, 1) == 0)
> 
> Check is redundant, AFAICT. pp would never release a netmem unless the
> pp refcount is 1.

Good catch, it was applied to v10

-- 
Pavel Begunkov

Powered by blists - more mailing lists