svn commit: r355412 - head/sys/geom

[Alexander Motin]

Hi.

On 05.12.2019 00:00, Ravi Pokala wrote:
> -----Original Message-----
> From: <owner-src-committers at freebsd.org> on behalf of Alexander Motin <mav at FreeBSD.org>
> Date: 2019-12-04, Wednesday at 20:52
> To: <src-committers at freebsd.org>, <svn-src-all at freebsd.org>, <svn-src-head at 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