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 99B39C7EE29 for ; Sat, 10 Jun 2023 22:35:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D14356B0075; Sat, 10 Jun 2023 18:35:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CC4996B0078; Sat, 10 Jun 2023 18:35:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B65468E0002; Sat, 10 Jun 2023 18:35:05 -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 A2B5A6B0075 for ; Sat, 10 Jun 2023 18:35:05 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 61C5A140354 for ; Sat, 10 Jun 2023 22:35:05 +0000 (UTC) X-FDA: 80888294970.16.B8189A0 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) by imf15.hostedemail.com (Postfix) with ESMTP id 79024A0012 for ; Sat, 10 Jun 2023 22:35:03 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="NP0/Wz1T"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.43 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686436503; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Ag0KvnrINiKF0/PZIxk1qYCQu6jO10rq427ozyOq42c=; b=jKDayvSHw9cc1WI+Ck9pNUXTghIEGoAlRNaHRTYbnmyh46OzjXegoAZT4ANyo2Etn5Or6w PIW8GJURNCAx6rl+uWBAfDFYViVFhkeKIpZEdLN8OR7E3870q4aXA6ETuTuSFHAeLaAPfx ZYqGX6vHxpYx3LGzQbiI/3M8kKCImOQ= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="NP0/Wz1T"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.43 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686436503; a=rsa-sha256; cv=none; b=Wza7BKhYKTTWLVvQwK+gkGMAd9A3wG9/d5a8YOPSYDwqDIZt7CQymYE2+zflnmBcfiF8Di 6fbRsqlHdMg9vyjcKYq2cuycUtQ4Cwd8poRdCuqzBdDD7Gqh5hjyz8ew5TbCdTvghxrJNX fwm/a58Ix7WQN17a3YSP34vHa4YHa6A= Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-3f6d38a140bso22176735e9.1 for ; Sat, 10 Jun 2023 15:35:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686436502; x=1689028502; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=Ag0KvnrINiKF0/PZIxk1qYCQu6jO10rq427ozyOq42c=; b=NP0/Wz1Tz1fKtEVZa781o3518k6MVjaghaie2tpAXbvIOEzk2JT3MKtHQEg+W4uTcz cb7DKdgslE0+gMYmNxmJ9IdSuf7jnLlmMV/RIS5NSj+xUHlJmbal+H/eBY/c5LYKdQMS baDq9w7DfbB2PIF9aZ8hg6cjAT7w6Mxr54ufbINjUGArE7v0hWuw6eQkDWVs6mPDMVLi CHfLcWpJDxplNDHGV+q/nti89JLtL1zqVhXfmSaIMWW3Tr6uaiGafu3SuftrCCHPkmOp 3dRoJtmEamVw81AjfibcurDnDwCFQHEoJWTPXC6CH6DCyIlyh48MoEca1Ok941nqzADt OfcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686436502; x=1689028502; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Ag0KvnrINiKF0/PZIxk1qYCQu6jO10rq427ozyOq42c=; b=f1VaKOrS70U/Re3NUW2G1s0wdfj0n0MQLAGKwN57Ovtp0oRc4nxnFeONn5fPrUc9ii NUcRre+Ban17H1mAIvfLjorLd75K2smW6Qdv/Yh9Yo0/LGc2zxwGC9IrqdwYDrHYkUjQ H7Xo7KTn8SgA3ImO2zKXYaZFYBPEok0kXip4l3izMe5fcPWqxeb/jxb10hAlueaPtaIK gUgZR4peYvvTMZbd3iehkfq1tUPajh1xnVj0820qd+YexyMOjFhMUfUM4hILRAuTMh3S 2yC8Y/DpQo9zvtZL7PHxwAVHAe+9pVmXDpD2k73UTxTc5bCDckbKa6eohfwbodv60QzO 7mUg== X-Gm-Message-State: AC+VfDzZCVNiWGXe26Sh58JKGAHECI+CSy0bTAP3RMdxnbJtskU+kLWg R9IS3tEZKT3I6Q5Bbxpq3ZI= X-Google-Smtp-Source: ACHHUZ4RyqsUbtYL/K2/C9gh98Wubcn46BOE9v/d4wUY7YupfJZVkkmCUXfMaQAoLDeUA69ts6poBQ== X-Received: by 2002:a1c:f418:0:b0:3f6:1ac:5feb with SMTP id z24-20020a1cf418000000b003f601ac5febmr4580845wma.16.1686436501520; Sat, 10 Jun 2023 15:35:01 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id f13-20020a056000036d00b0030aefa3a957sm8085429wrf.28.2023.06.10.15.35.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 10 Jun 2023 15:35:00 -0700 (PDT) Date: Sat, 10 Jun 2023 23:35:00 +0100 From: Lorenzo Stoakes To: David Laight 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() Message-ID: <5597fc8f-43ed-457a-a777-4dbf52d0fbe0@lucifer.local> References: <20230609061309.42453-1-luhongfei@vivo.com> <832d7c69-ffd5-4764-8ffe-3a02bef0efb0@lucifer.local> <3fc87d60-4e81-4f49-95f0-0503ad5cdf35@lucifer.local> <3e35b679f17a434b9da2ceffba086bfa@AcuMS.aculab.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3e35b679f17a434b9da2ceffba086bfa@AcuMS.aculab.com> X-Rspamd-Queue-Id: 79024A0012 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: x358dg8tfidme6hhsufp3qq5hnrj81hw X-HE-Tag: 1686436503-110682 X-HE-Meta: U2FsdGVkX19utC/zZm71EGkslJ+yf+hUJpyP7Qk52971rIsj4WT+1aRnykCBC9piuo2Y0JWyzNfXxmz70M0ZdQGLCTKvhzquz/ma9kgT0QLWo7DwuNTFahbjm5wbOwYOBP+O8H+EzIZ3iojD3f5Wwn+VBrrV5Si1kOMeQXaQaawkRTpu2D1/MyI02uAsuCUW1OmM2osuqyP6jQSDkGeHlF3a3Pe1gKt/8LiUEviPYxYQJT184AYF687QhHm8VnmD35eKGivVfRuOrXVwk9C/V/dTqp3HBHH+tZEYDw/WB1fBLbaDCHpkQLAsz8Y0f7ndEzAQfxhLnmeJssgnS7G/UXYFgo6jFR00XPdAG5vCHaWWMwMM08cIPWPOo4LsM/+V96p6HrACB+T6lirkx5zUidsT/cWJXyS77oigzec/U1L/gCqN+erR2UZdaR0CFKLBX2tk+DgGxgMHrQvb47mUgzsIdvFmXNqU27nGogtGQi1/41fPrDz5mlCxh9jVLR3Q7yMMsUcy5R6tjctcViugk9VSvnE82+C4nGUV3VB6tuUdbwxfd7VHm6y9JgY1FJDm3D+u/00xkhn6yJnh3jKfJn7tCWanTX2untQjw4dBiHFHShA6IboHOvw4mclufr+syIT0NSTuzYBS9/kEoT3AvxJMc/bYVSNyxYqMB0f5Zh1ejnp1nHBY9nvRFPBY4X+5RzWw8rf3nMHTPrR+6M9AaMvMO441O9imaAlAgqfIkqmVulPK8V37O0D37hhL4gxVk4x50lhMyMuKFEc1eZeJgCZydEmnLm4yl1GLH6fpxOSnvtgnGE+/U/tG5JIKbIql4blRD8X3sy3TXxCtLaECyrp+zfcnOl3otY5DjR2A334uIxfvMPW78OdT9mcDIKR72tcKiqNXfZjUaYpaNTCPB8vbrLb237Fx8jVzwpHmNQzrA4GJEly4TF/L6YnEWErzKldP2NVzjDIc+mTsIJl gOyd0IRQ avZEdVDJg7qTUvdyZev0PEASoG2TBi6tRw+WlCsPts7grUrBqStEGIpkMLMF8NBwAnAyfLnoPk0/va2T2mmwSJyZk/9KOLm0ywL8O3B0w5xf0TyNG8u+L2r/5gLiXCgVD/LOhWZ51/0+955nCQuF6JYjiABUm6gGZGZ7tgT05OInuivgIUxOiP+zrkDvibOUQsxfr020uzyyNtj9qrZqTTdtc0tPswa7TrUpxTaK2KqKNEugXshQ6w/sfKKhClXSVfbzfN5AxI8CBqJKuj2q7HVMVBWxGHpWZGmu2U0NYH0i/f5ErZ/lstCFwvZCrme9tZqTS+tZYuJ2ZeUQvM5FHFOl8ZcuiYSddnsZE78DM5ZuKTNg0hDySlK4EMwd8Uw0vG7qGSKmJMGTUymSSRnYJIsAxtPUSaFe5qyr4bVXt5A50zftij/LMhL+tgw== 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: On Sat, Jun 10, 2023 at 10:18:34PM +0000, David Laight wrote: > From: Lorenzo Stoakes > > Sent: 10 June 2023 22:07 > ... > > > > OK, as per the pedantic test bot, you'll need to change this to:- > > > > > > > > num = min_t(size_t, remains, PAGE_SIZE); > > > > > > > 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. > > > > > There has to be a valid reason why min/max have strong type checks. > > > > I really don't know what you mean by this? Yes there is a reason, I imagine > > 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. Yes this is precisely what I thought it ought to be protecting again > > > This is not applicable here (explained below...) > > > > > 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. > > > > 'All the time' - are you just having a general whine + moan about perceived > > 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 the > > 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 Sure, but it's not really relevant here for the reasons I went into. Probably as Andrew says elsewhere in the thread, PAGE_SIZE not being size_t is pretty annoying. > > ... > > > A 'safe' change is min(remains + 0ULL, PAGE_SIZE). > > > > 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. It's unsigned long in every arch I've seen right? And in a 32-bit arch long long will be 64-bit, so I think you'd still have the error here. > > 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 compelling > > reason is given (you've not given any). That's horrific. And again, you've > > 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(). Yes obviously instances of inappropriate narrowing are horrible, but that isn't happening here. In this specific instance, for any actual architecture in reality there is no issue. I do absolutely agree we should address the instances where an inappropriate type is used. > > ... > > > But, in reality, min/max should always be valid when one > > > value is a constant between 0 and MAX_INT. > > > > This is getting at a signed/unsigned comparison issue here afaict which is > > 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. All this ultimately sounds like a broader criticism of the min/max type checks and not really relevant to this patch, that should be addressed at a more general level. > > ... > > Now since you kicked off this 'all the time' stuff I feel like I have been > > given permission to make some broad comments myself... > > > > David, I am not one to commit-shame being a minor contributor myself buuuut > > 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. > Yes, sorry, I was probably a little too mean there (long hot day and up too early) I didn't mean to be personal, what I was saying in blunt fashion is the commentary needs to be proportionate to the problem and placed at the right position, I don't feel this is. By the way I can relate on this fully as I'm a 100% hobbyist who does this part time (and writing a book on mm, yes I should get out more), and I know how difficult it can be to get patches in! > I've been writing device driver and comms protocol stack code > for best part of 40 years. Yes again apologies, I really didn't mean anything personal, I just found the review a little frustrating. You are certainly more experienced than I am! :) Cheers, Lorenzo > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >