From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751694AbcGKTXb (ORCPT <rfc822;w@1wt.eu>);
	Mon, 11 Jul 2016 15:23:31 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:41183 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750726AbcGKTXa (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 11 Jul 2016 15:23:30 -0400
Date: Mon, 11 Jul 2016 12:23:28 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Jason Baron <jbaron@akamai.com>
Cc: <joe@perches.com>, <peterz@infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 7/7] dynamic_debug: add jump label support
Message-Id: <20160711122328.8dcd64432716765d2dad373b@linux-foundation.org>
In-Reply-To: <57839C9D.30801@akamai.com>
References: <cover.1467837322.git.jbaron@akamai.com>
	<d165b465e8c89bc582d973758d40be44c33f018b.1467837322.git.jbaron@akamai.com>
	<20160708144124.fa0b4d55aaf7e1d61a943d40@linux-foundation.org>
	<57839C9D.30801@akamai.com>
X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron <jbaron@akamai.com> wrote:

> On 07/08/2016 05:41 PM, Andrew Morton wrote:
> > On Wed,  6 Jul 2016 17:42:36 -0400 Jason Baron <jbaron@akamai.com> wrote:
> >
> >> Although dynamic debug is often only used for debug builds, sometimes its
> >> enabled for production builds as well. Minimize its impact by using jump
> >> labels. This reduces the text section by 7000+ bytes in the kernel image
> >> below. It does increase data, but this should only be referenced when
> >> changing the direction of the branches, and hence usually not in cache.
> >>
> >> ...
> >>
> >> +#ifdef HAVE_JUMP_LABEL
> >> +
> >> +#define dd_key_init(key, init) key = (init)
> >>
> >> ...
> >>
> >> +#else
> >> +
> >> +#define dd_key_init(key, init)
> >> +
> > umm, lazy.  One is an lval and returns a value and the other does
> > neither.  Lack of parenthesization in the first version doubtless
> > exposes various horrors.
> >
> > Care to do something more robust and conventional here?  Presumably use
> > symmetrical do{}while(0) things, neither of which is an lval, both of
> > which don't return anything.
> >
> 
> Hi,
> 
> The 'dd_key_init()' macro is being used here to help initialize
> the 'key' field in the 'struct _ddebug', and its not being used as a
> statement.
> 
> In the 'HAVE_JUMP_LABEL' case, we are initializing the 'key' field, while in
> the not-'HAVE_JUMP_LABEL' case, the 'key' field is simply not present
> in the structure (to conserve space).

Well yeah.  And it's doing it wrongly, isn't it?

: @@ -68,13 +78,51 @@ void __dynamic_netdev_dbg(struct _ddebug
:  		.filename = __FILE__,				\
:  		.format = (fmt),				\
:  		.lineno = __LINE__,				\
: -		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
: +		.flags = _DPRINTK_FLAGS_DEFAULT,		\
: +		dd_key_init(key, init)				\
:  	}
:  
: +#ifdef HAVE_JUMP_LABEL
: +
: +#define dd_key_init(key, init) key = (init)

Shouldn't it be ".key = (init)"?

Anyway, it's odd-looking.  I guess something like

#define DD_KEY_INIT(init)	.key = (init)

would be more idiomatic.