Re: [PATCH net-next] rhashtable: further improve stability of rhashtable_walk

From: Herbert Xu <herbert@gondor.apana.org.au>
To: NeilBrown <neilb@suse.com>
Cc: Thomas Graf <tgraf@suug.ch>, Tom Herbert <tom@quantonium.net>,
	David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] rhashtable: further improve stability of rhashtable_walk
Date: Thu, 13 Dec 2018 09:43:09 +0800	[thread overview]
Message-ID: <20181213014309.uufaeoijhympgxbz@gondor.apana.org.au> (raw)
In-Reply-To: <87bm5ruo3f.fsf@notabene.neil.brown.name>

On Wed, Dec 12, 2018 at 07:49:08PM +1100, NeilBrown wrote:
> On Wed, Dec 12 2018, Herbert Xu wrote:
> 
> > On Wed, Dec 12, 2018 at 05:41:29PM +1100, NeilBrown wrote:
> >>
> >> So you would substantially slow down the rhashtable_walk_start() step.
> >
> > This whole thing is meant for uses such as /proc and netlink
> > enumeration.  Speed is definitely not a prerogative of these
> > iterators.
> 
> An RCU grace period is typically 10s of milliseconds.  It doesn't take
> very many of those to start being noticed.

Why would you even need an RCU grace period for deleting and adding
the walker object to the bucket list? You just allocate a new one
each time and free the old one after an RCU grace period.  I don't
see where the latency is coming from.

> > For that matter, if speed was an issue then surely you would
> > not drop out of the RCU read lock at all while iterating.
> 
> tipc_nl_sk_walk() drops out of RCU for every object.
> I don't know what it is used for, but I doubt that making it thousands
> of times slower would be a good thing.

Now you're conflating two different things.  Dropping the RCU
isn't necessarily slow.  We were talking about waiting for an
RCU grace period which would only come into play if you were
suspending the walk indefinitely.  Actually as I said above even
there you don't really need to wait.

> > It sounds to me like you're trying to use this interface for
> > something that it's simply not designed to do.
> 
> I'm just trying to make sure we have a reliable data structure which I
> can use without having to be worried that I might accidentally hit some
> unsupported behaviour.

Now that is something I agree with.

> I don't really see why you think my patch is all that complex.
> The only conceptual change is that hash chains are now ordered, and
> ordered chains are easy to understand.
> The biggest part of the code change is just moving some code from
> rhashtable_walk_start() to rhashtable_walk_next().
> 
> Why don't you like it?

My main beef is the fact that it doesn't work with rhlist.  So down
the track either we'll have to add more complexity for rhlist or
we will have to rip this out again do something completely different.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt