Subject: Re: [PATCH] mtd: cfi: convert inline functions to macros
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Arnd Bergmann <arnd@arndb.de>, David Woodhouse <dwmw2@infradead.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Richard Weinberger <richard@nod.at>,
        Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
References: <20171011135419.3492681-1-arnd@arndb.de>
 <b109f253-ac22-82b0-6716-6a069eb3e4f2@gmail.com>
 <20171217094345.3d327c54@bbrezillon>
From: Marek Vasut <marek.vasut@gmail.com>
Message-ID: <00927ce2-9588-5343-ddc6-e572173b1b8e@gmail.com>
Date: Sun, 17 Dec 2017 20:34:40 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171217094345.3d327c54@bbrezillon>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2767
Lines: 59

On 12/17/2017 09:43 AM, Boris Brezillon wrote:
> Hi Marek,
> 
> On Wed, 11 Oct 2017 23:34:33 +0200
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 10/11/2017 03:54 PM, Arnd Bergmann wrote:
>>> The map_word_() functions, dating back to linux-2.6.8, try to perform
>>> bitwise operations on a 'map_word' structure. This may have worked
>>> with compilers that were current then (gcc-3.4 or earlier), but end
>>> up being rather inefficient on any version I could try now (gcc-4.4 or
>>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
>>> fail to reuse the stack space for local variables.
>>>
>>> This can be seen immediately in the stack consumption for
>>> cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
>>> can be up to 2200 bytes. Changing the inline functions into macros brings
>>> this down to 1280 bytes.  Without KASAN, the same problem exists, but
>>> the stack consumption is lower to start with, my patch shrinks it from
>>> 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
>>> 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
>>> structures for each call to one of these helpers.
>>>
>>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
>>> but nobody uses that yet, so we should still work around it in mainline
>>> kernels and probably backport the workaround to stable kernels as well.
>>> We had a couple of other functions that suffered from the same gcc bug,
>>> and all of those had a simpler workaround involving dummy variables
>>> in the inline function. Unfortunately that did not work here, the
>>> macro hack was the best I could come up with.
>>>
>>> It would also be helpful to have someone to a little performance testing
>>> on the patch, to see how much it helps in terms of CPU utilitzation.
>>>
>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
>>
>> Don't you lose type-checking with this conversion to macros ?
>>
> 
> Yes, we loose strict type checking, but if you look at the code, you'll
> see that the macros do (valN).x[i], so, if valN is not a struct or
> a union containing a field named x, the compiler will complain. That
> should save us from devs passing random arguments to those macros.

You could add some typeof() checking into those macros to make it even
more secure ... or, you could keep them as functions.

> Anyway, this code is not seeing a lot of changes lately, so I wouldn't
> be so worried by the lack of strict type-checking implied by this
> transition to macros. 

Well this transition makes the typechecking worse, not better, and I'm
not super happy about that.

-- 
Best regards,
Marek Vasut