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 BBADDC64EC4 for ; Tue, 7 Mar 2023 01:00:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 333336B0075; Mon, 6 Mar 2023 20:00:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2EBBE280002; Mon, 6 Mar 2023 20:00:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 18336280001; Mon, 6 Mar 2023 20:00:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 097616B0075 for ; Mon, 6 Mar 2023 20:00:44 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id CF0EB404A2 for ; Tue, 7 Mar 2023 01:00:43 +0000 (UTC) X-FDA: 80540297166.12.2EAFE2D Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf06.hostedemail.com (Postfix) with ESMTP id 60813180029 for ; Tue, 7 Mar 2023 01:00:41 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Z0Q0SYpr; spf=pass (imf06.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678150841; 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=VHFntejT56JbD/rUEQWs9tWFnUbRv9RbZFyVXVHrMio=; b=iJFu9GzCEIgC5PorrxrZR7ueHiXz6i5rxAdfGRKY6gCaXmvtpJgqRhTlphSU8WX2qyYwHx yDkzLYU4pWhi+7MhTPAIeagfPNbx2gosHupQ+j+W0H0dINVVOKSA6xUbfrZW83X6uLn7or TpxigN12hJ6mmcV6MoGEO6OA+PTGqcg= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Z0Q0SYpr; spf=pass (imf06.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678150841; a=rsa-sha256; cv=none; b=uIN8Z3kghlRQc7VFtQj4lohR0EHyIS+vdW56l3kaWaIqZC3KWP0nXCSLWGA9A9wMPNIZ1s Of8D76K2ZQ3X42sG8Xh2ylumsS01Xsx3FJed+lM/yl4kKIRXq0j4WoHL7xllgpoKBg9XaB Fpq4UseLH1t9sckm8JttRxZgATMzElU= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1678150840; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=VHFntejT56JbD/rUEQWs9tWFnUbRv9RbZFyVXVHrMio=; b=Z0Q0SYprI3rnoGDgyEAuLgy/++qHk3EMx6m/G163T5liwNlwrmuDAJG4uE3AoQ48N3CDKi NdHEqY34xAWTrkHBf+/1imvdh+L03eDbC8rDfJ9Co0kczFrS1TG+24WUWjRkrKQEZVw3ct P2ICCCjehAR/5ZZuXs70H5cMk53EQ4E= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-251-zilQ_T7JPn-1im0JjfTutg-1; Mon, 06 Mar 2023 20:00:38 -0500 X-MC-Unique: zilQ_T7JPn-1im0JjfTutg-1 Received: by mail-qv1-f72.google.com with SMTP id pp11-20020a056214138b00b0056c228fa15cso6556857qvb.4 for ; Mon, 06 Mar 2023 17:00:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678150838; 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=VHFntejT56JbD/rUEQWs9tWFnUbRv9RbZFyVXVHrMio=; b=I756w1a+HvwUPnATnixzhFKCctwOXknpCtvp5tlbZPJIvT1m+6mC7zWDZJ9v0aDVon YDyhxWjBAnJ6Fc59j1VkKoQ4lEET6DaLcZESRGqyesStWn36C7fPUWrsnFsNafPWzAbq khgDOa790i5nPk5VtMKzQ82lvvxkDA5x1AyrAXuXbELsdkzhnaABK2L/IiQbMHGN3Y3b 4yqLO7ipEG5UIPe1ijMO55vyNXVXtywlAHjV3/Siv2fSHZ8WMtGX2aFrM24VoWbP04Na lAhBtNKIXgECgzcwcg3enKR4me/Uvqqug6ou5TWubGw1Y55ML+SU5cgnleL+ZNVoQvQe M6XA== X-Gm-Message-State: AO0yUKXd09NPs4nwgR7ylZFUBHXy/8AQ8qWXQKtgzLGazQD86oKLqdMH BVCGtExNkm5XK5lJYEyeltb7tRdbsdMp1SZajBwpDngzJLgPqEVtm+v+WKnOIlR/U29+4RNtlkJ CjgbsCIvqD+U= X-Received: by 2002:ac8:4e49:0:b0:3bf:a564:573b with SMTP id e9-20020ac84e49000000b003bfa564573bmr22624844qtw.0.1678150837976; Mon, 06 Mar 2023 17:00:37 -0800 (PST) X-Google-Smtp-Source: AK7set9mNewiLEJ/Qcl0fkXX58JVHG+qYCqPmSyNkMCeEvogUl0mdbdtpfbHSmtHHB3rrhxwI1DA3g== X-Received: by 2002:ac8:4e49:0:b0:3bf:a564:573b with SMTP id e9-20020ac84e49000000b003bfa564573bmr22624808qtw.0.1678150837670; Mon, 06 Mar 2023 17:00:37 -0800 (PST) Received: from x1n (bras-base-aurron9127w-grc-56-70-30-145-63.dsl.bell.ca. [70.30.145.63]) by smtp.gmail.com with ESMTPSA id v25-20020ac873d9000000b003c033b23a9asm1407231qtp.12.2023.03.06.17.00.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Mar 2023 17:00:37 -0800 (PST) Date: Mon, 6 Mar 2023 20:00:35 -0500 From: Peter Xu To: Axel Rasmussen 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 Subject: Re: [PATCH v3 3/5] mm: userfaultfd: combine 'mode' and 'wp_copy' arguments Message-ID: References: <20230306225024.264858-1-axelrasmussen@google.com> <20230306225024.264858-4-axelrasmussen@google.com> MIME-Version: 1.0 In-Reply-To: <20230306225024.264858-4-axelrasmussen@google.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 60813180029 X-Rspam-User: X-Stat-Signature: ry4ufmjdgawynzrkmjdmw1kakbwg9m8m X-HE-Tag: 1678150841-573991 X-HE-Meta: U2FsdGVkX1+mij4/VTrsLSqGRUbLlOBmyPqEmaC9HGNe0xHnqGhpfScyzdhpViRiKchY2Dr7iKQLCzFUEc+nd+syIHK/xLp4YXGME3zNRINv6EV2gxSk5Cc0xA4yGEGadhOXfOchllupkal8vBnn2WAdhLbNHZifItvWvtCjnJZ1kpwTbcZISdSdhP8tx5Mf+Bkjt+DYalNIB3j512xMKPEs6E/4GZmGTGN4KeShvtcORx5MtZvp4Gx9G7I2wJG8dYGjNfqeIYb0YmlOmPPXWj3CfzVhCeCJ8VdfnP/CxWGKI/H5DoFtLGoG/KO525Hbs4vA67JS6QZe8LHtETf/6m0MHOuM1ZMY03A8oc1bGHHszaZL0Wv0PoqeSMpAsBDw9INx3gd3Dy4yzqJ38p2s1vn/PhRYT3SC8j/bCLCzZMLf9oEQhFqkN2ctHwTyiSqZ0acFiF+ZKFpKuycG25unrdDjEYKx8497JJDHZcEPeMcJeV8+zg8b+r3obOg54PgAWR/vVGs7CWEtVjn1Y0KYzK/oiQX7g2SRT6Lw/N3zv+Fnm9Z1/ey7Um4UcRAFLyvi6gEhwLstXVKsIBTYhjBjCF/n5Zh61/+PxjUjaznonqGz3bau4eeCK6AhSrui1VEYGCwXtwfcuau5gmiE7QUGY92YfORz4QdrFrY7o35CUAlvUTRxHhxUJ+tnfX2KJFPI3iY7zY/ueZYtZXvDXrrBwJNQ92oJUnzKvUxkAexX2jKDI1FhQ1zW4Qt7AQrprkbo3tBe7MQ2zWHaWgbMBLKDrvgsm2Yn7+M/VU39SQCGuf71LOXNKyr3zk9jihcA9g3gIejtg7jMG6OxRak3dSMinuWn9kr07EFyJqd3dbBG2X7GL1KFpxZhPtaaRg2Ey/HoLd0VrDcaQ4CMirpUZ4CDD3Gx7X+DJS9IYn6dIWeWx10CozyFYQbnpMIgVxLNwwGJimNhFEMwmcO7ofbhYrm eZno2U74 aU+VwcMxGpPwFkA215vfv7JJxBqcupxXRC9Wspn/mb/Fea47pcu3BkXC+IslI9cFNNfNF3O5WL8q3sfZTx0wuyliKzSIGFyJmIFnSFqhq1566CgUpp5Ql+vhg+/mhveov59k/aEG2tyun91aNGbJnlsOgAlFe7uOSjUroa+r0BJFEAGaWI10+PiX7p5hnLO1PxZbW+9BKvh6Np6WoT3HLN+s7fN+OFSUC3LIUipaUjRH2XY6/A51Sg5uu6xuVRvbQfjd6Ck+MfvCPLjSr7oWMW/pXWUcrjOFl/5cs9oaBT6rVW37tF5uM8PXQ/wZiB4ykID5ATVVDP3fAfM4GLYGehsGlX5BCJYGpPBcbfqzccglvb25rx/Ed2SWsYlixl6h0x5XlDphz0FeuJEkHyvMpaM3AwtCPIdnWEJlF1jq8NlUl8Uscirq/FDXb4EOy959cBIdgZVnoCEiZ0t7QSsXL1owPgr+SoJc2ale4NrzpXfeOCoKFE1r3EJFzQYeNDMufsG+qSRxa/QL13yTcZpbbJam8Yw== 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 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, store > 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 handled 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 = (__force uffd_flags_t) 0, > + MFILL_ATOMIC_ZEROPAGE = (__force uffd_flags_t) 1, > + MFILL_ATOMIC_CONTINUE = (__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 = (__attribute__((force)) flags_t) 0, FLAG_NUM, }; void main(void) { uffd_flags_t flags = FLAG1; } $ sparse a.c a.c:5:5: error: can't increment the last enum member ---8<--- Maybe just use the simple "#define"s? > > +#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? > +#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 = flags & MFILL_ATOMIC_MODE_MASK; > struct mm_struct *dst_mm = dst_vma->vm_mm; > int vm_shared = 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 == MCOPY_ATOMIC_ZEROPAGE) { > + if (mode == MFILL_ATOMIC_ZEROPAGE) { The mode comes from "& MFILL_ATOMIC_MODE_MASK" but it doesn't quickly tell whether there's a shift for the mask. Would it look better we just have a helper to fetch the mode? The function tells that whatever it returns must be the mode: if (uffd_flags_get_mode(flags) == MFILL_ATOMIC_ZEROPAGE) We also avoid quite a few "mode" variables. All the rest bits will be fine to use "flags & FLAG1" if it's a boolean (so only this "mode" is slightly tricky). What do you think? Thanks, -- Peter Xu