Page MenuHomeFreeBSD

tcp: drain tcp_hostcache_list in between per-bucket locks
ClosedPublic

Authored by rscheff on Mar 29 2021, 9:57 AM.

Details

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

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

  • respect lock per bucket, adjust buffer to allow maximum bucket info
rscheff retitled this revision from tcp: drain tcp_hostcache_list while relinquishing locks to tcp: drain tcp_hostcache_list in between per-bucket locks.Mar 29 2021, 1:25 PM
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?

657

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);
663

This would then be just

if (do_drain)
        sbuf_drain(&sb);
686

This line could be removed.

rscheff marked 5 inline comments as done.
  • 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.

657

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.

sys/netinet/tcp_hostcache.c
657

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.

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
657

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.

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 edited the summary of this revision. (Show Details)
  • call sb_drain unconditionally
rscheff added inline comments.
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.

This revision is now accepted and ready to land.Mar 29 2021, 8:42 PM
This revision was automatically updated to reflect the committed changes.
rscheff marked an inline comment as done.