Date: Thu, 30 May 2024 17:25:43 +0200
From: Petr Machata <petrm@...dia.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>
CC: Petr Machata <petrm@...dia.com>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
 Abeni" <pabeni@...hat.com>, <netdev@...r.kernel.org>, Ido Schimmel
	<idosch@...dia.com>, David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net-next 0/4] Allow configuration of multipath hash seed


Nikolay Aleksandrov <razor@...ckwall.org> writes:

> I think that using memory management for such simple task is an
> overkill. Would it be simpler to define 2 x 4 byte seed variables
> in netns_ipv4 (e.g. user_seed, mp_seed). One is set only by the
> user through the sysctl, which would also set mp_seed. Then you
> can use mp_seed in the fast-path to construct that siphash key.
> If the user_seed is set to 0 then you reset to some static init
> hash value that is generated on init_net's creation. The idea

Currently the flow dissector siphash key is initialized lazily so that
the pool of random bytes is full when the initialization is done:

    https://lore.kernel.org/all/20131023180527.GC2403@order.stressinduktion.org
    https://lore.kernel.org/all/20131023111219.GA31531@order.stressinduktion.org

I'm not sure how important that is -- the mailing does not really
discuss much in the way of rationale, and admits it's not critical. But
initializing the seed during net init would undo that. At the same time,
initializing it lazily would be a bit of a mess, as we would have to
possibly retroactively update mp_hash as well, which would be racy vs.
user_hash update unless locked. So dunno.

If you are OK with giving up on the siphash key "quality", I'm fine with
this.

Alternatively I can keep the dispatch in like it currently is. I.e.:

	if (user_seed) {
		sip_hash = construct(user_seed)
		return flow_hash_from_keys_seed(sip_hash)
	} else {
		return flow_hash_from_keys()
	}

I wanted to have the flow dispatcher hash init early as well, as it made
the code branch-free like you note below, but then Ido dug out that
there are $reasons for how it's currently done.

> is to avoid leaking that initial seed, to have the same seed
> for all netns (known behaviour), be able to recognize when a
> seed was set and if the user sets a seed then overwrite it for
> that ns, but to be able to reset it as well.
> Since 32 bits are enough I don't see why we should be using
> the flow hash seed, note that init_net's initialization already

No deep reason in using the dissector hash as far as I'm concerned.
I just didn't want to change things arbitrarily, so kept the current
behavior except where I needed it to change.

> uses get_random_bytes() for hashes. This seems like a simpler
> scheme that doesn't require memory management for a 32 bit seed.
> Also it has the benefit that it will remove the test when generating
> a hash because in the initial/non-user-set case we just have the
> initial seed in mp_seed which is used to generate the siphash key,
> i.e. we always use that internal seed for the hash, regardless if
> it was set by the user or it's the initial seed.
>
> That's just one suggestion, if you decide to use more memory you
> can keep the whole key in netns_ipv4 instead, the point is I don't
> think we need memory management for this value.

I kept the RCU stuff in because it makes it easy to precompute the
siphash key while allowing readers to access it lock-free. I could
inline it and guard with a seqlock instead, but that's a bit messier
code-wise. Or indeed construct in-situ, it's an atomic access plus like
four instructions or something like that.

Powered by blists - more mailing lists