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/16/2015 02:02 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> On 10/16/2015 10:17 AM, Markus Armbruster 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.
>>> Really?  We use __VA_ARGS__ all over the place, why won't it work here?
>> I have received warning like this
>>    "__VA_ARGS__ can only appear in the expansion of a C99 variadic macro"
>> with intermediate version of the patch.
>>
>> At the moment (with the current version) the replacement to __VA_ARGS__
>> works. Something strange has been happen. This syntax is definitely
>> better for me.
>>
>> Will change.
>>
>>>> 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)
>>> Feels stilted.  Like Alex's, I'd prefer something like
>>>
>>>       #define qemu_log(fmt, ...)				\
>>>           do {                                                \
>>>               if (unlikely(qemu_log_enabled())) {		\
>>>                   fprintf(qemu_logfile, fmt, ## __VA_ARGS__);	\
>>>               }						\
>>>           } while (0)
>>>
>>> I'm no fan of hiding qemu_logfile in qemu_log_enabled(), then using it
>>> directly to print to it, but that's a different conversation.
>> actually I am fine with any approach :) as there is no difference to me.
>> In general, this was taken from another project where I have
>> had more code below if. This is just an option to reduce indentation
>> to a meaningful piece of the code.
>>
>>> However, we already have
>>>
>>>       static inline void GCC_FMT_ATTR(1, 0)
>>>       qemu_log_vprintf(const char *fmt, va_list va)
>>>       {
>>>           if (qemu_logfile) {
>>>               vfprintf(qemu_logfile, fmt, va);
>>>           }
>>>       }
>>>
>>> Wouldn't static inline work for qemu_log(), too?
>> AFAIK no and the problem is that this could be compiler
>> specific.
>>
>> irbis ~ $ cat 1.c
>> #include <stdio.h>
>>
>> int f()
>> {
>>      return 1;
>> }
>>
>> static inline int test(int a, int b)
>> {
>>      if (a == 1) {
>>          printf("%d\n", b);
>>      }
>> }
>>
>> int main()
>> {
>>      test(2, f());
>>      return 0;
>> }
>> irbis ~ $
>>
>> 000000000040056b <main>:
>>    40056b:    55                       push   %rbp
>>    40056c:    48 89 e5                 mov    %rsp,%rbp
>>    40056f:    b8 00 00 00 00           mov    $0x0,%eax
>>    400574:    e8 bd ff ff ff           callq  400536 <f>
>>    400579:    89 c6                    mov    %eax,%esi
>>    40057b:    bf 02 00 00 00           mov    $0x2,%edi
>>    400580:    e8 bc ff ff ff           callq  400541 <test>
>>    400585:    b8 00 00 00 00           mov    $0x0,%eax
>>    40058a:    5d                       pop    %rbp
>>    40058b:    c3                       retq
>>    40058c:    0f 1f 40 00              nopl   0x0(%rax)
>>
>>
>> as you can see here f() is called before calling to test()
>>
>> Thus I feel that this inline should be replaced too ;)
> Well, what did you expect?  You asked the compiler to inline test(), and
> it inlined test().  You didn't ask it to inline f(), and it didn't
> inline f().
>
> [...]
you have asked whether 'static inline' will help
to avoid arguments evaluation. This does not happen,
compiler has not inlined inline function, (call test)
is here and arguments have been calculated.

For me this means that macros are better than
inline functions for logging functions. I do not
want to bother about interesting side-effects.

No log - no parameter evaluation under all
conditions.

Den