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 E985BC678D5 for ; Tue, 7 Mar 2023 23:27:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E32BB6B0072; Tue, 7 Mar 2023 18:27:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DE2AD6B0073; Tue, 7 Mar 2023 18:27:58 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C832E280001; Tue, 7 Mar 2023 18:27:58 -0500 (EST) 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 B5C286B0072 for ; Tue, 7 Mar 2023 18:27:58 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 703E3C0EDD for ; Tue, 7 Mar 2023 23:27:58 +0000 (UTC) X-FDA: 80543692236.17.8B4258A Received: from mail-lj1-f173.google.com (mail-lj1-f173.google.com [209.85.208.173]) by imf02.hostedemail.com (Postfix) with ESMTP id 8227B8000C for ; Tue, 7 Mar 2023 23:27:56 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=TGT2yqt1; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf02.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.208.173 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678231676; 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:dkim-signature; bh=dgM5UWACpA1ziJGubEWxyksAdshP8CY52eV8GhV8Gkg=; b=wUOQ+BZNwjTY4P3MpmLailotJVVJvgdlXFzvm5ns+0MJhup/m8ec3tMAQDiABGfYnhU8Or nSCDD0f0RFr5DB2VIWtg2VukSBp+4n7I0hSNGqCqBlegHEOHLqtmS6as6xU5shUcpDMD3i M/nwQ88rVbJe/8MRDoWoziVBokRex7E= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=TGT2yqt1; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf02.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.208.173 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678231676; a=rsa-sha256; cv=none; b=HiYIy23R8NQLiTzjZniqJutJXzrdf56dzdYgwxp4ACMAK6MGaM2cKqV1MkQmT5tQAnxAgB /m/htLM/wTBi5hew1u8a9Qzg6SG622dP1ujrSlmcouqBO08Am0Qaiv5jiybyXSUP+mHSGL kShdTjnLVYBGr+4cuBXGcsYsvtkKDkM= Received: by mail-lj1-f173.google.com with SMTP id h3so14849725lja.12 for ; Tue, 07 Mar 2023 15:27:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678231675; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=dgM5UWACpA1ziJGubEWxyksAdshP8CY52eV8GhV8Gkg=; b=TGT2yqt1nWpJRPljNttb9FcVv36E/Jt8A84ByzTR4qfutxQ7fHbpdB+EgF/6Hu8QEA lGav8SrhhYCem9wRNTeicDzl5ewmc3qyFq1vE8KfXRp3zg47pIamj2eOLTILU7cqykpP eIRethLM6K4/tMKQANyi6ZEnJZjZRMB/OCTP+DWXQsvO2zKmMTsPyQD8swSXLVTS2D5g zpu9uzYUHyfgWtbbJ7XEYTACwoPmJeHTPKHV/ad884DyulUXcTRi/oc/IJ8NR/H3zqZs tKa1AESMTbEZ0h1SvjwaE/87vK4azcWvVNgcCANUEXMYEwcMUPSMOuu0EJJvRrTgBIy/ aRPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678231675; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=dgM5UWACpA1ziJGubEWxyksAdshP8CY52eV8GhV8Gkg=; b=ZANBX4eshgTMmfv3SpN41mOMNNn2Nb5UQumSiDcFzWF1LAJH5GLHfYJ+HDxyPgDEad 5WjqswtaxeK9D1Wx/mKRfhzLCzApmXkbs0qDQ101KzSj/yomknTT5Da3Vx5ibUSD2NDw +ekPUXGsArhE5uuBKkuUFEUaEmM+lNkVlzp/j11Bh/fGRJwhJezOCk8AMy9Ma/eOVKOa Cu6Zx1SKiUVw6dHdMAEiRMLLEZDXTSmxE9Z87nbt6nBVMUTUacWSeNc5IVXIucs29JEr C/K1JFsexWADI1LoZmrf9lBcunQiuop1J5fRmBPSasavuQoFdrOJubVOWBwgl8vqcM+0 LY6w== X-Gm-Message-State: AO0yUKVm2q8MP8ZfQF2hzl7gxROH3HBlRJfjO9GJ7OGDIsFQ+KYA7iL2 QOpGZ5V/ssBHLUVASAn54dGqeJ1XuShCDOQafpjnZA== X-Google-Smtp-Source: AK7set9qzhTKfHxML+MgHqsSuJRnPMnUneATXidb0+SUYjXa+h00K/U7lnIkoXpbPJNdsASUF+bM31GH9FkDBDE6CkE= X-Received: by 2002:a05:651c:11c6:b0:295:d460:5a2d with SMTP id z6-20020a05651c11c600b00295d4605a2dmr4907216ljo.2.1678231674431; Tue, 07 Mar 2023 15:27:54 -0800 (PST) MIME-Version: 1.0 References: <20230306225024.264858-1-axelrasmussen@google.com> <20230306225024.264858-4-axelrasmussen@google.com> In-Reply-To: From: Axel Rasmussen Date: Tue, 7 Mar 2023 15:27:17 -0800 Message-ID: Subject: Re: [PATCH v3 3/5] mm: userfaultfd: combine 'mode' and 'wp_copy' arguments To: Peter Xu Cc: Alexander Viro , Andrew Morton , Hugh Dickins , Jan Kara , "Liam R. Howlett" , Matthew Wilcox , Mike Kravetz , Mike Rapoport , Muchun Song , Nadav Amit , Shuah Khan , James Houghton , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 8227B8000C X-Stat-Signature: 8f7mp5ur5tp8gkw6pax1aezqmdxgirmf X-HE-Tag: 1678231676-640253 X-HE-Meta: U2FsdGVkX19J/3/1XyjDwtlsDNUuYrl+WFD+DNQdkOtYDaEtyRjx+ngiVXk93RRZJiidJAVjwfAh9UZTdOhjzUKHW+W4q2GV4/aJdsbBrzmBLeUbXzJM+6XaTJY8P0RVonoTFFPFiN7aqk1V9VmeQ6ONmclfOlifPPM+9FOveDvW0vKg8P/ZPg3MCmAAIyKLWxTUhBnb+cjP5X9hc17j0KGaC8etNSxNKPUqtGOxXRN9tkKs0R/i8fZZo9BJaO1LGSL5DnwtCwPQTgfIDNRVjnD5Kn8oUgUOcR5vWAGmxqG/jAxG30vblvYeV6f8n7OcQbPlYRlSLcKMWFPLVr4qGnoHoyO7BaD5vo634frYw5b8i6H4FTeDyt2e4CdgD/6rF45E3CjQOTBPcGN2sQGYOuWjXombKrioIAipsYWSMBZPb5asSNpVp1UqttvA/3gLcZHO96Y+7Me1kVRumZRb8dNJTXYJzvbk90BTWe0hCRM9HXPpbJtmnI1oNvOvzg6CVArgQ1ZyzaEJatRjKRgfL5XGStiPSXXq5R9vNB/q6HsbE8UTGZW66ATkUiSdVttdfWXmn9dbdKZNwggB/lnPsIEq5a2fOw4Mp9ZYzdz0ATlA/7Tjhk3k3+V1oWCeRLUAVgT1XcefRtf2Jrc9JjucCTJQtOojpyhkmHvDzsWjNq3EvKtJf+/sUSRAUqapOSJqzi00zYMWS0NaE8IkUo89ZTJml6hNoGAzs2R4pkHltvoGg+j2AD9M+De+EPQVAM4P4p3mCahYpRjCYhYv+8JYwbv/OGxekLxSCjYXqs+LTZM8fnbtS7unwoipsqxe34AzbSdCAOBIEkK9tV+xNFl059/wJiA31y9HCIW1VAOTXdwi7nVHb0xXeNRMtsyJKn7043/xHomSEI3vnQQDeih3uDsDCTrz0Nv7qehmfz3F6Z+NFiOod1hdiU0fmeySnupbNo+VptwTYAxObyr8U3b P3wUPbRm EEnbAK/y+myFKGyCLnlUiJQQqhsYVkvkaaAFWAg7yGdpiV7KOCvKFrJwP4+Nh8vFhwEA3vf5TR9IDr7vZlAbtgA/OAPBoSL07Z9i+0goLcIZXeLJny763M9Lk2NHif1Z+IxMAYBhYRCqchWt7XsCZyHcbcTjKt5eacxPABrwsNbujv27u2YZZK2S7mTN+EBLlp9W9qYEnBU5N7xTM0tnLO83bWaKWCki+0GJIQZxfOLYpQJq8s4hBvROuvnT0+6UqKtr/8+BGDuTch6Sjc4/+zSu7/3Xa+CNrdl+8XQWJhMAN0qyc/Pc8UaD7SMlOyGHpOrORneYTqdnz66r+DbKskJ+bmHozDjtNJIb8WexQJTKxkQJt9o0kHvlkzPL26D002cyrcEeS1o2FtkhIbJMoNSSNkzjwmW+HJgm/MWGN2EsSDbo= 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 Mon, Mar 6, 2023 at 5:00=E2=80=AFPM Peter Xu wrote: > > On Mon, Mar 06, 2023 at 02:50:22PM -0800, Axel Rasmussen wrote: > > Many userfaultfd ioctl functions take both a 'mode' and a 'wp_copy' > > argument. In future commits we plan to plumb the flags through to more > > places, so we'd be proliferating the very long argument list even > > further. > > > > Let's take the time to simplify the argument list. Combine the two > > arguments into one - and generalize, so when we add more flags in the > > future, it doesn't imply more function arguments. > > > > Since the modes (copy, zeropage, continue) are mutually exclusive, stor= e > > them as an integer value (0, 1, 2) in the low bits. Place combine-able > > flag bits in the high bits. > > > > This is quite similar to an earlier patch proposed by Nadav Amit > > ("userfaultfd: introduce uffd_flags" - for some reason Lore no longer > > has a copy of the patch). The main difference is that patch only handle= d > > Lore has. :) > > https://lore.kernel.org/all/20220619233449.181323-2-namit@vmware.com > > And btw sorry to review late. > > > flags, whereas this patch *also* combines the "mode" argument into the > > same type to shorten the argument list. > > > > Acked-by: James Houghton > > Signed-off-by: Axel Rasmussen > > Mostly good to me, a few nitpicks below. > > [...] > > > +/* A combined operation mode + behavior flags. */ > > +typedef unsigned int __bitwise uffd_flags_t; > > + > > +/* Mutually exclusive modes of operation. */ > > +enum mfill_atomic_mode { > > + MFILL_ATOMIC_COPY =3D (__force uffd_flags_t) 0, > > + MFILL_ATOMIC_ZEROPAGE =3D (__force uffd_flags_t) 1, > > + MFILL_ATOMIC_CONTINUE =3D (__force uffd_flags_t) 2, > > + NR_MFILL_ATOMIC_MODES, > > }; > > I never used enum like this. I had a feeling that this will enforce > setting the enum entries but would the enforce applied to later > assignments? I'm not sure. > > I had a quick test and actually I found sparse already complains about > calculating the last enum entry: > > ---8<--- > $ cat a.c > typedef unsigned int __attribute__((bitwise)) flags_t; > > enum { > FLAG1 =3D (__attribute__((force)) flags_t) 0, > FLAG_NUM, > }; > > void main(void) > { > uffd_flags_t flags =3D FLAG1; > } > $ sparse a.c > a.c:5:5: error: can't increment the last enum member > ---8<--- > > Maybe just use the simple "#define"s? Agreed, if sparse isn't happy with this then using the force macros is pointless. The enum is valuable because it lets us get the # of modes; assuming we agree that's useful below ... > > > > > +#define MFILL_ATOMIC_MODE_BITS (const_ilog2(NR_MFILL_ATOMIC_MODES - 1)= + 1) > > Here IIUC it should be "const_ilog2(NR_MFILL_ATOMIC_MODES) + 1", but > maybe.. we don't bother and define every bit explicitly? If my reading of const_ilog2's definition is correct, then: const_ilog2(4) =3D 2 const_ilog2(3) =3D 1 const_ilog2(2) =3D 1 For either 3 or 4 modes, we need 2 bits to represent them (0, 1, 2, 3), i.e. we want MFILL_ATOMIC_MODE_BITS =3D 2. I think this is correct as is, because const_ilog2(4 - 1) + 1 =3D 2, and const_ilog2(3 - 1) + 1 =3D 2. In other words, I think const_ilog2 is defined as floor(log2()), whereas what we want is ceil(log2()). The benefit of doing this vs. just doing defines with fixed values is, if we ever added a new mode, we wouldn't have to do bit twiddling and update the mask, flag bits, etc. - it would happen "automatically". I prefer it this way, but I agree it is a matter of opinion / taste. :) If you or others feel strongly this is overcomplicated, I can take the other approach. > > > +#define MFILL_ATOMIC_BIT(nr) ((__force uffd_flags_t) BIT(MFILL_ATOMIC_= MODE_BITS + (nr))) > > +#define MFILL_ATOMIC_MODE_MASK (MFILL_ATOMIC_BIT(0) - 1) > > + > > +/* Flags controlling behavior. */ > > +#define MFILL_ATOMIC_WP MFILL_ATOMIC_BIT(0) > > [...] > > > @@ -312,9 +312,9 @@ static __always_inline ssize_t mfill_atomic_hugetlb= ( > > unsigned long dst_start, > > unsigned long src_start, > > unsigned long len, > > - enum mcopy_atomic_mode mode= , > > - bool wp_copy) > > + uffd_flags_t flags) > > { > > + int mode =3D flags & MFILL_ATOMIC_MODE_MASK; > > struct mm_struct *dst_mm =3D dst_vma->vm_mm; > > int vm_shared =3D dst_vma->vm_flags & VM_SHARED; > > ssize_t err; > > @@ -333,7 +333,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb= ( > > * by THP. Since we can not reliably insert a zero page, this > > * feature is not supported. > > */ > > - if (mode =3D=3D MCOPY_ATOMIC_ZEROPAGE) { > > + if (mode =3D=3D MFILL_ATOMIC_ZEROPAGE) { > > The mode comes from "& MFILL_ATOMIC_MODE_MASK" but it doesn't quickly tel= l > whether there's a shift for the mask. > > Would it look better we just have a helper to fetch the mode? The functi= on > tells that whatever it returns must be the mode: > > if (uffd_flags_get_mode(flags) =3D=3D MFILL_ATOMIC_ZEROPAGE) > > We also avoid quite a few "mode" variables. All the rest bits will be fi= ne > to use "flags & FLAG1" if it's a boolean (so only this "mode" is slightly > tricky). Agreed, this is simpler. I'll make this change. > > What do you think? > > Thanks, > > -- > Peter Xu >