From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751016AbcBHIq6 (ORCPT ); Mon, 8 Feb 2016 03:46:58 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:32257 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750873AbcBHIq5 (ORCPT ); Mon, 8 Feb 2016 03:46:57 -0500 X-AuditID: cbfec7f4-f79026d00000418a-1f-56b855fed115 Subject: Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types To: Arnd Bergmann , Andrew Morton References: <20160202163350.f7d42f4b97f48756f3900e9a@linux-foundation.org> <13351313.c4ZqBbQEld@wuerfel> <20160204105931.7422c17d0bd1b92a387d97c9@linux-foundation.org> <2046663.fHIlWH1ph1@wuerfel> Cc: Bartlomiej Zolnierkiewicz , Marek Szyprowski , open list , Bob Peterson From: Andrzej Hajda Message-id: <56B855C3.5010006@samsung.com> Date: Mon, 08 Feb 2016 09:45:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: <2046663.fHIlWH1ph1@wuerfel> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrKLMWRmVeSWpSXmKPExsVy+t/xq7r/QneEGTRO07KYs34Nm8XfScfY LTbOWM9qcXnXHDaLtUfuslt0zW5lc2Dz+P1rEqPHiRm/WTze77vK5tG3ZRWjx+dNcgGsUVw2 Kak5mWWpRfp2CVwZl87eYy54rVWx8HYPWwPjftkuRk4OCQETiUMHlzNC2GISF+6tZ+ti5OIQ EljKKDHvzl8WCOc5o8S0m7+BMhwcwgL+EkseqYI0iAj4SLxYO4cRouYUo8SVf9fYQBLMAtsZ JZ7/igCx2QQ0Jf5uvgkW5xXQkni2u5cVxGYRUJW4uaCNBcQWFYiQONzZxQ5RIyjxY/I9sDgn UO/VwwvZQfYyC+hJ3L+oBTFeXmLzmrfMExgFZiHpmIVQNQtJ1QJG5lWMoqmlyQXFSem5hnrF ibnFpXnpesn5uZsYIYH9ZQfj4mNWhxgFOBiVeHgrOraHCbEmlhVX5h5ilOBgVhLhPR+0I0yI NyWxsiq1KD++qDQntfgQozQHi5I479xd70OEBNITS1KzU1MLUotgskwcnFINjFPqH/pOX8s1 qTfwX9gqFYGNVbMf7HFJnWQv3vZLLflf/oSTO0PjSls60p/xX2YXO3Tvr1ZFWmLuDkmV5Qy5 rppfGq5x/f7ge+muBKdWZ/q09TzNs/6pMDz1STp6dfuRzksRqeXXErmnLuh7UX/Dme3ZGrsu 4+h5pbKxE5crztNnrjReO2WXthJLcUaioRZzUXEiANe1TM5oAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/05/2016 11:52 AM, Arnd Bergmann wrote: > On Thursday 04 February 2016 10:59:31 Andrew Morton wrote: >> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann wrote: >> >>> diff --git a/include/linux/err.h b/include/linux/err.h >>> index b7d4a9ff6342..bd4936a2c352 100644 >>> --- a/include/linux/err.h >>> +++ b/include/linux/err.h >>> @@ -18,9 +18,7 @@ >>> >>> #ifndef __ASSEMBLY__ >>> >>> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \ >>> - ? unlikely((x) <= -1) \ >>> - : unlikely((x) >= (typeof(x))-MAX_ERRNO)) >>> +#define IS_ERR_VALUE(x) (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO)) >>> >>> static inline void * __must_check ERR_PTR(long error) >>> { >>> >>> >>> I'm not sure if the cast to 'unsigned long long' might cause less >>> efficient code to be generated by gcc. I would hope that it is smart >>> enough to not actually extend shorter variables to 64 bit before >>> doing the comparison but I have not checked yet. >> I did a quick test with i386 on drivers/nvmem/core.o. The patch takes >> the text size from 9098 bytes to 9133. That file has 11 instances of >> IS_ERR_VALUE(). > This seems to be because it brings back the logic to what it was before > in case of 'int' arguments. I checked the assembly output and found mine > to be identical to v4.4 in this case: > > text data bss dec hex filename > v4.4 9942 1872 2856 14670 394e drivers/nvmem/core.o > a.hajda 9922 1872 2856 14650 393a drivers/nvmem/core.o > arnd 9942 1872 2856 14670 394e drivers/nvmem/core.o I have compared all proposed version with all compilers I have: text data bss dec hex filename gcc-4.4 old 8188 1016 2968 12172 2f8c .x86/drivers/nvmem/core.o andrzej 8155 1016 2968 12139 2f6b .x86/drivers/nvmem/core.o arnd 8188 1016 2968 12172 2f8c .x86/drivers/nvmem/core.o rasmus 8266 1016 2968 12250 2fda .x86/drivers/nvmem/core.o --- gcc-4.7 old 7642 3816 3248 14706 3972 .x86/drivers/nvmem/core.o andrzej 7606 3816 3248 14670 394e .x86/drivers/nvmem/core.o arnd 7642 3816 3248 14706 3972 .x86/drivers/nvmem/core.o rasmus 7719 3816 3248 14783 39bf .x86/drivers/nvmem/core.o --- gcc-4.8 old 7735 3888 3272 14895 3a2f .x86/drivers/nvmem/core.o andrzej 7698 3888 3272 14858 3a0a .x86/drivers/nvmem/core.o arnd 7735 3888 3272 14895 3a2f .x86/drivers/nvmem/core.o rasmus 7812 3888 3272 14972 3a7c .x86/drivers/nvmem/core.o --- arm-linux-gnueabi-gcc-4.7 old 12776 1680 3432 17888 45e0 .arm/drivers/nvmem/core.o andrzej 12772 1680 3432 17884 45dc .arm/drivers/nvmem/core.o arnd 12776 1680 3432 17888 45e0 .arm/drivers/nvmem/core.o rasmus 12948 1680 3432 18060 468c .arm/drivers/nvmem/core.o --- aarch64-linux-gnu-gcc-4.8 old 5967 440 48 6455 1937 .arm64/drivers/nvmem/core.o andrzej 5947 440 48 6435 1923 .arm64/drivers/nvmem/core.o arnd 5967 440 48 6455 1937 .arm64/drivers/nvmem/core.o rasmus 5991 440 48 6479 194f .arm64/drivers/nvmem/core.o --- My version produces shortest code, Arnd's is the same as the old one. On the other side Rasmus proposition seems to be the most straightforward to me. Anyway I am not sure if the code length is the most important here. By the way .data segment size grows almost 4 times between gcc 4.4 and 4.8 :) Also numbers for arm64 looks interesting. Just for the record below all proposed implementations: #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO) #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \ ? unlikely((x) <= -1) \ : unlikely((x) >= (typeof(x))-MAX_ERRNO)) #define IS_ERR_VALUE_arnd(x) (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO)) #define IS_ERR_VALUE_rasmus(x) ({\ typeof(x) _x = (x);\ unlikely(_x >= (typeof(x))-MAX_ERRNO && _x <= (typeof(x))-1);\ }) > > Andrzej's version is a little shorter on ARM because in case of signed numbers > it only checks for negative values, rather than checking for values in the > [-MAX_ERRNO..-1] range. I think the original behavior is more logical > in this case, and my version restores it. As I looked at the usage of the macro in the kernel I have not found any code which could benefit from the original behavior, except some buggy code in staging which have already pending fix[1]. But maybe it would be better to use IS_ERR_VALUE to always check if err is in range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of signed types. [1]: http://permalink.gmane.org/gmane.comp.file-systems.lustre.devel/4164 Regards Andrzej