tcp: drain tcp_hostcache_list in between per-bucket locks

[rscheff]

Summary

Explicitly drain the sbuf after completing each hash bucket
to minimize the work performed while holding the hash
bucket lock.

This may change the output to not give a complete
snapshot in time (it didn't before between hash buckets
either), but seems to be a reasonable trade off
between visibility and excessive locking. The latter
may prevent the use of stored TCPCB data for subsequent
sessions as long as the hostcache dump lasts - either
stalling the session setup, or proceeding with non-
optimal parameters.

Test Plan

PR: 254333
MFC after: 2 weeks

[rscheff]

• respect lock per bucket, adjust buffer to allow maximum bucket info

[tuexen]

sys/netinet/tcp_hostcache.c

651–652

The comment needs to be updated.

652–653

Don't you want to use V_tcp_hostcache.bucket_limit instead of TCP_HOSTCACHE_BUCKETLIMIT here?

658

I would suggest a boolean variable do_drain or so here, if you want to optimise to not drain for empty buckets. This line would read:

do_drain = true;

662

Here you would need to add

do_drain = !TAILQ_EMPTY(&V_tcp_hostcache.hashbase[i].hch_bucket);

664

This would then be just

if (do_drain)
        sbuf_drain(&sb);

687

This line could be removed.

[rscheff]

• use dedicated variable to track when to drain
• deal with variable sized hash buckets

sys/netinet/tcp_hostcache.c

652–653

I have not checked if the hostcache tunables actually make a difference - as these all seem to be initialized only once. But fair point to prepare here properly.

658

traditionally, the base stack uses int for bool...

662

I rather go with unconditional assignments, in the existing conditionals, not incurring additonal memory access beyond that.

[tuexen]

sys/netinet/tcp_hostcache.c

658

I was told that new or modern code should use bool. At least there is some usage:

tuexen@bsd6:~/freebsd-src/sys/netinet % grep bool tcp*.[ch] | wc -l
      37

But I leave it up to you.

[jhb]

FYI, the best way to indicate dependencies is to setup needed commits as "parent revisions" via "Edit Related Revisions" at the top-right (or use 'git arc' to upload changes together).

I might suggest tweaking the language in the first paragraph to something like:

"Explicitly drain the sbuf after completing each hash bucket to minimize the work performed while holding the hash bucket lock."

Also, I'm not sure if your change ensures that the drain is never called mid-bucket (that is, does the change to the sbuf size ensure it is always big enough for a single bucket? If it doesn't, you might want to add something like:

"For large buckets, the drain routine may still be called while walking the bucket."

sys/netinet/tcp_hostcache.c

658

Michael is correct that for new code we prefer bool (the kernel assumes C99 now) to int. We haven't done sweeps to replace existing ints with bools, but as new code is added (or if a bit of code is being refactored), we prefer the use of bool for booleans.

690

I would find it more intuitive to call sbuf_drain() here after the unlock. Also, I don't think this sysctl is really a hot-path, so you could also just call it unconditionally without needing the complexity of do_drain.

[rscheff]

In D29483#660649, @jhb wrote:

FYI, the best way to indicate dependencies is to setup needed commits as "parent revisions" via "Edit Related Revisions" at the top-right (or use 'git arc' to upload changes together).

Thanks, added.

I might suggest tweaking the language in the first paragraph to something like:

"Explicitly drain the sbuf after completing each hash bucket to minimize the work performed while holding the hash bucket lock."

Will do.

Also, I'm not sure if your change ensures that the drain is never called mid-bucket (that is, does the change to the sbuf size ensure it is always big enough for a single bucket? If it doesn't, you might want to add something like:

The reserved sbuf should be large enough - the line length (128) is larger than the typical printf line (91 char), and a bucket should not hold more than the prescribed number of maximum entries.

With the typical sizes (unchanged defaults), the new sbuf allocation is only 30*128 ~= 4kB, vs prior 15360*128 = 1.9 MB still achieving the goal of preventing excessively large blocking mallocs...

[rscheff]

• call sb_drain unconditionally

[rscheff]

sys/netinet/tcp_hostcache.c

690

I like the unconditional sb_drain approach the most. Must have been some sleep depreviation that I didn't think of this. Thx.