Changeset View
Standalone View
sys/netinet/tcp_hostcache.c
| Show First 20 Lines • Show All 643 Lines • ▼ Show 20 Lines | if (req->oldptr == NULL) { | ||||
| return (SYSCTL_OUT(req, NULL, len)); | return (SYSCTL_OUT(req, NULL, len)); | ||||
| } | } | ||||
| error = sysctl_wire_old_buffer(req, 0); | error = sysctl_wire_old_buffer(req, 0); | ||||
| if (error != 0) { | if (error != 0) { | ||||
| return(error); | return(error); | ||||
| } | } | ||||
| /* Use a buffer for 16 lines */ | /* Use a buffer for 16 lines */ | ||||
tuexen: The comment needs to be updated. | |||||
| sbuf_new_for_sysctl(&sb, NULL, 16 * linesize, req); | sbuf_new_for_sysctl(&sb, NULL, 16 * linesize, req); | ||||
Done Inline ActionsDon't you want to use V_tcp_hostcache.bucket_limit instead of TCP_HOSTCACHE_BUCKETLIMIT here? tuexen: Don't you want to use `V_tcp_hostcache.bucket_limit` instead of `TCP_HOSTCACHE_BUCKETLIMIT`… | |||||
Done Inline ActionsI 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. rscheff: I have not checked if the hostcache tunables actually make a difference - as these all seem to… | |||||
| sbuf_printf(&sb, | sbuf_printf(&sb, | ||||
| "\nIP address MTU SSTRESH RTT RTTVAR " | "\nIP address MTU SSTRESH RTT RTTVAR " | ||||
| " CWND SENDPIPE RECVPIPE HITS UPD EXP\n"); | " CWND SENDPIPE RECVPIPE HITS UPD EXP\n"); | ||||
Done Inline ActionsI 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; tuexen: I would suggest a boolean variable `do_drain` or so here, if you want to optimise to not drain… | |||||
Done Inline Actionstraditionally, the base stack uses int for bool... rscheff: traditionally, the base stack uses int for bool... | |||||
Done Inline ActionsI 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
37But I leave it up to you. tuexen: I was told that new or modern code should use bool. At least there is some usage:
```… | |||||
Done Inline ActionsMichael 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. jhb: Michael is correct that for new code we prefer bool (the kernel assumes C99 now) to int. We… | |||||
| #define msec(u) (((u) + 500) / 1000) | #define msec(u) (((u) + 500) / 1000) | ||||
| len = 0; | |||||
| for (i = 0; i < V_tcp_hostcache.hashsize; i++) { | for (i = 0; i < V_tcp_hostcache.hashsize; i++) { | ||||
| THC_LOCK(&V_tcp_hostcache.hashbase[i].hch_mtx); | THC_LOCK(&V_tcp_hostcache.hashbase[i].hch_mtx); | ||||
Done Inline ActionsHere you would need to add do_drain = !TAILQ_EMPTY(&V_tcp_hostcache.hashbase[i].hch_bucket); tuexen: Here you would need to add
```
do_drain = !TAILQ_EMPTY(&V_tcp_hostcache.hashbase[i].hch_bucket)… | |||||
Done Inline ActionsI rather go with unconditional assignments, in the existing conditionals, not incurring additonal memory access beyond that. rscheff: I rather go with unconditional assignments, in the existing conditionals, not incurring… | |||||
| TAILQ_FOREACH(hc_entry, &V_tcp_hostcache.hashbase[i].hch_bucket, | TAILQ_FOREACH(hc_entry, &V_tcp_hostcache.hashbase[i].hch_bucket, | ||||
| rmx_q) { | rmx_q) { | ||||
Done Inline ActionsThis would then be just if (do_drain)
sbuf_drain(&sb);tuexen: This would then be just
```
if (do_drain)
sbuf_drain(&sb);
``` | |||||
| sbuf_printf(&sb, | sbuf_printf(&sb, | ||||
| "%-15s %5u %8u %6lums %6lums %8u %8u %8u %4lu " | "%-15s %5u %8u %6lums %6lums %8u %8u %8u %4lu " | ||||
| "%4lu %4i\n", | "%4lu %4i\n", | ||||
| hc_entry->ip4.s_addr ? | hc_entry->ip4.s_addr ? | ||||
| inet_ntoa_r(hc_entry->ip4, ip4buf) : | inet_ntoa_r(hc_entry->ip4, ip4buf) : | ||||
| #ifdef INET6 | #ifdef INET6 | ||||
| ip6_sprintf(ip6buf, &hc_entry->ip6), | ip6_sprintf(ip6buf, &hc_entry->ip6), | ||||
| #else | #else | ||||
| "IPv6?", | "IPv6?", | ||||
| #endif | #endif | ||||
| hc_entry->rmx_mtu, | hc_entry->rmx_mtu, | ||||
| hc_entry->rmx_ssthresh, | hc_entry->rmx_ssthresh, | ||||
| msec((u_long)hc_entry->rmx_rtt * | msec((u_long)hc_entry->rmx_rtt * | ||||
| (RTM_RTTUNIT / (hz * TCP_RTT_SCALE))), | (RTM_RTTUNIT / (hz * TCP_RTT_SCALE))), | ||||
| msec((u_long)hc_entry->rmx_rttvar * | msec((u_long)hc_entry->rmx_rttvar * | ||||
| (RTM_RTTUNIT / (hz * TCP_RTTVAR_SCALE))), | (RTM_RTTUNIT / (hz * TCP_RTTVAR_SCALE))), | ||||
| hc_entry->rmx_cwnd, | hc_entry->rmx_cwnd, | ||||
| hc_entry->rmx_sendpipe, | hc_entry->rmx_sendpipe, | ||||
| hc_entry->rmx_recvpipe, | hc_entry->rmx_recvpipe, | ||||
| hc_entry->rmx_hits, | hc_entry->rmx_hits, | ||||
| hc_entry->rmx_updates, | hc_entry->rmx_updates, | ||||
| hc_entry->rmx_expire); | hc_entry->rmx_expire); | ||||
| if (++len >= 16) { | |||||
Done Inline ActionsThis line could be removed. tuexen: This line could be removed. | |||||
| THC_UNLOCK(&V_tcp_hostcache.hashbase[i].hch_mtx); | |||||
| sbuf_drain(&sb); | |||||
| len = 0; | |||||
| THC_LOCK(&V_tcp_hostcache.hashbase[i].hch_mtx); | |||||
| } | } | ||||
| } | |||||
| THC_UNLOCK(&V_tcp_hostcache.hashbase[i].hch_mtx); | THC_UNLOCK(&V_tcp_hostcache.hashbase[i].hch_mtx); | ||||
| if (len > 0) | |||||
| sbuf_drain(&sb); | |||||
| len = 0; | |||||
| } | } | ||||
Done Inline ActionsI 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. jhb: I would find it more intuitive to call `sbuf_drain()` here after the unlock. Also, I don't… | |||||
Done Inline ActionsI like the unconditional sb_drain approach the most. Must have been some sleep depreviation that I didn't think of this. Thx. rscheff: I like the unconditional sb_drain approach the most. Must have been some sleep depreviation… | |||||
| #undef msec | #undef msec | ||||
| error = sbuf_finish(&sb); | error = sbuf_finish(&sb); | ||||
| sbuf_delete(&sb); | sbuf_delete(&sb); | ||||
| return(error); | return(error); | ||||
| } | } | ||||
| /* | /* | ||||
| * Caller has to make sure the curvnet is set properly. | * Caller has to make sure the curvnet is set properly. | ||||
| ▲ Show 20 Lines • Show All 66 Lines • Show Last 20 Lines | |||||
The comment needs to be updated.