From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751997AbcBKHCF (ORCPT ); Thu, 11 Feb 2016 02:02:05 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:45097 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751314AbcBKHCC (ORCPT ); Thu, 11 Feb 2016 02:02:02 -0500 X-AuditID: cbfec7f5-f79b16d000005389-cf-56bc31e649a1 Subject: Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types To: Arnd Bergmann References: <20160202163350.f7d42f4b97f48756f3900e9a@linux-foundation.org> <5549573.1i7GpRbvFd@wuerfel> <56B9A672.1080905@samsung.com> <2469004.dGELCApkKV@wuerfel> Cc: Andrew Morton , Bartlomiej Zolnierkiewicz , Marek Szyprowski , open list , Bob Peterson , linux@rasmusvillemoes.dk From: Andrzej Hajda Message-id: <56BC31A6.4060102@samsung.com> Date: Thu, 11 Feb 2016 08:00:54 +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: <2469004.dGELCApkKV@wuerfel> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrPLMWRmVeSWpSXmKPExsVy+t/xy7rPDPeEGbxdaGkxZ/0aNou/k46x W2ycsZ7V4vKuOWwWj2fNY7NYe+Quu0XX7FY2B3aP378mMXqcmPGbxeNCV7bH+31X2Tz6tqxi 9Pi8SS6ALYrLJiU1J7MstUjfLoEr4/62ZSwFr2UrPu5YwNbAeEa8i5GTQ0LARGLCrxZmCFtM 4sK99WxdjFwcQgJLGSWurLvIBOE8Z5R4NKuLvYuRg0NYwF9iySNVkAYRAUWJqS+eMUPULGeU uPW4DaybWaCBSeLIwg9MIFVsApoSfzffZAOxeQW0JDZsuQJmswioSrw8c44VxBYViJA43Amy AKRGUOLH5HssIDYnUO+CT38ZQRYzC+hJ3L+oBRJmFpCX2LzmLfMERoFZSDpmIVTNQlK1gJF5 FaNoamlyQXFSeq6RXnFibnFpXrpecn7uJkZIsH/dwbj0mNUhRgEORiUe3oD63WFCrIllxZW5 hxglOJiVRHh3PAYK8aYkVlalFuXHF5XmpBYfYpTmYFES5525632IkEB6YklqdmpqQWoRTJaJ g1OqgXGm0RstyWtSV1yfW9058sfXd2tj9w6lCi8x6Qttx7PrFNZsEhbknuCxN+bnBhfjbUnv My7yL3ySERLPfzlElC1nl8tsrm2/Yz6KRe758ySoVXUZnyZTzwk5n/8XzmY8zf29qp9nXs1D 5R8bNtyu8Khvv1Ko+ljHM/qjwqc7vdUthdael6OfPVNiKc5INNRiLipOBAA8x81ecgIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/10/2016 10:01 PM, Arnd Bergmann wrote: > On Tuesday 09 February 2016 09:42:26 Andrzej Hajda wrote: >> +cc Rasmus Villemoes, I forgot to add him earlier. >> >> On 02/08/2016 01:01 PM, Arnd Bergmann wrote: >>> On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote: >>>> On 02/05/2016 11:52 AM, Arnd Bergmann wrote: >>>>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote: >>>> 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. >>> If we do that, should we also make it illegal to use an invalid type >>> for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned >>> char' that are still broken. >> I meant rather to make such 'policy' for future code by adding some >> comment to the macro. Optionally adding compile time warning >> to encourage developers to change current usage, however I am >> not sure if it is not too harsh. >> This way it could be also good to use your version of the macro. >> It could be also good to add compiletime_assert to prevent char types >> as suggested by Rasmus. >> >> Finally it could look like: >> /* >> * Use IS_ERR_VALUE only on unsigned types of at least two bytes size. >> * For signed types use '< 0' comparison. >> */ >> #define IS_ERR_VALUE(x)\ >> ({\ >> compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle >> byte-size types");\ >> compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE >> should be called on unsigned types only, use '< 0' instead");\ >> (unlikely((unsigned long long)(x) >= (unsigned long >> long)(typeof(x))-MAX_ERRNO));\ >> }) >> > I think the easiest way to express this would be to ensure that the argument > is 'unsigned long', like: > > #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \ > unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO)) This way you will limit it only to unsigned long type, which seems too strict to me. I think the macro should accept all long enough unsigned types, otherwise we could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL... Regards Andrzej