Add TCP socket send and receive buffer size to procstat -f output.
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 9276 Build 9730: arc lint + arc unit
Event Timeline
| lib/libprocstat/libprocstat.h | ||
|---|---|---|
| 161 | This breaks libprocstat ABI. You need to provide compat symver symbols. Why do you use explicitly sized type for the new members ? | |
| usr.bin/procstat/procstat_files.c | ||
| 538 | printf is probably wrong there, since procstat(1) uses libxo. Also, why do you only provide the information when lengths are non-zero ? | |
| lib/libprocstat/libprocstat.h | ||
|---|---|---|
| 161 |
On CURRENT?
What type would you prefer? | |
| usr.bin/procstat/procstat_files.c | ||
| 538 |
Good catch. This patch came from ~BSD10.0 procstat, which predates libxo-ification. That said, I do not know how to use xo_emit so this will take some research.
It would be spammy for non-TCP sockets, which will just see zero values. Also many TCP connections may have zero values, which could be spammy. We could condition it on sock->proto == IPPROTO_TCP instead? | |
| lib/libprocstat/libprocstat.h | ||
|---|---|---|
| 161 | Sure, there are no differences between ABI stability requirements on CURRENT vs. on non-CURRENT. Symver-ed libraries must never change ABI of existing symbols. Non-symver-ed libraries can, but then the dso version must be incremented. Otherwise, we cannot provide backward ABI compatibility. WRT type, why not use u_int, same as the current kernel counter ? | |
| usr.bin/procstat/procstat_files.c | ||
| 538 | It is reasonable to only print queues for tcp sockets, might be for sctp, but I do not know. But for types of sockets where we do print the values, I think we should do it always. | |
| sys/sys/user.h | ||
|---|---|---|
| 379 | It seems awkward to split the kf_sock fields this way. Maybe 99% of the time you're only interested in sockbuf lengths for TCP sockets, but the kernel shouldn't be enforcing that policy. PF_LOCAL sockets may certainly have non-empty sockbufs, for instance. Why not collect the sockbuf sizes for all sockets and have procstat decide what to print? It can use kf_sock_protocol to determine whether a given socket uses TCP. And if someone later wants sockbuf sizes for, say, SCTP or SDP, it's a simple change to procstat(1). | |
| sys/sys/user.h | ||
|---|---|---|
| 379 | I did not commented on this at all because ino64 severely conflicts with the changes, both in sys/user.h and in libprocstat. In fact, after ino64 this should become much easier WRT at least libprocstat since compat11 is somewhat handled. I have a hope that ino64 is on its way to svn HEAD, so kinfo will be made over. | |
| sys/sys/user.h | ||
|---|---|---|
| 379 |
There aren't enough spare bits in the merged kf_sock structure to add the statistics, at least without more (ab)use of unions. Expanding kf_sock breaks kinfo_file_size ABI.
Yes, ino64 may make it easier to expand the kf_sock structure without splitting it like this. | |
| sys/sys/user.h | ||
|---|---|---|
| 379 | There are two spare ints in kf_sock: | |
Comment Actions
- Rebase on kib's ino64 branch to use the spare kf_sock fields and avoid conflicting changes; unsplit kf_sock.
- ABI in libprocstat still broken for now
- Set sendq/recvq for unix domain sockets too.
- Use xo_emit in procstat(1); condition on a streaming socket type.
Comment Actions
Attempting to build test on ino64 branch runs into some failures apparently unrelated to this patch:
/home/cem/freebsd/sys/kern/kern_descrip.c:1309:43: error: declaration of 'struct freebsd11_fstat_args' will not be visible outside of this function [-Werror,-Wvisibility]
freebsd11_fstat(struct thread *td, struct freebsd11_fstat_args *uap)
^Anyway, I would wait until ino64 lands to commit this (and test again then).
Comment Actions
You need to regen syscall tables both in sys/kern and in sys/compat32. This is noted in /testing.txt.
Otherwise changes looks good, but of course compat for libprocstat is still missing.
Comment Actions
I did try to do that. I may be invoking it wrong:
% make -C sys/kern syscalls cc -O2 -pipe syscalls.c -o syscalls /usr/lib/crt1.o: In function `_start': /usr/src/lib/csu/amd64/crt1.c:(.text+0x17b): undefined reference to `main' cc: error: linker command failed with exit code 1 (use -v to see invocation) *** Error code 1