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 3996BC433EF for ; Wed, 15 Jun 2022 18:40:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 914C56B0071; Wed, 15 Jun 2022 14:40:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8C4376B0072; Wed, 15 Jun 2022 14:40:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7643B6B0074; Wed, 15 Jun 2022 14:40: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 669D86B0071 for ; Wed, 15 Jun 2022 14:40:05 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 309AC80FC9 for ; Wed, 15 Jun 2022 18:40:05 +0000 (UTC) X-FDA: 79581334770.24.3E6C0AE Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf09.hostedemail.com (Postfix) with ESMTP id C1EB0140087 for ; Wed, 15 Jun 2022 18:40:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1655318404; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NCUMLxMDONU55N69UPBhfgO9xoAmHEBDMarmTws3Xc0=; b=eEmDVxdHkY7bOtpVDy28S/J2ymO1ZRe1aHkjPrQBO05FDmFse3zg1VQ1PhP5arAOrkynik MJWcXRpCeIVNQ6Zk2mFd4Xskf6Jn7AxHQ9Ig/kHmQGYh9qE9Zj3JnQnEjmdT346IAfiRwm 5/GeVhnYY4OK6TcYTstgDZXT6J04v8E= Received: from mail-il1-f197.google.com (mail-il1-f197.google.com [209.85.166.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-36-GeutawM9O3-YYHs3OxVmAA-1; Wed, 15 Jun 2022 14:40:01 -0400 X-MC-Unique: GeutawM9O3-YYHs3OxVmAA-1 Received: by mail-il1-f197.google.com with SMTP id v1-20020a922e01000000b002d40b2f60e5so8899705ile.13 for ; Wed, 15 Jun 2022 11:40:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=NCUMLxMDONU55N69UPBhfgO9xoAmHEBDMarmTws3Xc0=; b=AHrKDVdGM9/OhqxMF6ijur+Co5x40YIv8YrbSfqpVtMiu1n+yf/6pKHpL3Ak9bsZKY PFIut1ZqLigexYXPGi+UPUtpdKLIsN+U47JgEOCPnGb3x6Jqy6VexO1JkJZB5KO4aBpi 7unLW81pA+I6Z9Ao+dpNBEA6GusnxGi97YKKF7rqG2t3DvJc3lV1gF9iF6rVIctcSCGQ NdsgLWxtytVZVVkIyHy38lrSP5HdDmTXY3y/r6Wn7REfTr+S/P8YW0/ozLcc5hMWw1aF XhNaGoSjksQs8n4o2rlaOX+qtNbbvFhAOa7Dx8/ZBx+AVwDcs/nyqTTv3lFlA24yplwS uzTg== X-Gm-Message-State: AJIora9YOe5hWfUZjArrcdud+70LNPjyUYKG7wvU1rEJu0J1cjnTTll3 idxyRnCiCK4G5ldu6DbdGgcuI5joGrI1JBZHfz4g3l/Q/2v23JrN1OR0NaPXyH5iD25UzzeB86Y 2OMbIII9+7Eo= X-Received: by 2002:a92:d18f:0:b0:2d7:a699:6769 with SMTP id z15-20020a92d18f000000b002d7a6996769mr713410ilz.110.1655318400157; Wed, 15 Jun 2022 11:40:00 -0700 (PDT) X-Google-Smtp-Source: AGRyM1tfUqFZfVWnoHK7aPjyPPmIV8Cih8AXcSVSzZ3QebWPBj6q34uw9sk4+DxoDbeh6ahuMO23NQ== X-Received: by 2002:a92:d18f:0:b0:2d7:a699:6769 with SMTP id z15-20020a92d18f000000b002d7a6996769mr713395ilz.110.1655318399884; Wed, 15 Jun 2022 11:39:59 -0700 (PDT) Received: from xz-m1.local (cpec09435e3e0ee-cmc09435e3e0ec.cpe.net.cable.rogers.com. [99.241.198.116]) by smtp.gmail.com with ESMTPSA id bi11-20020a05663819cb00b00331b6dcfa07sm6335803jab.61.2022.06.15.11.39.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jun 2022 11:39:58 -0700 (PDT) Date: Wed, 15 Jun 2022 14:39:57 -0400 From: Peter Xu To: Nadav Amit Cc: Mike Rapoport , John Hubbard , David Hildenbrand , Linux MM , Mike Kravetz , Hugh Dickins , Andrew Morton , Axel Rasmussen Subject: Re: [PATCH RFC] userfaultfd: introduce UFFDIO_COPY_MODE_YOUNG Message-ID: References: <20220613204043.98432-1-namit@vmware.com> <3eea2e6e-1646-546a-d9ef-d30052c00c7d@redhat.com> <481fc9d0-6122-bf59-9d04-23c10d256764@nvidia.com> <0BB58ACF-2801-4622-BF3B-9913A23AE46C@gmail.com> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655318404; a=rsa-sha256; cv=none; b=6rqIBEaCtFLQ4h1+CReLZBcMgtpVGvRhdctTLU/n9huKKeiM0duGEqFLBdPN26pTh1lCBN //aPVw+ytrz/42fnE5+tz5ZVWquwbcEmxI37IgRdY9cncbl3GAmHLbTzhUMRy+/OBqhjUX 5+eAJWO8IlPU6IdoH+YyalEqzlArwNo= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=eEmDVxdH; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf09.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=peterx@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655318404; 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=NCUMLxMDONU55N69UPBhfgO9xoAmHEBDMarmTws3Xc0=; b=ail1urNIhM6B7f0xWDU3ARUVHHoBaEKSXoThlSnBDsj7vCnjLSvRqIvmBac0O3ku8xYdE/ /EFx6Gf3CQKxIICRKLgCkpbyCX77UnUIBPJ7ZlFDL3PrxyT+/hcICufIswUrvqSUVlbLDx rtMYXQGUZN4RrS9dGiNLhhGHZEzuKLA= X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: C1EB0140087 X-Stat-Signature: 4zpxnjwwf3rz78ijyzu7mybtmuro4jks Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=eEmDVxdH; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf09.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=peterx@redhat.com X-Rspam-User: X-HE-Tag: 1655318404-30790 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 Wed, Jun 15, 2022 at 09:58:27AM -0700, Nadav Amit wrote: > On Jun 15, 2022, at 8:43 AM, Peter Xu wrote: > > > On Wed, Jun 15, 2022 at 10:26:21AM +0300, Mike Rapoport wrote: > >> On Tue, Jun 14, 2022 at 01:56:56PM -0700, Nadav Amit wrote: > >>> On Jun 14, 2022, at 1:40 PM, John Hubbard wrote: > >>> > >>>> On 6/14/22 11:56, Mike Rapoport wrote: > >>>>>> But, I cannot take it anymore: the list of arguments for uffd stuff is > >>>>>> crazy. I would like to collect all the possible arguments that are used for > >>>>>> uffd operation into some “struct uffd_op”. > >>>>> Squashing boolean parameters into int flags will also reduce the insane > >>>>> amount of parameters. No strong feelings though. > >>>> > >>>> Just a quick drive-by comment about boolean arguments: they ruin the > >>>> readability of the call sites. In practice, sometimes a single boolean > >>>> argument can be OK-ish (still poor to read at the call site, but easier > >>>> to code initially), but once you get past one boolean argument in the > >>>> function, readability is hopeless: > >>>> > >>>> foo(ptr, true, false, a == b); > >>>> > >>>> So if you have a choice, I implore you to prefer flags and/or enums. :) > >>> > >>> Thanks for the feedback - I am aware it is very confusing to have booleans > >>> and especially multiple ones in a func call. > >>> > >>> Just not sure how it maps to what I proposed. I thought of passing as an > >>> argument reference (pointer) to something similar to the following struct, > >>> which I think is very self-descriptive: > >>> > >>> struct uffd_op { > >>> /* various fields */ > >>> struct vm_area_struct *dst_vma; > >>> unsigned long len; > >>> atomic_t *mmap_changing; > >>> > >>> ... > >>> > >>> /* ... and some flags */ > >>> int wp: 1; > >>> int zero: 1; > >>> int read_likely: 1; > >>> > >>> ... > >>> }; > >>> > >>> I think that fits what you were asking for. The only thing I am not sure of, > >>> is whether to include in uffd_op fields that are internal to mm/userfaultfd > >>> such as “page” and “newly_allocated”. I guess not. > >> > >> mfill_atomic_install_pte() is called by shmem_mfill_atomic_pte() so it's > >> not entirely internal to mm/userfaultfd.c. > >> > >> Another thing is that with all the parameters packed into a struct, the > >> call sites could become really hairy, so maybe the best way would be to > >> pack some of the parameters and leave the others. > >> > >> But you'll never know until you try :) > > > > Yeh. Axel packed some booleans in f619147104c8e into mcopy_atomic_mode. > > The other option (besides uffd_ops) could be making mcopy_atomic_mode a > > bitmask and keep the rest, the mode itself only took 2 bits. > > > > uffd_ops sounds good too if the final outcome looks clean, since we do pass > > quite a few things over and over deep into the stack. > > Thanks. > > I see 3 options: > > 1. Pack only fs/mm flags: WP, read-likely, write-likely. > 2. (1) + as part of the flags internally include Axel’s copy_atomic_mode. > 3. The uffd_op approach: include all relevant fields. > > For the time being I’m going with (1) since I do not have too much time to > finish all of that and upstream the rest of my work (Broadcom is knocking). > > (3) also has the downside of stack-protector that would be added due to > stack-protector strong, which is not-that-bad, but I hate it. Any short (but further) explanations? > > > Three more points for consideration in future cleanups: > > 1. This __always_inline thingy is crazy IMHO. The size of the compilation > unit is almost double because of it, and I saw no explanation for its use in > the commit log (unless I missed it). The overheads in userfaultfd are mostly > due to memory copying, scheduling, IPIs. Do you meant __mcopy_atomic()? I agree it's suspicious. That's a huge function with three callers. Even if to mark it inline I think it's more proper to mark it at the three callers, or I'd think we can leave that for the compilers. > > 2. I think it makes more sense to strive not to have more than 6 arguments > for each function (as supported in registers on x86). For that it is possible > to get rid of dst_mm when it can be retrieved from dst_vma. Anyhow we access > dst_vma->vm_flags which share a cache-line with dst_vma->vm_mm. > > 3. These BUG_ON()s all around are also ... excessive. I guess they were > introduced before the age in which Linus got angry on each BUG_ON(). Is > there any good reason not to change them into VM_BUG_ON()? I'm not sure how that was handled, I'd personally think it's fine to keep them if they're there for ages. They're gone already if they can be triggered in any bad ways anyway.. But for sure we'd keep an eye on new ones when reviewing. -- Peter Xu