From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55947) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmnOj-0003PN-7s for qemu-devel@nongnu.org; Thu, 15 Oct 2015 14:37:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmnOf-0008S8-0B for qemu-devel@nongnu.org; Thu, 15 Oct 2015 14:37:05 -0400 Received: from mail-lf0-f52.google.com ([209.85.215.52]:35877) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmnOe-0008Rx-KX for qemu-devel@nongnu.org; Thu, 15 Oct 2015 14:37:00 -0400 Received: by lfeh64 with SMTP id h64so40138270lfe.3 for ; Thu, 15 Oct 2015 11:36:59 -0700 (PDT) References: <1444894224-9542-1-git-send-email-den@openvz.org> <1444894224-9542-2-git-send-email-den@openvz.org> <8737xc6o6w.fsf@linaro.org> <561FE515.7030402@openvz.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <561FE515.7030402@openvz.org> Date: Thu, 15 Oct 2015 19:36:57 +0100 Message-ID: <871tcw6ks6.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 1/3] log: improve performance of qemu_log and qemu_log_mask if disabled List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: qemu-devel@nongnu.org, Peter Maydell , Luiz Capitulino , Markus Armbruster , Pavel Butsykin Denis V. Lunev writes: > On 10/15/2015 08:23 PM, Alex Bennée wrote: >> Denis V. Lunev 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 >>> Signed-off-by: Pavel Butsykin >>> CC: Markus Armbruster >>> CC: Luiz Capitulino >>> CC: Eric Blake >>> CC: Peter Maydell >>> --- >>> 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. Yeah it has been sitting in my queue for a while as part of a larger series. I haven't been pushing it because the demand for the other changes isn't super high. Yes it would be better to do both qemu_log and qemu_log_mask. > The difference is that old patch has unlikely() hint and does not > contain break. I'm not sure what the break form brings. It's personal preference I guess but to me it is more natural to wrap things inside the condition then use break to skip out of a block. > 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? I'll certainly review the next submission. I also suggest you CC qemu-trivial once you have collected all your r-b and a-b/s-o-b tags. > > Den -- Alex Bennée