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 450F8C02180 for ; Mon, 13 Jan 2025 21:08:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D203E6B009B; Mon, 13 Jan 2025 16:08:43 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CA8D66B009C; Mon, 13 Jan 2025 16:08:43 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B22826B009D; Mon, 13 Jan 2025 16:08:43 -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 916336B009B for ; Mon, 13 Jan 2025 16:08:43 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 1506F1A037D for ; Mon, 13 Jan 2025 21:08:43 +0000 (UTC) X-FDA: 83003667726.30.F1A6E3E Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf06.hostedemail.com (Postfix) with ESMTP id 1E6A9180014 for ; Mon, 13 Jan 2025 21:08:40 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=DvPjY3kc; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf06.hostedemail.com: domain of surenb@google.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736802521; a=rsa-sha256; cv=none; b=NKs3Yj5vZPAFdDZAStMAeaRd8fNcdPU8YPRmusfa+KURyDyUNaez5mmU0C193CozweZQdk NtKc/GNfGazSf/x7trBpOG56nYVjEbqWgAef6SsSi4Ohf/Tn3sbe/8ES2KxsmlHK/k0Tlr MN9Yg2urpDlB3UB/BOKivSXciiAlKnM= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=DvPjY3kc; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf06.hostedemail.com: domain of surenb@google.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736802521; 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=wu/nG6orYg47gQ+AqOtZPPOYCw8QfPvh5WeBFkmFbCc=; b=cpE9hk7EafhqlPTOvEKSblhazFwC+V8cMkvT/eHQhM8J8/z1F1g+POJxXHy5FU70LXtMSJ up20txrmyFPY+8whjQv6yvGXnhHAa5Wni7WZ4IsNSl9MLz/fNNBTheA0FVBGa/Fmq5w2zG Qjv00YOJiavJqh61OuBh1+idOqtZ77o= Received: by mail-qt1-f182.google.com with SMTP id d75a77b69052e-4679b5c66d0so5111cf.1 for ; Mon, 13 Jan 2025 13:08:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736802520; x=1737407320; darn=kvack.org; 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=wu/nG6orYg47gQ+AqOtZPPOYCw8QfPvh5WeBFkmFbCc=; b=DvPjY3kcaSBhiArlMeHKjanXu+x0X/fR1hiAfaEPNwI6KF6RBldbfo5WK1UQ3NC3oC nz2fBJRzl9Zx0/818JbwRSY/jdXSxRuRB7wf8FThjkbFhtzb1dMXiwsksM+WVhyKkdBf spgUKCmMjzsEa5jm89XyvRo4jTWH7DTsRXa1R5uP9f0mC+t7yptoK6xx+nejkouwMVpA w/q+Hy9nFikUPtGbzEwxSCDQMDqMAN0Y73bDT0SVBbc9Lj6hC7EpeRxHK5JqOInWCWsY 3gqF/UVU49BjjBrm/AsVghRrN+rpy1J3pE8cgxod/JwniKMiHrjAACf0TkDm2ddwnTGn Vt0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736802520; x=1737407320; 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=wu/nG6orYg47gQ+AqOtZPPOYCw8QfPvh5WeBFkmFbCc=; b=qVYd0LDgmmiywGn5Sq3j0D6tmig9kodbJ0b5ZGwN9Cs/EDesqaeM4LPWqrDNZW1Rkr RnmOKOEeKQ+qMGTOuUMU7iozGbs4YhGDkLzoF5tXCVTjLutVObtUI3E1qOnFEjvdst6M HkDf//lwtVooiRXYiCQ+o5P1VLGHLAsmCdGhjNsm9HFhR3FhVqeWAW4QUY3KtlzvLjBD s3gi6zDH8pGEx//Q47mdfj7HqbU/i4CjaliT2pkrGQBOzcEUQONRvtWPi98SAD5tD3+C RX0MCAhZTl4UbR6MkjVE0orUC0YwcD7MRsXBDCF7RzSigb2TyVm/iX3N49OuRLnZJH6W HI0Q== X-Forwarded-Encrypted: i=1; AJvYcCUltwokg48GTlFERLp25UpmV/vGvzPg3e5qMXDIFd361kYwppMm+D7YZtX4DBU+pYjQjkVgM0u7+Q==@kvack.org X-Gm-Message-State: AOJu0YyXfwYjfrnXNuJz84UF6vQjMpDclHQGl4kbdS4u+ssY8U20WS9N FF/1YPJP5jctCCGWMT8ZDNSe3cdfev3pfIN+CskqAWx9OQzGtxyYjjYmeeKGf3HLpVxNEKzBFP4 jjYREd0lNri2kkjlnIyhHrVvEo2rkPmxjULKF X-Gm-Gg: ASbGncvUpJpUv73e58mzYjhPgjpGO++c79qSIkNJ4e1BVbz3MUYC6X6hTMOqLh7JOoa rMQazQxsccQdo7JzqSiiEVpPd8NPnFIcD1fIOvA== X-Google-Smtp-Source: AGHT+IGe7S1/aB4dFspMIFJVlQNEFLRUC7b1lRSOyvwJKhMCo1+Ol6kucHZ6LetMmLESzC/RseERgXBBIFgeH+0xsh4= X-Received: by 2002:ac8:5acd:0:b0:466:9660:18a2 with SMTP id d75a77b69052e-46de96eac52mr598091cf.16.1736802519806; Mon, 13 Jan 2025 13:08:39 -0800 (PST) MIME-Version: 1.0 References: <20250111042604.3230628-1-surenb@google.com> <20250111042604.3230628-12-surenb@google.com> <20250113014729.ms5sdfnhynlamgrk@master> In-Reply-To: <20250113014729.ms5sdfnhynlamgrk@master> From: Suren Baghdasaryan Date: Mon, 13 Jan 2025 13:08:28 -0800 X-Gm-Features: AbW1kvZoBMx33p2ZfPu8c19JCyuLeA_eoFBFTur6_G3RyvhowsY0pclQkJ14OQs Message-ID: Subject: Re: [PATCH v9 11/17] mm: replace vm_lock and detached flag with a reference count To: Wei Yang Cc: Mateusz Guzik , akpm@linux-foundation.org, peterz@infradead.org, willy@infradead.org, liam.howlett@oracle.com, lorenzo.stoakes@oracle.com, david.laight.linux@gmail.com, mhocko@suse.com, vbabka@suse.cz, hannes@cmpxchg.org, oliver.sang@intel.com, mgorman@techsingularity.net, david@redhat.com, peterx@redhat.com, oleg@redhat.com, dave@stgolabs.net, paulmck@kernel.org, brauner@kernel.org, dhowells@redhat.com, hdanton@sina.com, hughd@google.com, lokeshgidra@google.com, minchan@google.com, jannh@google.com, shakeel.butt@linux.dev, souravpanda@google.com, pasha.tatashin@soleen.com, klarasmodin@gmail.com, corbet@lwn.net, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: fc6q761idhctyuq6ey5u19kga7n3qtm7 X-Rspam-User: X-Rspamd-Queue-Id: 1E6A9180014 X-Rspamd-Server: rspam08 X-HE-Tag: 1736802520-489162 X-HE-Meta: U2FsdGVkX1+THCTcb+i6jmn1Xd0BgJzheILL2NiamGH7NzktI6y9XjgMgkipGb+J7L4b1dzYMI9KLgQk9mijLQyXLbyjbreo51txMGH1/ipk97Pp6PCVcT7PXsBB/6w1gYCoo3aShHSuMCOrVIN4bA8wMwVn1UIUMLZNFBDsdja4yAXgm/RD3+BiTIujxOkMw0ji/BJ9qY1yVMBTwwusd/s9ZnyFeOFby7G6AlxkrVVY6bOIYHS2xuoglwz5QK9fa0IL86nYCiXJIVRPi7wePmV1PuzG1dBWtGEqzQY1NDimELU+A6Wg3W8fdQ9pp7obRAjdbW6FeJbKtMRboy+UDx59XueTtUOXufcdtUdWCqobq1eP7sUtmi7H66XejYtHFiT+Jqo7/Kfn8ZQ0Kd10gOHWR8IZ5YTiXod0WTwsRv/Xho0YxhAyfQvMiAxb7Ez5gatXWMLdG6018xAEsdivFbgsiXLUq5U8w6W1nFzVHR9wmvq31hywb6tM+sPx/XfEAsIn2AiXAf/DqTl0o5EcJIP1i2MM+GRc8bgFY366AyeyApt0n7YBKho2nRT9oUCgQmLl+BxoeuWD+kuIAFYbGtz66RQOKeWOSkbQe99KTwPb2eCiX+wRYmiifBzQNH1g73gkUIkHFN2klrqyTmn9k2DsMXfMYibnMTyxnQvA2lT/Ak1B+GfzEAcWCNloc5Hdf6ilD7GZpJKAFHaRNgzBMb65B+5BTHJLyjGWf4O8CnyzgPVvVl/mYVSffcfyiCG+UE2CHf5NZ86YYExdch3N2kjn7O8ilSnO7rYpd155L1+NMUBoQg4UNlptVJPri4v/n19BMQkOu5dwgYvIIfDizTYLREvr5h178h+MDpF8akvLzu+X18h8JZfb5lnG67iW2W51t2QMAn7Kkibl/7lw8O3DLoKb+roD93wsTG9jSexfdAxSMUEb3sxmMR5R4NzVE2Ve2iyTv2ORfIREi69 ucg1DktG XpGtIdnLH3cAar+A/O7nt1RJSj27mmjj3eqs0qkJ2Krq+HJ45NjsL0buo25lJ5HsPUn4ocw5gzqZkFpqUuyfByJAfxRtY/EaRvKGGLUEYg/Hx2Jbbj0cSQPq/OEI+YugWKRsXYmrD31QhPr0URNzaFEHxOjr3EyGVZ8HpAtioiLsC2RvMow2AhRABVTLaklo0FZO5dcpnHBaSkcznqgnDgh+6NFJnIp5+Mp3oYrvesLt5WXW+auM6kPC1dTxnZaXgaDzgy7w1huvaW6m/z8Pa3gnwBRMuASufMdCf8rLeEUxEKDkv9GVQyerUrDQtCjbgw1vtNauvpkNXuJJYh1cEWJRX7dHfX2PwEiCmiPB2OgeJBETgYwsGk2u4Z9qEAq9bZ8zyDFuYFGh7eweNf9IhPQZPdyFi/XyERPhT 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: List-Subscribe: List-Unsubscribe: On Sun, Jan 12, 2025 at 5:47=E2=80=AFPM Wei Yang wrote: > > On Sat, Jan 11, 2025 at 12:14:47PM -0800, Suren Baghdasaryan wrote: > >On Sat, Jan 11, 2025 at 3:24=E2=80=AFAM Mateusz Guzik wrote: > >> > >> On Fri, Jan 10, 2025 at 08:25:58PM -0800, Suren Baghdasaryan wrote: > >> > >> So there were quite a few iterations of the patch and I have not been > >> reading majority of the feedback, so it may be I missed something, > >> apologies upfront. :) > >> > > Hi, I am new to memory barriers. Hope not bothering. > > >> > /* > >> > * Try to read-lock a vma. The function is allowed to occasionally = yield false > >> > * locked result to avoid performance overhead, in which case we fa= ll back to > >> > @@ -710,6 +742,8 @@ static inline void vma_lock_init(struct vm_area_= struct *vma) > >> > */ > >> > static inline bool vma_start_read(struct vm_area_struct *vma) > >> > { > >> > + int oldcnt; > >> > + > >> > /* > >> > * Check before locking. A race might cause false locked resul= t. > >> > * We can use READ_ONCE() for the mm_lock_seq here, and don't = need > >> > @@ -720,13 +754,19 @@ static inline bool vma_start_read(struct vm_ar= ea_struct *vma) > >> > if (READ_ONCE(vma->vm_lock_seq) =3D=3D READ_ONCE(vma->vm_mm->m= m_lock_seq.sequence)) > >> > return false; > >> > > >> > - if (unlikely(down_read_trylock(&vma->vm_lock.lock) =3D=3D 0)) > >> > + /* > >> > + * If VMA_LOCK_OFFSET is set, __refcount_inc_not_zero_limited(= ) will fail > >> > + * because VMA_REF_LIMIT is less than VMA_LOCK_OFFSET. > >> > + */ > >> > + if (unlikely(!__refcount_inc_not_zero_limited(&vma->vm_refcnt,= &oldcnt, > >> > + VMA_REF_LIMIT))) > >> > return false; > >> > > >> > >> Replacing down_read_trylock() with the new routine loses an acquire > >> fence. That alone is not a problem, but see below. > > > >Hmm. I think this acquire fence is actually necessary. We don't want > >the later vm_lock_seq check to be reordered and happen before we take > >the refcount. Otherwise this might happen: > > > >reader writer > >if (vm_lock_seq =3D=3D mm_lock_seq) // check got reordered > > return false; > > vm_refcnt +=3D VMA_LOCK_OFFSET > > vm_lock_seq =3D=3D mm_lock_seq > > vm_refcnt -=3D VMA_LOCK_OFFSET > >if (!__refcount_inc_not_zero_limited()) > > return false; > > > >Both reader's checks will pass and the reader would read-lock a vma > >that was write-locked. > > > > Here what we plan to do is define __refcount_inc_not_zero_limited() with > acquire fence, e.g. with atomic_try_cmpxchg_acquire(), right? Correct. __refcount_inc_not_zero_limited() does not do that in this version but I'll fix that. > > >> > >> > + rwsem_acquire_read(&vma->vmlock_dep_map, 0, 1, _RET_IP_); > >> > /* > >> > - * Overflow might produce false locked result. > >> > + * Overflow of vm_lock_seq/mm_lock_seq might produce false loc= ked result. > >> > * False unlocked result is impossible because we modify and c= heck > >> > - * vma->vm_lock_seq under vma->vm_lock protection and mm->mm_l= ock_seq > >> > + * vma->vm_lock_seq under vma->vm_refcnt protection and mm->mm= _lock_seq > >> > * modification invalidates all existing locks. > >> > * > >> > * We must use ACQUIRE semantics for the mm_lock_seq so that i= f we are > >> > @@ -735,9 +775,10 @@ static inline bool vma_start_read(struct vm_are= a_struct *vma) > >> > * This pairs with RELEASE semantics in vma_end_write_all(). > >> > */ > >> > if (unlikely(vma->vm_lock_seq =3D=3D raw_read_seqcount(&vma->v= m_mm->mm_lock_seq))) { > > One question here is would compiler optimize the read of vm_lock_seq here= , > since we have read it at the beginning? > > Or with the acquire fence added above, compiler won't optimize it. Correct. See "ACQUIRE operations" section in https://www.kernel.org/doc/Documentation/memory-barriers.txt, specifically this: "It guarantees that all memory operations after the ACQUIRE operation will appear to happen after the ACQUIRE operation with respect to the other components of the system.". > Or we should use REACE_ONCE(vma->vm_lock_seq) here? > > >> > >> The previous modification of this spot to raw_read_seqcount loses the > >> acquire fence, making the above comment not line up with the code. > > > >Is it? From reading the seqcount code > >(https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/seqlock= .h#L211): > > > >raw_read_seqcount() > > seqprop_sequence() > > __seqprop(s, sequence) > > __seqprop_sequence() > > smp_load_acquire() > > > >smp_load_acquire() still provides the acquire fence. Am I missing someth= ing? > > > >> > >> I don't know if the stock code (with down_read_trylock()) is correct a= s > >> is -- looks fine for cursory reading fwiw. However, if it indeed works= , > >> the acquire fence stemming from the lock routine is a mandatory part o= f > >> it afaics. > >> > >> I think the best way forward is to add a new refcount routine which > >> ships with an acquire fence. > > > >I plan on replacing refcount_t usage here with an atomic since, as > >Hillf noted, refcount is not designed to be used for locking. And will > >make sure the down_read_trylock() replacement will provide an acquire > >fence. > > > > Hmm.. refcount_t is defined with atomic_t. I am lost why replacing refcou= nt_t > with atomic_t would help. My point is that refcount_t is not designed for locking, so changing refcount-related functions and adding fences there would be wrong. So, I'll move to using more generic atomic_t and will implement the functionality I need without affecting refcounting functions. > > >> > >> Otherwise I would suggest: > >> 1. a comment above __refcount_inc_not_zero_limited saying there is an > >> acq fence issued later > >> 2. smp_rmb() slapped between that and seq accesses > >> > >> If the now removed fence is somehow not needed, I think a comment > >> explaining it is necessary. > >> > >> > @@ -813,36 +856,33 @@ static inline void vma_assert_write_locked(str= uct vm_area_struct *vma) > >> > > >> > static inline void vma_assert_locked(struct vm_area_struct *vma) > >> > { > >> > - if (!rwsem_is_locked(&vma->vm_lock.lock)) > >> > + if (refcount_read(&vma->vm_refcnt) <=3D 1) > >> > vma_assert_write_locked(vma); > >> > } > >> > > >> > >> This now forces the compiler to emit a load from vm_refcnt even if > >> vma_assert_write_locked expands to nothing. iow this wants to hide > >> behind the same stuff as vma_assert_write_locked. > > > >True. I guess I'll have to avoid using vma_assert_write_locked() like th= is: > > > >static inline void vma_assert_locked(struct vm_area_struct *vma) > >{ > > unsigned int mm_lock_seq; > > > > VM_BUG_ON_VMA(refcount_read(&vma->vm_refcnt) <=3D 1 && > > !__is_vma_write_locked(vma, > >&mm_lock_seq), vma); > >} > > > >Will make the change. > > > >Thanks for the feedback! > > -- > Wei Yang > Help you, Help me