Re: [PATCH net-next 05/15] net: page_pool: record pools per netdev

On Wed, Oct 25, 2023 at 1:17 PM Jakub Kicinski [off-list ref] wrote:

On Wed, 25 Oct 2023 12:56:44 -0700 Mina Almasry wrote:

quoted

quoted

+#if IS_ENABLED(CONFIG_PAGE_POOL)
+       /** @page_pools: page pools created for this netdevice */
+       struct hlist_head       page_pools;
+#endif

I wonder if this per netdev field is really necessary. Is it not
possible to do the same simply looping over the  (global) page_pools
xarray? Or is that too silly of an idea. I guess on some systems you
may end up with 100s or 1000s of active or orphaned page pools and
then globally iterating over the whole page_pools xarray can be really
slow..

I think we want the per-netdev hlist either way, on netdev
unregistration we need to find its pools to clear the pointers.
At which point we can as well use that to dump the pools.

I don't see a strong reason to use one approach over the other.
Note that other objects like napi and queues (WIP patches) also walk
netdevs and dump sub-objects from them.

Sorry for the very late reply.

I'm not sure we need a per-netdev hlist either way, seems to me
looping over the global list and filterying by pp->netdev is the
equivalent of looping over the per-netdev hlist, I think.

The reason I was suggesting to only do the global list is because when
the same info is stored in 2 places you may end up with the info going
out of sync. Here, netdev->page_pools needs to match the entries in
each pp->netdev. But, I think your approach is more reasonable as
looping over the global array at all times may be a pain if there are
lots of page_pools on the system. I also can't find any issues with
how you're keeping netdev->page_pools & the page_pools->netdev, so
FWIW:

Reviewed-by: Mina Almasry <redacted>

quoted

quoted

@@ -48,6 +49,7 @@ struct pp_alloc_cache {
  * @pool_size: size of the ptr_ring
  * @nid:       NUMA node id to allocate from pages from
  * @dev:       device, for DMA pre-mapping purposes
+ * @netdev:    netdev this pool will serve (leave as NULL if none or multiple)

Is this an existing use case (page_pools that serve null or multiple
netdevs), or a future use case? My understanding is that currently
page_pools serve at most 1 rx-queue. Spot checking a few drivers that
seems to be true.

I think I saw one embedded driver for a switch-like device which has
queues servicing all ports, and therefore netdevs.
We'd need some help from people using such devices to figure out what
the right way to represent them is, and what extra bits of
functionality they need.

quoted

I'm guessing 1 is _always_ loopback?

AFAIK, yes. I should probably use LOOPBACK_IFINDEX, to make it clearer.

-- 
Thanks,
Mina