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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 38225 Build 35114: arc lint + arc unit
Event Timeline
Comment Actions
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...
Comment Actions
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.
| sys/netinet/tcp_hostcache.c | ||
|---|---|---|
| 728 | I think you need V_tcp_hostcache.bucket_limit + 1 here. | |
| 738 | The check needs to be <= instead of <. For consistency you might want to get the mutex. | |
| 739 | What using a KASSERT V_tcp_hostcache.hashbase[i].hch_length <= V_tcp_hostcache.bucket_limit and then just update the array. | |
| 747 | You need <= instead of <. | |
| sys/netinet/tcp_hostcache.c | ||
|---|---|---|
| 728 | Indeed. | |
| 738 | 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. | |
| 739 | 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. | |
| sys/netinet/tcp_hostcache.c | ||
|---|---|---|
| 739 | 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... | |
| sys/netinet/tcp_hostcache.c | ||
|---|---|---|
| 718 | 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. | |
| 777 | I would move the KASSERT up, just before it is decremented and check that it is positive and not exceeding the limit. | |
Comment Actions
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.