From owner-svn-src-head@freebsd.org Tue Jul 14 13:38:52 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id A785E363347; Tue, 14 Jul 2020 13:38:52 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4B5hSD0wHyz4dcr; Tue, 14 Jul 2020 13:38:52 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qk1-f171.google.com (mail-qk1-f171.google.com [209.85.222.171]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id E871618F0B; Tue, 14 Jul 2020 13:38:51 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qk1-f171.google.com with SMTP id j80so15570894qke.0; Tue, 14 Jul 2020 06:38:51 -0700 (PDT) X-Gm-Message-State: AOAM5317YNHf4rre4N3Njislck39QGxcUC1a1xEYhxzFSMVfhQ2O2tgN W1YDCYJuJFpGzKEYWepNIuDYzqO8mqgg8JrKUL0= X-Google-Smtp-Source: ABdhPJzBVn3XxNlmtup/r4z+dmoCMBmCgmRRqbXvyTqDJN7DYSoMLtUuhMOnkm26mDM8hmU6Q5Fi38WtcnKB6fhnvLo= X-Received: by 2002:a37:40e:: with SMTP id 14mr4519320qke.493.1594733931149; Tue, 14 Jul 2020 06:38:51 -0700 (PDT) MIME-Version: 1.0 References: <201910071905.x97J56t0039812@repo.freebsd.org> In-Reply-To: <201910071905.x97J56t0039812@repo.freebsd.org> From: Kyle Evans Date: Tue, 14 Jul 2020 08:38:39 -0500 X-Gmail-Original-Message-ID: Message-ID: 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 To: Edward Tomasz Napierala Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Jul 2020 13:38:52 -0000 On Mon, Oct 7, 2019 at 2:05 PM Edward Tomasz Napierala 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 + 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