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 2BA78C678D4 for ; Tue, 7 Mar 2023 18:53:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 931F66B0071; Tue, 7 Mar 2023 13:53:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8E1E76B0073; Tue, 7 Mar 2023 13:53:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 78287280001; Tue, 7 Mar 2023 13:53:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 6A6666B0071 for ; Tue, 7 Mar 2023 13:53:20 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 34DE4C0338 for ; Tue, 7 Mar 2023 18:53:20 +0000 (UTC) X-FDA: 80543000160.14.D28D78B Received: from mail-lj1-f182.google.com (mail-lj1-f182.google.com [209.85.208.182]) by imf23.hostedemail.com (Postfix) with ESMTP id 5453914000A for ; Tue, 7 Mar 2023 18:53:18 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=KD0Omca0; spf=pass (imf23.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.208.182 as permitted sender) smtp.mailfrom=axelrasmussen@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678215198; 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=l+t4fNsGuLQ/9qhHPQFOKl4VOAQaztAyIDwfYabOYF8=; b=TzktvFLLeBcflUf4XoPCx9bCt9IUjGxwNwbK6esrNM6zEUQyxSzGw/3a7uSCfi74BJPTOJ Kp9G4pJOVSpr4taTLkjcAmewjBNak54E+uEbzcI8gjNtp/CEBpoCXrjK7Y54dHEfWPi4av AOBxiiRlaw6LaVXtYWSVcaNYxcJZXqs= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=KD0Omca0; spf=pass (imf23.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.208.182 as permitted sender) smtp.mailfrom=axelrasmussen@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678215198; a=rsa-sha256; cv=none; b=AQ7haq8fN6ZyEfqHMM3/pa3Mz58p/2PAS5wRrGi78SUaPAcdpID73/bdZT0fd2oz8NhVr1 oi4XSRI4S5L2/j5Q7pG7GFcbwQY9jmKvwV48c/dBhMHWc3QvX0JOJ2eLee/Z2TyQqIIhH+ fQWZpp7atI/qO2B9RUNS31Cqg6jDrZg= Received: by mail-lj1-f182.google.com with SMTP id g18so14230266ljl.3 for ; Tue, 07 Mar 2023 10:53:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678215196; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=l+t4fNsGuLQ/9qhHPQFOKl4VOAQaztAyIDwfYabOYF8=; b=KD0Omca0Ir8qtCtqLWtM0sdOoO6ef+BYEsuJg/YkkisV5ux3mi1lnPoXOU6CIULA+y FLQLQ0nugpTqK0A89SPrfAhcapbPbgC3I0RLaTfwgJI6hGsiocViWOzt+g7jQuxZraiF frjpcCrPSMWlA8VyvAoqla75QESPqf2ztl4pwwNWY0zDjKdbrE0FyOmwBq/vvId0lK/L xMlhERiEFQcetcEgaVrZcr1MlSfMPtRQpjTOhCIfUa4M7hwOA31GhrE4+C2bjgQvKa1q FKwWMNdeWCyKdSQkrt2sk/e2TVJNlpKacvavFMUMeJyCFTGIJcNZksQ94VqANw0RKCHQ GDiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678215196; h=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=l+t4fNsGuLQ/9qhHPQFOKl4VOAQaztAyIDwfYabOYF8=; b=g9+ivHQFHD/Vu6Z5FO3zjGYWsvM2M1Yl2KJESQpc8DpbCJ27RNbURuIcEmJQ5Bnd3X B+XqzpaWFHzqZoMT+W8n1Uxrr6Oi4k3uNpi2f+yXRMu2aCnTPTVfOvqmtsw2dsb/AUfZ 2QJ2cJwrcQESfNHZnneK2YesxF1bzhcooU4/ZitDqOC3qb0FOYvXhVw7D9/Fu7WjFQ6/ +pKBobSDV6gdvEpnxk/KnqHzy0AfKrKiOW2vXZKtjmXrZyVkFp32awOSYByJh6AOc5wy 6jGtxmoXriowIA7i0X8uMObVE40cQEHLTrmO2w3AieiJ0q5YfZE6FE5B1o5VNszdAtPp GFRQ== X-Gm-Message-State: AO0yUKVNOA/AYzQFg6SQJguYkuHcN3HGhYCeDqbcbZxwM4AnEwnuP6Wf fIK6//fMEhp7Cic9nB1NF2C04x/kY3PrET43rUWpVA== X-Google-Smtp-Source: AK7set+DkY7n8PEixzFt50Mmz0A0m9g6yHicymAzB8U0OOev3FdH34qmEMrmDo+cCvwmkbo4342uzbzaGwwGS85HZ5M= X-Received: by 2002:a05:651c:1725:b0:295:8ef2:8707 with SMTP id be37-20020a05651c172500b002958ef28707mr4698679ljb.2.1678215196362; Tue, 07 Mar 2023 10:53:16 -0800 (PST) MIME-Version: 1.0 References: <20230306225024.264858-1-axelrasmussen@google.com> <20230306225024.264858-5-axelrasmussen@google.com> <209840DC-22D3-422E-A035-B7ADCB8E531E@vmware.com> In-Reply-To: <209840DC-22D3-422E-A035-B7ADCB8E531E@vmware.com> From: Axel Rasmussen Date: Tue, 7 Mar 2023 10:52:39 -0800 Message-ID: Subject: Re: [PATCH v3 4/5] mm: userfaultfd: don't separate addr + len arguments To: Nadav Amit Cc: Peter Xu , Alexander Viro , Andrew Morton , Hugh Dickins , Jan Kara , "Liam R. Howlett" , Matthew Wilcox , Mike Kravetz , Mike Rapoport , Muchun Song , Shuah Khan , James Houghton , linux-fsdevel , kernel list , linux-mm , linux-kselftest Content-Type: multipart/alternative; boundary="000000000000a368e105f653ec75" X-Stat-Signature: 63qxfgz8zneyeqz6r9s1u5siw8jt9ija X-Rspam-User: X-Rspamd-Queue-Id: 5453914000A X-Rspamd-Server: rspam06 X-HE-Tag: 1678215198-66609 X-HE-Meta: U2FsdGVkX18/HF7n2Ujc3Vaqi+b7b25U2LiC5Fyg7rVd9MHXVkxG/3g5kaRNmAeqtS0dHxh0SdjhFivHFYbCU9zqkS7Hda8dQ780HqLk1EEYgGtigwU5d9cxwRDuBS63WG6J9HAL15hUoAmY4ua2P7YIYXn6gBPEnKxyw+IRGRYc85xuSHtL2OqwFAZ6VBLfovVUQmdDR0WS9Seo4U9BFO7lySD6G+z/KPEgBhGF0zlANTxtNPAWZgmZcrWu3PmocKgg4f2UaRVEK3F7/9ZVtGKay0ozPVlMvD3mf0JBHOSWOy43ZvBB9n6N7ev0dTezCBOQdMFFrwmRY4IspK3MeoWkGq6jX+pwL3XgeABRAytqQSXhITcBA63MseGGWnGX/mik5pgJ9nfmoXkYy7Bt0wZynrvTSWD7+pfzAfg1IPJo0Z6ovfT/lKx9vprdI5N8OKTk6RMCqXFZ1/6Vmq8h4J7CqFN0pkfYDgqynD4P/8Lb1r++cHkWhHhCFynnHcaK0ZYAZIXVbAwwkgVyVbeWMB64dMNRNAnUKSciv0rRuZH+JcxufgMbo9daEj+5YBzAkQqYTziRMTVRvHa2a+7WGt49JNTkvTo/NAs8oiTCg2IhtrCNkl2uCxeOz0H8lnCsuPWxp50q1tUAHWEGNOa/w+ZxV7pN/BF95f7v+0SaOeMXoF9UL+WwHeWMd2OFbPr5TuDuNZSKuA13uI9kMuOdg0KbxplxbhRsPrK3I3HQWOstl3zP/Oty8TvqRCmDmGk2U0AoccroTqsLoQaNLvpHgIIRLI/2A+egoCHjOHXqOr0qTiuXere7VIfQE1uYpOf3ene9eZO13X0adowPOV8duv4C4RXtaGiG0ZtfDx7xmkgQlUatPceWevSSjiaS+kwRrL9oREVSIGA4Vok8e1LUrxVJerU/h5yC+i/VPWDGmpx21c3b/RrDmRHc+VbvsM87F7tDHOyRWAEiTDm18ST yfzBGuOc vbbTSKNBpiZYALA7q8rFn5J2U+nesDXemOpNcqL6l3R64d+/32TlRBw0KmcNIfdG0DT03CO+vXSAUeDAIK3jT6ion2booIZBJ9uPsyLA9TnEuKDz1Yj1QHZXGznjw8D5d5/4oNgdSw08T11ZASUtFsGPdGyQJ8Y8bXc3aV8hKaB8eh3kV3X7jg4itH35KwnZStEd+RLOShezwXTO8gYhiQoxx8PZZJprF039Wk2ukT6IC1jlfl4F/IrGf0IlMSzhRWyvnqRP1vnJupayWneMP6ZKmjK1IwOg6RZlqmscGJ2QQQ4qeS2z9MZIQ3CWaPNhJTM3im0n6xtFRsAKWcIKYnU2Gvo16oji6535uk6Tbs/1X9gqtKZyUBmIt1iLrarNxzkzVEVUeIz29/P+nDX3BLgdMpE4K1E9r/vQqvW2o0w5JpeM= 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: --000000000000a368e105f653ec75 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Mar 6, 2023 at 5:30=E2=80=AFPM Nadav Amit wrote: > > > > On Mar 6, 2023, at 5:19 PM, Peter Xu wrote: > > > > !! External Email > > > > On Mon, Mar 06, 2023 at 02:50:23PM -0800, Axel Rasmussen wrote: > >> We have a lot of functions which take an address + length pair, > >> currently passed as separate arguments. However, in our userspace API = we > >> already have struct uffdio_range, which is exactly this pair, and this > >> is what we get from userspace when ioctls are called. > >> > >> Instead of splitting the struct up into two separate arguments, just > >> plumb the struct through to the functions which use it (once we get to > >> the mfill_atomic_pte level, we're dealing with single (huge)pages, so = we > >> don't need both parts). > >> > >> Relatedly, for waking, just re-use this existing structure instead of > >> defining a new "struct uffdio_wake_range". > >> > >> Signed-off-by: Axel Rasmussen > >> --- > >> fs/userfaultfd.c | 107 +++++++++++++--------------------- > >> include/linux/userfaultfd_k.h | 17 +++--- > >> mm/userfaultfd.c | 92 ++++++++++++++--------------- > >> 3 files changed, 96 insertions(+), 120 deletions(-) > >> > >> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > >> index b8e328123b71..984b63b0fc75 100644 > >> --- a/fs/userfaultfd.c > >> +++ b/fs/userfaultfd.c > >> @@ -95,11 +95,6 @@ struct userfaultfd_wait_queue { > >> bool waken; > >> }; > >> > >> -struct userfaultfd_wake_range { > >> - unsigned long start; > >> - unsigned long len; > >> -}; > > > > Would there still be a difference on e.g. 32 bits systems? > My assumption is that __u64 is at least 64 bits wide on all platforms, and so it is sufficient. I believe the standard allows unsigned long to be 32-bits, so __u64 may be overkill on some platforms, but to me the cost is small enough I'd prefer to avoid defining a second almost-identical structure. Then again though as I say below, I don't feel strongly about this refactor= . > > > > [...] > > > >> static __always_inline int validate_range(struct mm_struct *mm, > >> - __u64 start, __u64 len) > >> + const struct uffdio_range > *range) > >> { > >> __u64 task_size =3D mm->task_size; > >> > >> - if (start & ~PAGE_MASK) > >> + if (range->start & ~PAGE_MASK) > >> return -EINVAL; > >> - if (len & ~PAGE_MASK) > >> + if (range->len & ~PAGE_MASK) > >> return -EINVAL; > >> - if (!len) > >> + if (!range->len) > >> return -EINVAL; > >> - if (start < mmap_min_addr) > >> + if (range->start < mmap_min_addr) > >> return -EINVAL; > >> - if (start >=3D task_size) > >> + if (range->start >=3D task_size) > >> return -EINVAL; > >> - if (len > task_size - start) > >> + if (range->len > task_size - range->start) > >> return -EINVAL; > >> return 0; > >> } > > > > Personally I don't like a lot on such a change. :( It avoids one > parameter > > being passed over but it can add a lot indirections. > > > > Do you strongly suggest this? Shall we move on without this so to not > > block the last patch (which I assume is the one you're looking for)? > I don't feel strongly, I'm fine with dropping this patch. I'll make that change in a v4 (I think there will be some conflicts to resolve in the patches after this one, so I'll post a new version to avoid troubling anyone). > > Just in case you missed, it is __always_inline, so I presume that from a > generated code point-of-view it is the same. > > Having said that, small assignments to local start, let and range variabl= es > would make the code easier to read and reduce the change-set. > > --000000000000a368e105f653ec75 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Mar 6, 2023 at 5:30=E2=80=AFP= M Nadav Amit <namit@vmware.com&g= t; wrote:


