All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com,
	mkubecek@suse.cz, lorenzo@kernel.org,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Neil Brown <neilb@suse.de>
Subject: Re: [PATCH net-next 1/2] net: store netdevs in an xarray
Date: Mon, 24 Jul 2023 08:41:26 -0700	[thread overview]
Message-ID: <20230724084126.38d55715@kernel.org> (raw)
In-Reply-To: <20788d4df9bbcdce9453be3fd047fdf8e0465714.camel@redhat.com>

On Mon, 24 Jul 2023 10:18:04 +0200 Paolo Abeni wrote:
> A possibly dumb question: why using an xarray over a plain list?

We need to drop the lock during the walk. So for a list we'd need 
to either 
 - add explicit iteration "cursor" or 
 - suffer O(n) for insertion + O(n^2) for restart?

Or there's a easier way to do it?
The cursor is not the worst option, I guess, a bit less intuitive
and harder to clean up on error, but doable?

> It looks like the idea is to additionally use xarray for device lookup
> beyond for dumping?

I was measuring it to find out if we can delete the hash table without
anyone noticing, but it's not really the motivation.

> WRT the above, have you considered instead replacing dev_name_head with
> an rhashtable? (and add the mentioned list)

The main motivation is ease of iteration for netlink dumps.

I'll admit that my understanding of rhashtable is superficial but
I don't think it's possible to do consistent dumps with it. Even 
if it supported "cursors" (which I'm not sure it does) a rhash could
rearrange buckets and mess the order up. The comment on
rhltable_walk_enter() says:

 * For a completely stable walk you should construct your own data
 * structure outside the hash table.

Given the iteration process is controlled by user space trying to
constrain a rhash also used by the fast path to get consistent dumps
seems risky.

Adding Herbert and Neil to keep me honest.

  reply	other threads:[~2023-07-24 15:41 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-22  1:42 [PATCH net-next 0/2] net: store netdevs in an xarray Jakub Kicinski
2023-07-22  1:42 ` [PATCH net-next 1/2] " Jakub Kicinski
2023-07-22  1:47   ` Jakub Kicinski
2023-07-24  8:18   ` Paolo Abeni
2023-07-24 15:41     ` Jakub Kicinski [this message]
2023-07-24 16:23       ` Paolo Abeni
2023-07-24 17:27         ` Jakub Kicinski
2023-07-24 19:07           ` Jakub Kicinski
2023-07-25 11:11             ` Paolo Abeni
2023-07-25 16:56               ` Jakub Kicinski
2023-07-25 17:54             ` Sabrina Dubroca
2023-07-25 19:45               ` Jakub Kicinski
2023-07-24 19:09   ` Leon Romanovsky
2023-07-22  1:42 ` [PATCH net-next 2/2] net: convert some netlink netdev iterators to depend on the xarray Jakub Kicinski
2023-07-24 15:28 ` [PATCH net-next 0/2] net: store netdevs in an xarray Simon Horman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230724084126.38d55715@kernel.org \
    --to=kuba@kernel.org \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=lorenzo@kernel.org \
    --cc=mkubecek@suse.cz \
    --cc=neilb@suse.de \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.