Date: Thu, 5 Dec 2019 00:14:31 -0500 From: Alexander Motin <mav@FreeBSD.org> To: Ravi Pokala <rpokala@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r355412 - head/sys/geom Message-ID: <3e7a9cfe-46c6-73bd-3bd8-3e3c5daf52b9@FreeBSD.org> In-Reply-To: <516874DA-E57F-4EAA-852B-1129A95BB998@panasas.com> References: <201912050452.xB54qKV0080126@repo.freebsd.org> <516874DA-E57F-4EAA-852B-1129A95BB998@panasas.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi. On 05.12.2019 00:00, Ravi Pokala wrote: > -----Original Message----- > From: <owner-src-committers@freebsd.org> on behalf of Alexander Motin <mav@FreeBSD.org> > Date: 2019-12-04, Wednesday at 20:52 > To: <src-committers@freebsd.org>, <svn-src-all@freebsd.org>, <svn-src-head@freebsd.org> > Subject: svn commit: r355412 - head/sys/geom > > Author: mav > Date: Thu Dec 5 04:52:19 2019 > New Revision: 355412 > URL: https://svnweb.freebsd.org/changeset/base/355412 > > Log: > Wrap g_trace() into a macro to avoid unneeded calls. > > In most cases with debug disabled this function does nothing, but argument > passing and the call still cost measurable time due to cache misses, etc. > > Hi Alexander, > > I'm having trouble understanding this change, on a few levels. > > - Why did you add parentheses around the function declaration and definition? To make pre-processor not replace them also. > - How can that function with that name co-exist with a macro of the same name? The macro is handled first by pre-processor, compiler goes second on the result. > - Isn't the "g_debugflags" test in geom_dump.c:g_trace() now redundant? > - Why not simply convert geom_dump.c:g_trace() into a 'static inline' in geom.h, and not have to bother with the macro at all? One answer to both questions: compatibility with already built modules require the function to be present and behave same as before, while newly built code should be more efficient without changes. > What am I missing? > > Thanks, > > Ravi (rpokala@) > > MFC after: 2 weeks > Sponsored by: iXsystems, Inc. > > Modified: > head/sys/geom/geom.h > head/sys/geom/geom_dump.c > > Modified: head/sys/geom/geom.h > ============================================================================== > --- head/sys/geom/geom.h Thu Dec 5 04:18:22 2019 (r355411) > +++ head/sys/geom/geom.h Thu Dec 5 04:52:19 2019 (r355412) > @@ -255,11 +255,15 @@ void g_dev_physpath_changed(void); > struct g_provider *g_dev_getprovider(struct cdev *dev); > > /* geom_dump.c */ > -void g_trace(int level, const char *, ...); > +void (g_trace)(int level, const char *, ...) __printflike(2, 3); > # define G_T_TOPOLOGY 1 > # define G_T_BIO 2 > # define G_T_ACCESS 4 > - > +extern int g_debugflags; > +#define g_trace(level, fmt, ...) do { \ > + if (__predict_false(g_debugflags & (level))) \ > + (g_trace)(level, fmt, ## __VA_ARGS__); \ > +} while (0) > > /* geom_event.c */ > typedef void g_event_t(void *, int flag); > > Modified: head/sys/geom/geom_dump.c > ============================================================================== > --- head/sys/geom/geom_dump.c Thu Dec 5 04:18:22 2019 (r355411) > +++ head/sys/geom/geom_dump.c Thu Dec 5 04:52:19 2019 (r355412) > @@ -319,7 +319,7 @@ g_confxml(void *p, int flag) > } > > void > -g_trace(int level, const char *fmt, ...) > +(g_trace)(int level, const char *fmt, ...) > { > va_list ap; -- Alexander Motin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3e7a9cfe-46c6-73bd-3bd8-3e3c5daf52b9>