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 1E75BEE57D7 for ; Wed, 11 Sep 2024 21:35:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5D9266B007B; Wed, 11 Sep 2024 17:35:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 562136B0082; Wed, 11 Sep 2024 17:35:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 402AB6B0083; Wed, 11 Sep 2024 17:35:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 21F926B007B for ; Wed, 11 Sep 2024 17:35:14 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id B95B31A086A for ; Wed, 11 Sep 2024 21:35:13 +0000 (UTC) X-FDA: 82553763306.14.F3380F4 Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by imf14.hostedemail.com (Postfix) with ESMTP id EEA11100012 for ; Wed, 11 Sep 2024 21:35:11 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=H+Ylskkb; spf=pass (imf14.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.48 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1726090483; 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=T+jEybtqgHr3PVAiuDdOVn7cuVbpZHXqKxPkgKvDJyU=; b=euj4IvfLObVAVlMn64eT3al3SSyXQlpObq0MeMyjCME/TAZBe9bJ2EVpQ5emMfjfes9OYE GudbshPQpeZ7c54FWalNr7znm3EJfwrhlI43k3g99WxrtMcoH0a9JRK5L1JsWlzU/POzKu 1+X6jHw9T+Hxg5NYADOJufHzVUx4Ad0= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=H+Ylskkb; spf=pass (imf14.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.48 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726090483; a=rsa-sha256; cv=none; b=T9GCCKiOnDm/oZldAm18kJi/5XZv/O6LWeMShfE+sVok/b8Ydb9if+8BDfjC7W2YHN3w0B JK50l5tRQm5Qukz6kcgE2lS8A8FwwpQ47CFEQqMRumjt7cDZD0Zmj9xQC0GTiNLnZEBg3Q fcdWe/M56jBqnxKsE+UYnZezb4lhNfg= Received: by mail-pj1-f48.google.com with SMTP id 98e67ed59e1d1-2d889ba25f7so188361a91.0 for ; Wed, 11 Sep 2024 14:35:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1726090510; x=1726695310; 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=T+jEybtqgHr3PVAiuDdOVn7cuVbpZHXqKxPkgKvDJyU=; b=H+YlskkbECMoveNaPLhQem/QEzhouBI+ws5pbO8cx4mebDrcwqryCScJSOx1SDwaaT DJfX6rjQLqjaJEbhOF7QsohZBrbU/tii/hIgDhocAr/tAxQ0umSQokEdunTm0wrOKh6j LBTGggvb8KtzOgA/izd7bg3641a4YdFBcX6ZznZuXlxFOICBaxI66asOZpzr4ISzohd6 zWE1SEybLPaTb1MgrGcS9/dOeKMEU1BWpvjcYFsLV5xipkqJ9cCp5u9yrzYpSCn/shKu hz7DQXb+ZjWK/3X1pybGh5dNM65DQu5fRZHIqUXcNfhg9Fs7QUxgUp+CuVmZqYQJVuw7 0oyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726090510; x=1726695310; 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=T+jEybtqgHr3PVAiuDdOVn7cuVbpZHXqKxPkgKvDJyU=; b=lvxeQUUdgrhpLWAqwh05hjYrvrOr1AlrqkhlEPrPur7OXFfNyClGlYxndb9Svw8nfb KqI/7BgShMOqDCnYUzeeWr749kkpqxtQBM080e9QazAy+4P/9yVfj0tgbSkshAKnwu68 +hHEUijGzCfsoSY75JjHzMRO/4PDk5l9HwYoILF/kzuIlmEczCDALMTlV4YUxDy3MJLo 36q4eM5xpQYBJFJingTk1TSRLzGt2T5+oi5DmIna2KDWAOdDbMmcrS3vtfbL2ZCaVhRx h4VZC0GXvOXTHYx4u5r4XGxYCTG/+ZXdgvloX9ulR+aWswFRvyWOm1Loxzzjm8ZccDnA fMVQ== X-Forwarded-Encrypted: i=1; AJvYcCWK4fdYxwAyiJues6D8upIzk6VXSZCToBhVgpVb2IDX6xJjMD3VcmAisySQfBeExUjnfLwBCxd7bw==@kvack.org X-Gm-Message-State: AOJu0YzhGWNI7spBO45A3/5SmgUYBNPC1yWy8cneO2r9Mmi6Lwb3Z6sZ oEhnxC5Ni0KaewLzuN0kqNv6hmQ6jLsiBbUw3C7YCYMrXqLqzfFCQKdhxt55ztDUHMbUAn75//y V195tm6j6BbGy/MlneawZHasrq+A= X-Google-Smtp-Source: AGHT+IGpXC3R7Ca0taMUwh/MVl1yLymIr33cDWnZgZwGWtPi5Mtd23oK4XL2nH75+xxRhAVXb0qYJbp+sa10tNeg9ro= X-Received: by 2002:a17:90b:17c5:b0:2c4:b0f0:8013 with SMTP id 98e67ed59e1d1-2db9ffbfbf2mr650796a91.11.1726090510597; Wed, 11 Sep 2024 14:35:10 -0700 (PDT) MIME-Version: 1.0 References: <20240906051205.530219-1-andrii@kernel.org> <20240906051205.530219-2-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Wed, 11 Sep 2024 14:34:58 -0700 Message-ID: Subject: Re: [PATCH 1/2] mm: introduce mmap_lock_speculation_{start|end} To: Suren Baghdasaryan Cc: Jann Horn , Andrii Nakryiko , linux-trace-kernel@vger.kernel.org, peterz@infradead.org, oleg@redhat.com, rostedt@goodmis.org, mhiramat@kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org, jolsa@kernel.org, paulmck@kernel.org, willy@infradead.org, akpm@linux-foundation.org, linux-mm@kvack.org, mjguzik@gmail.com, brauner@kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: uwgr1usctadrp387rn6ifcwr9kb9d3q8 X-Rspamd-Queue-Id: EEA11100012 X-Rspamd-Server: rspam11 X-HE-Tag: 1726090511-363582 X-HE-Meta: U2FsdGVkX1+KwB7Es6gwlSV8gME/LruXfqXcrFo9azLkX+Cmh4Ax52ZPkpbtLSI1s9cLIe+vp1YTo083Sev5+nED1ikcKRwcv+g/qPI+i7E2D5VKOcU4WAU8ngt/0/1qoytPP9mI9WeZ5M1ktRVnFQFokafotcLuGbklz8iDzYBSQCx07k6FA/01bXyWVmb7IWkyiwpJCFoKoIAtwuZ4V/kPKbRiElwPon/9A996irMRQVUfW/L0NpuNAcBMs88aSRFBeTzGF+IloDTTO0tTAa9X7+LhgOaRfgjcCyuyNC1BafkjOIl9nYO5WFmnEhMdwT+QOg/F3/r1dCcwd8chKDv4jlhFhqgBe+Y433/InWVy9Hv0bag3r1WvTDQ4F3SufKyzg4ZhOiaevijg8G3Uytad94UPbauwdwR/xlCcfov5NVOAW64LLt4e1JbFzbwmQjti63u+JFDjk/Ey7Dc9hAsLPkjBXSeZE4jibK+963+J0EwgNI18ynBdq73r7ZG0GUYY2enRIcSsqAeIY5P8A+aAu+FF+EqeB8nQ6T9kYinLjtPx1eyUhYf8d8aaaU7HsNOQSjQZOPJI9fBHEYR58/pYtt1/bq0fxhkKiVzdWkLKdP64nct+wAyWX8+mKKD52jGkpbN0cEe19TTas59Ghwi9ZuE2uEhJuWZFVNSJvFVz8IjLlyPxJUIUqgvlN/FdmgmsbhzDOz6beGIaZuOn+n7F48IH3206XVAhf/HOs9fieb4tH/fXVImMTxrO054W0nSESjWRQd8yoXSMuOnPum9rtRp2TJFINMGbr405NDLhWQ4j4KP0makmqhQM+G5wct16FJFPy+bjyXI26s2IidD6Oz99OclnJDSYx2yjewCmYtsxoGJCv265FxNskxxCwpzpKgk2psPYKuL0CkaRwMV8wc0qLT1X/jsRUMK0ClzzrwOYlRQ44A4m17QPzniylKKI+yNgLL2jtYcoTfQ C9Hfup2b tHoD6slRIfd381Jyj2x+zBVLkbQ2x+prVJNYLcEvcR7ARJi4CPwAaWM9dDnaUbHkhhLCqqSRxxc3d4+lmlJq51o/WbOaErXsxs7GhmAEaxvxmLOpUOc9yXYekRkyGgJjCnxPcCfm2RF8F32pRcTctZ89PN4uzNhJ6PandLcPp35yrolS5wxd80W50so3R161JU8TIQAikGOTfLpgqJmyavNfcYme+sNjtS2BcF+dFjrMYm+qgWU8JUMpsTttmxS0q8BpPBPfizB3HKvYpZbmkKMsGy1o12nxxkc8+lmEWWoH9AOdsB4YC7iGM1hPeYQZX7EqUj9EB/KnTBmeX18WWOcr+sWMn7YcbCVLbsfEZf/ZEJp+a/BnUvNoWC7snjiwc1/rsW4o0NWEuws5omdNzn9T2UtUUJvIF1rSlXw7sPT2rqtlN5k25NLE4fZPcv58ND0f++bUcopxXDaI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000038, 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 Mon, Sep 9, 2024 at 7:09=E2=80=AFPM Suren Baghdasaryan wrote: > > On Mon, Sep 9, 2024 at 5:35=E2=80=AFAM Jann Horn wrote= : > > > > On Fri, Sep 6, 2024 at 7:12=E2=80=AFAM Andrii Nakryiko wrote: > > > +static inline bool mmap_lock_speculation_end(struct mm_struct *mm, i= nt seq) > > > +{ > > > + /* Pairs with RELEASE semantics in inc_mm_lock_seq(). */ > > > + return seq =3D=3D smp_load_acquire(&mm->mm_lock_seq); > > > +} > > > > A load-acquire can't provide "end of locked section" semantics - a > > load-acquire is a one-way barrier, you can basically use it for > > "acquire lock" semantics but not for "release lock" semantics, because > > the CPU will prevent reordering the load with *later* loads but not > > with *earlier* loads. So if you do: > > > > mmap_lock_speculation_start() > > [locked reads go here] > > mmap_lock_speculation_end() > > > > then the CPU is allowed to reorder your instructions like this: > > > > mmap_lock_speculation_start() > > mmap_lock_speculation_end() > > [locked reads go here] > > > > so the lock is broken. > > Hi Jann, > Thanks for the review! > Yeah, you are right, we do need an smp_rmb() before we compare > mm->mm_lock_seq with the stored seq. > > Otherwise reads might get reordered this way: > > CPU1 CPU2 > mmap_lock_speculation_start() // seq =3D mm->mm_lock_seq > reloaded_seq =3D mm->mm_lock_seq; // reordered read > mmap_write_lock() // inc_mm_lock_seq(mm) > vma->vm_file =3D ...; > mmap_write_unlock() // inc_mm_lock_seq(m= m) > > mmap_lock_speculation_end() // return (reloaded_seq =3D=3D seq) > > > > > > static inline void mmap_write_lock(struct mm_struct *mm) > > > { > > > __mmap_lock_trace_start_locking(mm, true); > > > down_write(&mm->mmap_lock); > > > + inc_mm_lock_seq(mm); > > > __mmap_lock_trace_acquire_returned(mm, true, true); > > > } > > > > Similarly, inc_mm_lock_seq(), which does a store-release, can only > > provide "release lock" semantics, not "take lock" semantics, because > > the CPU can reorder it with later stores; for example, this code: > > > > inc_mm_lock_seq() > > [locked stuff goes here] > > inc_mm_lock_seq() > > > > can be reordered into this: > > > > [locked stuff goes here] > > inc_mm_lock_seq() > > inc_mm_lock_seq() > > > > so the lock is broken. > > Ugh, yes. We do need smp_wmb() AFTER the inc_mm_lock_seq(). Whenever Suren, can you share with me an updated patch for mm_lock_seq with the right memory barriers? Do you think this might have a noticeable impact on performance? What sort of benchmark do mm folks use to quantify changes like that? > we use inc_mm_lock_seq() for "take lock" semantics, it's preceded by a > down_write(&mm->mmap_lock) with implied ACQUIRE ordering. So I thought > we can use it but I realize now that this reordering is still > possible: > CPU1 CPU2 > mmap_write_lock() > down_write(&mm->mmap_lock); > vma->vm_file =3D ...; > > mmap_lock_speculation_start() // seq =3D mm->mm_lock_seq > > mmap_lock_speculation_end() // return (mm->mm_lock_seq =3D=3D seq) > > inc_mm_lock_seq(mm); > mmap_write_unlock() // inc_mm_lock_seq(m= m) > > Is that what you were describing? > Thanks, > Suren. > > > > > For "taking a lock" with a memory store, or "dropping a lock" with a > > memory load, you need heavier memory barriers, see > > Documentation/memory-barriers.txt.