> On Mar 6, 2023, at 5:19 PM, Peter Xu <peterx@redhat.com> wrote:
>
> !! External Email
>
> On Mon, Mar 06, 2023 at 02:50:23PM -0800, Axel Rasmussen wrote:
>> We have a lot of functions which take an address + length pair, >> currently passed as separate arguments. However, in our userspace = API we
>> already have struct uffdio_range, which is exactly this pair, and = this
>> is what we get from userspace when ioctls are called.
>>
>> Instead of splitting the struct up into two separate arguments, ju= st
>> plumb the struct through to the functions which use it (once we ge= t to
>> the mfill_atomic_pte level, we're dealing with single (huge)pa= ges, so we
>> don't need both parts).
>>
>> Relatedly, for waking, just re-use this existing structure instead= of
>> defining a new "struct uffdio_wake_range".
>>
>> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
>> ---
>> fs/userfaultfd.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |= 107 +++++++++++++---------------------
>> include/linux/userfaultfd_k.h |=C2=A0 17 +++---
>> mm/userfaultfd.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |= =C2=A0 92 ++++++++++++++---------------
>> 3 files changed, 96 insertions(+), 120 deletions(-)
>>
>> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
>> index b8e328123b71..984b63b0fc75 100644
>> --- a/fs/userfaultfd.c
>> +++ b/fs/userfaultfd.c
>> @@ -95,11 +95,6 @@ struct userfaultfd_wait_queue {
>>=C2=A0 =C2=A0 =C2=A0 bool waken;
>> };
>>
>> -struct userfaultfd_wake_range {
>> -=C2=A0 =C2=A0 =C2=A0unsigned long start;
>> -=C2=A0 =C2=A0 =C2=A0unsigned long len;
>> -};
>
> Would there still be a difference on e.g. 32 bits systems?

My assumption is that __u64 is at least 64 bits wi= de on all platforms, and so it is sufficient. I believe the standard allows= unsigned long to be 32-bits, so __u64 may be overkill on some platforms, b= ut to me the cost is small enough I'd prefer to avoid defining a second= almost-identical structure.

Then again though as = I say below, I don't feel strongly about this refactor.
=C2= =A0
>
> [...]
>
>> static __always_inline int validate_range(struct mm_struct *mm, >> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0__u64 start, __u64 len)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0const struct uffdio_range *range)
>> {
>>=C2=A0 =C2=A0 =C2=A0 __u64 task_size =3D mm->task_size;
>>
>> -=C2=A0 =C2=A0 =C2=A0if (start & ~PAGE_MASK)
>> +=C2=A0 =C2=A0 =C2=A0if (range->start & ~PAGE_MASK)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; >> -=C2=A0 =C2=A0 =C2=A0if (len & ~PAGE_MASK)
>> +=C2=A0 =C2=A0 =C2=A0if (range->len & ~PAGE_MASK)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; >> -=C2=A0 =C2=A0 =C2=A0if (!len)
>> +=C2=A0 =C2=A0 =C2=A0if (!range->len)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; >> -=C2=A0 =C2=A0 =C2=A0if (start < mmap_min_addr)
>> +=C2=A0 =C2=A0 =C2=A0if (range->start < mmap_min_addr)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; >> -=C2=A0 =C2=A0 =C2=A0if (start >=3D task_size)
>> +=C2=A0 =C2=A0 =C2=A0if (range->start >=3D task_size)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; >> -=C2=A0 =C2=A0 =C2=A0if (len > task_size - start)
>> +=C2=A0 =C2=A0 =C2=A0if (range->len > task_size - range->= start)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; >>=C2=A0 =C2=A0 =C2=A0 return 0;
>> }
>
> Personally I don't like a lot on such a change. :( It avoids one p= arameter
> being passed over but it can add a lot indirections.
>
> Do you strongly suggest this?=C2=A0 Shall we move on without this so t= o not
> block the last patch (which I assume is the one you're looking for= )?

I don't feel strongly, I'm f= ine with dropping this patch. I'll make that change in a v4 (I think th= ere will be some conflicts to resolve in the patches after this one, so I&#= 39;ll post a new version to avoid troubling anyone).
=C2=A0
=

Just in case you missed, it is __always_inline, so I presume that from a generated code point-of-view it is the same.

Having said that, small assignments to local start, let and range variables=
would make the code easier to read and reduce the change-set.

--000000000000a368e105f653ec75--