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 423D5C54EBE for ; Mon, 16 Jan 2023 23:11:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CA1246B0075; Mon, 16 Jan 2023 18:11:14 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C50726B007B; Mon, 16 Jan 2023 18:11:14 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AF2826B007D; Mon, 16 Jan 2023 18:11:14 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 9C8826B0075 for ; Mon, 16 Jan 2023 18:11:14 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 82CC2A084A for ; Mon, 16 Jan 2023 23:11:14 +0000 (UTC) X-FDA: 80362210068.21.EBC11AD Received: from mail-yb1-f169.google.com (mail-yb1-f169.google.com [209.85.219.169]) by imf27.hostedemail.com (Postfix) with ESMTP id EA1E44001A for ; Mon, 16 Jan 2023 23:11:12 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=ZixO1TRw; spf=pass (imf27.hostedemail.com: domain of surenb@google.com designates 209.85.219.169 as permitted sender) smtp.mailfrom=surenb@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=1673910673; 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=eY/KJZ4lbmc+2XWsZ1z/ZDsZuzYLlrOu48ChSc6SgWY=; b=iyEWHJniX+Ud9nIhG/HwF71Xd1IRJm4L8HGlegpWiW70lveXi1eIMP6Ihw8ANJ80pKatro cr+bbresVHzCA7szxe4pcYLkd8T/G933a+0UTKsM7So+ey03vNwcqw7F968zlPj1gZ5ttr YhPjaXIAPDpYT157zIJUoIVTl/VRnBA= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=ZixO1TRw; spf=pass (imf27.hostedemail.com: domain of surenb@google.com designates 209.85.219.169 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1673910673; a=rsa-sha256; cv=none; b=UZMhT5mpXDWaG/GH/z7f7j8xUjs1W2Gv4SDS5hMVgjHWcjBHzFuHIDJ/7JWs2I8kacNri1 Hc+Fj6HWmXb3wZeutlIN9fW1y+cVdhy1EAqfCy+BnOp18OyNPNEmg6jEgIT9n7R2eE8r1y R6Aa71KdWAQcKWW/EEsylNnUo1S5s1k= Received: by mail-yb1-f169.google.com with SMTP id v19so26077317ybv.1 for ; Mon, 16 Jan 2023 15:11:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=eY/KJZ4lbmc+2XWsZ1z/ZDsZuzYLlrOu48ChSc6SgWY=; b=ZixO1TRwnXk+Zt8AF0h/qfqGg+VFnuqWHiC/HyBxquWFYjlRl0YLLeTOV5ebh4DJ/q 3NA4XIFmHBMtm/+65vSu6ODMVBvpV/4Z4Q2N0PSP8VOhYVM2f9ttOWIa9XLK8RuzgQ6d zVxv6Y8B2O35YV0ERZgJZtS5jd/n8ERgHrLmUQ7KIW3idzhC0N6Dj25drlUsemu5Bc5B zUp79W76z07v0rq5EpdGvLb2As2Cx4TQ7puDyBiu38lkrOO/ch3iv8VQAbENPsRHXrQM s7ks6/ymncWZJ3W6sGbVgjATyUXYNlRtKW7K1T5NgM9jJg9D4ibCIUoC4OljjI/jlfRh TQ1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=eY/KJZ4lbmc+2XWsZ1z/ZDsZuzYLlrOu48ChSc6SgWY=; b=1OPml598Q0kVL1PF+Cj6CwkjZ2YrO4ACltqCXuwCPz5Q5IJgnRwK0mvs3YSVLzY99j pBC5GJ96jzI7jxSX0xo3Va2rUJDk90H81EjHNQxR2oksGyPv+iKTwTIa+daV2zPVl4zN /SIsGuH0c3GCANMwW7nh/szY3/zo+CAraE4Pzv4fVl4c2l3zUql9PkVSriTS1NVKD9rc rLHEUBHRzY2MTimK9HtgutJ0y3Hr9Ku7LeDk5zgag9Dcmt2UygSWa6o/ME0AbEaLjxSl qvVKr8lksjbA+alffUEZ2Z4YRvywCTV+dRItC6tuNvyXBTb8Is7eI+0DGyT4Xza5PeGk Xfcg== X-Gm-Message-State: AFqh2kp88gb2XeckEpKllTYvbtI1T8sGSsLPEOMFbnf2rHIZ6rfQcA0i JAGW07kqMGrWqG6VrBSXU9fXnL+VsYegnayq3wHy0A== X-Google-Smtp-Source: AMrXdXvxjMFnv3sXKfnTKH4PTNF/OT1rqzcwKMDy96OvxnpCIj9/MWgAZnN/R9Th80cE+rQd8BICuY6ElJpUSZTS8r0= X-Received: by 2002:a5b:cc8:0:b0:7ba:78b1:9fcc with SMTP id e8-20020a5b0cc8000000b007ba78b19fccmr160434ybr.593.1673910671886; Mon, 16 Jan 2023 15:11:11 -0800 (PST) MIME-Version: 1.0 References: <20230109205336.3665937-42-surenb@google.com> <20230116140649.2012-1-hdanton@sina.com> In-Reply-To: From: Suren Baghdasaryan Date: Mon, 16 Jan 2023 15:11:00 -0800 Message-ID: Subject: Re: [PATCH 41/41] mm: replace rw_semaphore with atomic_t in vma_lock To: Hillf Danton Cc: vbabka@suse.cz, hannes@cmpxchg.org, mgorman@techsingularity.net, peterz@infradead.org, hughd@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: 73e5c6p6ejp1ishns8n6zimzuhyinepf X-Rspam-User: X-Rspamd-Queue-Id: EA1E44001A X-Rspamd-Server: rspam06 X-HE-Tag: 1673910672-76837 X-HE-Meta: U2FsdGVkX19faEhEa04378A4HTDBYsS22pJOLMgfhhLouCYFY6YniijBHcred6t+zkkJm9FvkihH+52OXp+UZarDzMqHdVg4b/Zlu9TBvPLjRTsz/f2KQ+XPWsO+GDsXDpkFPK8Q+xM6i4A49DZMm1PbdTVYR94nPvIjYWQn82bIua8UyBVBrx9yq4lKaCaok78PY1qouRsA05KK5W+pFqjclpE5bHUDAbJprEcuKDWP6XdF/jADNyVflccFlCyqLYVMK/ZCfkkP5JNahQmQpugO1PX2+F2qjmN1EcIMNF+d8l+62yuGWhdBA/Ru47g40nzN5ANTNzDMBp+VERq4Acd+kGo2XOuZDSaiW6cVBewkp0m3Tz1ZsWBKgDwN4tn1VIQcycn305D6CvJrNxnnZIq3AcNuzDKlSwoMSiDaOqGGxFP7LLAqrzxnCV45Sby8WCK3GMCNk0uJNtbF80sDj9h4PRYq8YkiK8DCAZmOaXQRsQtUntU7ybDs2Knj1jdpkMpPhqrrPFzACt8cxDDSsFq39qxtYbsZwXNzvYJhMFd8rD+SayeVSyNmxvwi1t+sSbb9ybFQ0Fa3VxDeK5YjNWmm05E3tDmvyAr3vNMZf+lYPgFswN7fHOWbzqqPs22i/Ezo0r7R/YbOBi9DnLFtKFSG96d8QvHdXjui7wV4GL9VEq3BFH5LhM++sXsZBRNCBCSh7kC8r33Bk2L681mHsHdb1tIAWEjtHx7txoynMqyDtrvH9C0Po7tdJ1/zpQmApJbqfm7aarHtWjXUFM/mV8p+nNx9eVHCqMHFiQaWc7+UY2Clqjziq8cQh6AgGTla11YX/tOH79mUmxquuE8tUTGmQlsH6dVk8vCsd+xVMiyHHlcTp4O8FP1Vbv4Ss8kF3MUzjeL9XzgoBQqQIsUVx4H4OxGtueMXUdjs+idjoSJMDYr7nuP5hbb7g4od+vRINSay66dpBAWLJcsiyGo d4BcAPk5 kVc5U7ap3PY3W+NF9f6+Uj0PfsqH+ATBAbsibZ0o5MgpUUTzGOoOdmv1sCQ== 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, Jan 16, 2023 at 3:08 PM Suren Baghdasaryan wrote: > > On Mon, Jan 16, 2023 at 6:07 AM Hillf Danton wrote: > > > > On Mon, 9 Jan 2023 12:53:36 -0800 Suren Baghdasaryan > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -627,12 +627,16 @@ static inline void vma_write_lock(struct vm_area_struct *vma) > > > * mm->mm_lock_seq can't be concurrently modified. > > > */ > > > mm_lock_seq = READ_ONCE(vma->vm_mm->mm_lock_seq); > > > - if (vma->vm_lock_seq == mm_lock_seq) > > > + if (vma->vm_lock->lock_seq == mm_lock_seq) > > > return; > > > > lock acquire for write to info lockdep. > > Thanks for the review Hillf! > > Good idea. Will add in the next version. > > > > > > > - down_write(&vma->vm_lock->lock); > > > - vma->vm_lock_seq = mm_lock_seq; > > > - up_write(&vma->vm_lock->lock); > > > + if (atomic_cmpxchg(&vma->vm_lock->count, 0, -1)) > > > + wait_event(vma->vm_mm->vma_writer_wait, > > > + atomic_cmpxchg(&vma->vm_lock->count, 0, -1) == 0); > > > + vma->vm_lock->lock_seq = mm_lock_seq; > > > + /* Write barrier to ensure lock_seq change is visible before count */ > > > + smp_wmb(); > > > + atomic_set(&vma->vm_lock->count, 0); > > > } > > > > > > /* > > > @@ -643,20 +647,28 @@ static inline void vma_write_lock(struct vm_area_struct *vma) > > > static inline bool vma_read_trylock(struct vm_area_struct *vma) > > > { > > > /* Check before locking. A race might cause false locked result. */ > > > - if (vma->vm_lock_seq == READ_ONCE(vma->vm_mm->mm_lock_seq)) > > > + if (vma->vm_lock->lock_seq == READ_ONCE(vma->vm_mm->mm_lock_seq)) > > > return false; > > > > Add mb to pair with the above wmb like > > The wmb above is to ensure the ordering between updates of lock_seq > and vm_lock->count (lock_seq is updated first and vm_lock->count only > after that). The first access to vm_lock->count in this function is > atomic_inc_unless_negative() and it's an atomic RMW operation with a > return value. According to documentation such functions are fully > ordered, therefore I think we already have an implicit full memory > barrier between reads of lock_seq and vm_lock->count here. Am I wrong? > > > > > if (READ_ONCE(vma->vm_lock->lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq)) { > > smp_acquire__after_ctrl_dep(); > > return false; > > } > > > > > > - if (unlikely(down_read_trylock(&vma->vm_lock->lock) == 0)) > > > + if (unlikely(!atomic_inc_unless_negative(&vma->vm_lock->count))) > > > return false; > > > > > > + /* If atomic_t overflows, restore and fail to lock. */ > > > + if (unlikely(atomic_read(&vma->vm_lock->count) < 0)) { > > > + if (atomic_dec_and_test(&vma->vm_lock->count)) > > > + wake_up(&vma->vm_mm->vma_writer_wait); > > > + return false; > > > + } > > > + > > > /* > > > * Overflow might produce false locked result. > > > * False unlocked result is impossible because we modify and check > > > * vma->vm_lock_seq under vma->vm_lock protection and mm->mm_lock_seq > > > * modification invalidates all existing locks. > > > */ > > > - if (unlikely(vma->vm_lock_seq == READ_ONCE(vma->vm_mm->mm_lock_seq))) { > > > - up_read(&vma->vm_lock->lock); > > > + if (unlikely(vma->vm_lock->lock_seq == READ_ONCE(vma->vm_mm->mm_lock_seq))) { > > > + if (atomic_dec_and_test(&vma->vm_lock->count)) > > > + wake_up(&vma->vm_mm->vma_writer_wait); > > > return false; > > > } > > > > Simpler way to detect write lock owner and count overflow like > > > > int count = atomic_read(&vma->vm_lock->count); > > for (;;) { > > int new = count + 1; > > > > if (count < 0 || new < 0) > > return false; > > > > new = atomic_cmpxchg(&vma->vm_lock->count, count, new); > > if (new == count) > > break; > > count = new; > > cpu_relax(); > > } > > > > (wake up waiting readers after taking the lock; > > but the write lock owner before this read trylock should be > > responsible for waking waiters up.) > > > > lock acquire for read. > > This schema might cause readers to wait, which is not an exact > replacement for down_read_trylock(). The requirement to wake up > waiting readers also complicates things and since we can always fall > back to mmap_lock, that complication is unnecessary IMHO. I could use > part of your suggestion like this: > > int new = count + 1; > > if (count < 0 || new < 0) > return false; > > new = atomic_cmpxchg(&vma->vm_lock->count, count, new); > if (new == count) > return false; Made a mistake above. It should have been: if (new != count) return false; > > Compared to doing atomic_inc_unless_negative() first, like I did > originally, this schema opens a bit wider window for the writer to get > in the middle and cause the reader to fail locking but I don't think > it would result in any visible regression. > > > > > > return true; > > > @@ -664,7 +676,8 @@ static inline bool vma_read_trylock(struct vm_area_struct *vma) > > > > > > static inline void vma_read_unlock(struct vm_area_struct *vma) > > > { > > lock release for read. > > Ack. > > > > > > - up_read(&vma->vm_lock->lock); > > > + if (atomic_dec_and_test(&vma->vm_lock->count)) > > > + wake_up(&vma->vm_mm->vma_writer_wait); > > > } > >