Re: [Qemu-devel] [PATCH 1/3] log: improve performance of qemu_log and qemu_log_mask if disabled

["Denis V. Lunev" <den@openvz.org>]

On 10/15/2015 08:23 PM, Alex Bennée wrote:
> Denis V. Lunev <den@openvz.org> writes:
>
>> The patch is intended to avoid to perform any operation including
>> calculation of log function arguments when the log is not enabled due to
>> various reasons.
>>
>> Functions qemu_log and qemu_log_mask are replaced with variadic macros.
>> Unfortunately the code is not C99 compatible and we can not use
>> portable __VA_ARGS__ way. There are a lot of warnings in the other
>> places with --std=c99. Thus the only way to achive the result is to use
>> args.. GCC extension.
>>
>> Format checking performed by compiler will not suffer by this patch. It
>> will be done inside in fprintf arguments checking.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Luiz Capitulino <lcapitulino@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   include/qemu/log.h | 17 ++++++++++++++---
>>   qemu-log.c         | 21 ---------------------
>>   2 files changed, 14 insertions(+), 24 deletions(-)
>>
>> diff --git a/include/qemu/log.h b/include/qemu/log.h
>> index f880e66..57b8c96 100644
>> --- a/include/qemu/log.h
>> +++ b/include/qemu/log.h
>> @@ -53,7 +53,13 @@ static inline bool qemu_loglevel_mask(int mask)
>>   
>>   /* main logging function
>>    */
>> -void GCC_FMT_ATTR(1, 2) qemu_log(const char *fmt, ...);
>> +#define qemu_log(args...)                   \
>> +    do {                                    \
>> +        if (!qemu_log_enabled()) {          \
>> +            break;                          \
>> +        }                                   \
>> +        fprintf(qemu_logfile, args);        \
>> +    } while (0)
>>
> I've had one of Peter's patches in my logging improvements queue for a
> while although it uses a slightly different form which I prefer:
>
> -/* log only if a bit is set on the current loglevel mask
> +/* log only if a bit is set on the current loglevel mask:
> + * @mask: bit to check in the mask
> + * @fmt: printf-style format string
> + * @args: optional arguments for format string
>    */
> -void GCC_FMT_ATTR(2, 3) qemu_log_mask(int mask, const char *fmt, ...);
> -
> +#define qemu_log_mask(MASK, FMT, ...)                   \
> +    do {                                                \
> +        if (unlikely(qemu_loglevel_mask(MASK))) {       \
> +            qemu_log(FMT, ## __VA_ARGS__);              \
> +        }                                               \
>
> See the message:
>
> qemu-log: Avoid function call for disabled qemu_log_mask logging

as far as I can see that patch is one year old at least
and my version is slightly better, it does the same for
qemu_log.

The difference is that old patch has unlikely() hint and does not
contain break.

I can revert the condition and add the hint in a couple
of minutes if this will increase the chance to get
both things merged. We should have both functions
as macros.

Will you accept that?

Den