From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BA6EEC7EE2E for ; Sat, 10 Jun 2023 22:18:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D1AA96B0072; Sat, 10 Jun 2023 18:18:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CCA458E0002; Sat, 10 Jun 2023 18:18:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B92046B0075; Sat, 10 Jun 2023 18:18:47 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id AA3EE6B0072 for ; Sat, 10 Jun 2023 18:18:47 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 6E6C9A0303 for ; Sat, 10 Jun 2023 22:18:47 +0000 (UTC) X-FDA: 80888253894.19.FA76D79 Received: from eu-smtp-delivery-151.mimecast.com (eu-smtp-delivery-151.mimecast.com [185.58.85.151]) by imf03.hostedemail.com (Postfix) with ESMTP id 6BF8D2000F for ; Sat, 10 Jun 2023 22:18:42 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=aculab.com; spf=pass (imf03.hostedemail.com: domain of david.laight@aculab.com designates 185.58.85.151 as permitted sender) smtp.mailfrom=david.laight@aculab.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686435524; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lZ2Xv4kuABO2+d4oIGzqz3W6wCl6R3Es06urUXO+DM4=; b=uTMv5yv/sGNx4Jo5imhWzIGKaPGcD9VyQPD6YEP7hps6u/V2ZJ4r8bYIB6XAqx6dbijUIy 0W0TPd3cUkvXccPHLCZ+0axuBw0fL8Yr+niem3aGLsbH3+b8nf/SNzSPl74PTLztZQMQek j8WA9p16nq+rEixWGy0RYi2QZDlmU1E= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=aculab.com; spf=pass (imf03.hostedemail.com: domain of david.laight@aculab.com designates 185.58.85.151 as permitted sender) smtp.mailfrom=david.laight@aculab.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686435524; a=rsa-sha256; cv=none; b=0EKF4zLMOog1/rjePchwcB7UjOxD+1dvLT2YOZh17scV+ZNZMsn7pE3miHu1cQd7qLUToP t5wdF0K9fk81ED+NYZAZAlkVcPJAixzUrZOzhpln/bHNG+EWBhbL1uv4p1SRUqFviPMav8 qP6bdoDvrYAEP1TKrN+nivVQlrAlnpw= Received: from AcuMS.aculab.com (156.67.243.121 [156.67.243.121]) by relay.mimecast.com with ESMTP with both STARTTLS and AUTH (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id uk-mta-130-z7zOhKmQOuS4sGCuOuqKdg-1; Sat, 10 Jun 2023 23:18:39 +0100 X-MC-Unique: z7zOhKmQOuS4sGCuOuqKdg-1 Received: from AcuMS.Aculab.com (10.202.163.4) by AcuMS.aculab.com (10.202.163.4) with Microsoft SMTP Server (TLS) id 15.0.1497.48; Sat, 10 Jun 2023 23:18:34 +0100 Received: from AcuMS.Aculab.com ([::1]) by AcuMS.aculab.com ([::1]) with mapi id 15.00.1497.048; Sat, 10 Jun 2023 23:18:34 +0100 From: David Laight To: 'Lorenzo Stoakes' CC: Lu Hongfei , Andrew Morton , Uladzislau Rezki , "Christoph Hellwig" , "open list:VMALLOC" , "open list" , "opensource.kernel@vivo.com" Subject: RE: [PATCH] mm/vmalloc: Replace the ternary conditional operator with min() Thread-Topic: [PATCH] mm/vmalloc: Replace the ternary conditional operator with min() Thread-Index: AQHZmq9RsZ2mMErDwEmVNd3QmnYBSK+Ed3ZwgAABSICAAB600A== Date: Sat, 10 Jun 2023 22:18:34 +0000 Message-ID: <3e35b679f17a434b9da2ceffba086bfa@AcuMS.aculab.com> References: <20230609061309.42453-1-luhongfei@vivo.com> <832d7c69-ffd5-4764-8ffe-3a02bef0efb0@lucifer.local> <3fc87d60-4e81-4f49-95f0-0503ad5cdf35@lucifer.local> In-Reply-To: Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: aculab.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 6BF8D2000F X-Stat-Signature: f5tsah69so15b5wufs9esiou5gsonek8 X-Rspam-User: X-HE-Tag: 1686435522-71957 X-HE-Meta: U2FsdGVkX1+7GA+hU/+mEP/x9J34bJLLvke7+uTPvsnny7160ofY3F3HMowfSkbwzOzm89bETvLwnnNqh2Zc19xdE/Y/+uw4d+GJNIyw5IsC9lR+4BuzTKnbmDiPt8RmOeB4myAGyDqSK3bcsO81+gzSfzpfOXpOc/MDvPyzO3YOssJFNsTAO9Tol/vO6UWh68uO65NCqKDjaTKrHsb8+KBhkrgdvvEi8O2ayINQPDcmf8kebg52x//EuzC6f8W8E8Mm0lguuvorIGR/LP7R1f0TTs5a9MwV4aI3xWiP+Tq79W1S3Lb3gNLI4SCmWO9QhKl5zS0gJ93T7HYSUTMos4yeDQDNGXKn8vI6HsKjemwT5ddTjrO3FCADFaEd0Lo9V7yos53q8bNmwB5bkwY6JqDs2IJD+Q9fG2quenWV9VksLFFUIUOc7HP9hJ/DkWi0aEYSzEd09BHOfmrh4CGw/PyO1jJB+IL30WA7cRKvx6+6zo4DzyJBZR/Gacw6U8BiDR3m8lzbPgt0UIv7gm6GALNmwyFAJMU79QvS6GEuBSRm4Y1bHnb1rZ2U7kVsqF7ZywGa5N6Dibpl5eqkAmxA7/kQ7oyIg3JCT7Kng9uHbzc+WWKmHO7FCybtsdDckKmN61BZ400scSkHzGKPG96Dm5E/3k0DL3j3dfKDhLnWZGBdXHn7tmATLV7TiBjvJX2vZkPWOtfs/LvEBdYcLFd0L1zEWYf+WvaAqPznSvtt/wQAPh584IRPtWEaIDbFnqted3vKJ9jhjHDGNDXwye0K0AeCe+8Ze4L+L9avu070ZimM/5IV8ZA2l5Xy3sk7JM5V3ZBmPkd0qoHCSd0fBUBiCBh3oEKwMfJhOYCgUsl6bXmWe/Pi869Q/fOprp5VL0+66yUtHkCyKU5Sf4c3hQ2v7FvFpDZXaym4F/QSvSltB45vlWCH0pF3rJs777nkkC4Kw7GmXB6eymuDRvC3tAM F+EeMBIU EjaGfY85Y110JoifxXFkg1xT37Da5OO+BbTNJGSOXXEry1Uq2hR8Ze4xMhIG7TlyG1krB1EPVf5z74HDV4Bk55cL09rlRQGnR8jrcx6BwZzdIYq/q6pNCYhjxOr0B3CngIITTInNiRlwVcIP3DRJskklSt26qrm1Iot/c1TKFvSvrpX4rkxfAS0E4JotdrWyk1vkLzfoeuV+AN7dtGJWVibCkLtPwx2O9GfyrV6i6jcxpfvI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: From: Lorenzo Stoakes > Sent: 10 June 2023 22:07 ... > > > OK, as per the pedantic test bot, you'll need to change this to:- > > > > > > num =3D min_t(size_t, remains, PAGE_SIZE); > > >=20 > Ordinarily I wouldn't respond to this (I go into why I feel this is not > useful commentary below) but I am concerned Lu will take you seriously. >=20 > > There has to be a valid reason why min/max have strong type checks. >=20 > I really don't know what you mean by this? Yes there is a reason, I imagi= ne > it's to avoid unfortunate and invalid type comparisons. Indeed, the 'unfortunate conversion' is a negative value being converted to a large positive one. That doesn't require anything like the type checking that min/max do. > This is not applicable here (explained below...) >=20 > > Using min_t() all the time is just subverting them and means that > > bugs are more likely than if the extra tests in min() were absent. >=20 > 'All the time' - are you just having a general whine + moan about perceiv= ed > kernel practices? Can you please keep it focused on the actual issues at > hand? I am not Linus and therefore not responsible for the entirety of th= e > kernel. I see a general problem (that Linus ought to worried about) is that whenever min() reports a type error the answer is do immediately drop in a min_t() instead of looking at the type of the values and fixing them to that min() doesn't complain. (Or fixing min() so it doesn't object to a much larger class of comparisons.0 ... > > A 'safe' change is min(remains + 0ULL, PAGE_SIZE). >=20 > So now we're promoting an unsigned int (and sometimes unsigned long of > course) to an unsigned long long (for reasons unknown) and comparing it > with an unsigned long? Wouldn't this trigger the sensitive type check > anyway? PAGE size is defined to be 'long long' - so min() will be happy. The compiler will just DTRT, even if 'remains' is 32bit it will optimise away all the implied 64-bit extension. Almost all the min_t() are min_t((some unsigned type),a,b). If the values are known to be positive then: #define min_unsigned(a, b) min((a) + 0u + 0ul + 0ull, (b) + 0u + 0ul + 0ull= )) will zero extend whatever type is supplied before the comparison. The compiler will just discard zero extensions. > To be clear, I'd nack any such ridiculous change unless a hugely compelli= ng > reason is given (you've not given any). That's horrific. And again, you'v= e > not provided one single example of an _actual_ bug or situation where the > 'problem' you tiresomely raise would occur. The (size_t) cast isn't in itself a problem - provided you've checked that it is larger than the types of both sides. But search the kernel and you'll find places when min_t((u8),a,b) is used. This all follows the same pattern of min() gave a warning so so use min_t(). ... > > But, in reality, min/max should always be valid when one > > value is a constant between 0 and MAX_INT. >=20 > This is getting at a signed/unsigned comparison issue here afaict which i= s > not the one we're dealing with here. But it is exactly what min() is checking for and almost why min() exists. A min_unsafe() that didn't try to do any checks would be better than train wreck that min_t() can create. ... > Now since you kicked off this 'all the time' stuff I feel like I have bee= n > given permission to make some broad comments myself... >=20 > David, I am not one to commit-shame being a minor contributor myself buuu= ut > I see 7,610 messages from you on lore and 4 commits, all from 4 years ago > (please correct me if I'm wrong). I don't work for google, intel, aws (etc). Getting patches accepted is surprisingly hard. I've been writing device driver and comms protocol stack code for best part of 40 years. =09David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1= PT, UK Registration No: 1397386 (Wales)