Date: Tue, 14 Jul 2020 08:38:39 -0500 From: Kyle Evans <kevans@freebsd.org> To: Edward Tomasz Napierala <trasz@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r353283 - in head: lib lib/libstats share/man/man3 share/mk sys/amd64/conf sys/conf sys/kern sys/sys tools/build/options Message-ID: <CACNAnaEiBgaaS-5Gc=bhPQmLNfMrm7ruK5yDrv-hM=aOYfpw4g@mail.gmail.com> In-Reply-To: <201910071905.x97J56t0039812@repo.freebsd.org> References: <201910071905.x97J56t0039812@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Oct 7, 2019 at 2:05 PM Edward Tomasz Napierala <trasz@freebsd.org> wrote: > > Author: trasz > Date: Mon Oct 7 19:05:05 2019 > New Revision: 353283 > URL: https://svnweb.freebsd.org/changeset/base/353283 > > Log: > Introduce stats(3), a flexible statistics gathering API. > > This provides a framework to define a template describing > a set of "variables of interest" and the intended way for > the framework to maintain them (for example the maximum, sum, > t-digest, or a combination thereof). Afterwards the user > code feeds in the raw data, and the framework maintains > these variables inside a user-provided, opaque stats blobs. > The framework also provides a way to selectively extract the > stats from the blobs. The stats(3) framework can be used in > both userspace and the kernel. > > See the stats(3) manual page for details. > > This will be used by the upcoming TCP statistics gathering code, > https://reviews.freebsd.org/D20655. > > The stats(3) framework is disabled by default for now, except > in the NOTES kernel (for QA); it is expected to be enabled > in amd64 GENERIC after a cool down period. > > Reviewed by: sef (earlier version) > Obtained from: Netflix > Relnotes: yes > Sponsored by: Klara Inc, Netflix > Differential Revision: https://reviews.freebsd.org/D20477 > > Added: > head/lib/libstats/ > head/lib/libstats/Makefile (contents, props changed) > head/share/man/man3/stats.3 (contents, props changed) > head/sys/kern/subr_stats.c (contents, props changed) > head/sys/sys/stats.h (contents, props changed) > head/tools/build/options/WITHOUT_STATS (contents, props changed) > head/tools/build/options/WITH_STATS (contents, props changed) > Modified: > head/lib/Makefile > head/share/man/man3/Makefile > head/share/man/man3/arb.3 > head/share/mk/bsd.libnames.mk > head/share/mk/src.libnames.mk > head/share/mk/src.opts.mk > head/sys/amd64/conf/NOTES > head/sys/conf/files > head/sys/conf/options > head/sys/sys/arb.h > > Modified: head/lib/Makefile > ============================================================================== > --- head/lib/Makefile Mon Oct 7 18:55:40 2019 (r353282) > +++ head/lib/Makefile Mon Oct 7 19:05:05 2019 (r353283) > @@ -152,6 +152,7 @@ SUBDIR.${MK_GSSAPI}+= libgssapi librpcsec_gss > SUBDIR.${MK_ICONV}+= libiconv_modules > SUBDIR.${MK_KERBEROS_SUPPORT}+= libcom_err > SUBDIR.${MK_LDNS}+= libldns > +SUBDIR.${MK_STATS}+= libstats > > # The libraries under libclang_rt can only be built by clang, and only make > # sense to build when clang is enabled at all. Furthermore, they can only be > > Added: head/lib/libstats/Makefile > ============================================================================== > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > +++ head/lib/libstats/Makefile Mon Oct 7 19:05:05 2019 (r353283) > @@ -0,0 +1,14 @@ > +# $FreeBSD$ > + > +LIB= stats > +SHLIBDIR?= /lib > +SHLIB_MAJOR= 0 > +SRCS= subr_stats.c > + > +# To debug, comment WITHOUT_ASSERT_DEBUG= and uncomment CFLAGS:= > +WITHOUT_ASSERT_DEBUG= > +#CFLAGS:=${CFLAGS:C/-O[0-9]/-O0 -g3/} -DDIAGNOSTIC > + Hi, What exactly is going on here? mjg pointed this out when we were looking at some runtime assertion related stuff. This looks like it's imposing an opinion of how it should be built and circumvent the normal way of doing things. Ideally, this would something more like with the following patch to just make sure that the CFLAGS manipulations properly happen when ASSERT_DEBUG is flipped on, and interested parties that don't want the assertions should turn ASSERT_DEBUG off. If there's a really really solid reason for libstats having its own knob, that should be considered as a formal knob rather than the ad-hockery that appears above. I'm not sure that the following patch is entirely correct, though; -DDIAGNOSTIC seems to be needed for assertions, but the -O$n replacement with -O0 -g3 looks like it should perhaps be split out to a different knob or.. something. diff --git a/lib/libstats/Makefile b/lib/libstats/Makefile index 89e10aa64c5..12345d1a61f 100644 --- a/lib/libstats/Makefile +++ b/lib/libstats/Makefile @@ -1,13 +1,16 @@ # $FreeBSD$ +.include <src.opts.mk> + LIB= stats SHLIBDIR?= /lib SHLIB_MAJOR= 0 SRCS= subr_stats.c tcp_stats.c # To debug, comment WITHOUT_ASSERT_DEBUG= and uncomment CFLAGS:= -WITHOUT_ASSERT_DEBUG= -#CFLAGS:=${CFLAGS:C/-O[0-9]/-O0 -g3/} -DDIAGNOSTIC +.if ${MK_ASSERT_DEBUG} != "no" +CFLAGS:=${CFLAGS:C/-O[0-9]/-O0 -g3/} -DDIAGNOSTIC +.endif .PATH: ${.CURDIR}/../../sys/kern ${.CURDIR}/../../sys/netinet
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaEiBgaaS-5Gc=bhPQmLNfMrm7ruK5yDrv-hM=aOYfpw4g>