From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752711AbcGKTwa (ORCPT ); Mon, 11 Jul 2016 15:52:30 -0400 Received: from prod-mail-xrelay08.akamai.com ([96.6.114.112]:20722 "EHLO prod-mail-xrelay08.akamai.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751911AbcGKTw2 (ORCPT ); Mon, 11 Jul 2016 15:52:28 -0400 Subject: Re: [PATCH v3 7/7] dynamic_debug: add jump label support To: Andrew Morton References: <20160708144124.fa0b4d55aaf7e1d61a943d40@linux-foundation.org> <57839C9D.30801@akamai.com> <20160711122328.8dcd64432716765d2dad373b@linux-foundation.org> CC: , , From: Jason Baron Message-ID: <5783F8FA.2090106@akamai.com> Date: Mon, 11 Jul 2016 15:52:26 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160711122328.8dcd64432716765d2dad373b@linux-foundation.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/11/2016 03:23 PM, Andrew Morton wrote: > On Mon, 11 Jul 2016 09:18:21 -0400 Jason Baron wrote: > >> On 07/08/2016 05:41 PM, Andrew Morton wrote: >>> On Wed, 6 Jul 2016 17:42:36 -0400 Jason Baron 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. Ok, so the 'key' field is a union and so the patch is effectively calling (after substitution): dd_key_init(.key.dd_key_true, STATIC_KEY_TRUE_INIT) and: dd_key_init(.key.dd_key_true, STATIC_KEY_FALSE_INIT) So we could have variations such as: #define dd_key_true_init() .key.dd_key_true = (STATIC_KEY_TRUE_INIT) #define dd_key_false_init() and #define dd_key_true_init() #define dd_key_false_init() .key.dd_key_false = (STATIC_KEY_FALSE_INIT) and finally: #define dd_key_true_init() #define dd_key_false_init() and then have both dd_key_true_init() and dd_key_fase_init() in the structure definition. It adds a bunch more definitions and I'm not sure if its more readable, but maybe it would look cleaner? Thanks, -Jason