Date: Sun, 12 Apr 2009 15:28:48 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: Luigi Rizzo <rizzo@iet.unipi.it> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: per-cpu counters (Re: svn commit: r190967 - head/sys/netinet) Message-ID: <alpine.BSF.2.00.0904121525540.19879@fledge.watson.org> In-Reply-To: <20090412143000.GB96365@onelab2.iet.unipi.it> References: <200904121406.n3CE6QSH027497@svn.freebsd.org> <20090412143000.GB96365@onelab2.iet.unipi.it>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 12 Apr 2009, Luigi Rizzo wrote: > On Sun, Apr 12, 2009 at 02:06:26PM +0000, Robert Watson wrote: > >> Update stats in struct pimstat using two new macros: PIMSTAT_ADD() >> and PIMSTAT_INC(), rather than directly manipulating the fields of >> the structure. This will make it easier to change the >> implementation of these statistics, such as using per-CPU versions >> of the data structure. >> >> MFC after: 3 days > > i understand that the change is only a cosmetic one, but do we really want > to MFC all these counter changes now ? Consider 3 days the minimum, rather than expected, MFC time. > The problem of per-cpu counters exist all over the kernel, so rather than > ad-hoc macros for each subsystem, it might be more interesting to first > design and discuss a global solution (using HEAD as a test platform, if you > like), and only at that stage start MFCing. > > As an example, an approach i was considering is replace the existing > counters with an index in a global_counters[MAXCPU][] array so the counter > update will become something like this: > > - pimstat.pims_rcv_registers_wrongiif++; > + global_counters[PCPU_GET(cpuid)] > + [pimstat.pims_rcv_registers_wrongiif]++; > > When you allocate an entry you also need to allocate an entry in the > global_counters, but entries have fixed size, and the modules that you build > will not depend on the MAXCPU value. > > All of this is still compatible with the macros you are adding, but I wonder > why we should have many PIMSTAT_INC, IPFW_INC, NATD_INC and so on instead of > more generic COUNTER64_INC() or COUNTER32_INC() I have a project along these lines in progress, and will post the proposal to arch@ once I've finished prototyping it. In particular, it provides common implementations of "reset" and "report" in order to expose a single userspace version of the structure via sysctl. The reason to use per-subsystem macros is that there's another dimmension here: virtualization. Some counters are per-CPU, others are per-VIMAGE, and many are per-CPU-per-VIMAGE. Since virtualization is a work-in-progress, and they haven't yet virtualized all the counter blocks, I figured it was best to push all the counter updates behind subsystem-specific macros, and then virtualization would touch only the macro definitions, not the implementation in every place. In the sample implementation you show above, changes have to be made to every instance of the counter when virtualization is added/removed/changed for PIM, making per-CPU changes hard to MFC. Robert N M Watson Computer Laboratory University of Cambridge
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.0904121525540.19879>