Hi Anthony,
On Tue, Mar 12, 2024 at 07:10:42PM -0400, Anthony Deschamps wrote:
> Hi Willy,
>
> Thanks for the feedback. I had been testing with smaller numbers of
> servers (usually between 4 and 32) so I hadn't noticed the performance
> impact.
Not surprised. I'm used to testing with some extreme setups for knowing
such exist in field (more than 1000 servers in a single backend were
already reported for example).
> Here's an updated patch (as an attachment, until I figure out how
> to make sure things are formatted correctly!) that should address that.
Thanks. It's not a problem attached this way because my mailer still
recognizes it as text and inlines it when responding, so that doesn't
alter my ability to comment.
> I added a "lb_server_key" to the server struct. I moved most of the hashing
> logic that I previously had in "chash_compute_node_key" into a new function,
> "chash_compute_server_key", and I store that value; nodes only need to be
> removed and reinserted if this value changes. To compute a node key, I just
> combine the server key with a hash of node_index. I considered using
> XXH32_round to do this, but that looks like it's intended to be an internal
> function, so I inlined something similar instead.
You could always decide to combine an existing hash with another value, or
combine one 32-bit key with a 32-bit index and apply a 64-bit hash, but I
think that's fine this way.
> Hopefully this all works. Thanks for taking the time to work through this, and
> please let me know if there's anything else to address.
I just got this:
src/lb_chash.c: In function 'chash_compute_server_key':
src/lb_chash.c:115:4: error: a label can only be part of a statement and a
declaration is not a statement
compilation terminated due to -Wfatal-errors.
That's because of this:
> + switch (hash_key) {
> + case SRV_HASH_KEY_ADDR_PORT:
> + case SRV_HASH_KEY_ADDR:
> + switch (srv_addr.family) {
> + case AF_INET:
> + u32 addr_key = full_hash(srv_addr.addr.v4.s_addr);
Here that's a declaration after a statement ("case" not being considered
as the beginning of a block since you can pretty well fall through from
a previous one).
I'll adjust it while merging.
I had a concern about the hashing function which seems to reduce the
input key space, and indeed it does, it reduces 4294967296 server keys
to only 2948667289 (68.65 %):
> + key ^= addr_key + 0x9e3779b9 + (key << 6) + (key >> 2);
Not critical but it would be better if we can avoid this loss. I tried
with the full_hash() function and it preserves all output keys. So I
think you should instead use:
key = full_hash(add_key);
and chash_compute_node_key() could then simply be achieved this way:
static inline u32 chash_compute_node_key(struct server *s, unsigned
node_index)
{
return full_hash(s->lb_server_key + node_index);
}
Other than that, I'm OK with the rest.
Thanks!
Willy
