I agree. I don't think any of these three lines need to be visible to userland. I think they can safely move within the #ifdef _KERNEL block. I suspect that this is a little cruft which managed to sneak in during the 3+ years of development on this project.

Actually, they do need to be visible to userland in the current way things work, but the comment does possibly need to be expanded to explain why (or my arguably hacky way of doing things needs to be improved).

The underlying issue here is the need for statsblob templates defined in the kernel or userland to be shared across both domains, such that a statsblob generated in one domain based on a particular template can be manipulated in the other domain. By shared, I don't mean in a direct memory access sense, but the statsblob template needs to exist or be discoverable in both domains (or in future, perhaps across machines).

The only direction that matters in the current code being upstreamed is for templates defined in kernel, and then statsblobs based on a kernel-defined template being exported to userspace, and manipulated in userspace e.g. for rendering to JSON for logging purposes.

The "right" solution would be to have an API for exporting/importing statsblob templates. I punted on doing it the "right" way and cheated.

Regarding the TCP-related stats code, tcp_stats_init() in tcp_stats.c defines and registers the "TCP_DEFAULT" statsblob template together with its associated variables of interest. tcp_stats.c is compiled into the kernel, and tcp_stats_init() is called from tcp_init() so that the kernel statsblob template registry contains the "TCP_DEFAULT" template for use by the TCP stack.

kern/subr_stats.c is both compiled into the kernel and into a userland library (similar to the way sbuf(9) is built for both domains), and in order to have the stats templates match between the kernel and userland, I compile tcp_stats.c into the userland library too with tcp_stats_init() tagged as a library constructor so that the "TCP_DEFAULT" template is defined the same as what's in the kernel, and is registered on userland library load. As a consequence, tcp_stats.c has a bit of #ifdef goo to make compilation in userland work correctly, and these tcp_var.h bits need to be visible to make the userland library compile.

This scheme does mean that the kernel and userland can get out of sync if a change is made to the template but only one of the kernel or userland library are recompiled against the new code. In such a situation, statsblobs generated e.g. in kernel, would have a template hash id which differs from the templates known in userland, and the userland code would fail any statsblob manipulation operations that required the template to be known (return an errno).

In practice, this scheme works well and has not caused problems in the Netflix use case. It was always the plan to eventually implement the API for template export/import and clean this up when that work is done, but I don't think the API is a requirement for upstreaming.

Can you point me at the userland code which "reads" these things? Given one is a sysctl already I'd think it'd be easy to adjust that. I am only wondering if the code was originally written with VIMAGE in mind?

@bz Oops, sorry for being unclear. The userland library does not read or interact with any of these tcp_var.h variables in kernel. If you look in tcp_stats.c you'll see where this all comes together.

tcp_stats_init() is executed in both kernel, and separately in the userland library as a library constructor. V_tcp_perconn_stats_dflt_tpl is used in tcp_stats_init() and therefore needs to be resolvable for compilation of stats code in both kernel and userland. In the userland library it just resolves to a library-scope global integer - no relation/access to the kernel variable of same name.

V_tcp_perconn_stats_enable does not strictly speaking need to be visible to userland as it's only referenced from kernel code, but it doesn't hurt having it here and keeping these bits together (though I would also not object to it moving elsewhere).

FWIW, I think this dual-use (especially of the V_tcp_perconn_stats_dflt_tpl variable) makes things unclear. I think it would be nice to eliminate it, if possible, without adding code duplication. However, I understand that may not be possible.

I *suspect* you may be able to make the constructor be a static function in the user-land library. This would avoid the need to expose it via headers.

Also, as best I can tell, V_tcp_perconn_stats_dflt_tpl is not used in the userland library outside of tcp_stats_init. Is that true? If so, it might be possible to do a trivial rewrite of this function to use a local variable, and only update V_tcp_perconn_stats_dflt_tpl in the kernel.

I've moved it from a header into tcp_stats.c. Does that look ok?