procstat(1): Add TCP socket send/recv buffer size

[cem]

Summary

Add TCP socket send and receive buffer size to procstat -f output.

[kib]

lib/libprocstat/libprocstat.h

161 ↗

(On Diff #28263)

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 ↗

(On Diff #28263)

printf is probably wrong there, since procstat(1) uses libxo.

Also, why do you only provide the information when lengths are non-zero ?

[cem]

lib/libprocstat/libprocstat.h

161 ↗

(On Diff #28263)

This breaks libprocstat ABI. You need to provide compat symver symbols.

On CURRENT?

Why do you use explicitly sized type for the new members ?

What type would you prefer?

usr.bin/procstat/procstat_files.c

538 ↗

(On Diff #28263)

printf is probably wrong there, since procstat(1) uses libxo.

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.

Also, why do you only provide the information when lengths are non-zero ?

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?

[kib]

lib/libprocstat/libprocstat.h

161 ↗

(On Diff #28263)

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 ↗

(On Diff #28263)

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.

[markj]

sys/sys/user.h

377 ↗

(On Diff #28263)

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).

[kib]

sys/sys/user.h

377 ↗

(On Diff #28263)

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.

[cem]

sys/sys/user.h

377 ↗

(On Diff #28263)

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.

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.

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.

Yes, ino64 may make it easier to expand the kf_sock structure without splitting it like this.

[kib]

sys/sys/user.h

377 ↗

(On Diff #28263)

There are two spare ints in kf_sock:
https://github.com/FreeBSDFoundation/freebsd/blob/ino64/sys/sys/user.h#L345

[cem]

• 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.

[cem]

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).

[kib]

In D10689#222486, @cem wrote:

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)
                                          ^

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.

[cem]

In D10689#222497, @kib wrote:

You need to regen syscall tables both in sys/kern and in sys/compat32. This is noted in /testing.txt.

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

[kib]

In D10689#222507, @cem wrote:

In D10689#222497, @kib wrote:

You need to regen syscall tables both in sys/kern and in sys/compat32. This is noted in /testing.txt.

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

make -C sys/kern sysent
make -C sys/compat/freebsd32 sysent

[cem]

Thanks!

[cem]

• Rebase on committed ino64 work
• Add symver compatibility