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 B1975EE57DB for ; Wed, 11 Sep 2024 21:48:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0B3776B007B; Wed, 11 Sep 2024 17:48:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 063596B0082; Wed, 11 Sep 2024 17:48:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E460C6B0083; Wed, 11 Sep 2024 17:48:41 -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 C4EAD6B007B for ; Wed, 11 Sep 2024 17:48:41 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 6511716122E for ; Wed, 11 Sep 2024 21:48:41 +0000 (UTC) X-FDA: 82553797242.09.EC8505B Received: from mail-il1-f178.google.com (mail-il1-f178.google.com [209.85.166.178]) by imf21.hostedemail.com (Postfix) with ESMTP id 9D8E21C0002 for ; Wed, 11 Sep 2024 21:48:39 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=2oMGbQLI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of surenb@google.com designates 209.85.166.178 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=1726091203; 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=J6Z8HdUc1+IRM76erXzE6hZcoAD0AgTlijcNod5P+zA=; b=YIqop2qzYFasgdIG+XKiCLye5ezMzA+NgC5+jPeMQdLoS7YGXBUdHrE74N23NQOYndfwDj ENNovVAwxvh3SQCIQIKDGxZ64SGrGiPIf0dzRKHLK1SHrocQjOLSo5kzHgKfWhnonsGRYE ik2DJIasRdOO7ooIL5v1I7SK0WhuI70= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726091203; a=rsa-sha256; cv=none; b=aTqTSsDc2w4nudF4ybh+UFsfzHWr+pq1IIkDrQkFJww11rzqgjUL3RUvkGs/6nppUyClMu BpnTwNvUSL13BGW1YWpQvCYpwi5U2Qj4LkIbl2J9wS3LFsL7K47YAomul6pW57xGt5VRhl NSbhpehELOnvns7dZMKwvigyup+/lBM= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=2oMGbQLI; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of surenb@google.com designates 209.85.166.178 as permitted sender) smtp.mailfrom=surenb@google.com Received: by mail-il1-f178.google.com with SMTP id e9e14a558f8ab-39d3ad05f8eso27585ab.1 for ; Wed, 11 Sep 2024 14:48:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1726091318; x=1726696118; 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=J6Z8HdUc1+IRM76erXzE6hZcoAD0AgTlijcNod5P+zA=; b=2oMGbQLIcGKifmvQBEBb82KwLOx9pDs7GqbEk0FQKxCfKihSSZz7TgeUwHeOC530H1 AsNZEXD4BEiChSgWayF4809o1sG+M5Xbxd36UgxlacDnKvnyRLzjVB3nFIuBoFdX9w9a bNrQbkH6JDm06aF7PtvEnbydxUfwsIRDOCxnidcuGnlylR77aHg8Su/ppFLPieSdT+cr ZinZv42OWnLcguDC8uZjhR+DaMnXbfge8tNz1m0IdOgfh7xOFW92GS8bsWarTzpCMjDF w3YhGOnPzYFww/MYmNpXvCnm/g6gYKGPcDwvQh148UyEFSSv3yWBXBoCiISfEL/7EO4n AJyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726091318; x=1726696118; 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=J6Z8HdUc1+IRM76erXzE6hZcoAD0AgTlijcNod5P+zA=; b=GGT3APyFuEOGJva8E036ptBa5JNLLiU4T3h9lE9itMATyaroG+3vAMFa9F3tNjJzov byuCeKOiiJqjHMmtIZc8nH1bY3nnUyC3XjVefadCJpAj02zxNfvPZ1ncfy3mSW6SYFZi jmaF0OWvnBKk9C3qe8lYh4O/sQyObnz9Hj8+5xZ2U4oUNxHGC7KXDtPxJLvL1M3vst3Y sK8M/FhVT1PLgqHGNnJySrWX4RfZeXRCzxFxiyOwlYW1oJQjrEbG6tLT3nwPyBfrAi9n I1ns8SetOYMLsjtMTrJQ/y3nsr+tdMr5ulwkJSu/vcCX5JwnCBG35kS/R+igHL1U8F2B xFBA== X-Forwarded-Encrypted: i=1; AJvYcCUchxCrNg1bnp1gRFBp4noTCw9PDmlZ6E6k1cY1nYuFj2y9at1AsFGFe9C67rvoB6yDdf1nk8KTWQ==@kvack.org X-Gm-Message-State: AOJu0Yy2Mhn2/lK4ZdRc2aRxUWyy9zE72u+5JzH3i8VeLV3NIKV+j1uv xE6Sssk1TQH5fdPXVkrn8X3RVrmbBRC6JYymGil/rMdcNb2T/m54ntv+26PmqlVCv0wCu9ojRnc rMjLuPlcLxq1rVu89jGGGFmrJwkloqftILaRx5Wudi+WR2ac6azBi X-Google-Smtp-Source: AGHT+IFmg4D1lfVet1nvJD1JAF6kEa9fIBsQooXq/7H7e2YTkj6xbHkrW2QYeQkv9L0W5OsOaKT7H6dmK3UPQKAEs3o= X-Received: by 2002:a05:6e02:1809:b0:398:fc12:d70f with SMTP id e9e14a558f8ab-3a08464d5f7mr1398105ab.0.1726091318258; Wed, 11 Sep 2024 14:48:38 -0700 (PDT) MIME-Version: 1.0 References: <20240906051205.530219-1-andrii@kernel.org> <20240906051205.530219-2-andrii@kernel.org> In-Reply-To: From: Suren Baghdasaryan Date: Wed, 11 Sep 2024 14:48:24 -0700 Message-ID: Subject: Re: [PATCH 1/2] mm: introduce mmap_lock_speculation_{start|end} To: Andrii Nakryiko 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-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 9D8E21C0002 X-Stat-Signature: crshxhy1eenx9hcz4umxbdw1djfnoizx X-Rspam-User: X-HE-Tag: 1726091319-124289 X-HE-Meta: U2FsdGVkX198awsVMH6okDrx+LoyTB2R90Jpi2MtDYOfd2cP4UfjeZi4slRuMoNEbmQ8Au1Es1vnR8CO4hAbK8eH59gkrqMrbwrxRg8+Ytzr8rTLECUMZpjV8Mh+6CVh3wN5l4fGMz+EnRoSYB/Q/8GJghbOQ3UFujKNDETzw7MavVUHn0E5hzvYxAGbr6eFVXHNvx6RFVJSTYaqTlId5JYplmJVCuZJbeC2ubuA9twWxiudVIJ9ES+96uqCKHa3VZ3xCox49dMbAOZMHaX3nOQ/YdHCAh5pE8qFNZ2NlKucMdwHYURoMo43TbfabPeiqCxicfpXw0KykmYeLM9mJ0pHhEX4XvdUf8TMYzP9U0B7hXZ8jLrOKonXZYZdGHqk04y3QU7C8EhgttYojNk5TSUtS6Pm5rIamAKatSDVf0Nb51bazIOLR7Oqy1a8Jh5npjn84f+o5xEHnXvVbdOlQlNspPdSqQhLZgepod/Ku9TrKFgzxxLNI6jFJDn7RftSgwXE/q9OSAscUZ1W2xgczkzpl1kHFqLtxsXFhsxlG/op4WPrWc/yG79XGhQ9QvI9Qnf1mKG7eBFxMIcJnOtMFFGVizzi1P20FoybposIerXXB5URA7V65gMo0czRi6/16UaKSKYwpixzBlCrrTZ7LO7220PwRdGAbZ2i23bNUNGH8s3E0Xg5+md2MGFrBp3PO9wuSE1G+mrTVjoSKcIMLgtTSgGY8d3vg83tk4T9nF3vYIzQzp+aC64a8rpqWZ6LRDfNou/p+drihiTLrbF0Y9W26uiUcGZ3UCwju51q41HgfYQ+NCgoWcJE7R5+xd+5zL8wsG+o44rWyWzza3dy/rsCaB94C13gAjWY5WKWB3uI1rmNjp42Gl4LQoEYH82d50izySKmtwukyudVJHhmm+0B0gSJmdYg0YUJ0dYPEP3NJBpAM7Wp+Uqp6Ck25PwTsDv77SG7CNWp04aKW4+ lTb8VAWD Q1aCLImpEDOvWHfZSlfseiVQlA4+IJf6k0RySEAPYCmqtuUx5M2LHrRwjg5GbheBcotazymVEBMJSsFVntMUQ06Zu5rp3d1gXs0slhVQDsgwWKMGXUrM+k/DDx5l83lb9iX28dsMcBggWsksrYVbUO+Lo8fJdi+Gkh2IO8VcRWzUiGajzSfCuicog3+WTCP0ILpfjrlbdgt8F27v4qNSeMH8VMJ33J63jiEBc4LyKrgkPk85jHQPc9rSMYIKgkKmqj2JIlDQFOZHE6u03YXaIGs/MJRBtEF3a5UXNk+BJEoyHuoAGV4Qz+CgSyOzASWI0g1Tyiv8+MlCRmRz23Xbdl23QXDkH8ZjfAdVQaUFmMSFfqc4kzb+FjwrOs6QEpRb25EQU X-Bogosity: Ham, tests=bogofilter, spamicity=0.000020, 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 Wed, Sep 11, 2024 at 2:35=E2=80=AFPM Andrii Nakryiko wrote: > > 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 wro= te: > > > > > > 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,= int 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, becaus= e > > > 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(m= m) > > vma->vm_file =3D ...; > > mmap_write_unlock() // inc_mm_lock_seq= (mm) > > > > 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? Yes, I think I can get it to you before leaving for LPC. It might end up affecting paths where we take mmap_lock for write (mmap/munmap/mprotect/etc) but these are not considered fast paths. I'll think about possible tests we can run to evaluate it. > > > 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= (mm) > > > > 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.