tcp: Add hash histogram output and validate bucket length accounting

[rscheff]

Summary

The current hostcache hashing mechanism apparently
ends up with an very uneven distribution of the
entries into the hostcache hash array.

This was observed by adding a histogram output
function.

[rscheff]

• only the histogram output

[rscheff]

• don't leak memory

[tuexen]

I'm contemplating if it makes sense to just provide the V_tcp_hostcache.hashbase[i].hch_length for each bucket and allow to user to do whatever analysis is wanted. I think the focus should be an admin using the system, not a developer debugging it. So we don't need to to count the entries and provide both information. I guess that is only interesting for us right now...

[rscheff]

• remove validating hch_length vs. TALQ members

[rscheff]

In D29506#661668, @tuexen wrote:

I'm contemplating if it makes sense to just provide the V_tcp_hostcache.hashbase[i].hch_length for each bucket and allow to user to do whatever analysis is wanted. I think the focus should be an admin using the system, not a developer debugging it. So we don't need to to count the entries and provide both information. I guess that is only interesting for us right now...

Indeed, fair point. removing the (costly) validation against the actual bucket (tailq) length, since the lock around that counter is highly unlikely to have contributed to an issue for hostcache.count.

[tuexen]

sys/netinet/tcp_hostcache.c

735

I think you need V_tcp_hostcache.bucket_limit + 1 here.

745

The check needs to be <= instead of <. For consistency you might want to get the mutex.

746

What using a KASSERT V_tcp_hostcache.hashbase[i].hch_length <= V_tcp_hostcache.bucket_limit and then just update the array.

754

You need <= instead of <.

[rscheff]

• missed and off-by-one for the histogram size

[rscheff]

sys/netinet/tcp_hostcache.c

735

Indeed.

745

I want to avoid the cost of the LOCK/UNLOCK, at the cost of absolute accuracy. Similar to the hostcache.list - which will dump the contents of each hashindex at different times... The idea for this histogram is more to give a sense of the hash usage distribution (which doesn't need to be perfectly accurate). E.g. the jenkins_hash32 offers the possibility to change a salt. Or at some future point, a dynamic resizing (hash array width vs. depth) could be thought about.

746

Since this is an advisory output only, I rather hint at a non-catastrophic state (the bucket should no longer grow - but that could be monitored over time, if detected), than stopping the system. If such a invalid state needs to be frozen and analyzed, there are other ways to achieve this already.

[tuexen]

sys/netinet/tcp_hostcache.c

746

My point is that I consider the occurrence of hch_length > V_tcp_hostcache.bucket_limit a bug in the kernel. So I would put a message on the console, not include it in the sysctl output...

[rscheff]

• rebase to main

[tuexen]

sys/netinet/tcp_hostcache.c

730

Is this necessary if you would move the sbuf_new_for_sysctl() call down to be just before the first sbuf_printf() call?

742

Can't this be moved down after the computation of the histogram?

[rscheff]

• wiring the sbuf is not needed when no lock is held

[tuexen]

sys/netinet/tcp_hostcache.c

725

I guess this needs to be unsigned int, since hch_length is unsigned.

777

This part of the condition is always true, since hch_length is an unsigned int.

782

I would move the KASSERT up, just before it is decremented and check that it is positive and not exceeding the limit.

[tuexen]

Please note that you are not only adding the histogram features anymore, but also added verification code that hch_length is sane. You might to to reflect that in the commit message.

[rscheff]

• do more complete validation of hch_length