From:   Nadav Amit <namit@vmware.com>
To:     Peter Xu <peterx@redhat.com>
CC:     Axel Rasmussen <axelrasmussen@google.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>, Jan Kara <jack@suse.cz>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Matthew Wilcox <willy@infradead.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Mike Rapoport <rppt@kernel.org>,
        Muchun Song <muchun.song@linux.dev>,
        Shuah Khan <shuah@kernel.org>,
        James Houghton <jthoughton@google.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        linux-mm <linux-mm@kvack.org>,
        linux-kselftest <linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH v3 4/5] mm: userfaultfd: don't separate addr + len
 arguments
Thread-Topic: [PATCH v3 4/5] mm: userfaultfd: don't separate addr + len
 arguments
Thread-Index: AQHZUH4V9u80YHiRfUSB2Rpal/Yol67uhN4AgAACwIA=
Date:   Tue, 7 Mar 2023 01:29:57 +0000
Message-ID: <209840DC-22D3-422E-A035-B7ADCB8E531E@vmware.com>
References: <20230306225024.264858-1-axelrasmussen@google.com>
 <20230306225024.264858-5-axelrasmussen@google.com> <ZAaRPCntR94hGBL2@x1n>
In-Reply-To: <ZAaRPCntR94hGBL2@x1n>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?JAl9XbqkuPiXObq3jEBJrWXOY6BQFVoV+8c6p85FHramlhqW9HzMTpryexU1?=
 =?us-ascii?Q?FCYMbPptBxsGuMFqtcze87b4Tx30Qq79huQqijmVstHeuiCBq6cX2HStrTuo?=
 =?us-ascii?Q?ASNDjm447sQBmgEoazTBX+zJ3s4/mFVwM4weYbkxQjjePLbec0G7KPFuHAI9?=
 =?us-ascii?Q?gEbJqNYaHAi0jX5nwsYoFEX1gl2yKz/gKIW7KjcXN8Ab6Zp6Suju/zionAyk?=
 =?us-ascii?Q?w12Wr4MT8/nhLWb8REzs7+FmMHl9JBPPn2G78vWq3V0R9d8yITGf1/9SBswG?=
 =?us-ascii?Q?pdPuYHYRBKx5My63tHvDKSwbaArqu6S9M03/2Fyo9SjHsK2PFgC2n5zhbqUh?=
 =?us-ascii?Q?DN5knr+EUk7zienS/+Pkko8jmo8VwNShal7EyBnIP5f6KnAG64RvEmHhysis?=
 =?us-ascii?Q?M9PS1aflRk3Ibihl2b47azm5n1Y1jNccYAH1MVDAtsNJAV7kueMHBNgddSCY?=
 =?us-ascii?Q?T9W2N8M5WKLbNkPVuEiLhKgzC9EWy3lMqo6E7OuHgox4K4IjzxTMSPUCHJNf?=
 =?us-ascii?Q?dofNl2jjKoz3JYqeXIQX2sK20IrJ5KFtNAKGGVIgBICpYS4cheE9jIFi6hOW?=
 =?us-ascii?Q?78wzWU6c6x8mJYH1WU/z35w6VTF9Ch0FiJ9IpURTR90zEO3S43feefw2G4y0?=
 =?us-ascii?Q?6VqtzLzskQx+g/jzs3At2EZo8tk9sQRCqWQhgfeqUt9CDPUlIMnoQOe7g2rZ?=
 =?us-ascii?Q?zYF/IHet1ZOCi99eu/fWZIKJWCslnhMMwvMBjhwZsOTXch+P2p1RNfDSdCd0?=
 =?us-ascii?Q?qmOlmNoiScL2bwxYu39wuoVzSuNpbIVLsdp1VgA/aLXGZB3Ta+ysHlUypEPP?=
 =?us-ascii?Q?NHplOuLCXKUBLA3xdsayJGqSOwVZmeBgt0auR86afJZ0F5GskyB43VnnxG06?=
 =?us-ascii?Q?pKdWNeBMvMHqdG0dtID0myn7kns5TRe/wj//YJtKM127LCflTKPhJTmd+pyz?=
 =?us-ascii?Q?CiuBVZZfn257qE79iYNwvF7TRmGeQOgW/iqYvYpCvWalx6kN83M/C38kICSF?=
 =?us-ascii?Q?7DtFExeZb4YWFO3ppphhkh2WfNyW4eAHL4itQT/sT5tvAoiuBDts+0um/WF5?=
 =?us-ascii?Q?zWFuFNUwP2v/nGtqZqUJhCTRqVHnWmA6UHNr/uyoS1unoCGmPxYJxq/WTFXy?=
 =?us-ascii?Q?bWInDS3rB0X/tyxo86zdIP+JltxkVUT2qAnbuxN1gwJvqG8SKRSt+fmxu+cN?=
 =?us-ascii?Q?jqmRlzsXuJHhMOfKXktCugPDEwwqJivj1dn33ZlAnEzveJL2EIq38RB/S+CF?=
 =?us-ascii?Q?SnJZ21wAd2+MLbHNAuNjjwalwh8P3LqSSrvGVS2nM1oEIHDwvDgAng9uA7Hc?=
 =?us-ascii?Q?/C6gsJxIvO4I75RJkNhO5caSIM7/C/e/yqynwd3257BtLdYV0rEdjSWo/Haf?=
 =?us-ascii?Q?SLUCwt+U8LMoAyJiGMiQA3XHVf6oo/Yu8KGjpBCcU1akDAHVVfKo708e+swc?=
 =?us-ascii?Q?fSMGlbyy0fr13WnF87tZyVXD+Tu8B9EvxBLVZFHXWRYCVdqgHYB/r46ljkb7?=
 =?us-ascii?Q?w4iTeYUyy2/62Iiw2ELKWxLWTMYSnJ+IdIKNuka32E3Vh/zGD+DbNJqAIG+N?=
 =?us-ascii?Q?nLtV5IPAf9ikfBXCVyWQ6+GMqniwzuvB3UHwDa25?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <04BC7981C92E304AABDBF36A01B39BCC@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: vmware.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BY3PR05MB8531.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 224da4cf-394e-410b-29c2-08db1eab7672
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Mar 2023 01:29:57.8335
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: bHmQBefQpAG+4IQ35WbXzmFFMKdFaCDjXJor1/DFtCug64QrnH01AN2bMvdKggO8MkzUckEwatZE0B4U3B2lGQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR05MB6667
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Mar 6, 2023, at 5:19 PM, Peter Xu <peterx@redhat.com> wrote:
>=20
> !! External Email
>=20
> 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.
>>=20
>> 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).
>>=20
>> Relatedly, for waking, just re-use this existing structure instead of
>> defining a new "struct uffdio_wake_range".
>>=20
>> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
>> ---
>> fs/userfaultfd.c              | 107 +++++++++++++---------------------
>> include/linux/userfaultfd_k.h |  17 +++---
>> mm/userfaultfd.c              |  92 ++++++++++++++---------------
>> 3 files changed, 96 insertions(+), 120 deletions(-)
>>=20
>> 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;
>> };
>>=20
>> -struct userfaultfd_wake_range {
>> -     unsigned long start;
>> -     unsigned long len;
>> -};
>=20
> Would there still be a difference on e.g. 32 bits systems?
>=20
> [...]
>=20
>> 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;
>>=20
>> -     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;
>> }
>=20
> Personally I don't like a lot on such a change. :( It avoids one paramete=
r
> being passed over but it can add a lot indirections.
>=20
> 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)?

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.