netdev - Re: [PATCH net-next 05/15] net: page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izP9ifXBuLs=q-tMcD-YC4o5R8gsG7B52LYGowetEaE4aw@mail.gmail.com>
Date: Wed, 8 Nov 2023 19:28:36 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, hawk@...nel.org, ilias.apalodimas@...aro.org
Subject: Re: [PATCH net-next 05/15] net: page_pool: record pools per netdev

On Wed, Oct 25, 2023 at 1:17 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 25 Oct 2023 12:56:44 -0700 Mina Almasry wrote:
> > > +#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 <almasrymina@...gle.com>

> > > @@ -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.
>
> > I'm guessing 1 is _always_ loopback?
>
> AFAIK, yes. I should probably use LOOPBACK_IFINDEX, to make it clearer.



-- 
Thanks,
Mina