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 4972AC83F33 for ; Mon, 4 Sep 2023 18:46:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9C4F88E000E; Mon, 4 Sep 2023 14:46:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 975478E0009; Mon, 4 Sep 2023 14:46:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 83D758E000E; Mon, 4 Sep 2023 14:46:20 -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 75AE58E0009 for ; Mon, 4 Sep 2023 14:46:20 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 4467BA07F0 for ; Mon, 4 Sep 2023 18:46:20 +0000 (UTC) X-FDA: 81199795320.28.301238A Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by imf11.hostedemail.com (Postfix) with ESMTP id 521A74000B for ; Mon, 4 Sep 2023 18:46:18 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=TLZpOC0r; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf11.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1693853178; 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=xPwa8jzfvAKIRzM093CaFHhTk58/oFuCdDDlLlEVSBw=; b=hmIAeo/IBbgXv9mEOrRoiXScpMpegUydo4vkNtuxWGU6XOhk1a56P7+TpPj7PlUP+eCos7 jBU6hZo3HJVFRsZCXCKnrDykPnfBwOMEC2105epAEEj0+vdWAhLwrnAyTBLw6aa8M+1MUG m/KMMD+DqB/X8gV6a3mb8owDyzpEi4s= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=TLZpOC0r; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf11.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1693853178; a=rsa-sha256; cv=none; b=1kpUDr6DVuTUJiPk5YDSCGtDrf2Qd1X7m2eetOoT4L+HY5sqklyARPFaMbLOCww2xbUzj8 Q+NcIIEtZy/ni9jMdpgFZfHre0NqkZPw4uj3WMTEEWnfYlxMCLStPS7SD/TZIHgGgqWnwG 0b/E/PTF2mNnYe7Z0wXlaq9HLiVZFgg= Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-26ef24b8e5aso1170335a91.0 for ; Mon, 04 Sep 2023 11:46:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693853177; x=1694457977; 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=xPwa8jzfvAKIRzM093CaFHhTk58/oFuCdDDlLlEVSBw=; b=TLZpOC0r1Y35upplgzVN7p87yJc/HkOTMhaqRRCETp6xotkAmmjojnYtzsWXjPOl2u r0JI/lHlyLF4Lgty1hVTujz4/apWBaLm+IYRBW2nKM5wM+29h77HKAsSgLsZ9qbcjS54 FdzKfC0LEPJyd7eP5UqjZ1bjvhVyNmKFTyuYw1qbsjAyhuZ/ZDK5988PZpI65ItMsBry m53WyFR5lCZbASBUWMfJeJ/F9yKwV/88DtQwC4vrnaa5QFFEw8xMI7TXtle76sCcTauE 6uXAxr1uDPMsv/+3WCbP6ix8e3RNP0MzyPuaLjPMC9swSkbsSUbvh6EpLSWo57StI18X QjzA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693853177; x=1694457977; 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=xPwa8jzfvAKIRzM093CaFHhTk58/oFuCdDDlLlEVSBw=; b=AJ8+OM67KwsNhvJt7ot/lZ4lFbwnNBAZIgU0VXpfcH87CYnMnCCM3gLPd6a30jlA5v AHrH8WqcgNF59UUEfuc9SmaQ2J1ceSTkrxgctBljiffiVVwGQqpsGcOlKj/gGvDUlG9Y b+WRBDTO+SFxoURJ2T67BaUfGCValFJK0Gd9H97NagYl+7pnvMnBp+rl8xOfBhiCA3Qr udfskrRQ+yMkACI4Unar9sqdBwaoNpszMZsJMPsWPNhOXa5C8LRwHSM5l11NoU2+FBo/ RviB/WBETpFntWNp4PfZISbxyLSmWkmuj5RkotKQqgETFLULD7aW9nl2IVCl1B9nSuWj bySA== X-Gm-Message-State: AOJu0YyEVua8v/LQEEg1kGr3ChqQzdH+e6Oc1hYQSCQvQTlnkg8/SNd/ 8yfTlatmm1z45Uo5qzR98UBTB43MvqxffP+NWCw= X-Google-Smtp-Source: AGHT+IEikSBJJP1dxY1WJ4P0PY/YwsxJnEhK8ggnWQCjwStB754ZabPYsyu6vtUGQYE/G2z+l7EsZU2urYWGzmAYQ9c= X-Received: by 2002:a17:90a:38a5:b0:262:fb5d:147b with SMTP id x34-20020a17090a38a500b00262fb5d147bmr9835987pjb.19.1693853177173; Mon, 04 Sep 2023 11:46:17 -0700 (PDT) MIME-Version: 1.0 References: <6db160185d3bd9b3312da4ccc073adcdac58709e.1693328501.git.andreyknvl@google.com> In-Reply-To: From: Andrey Konovalov Date: Mon, 4 Sep 2023 20:46:06 +0200 Message-ID: Subject: Re: [PATCH 11/15] stackdepot: use read/write lock To: Marco Elver Cc: andrey.konovalov@linux.dev, Alexander Potapenko , Dmitry Vyukov , Vlastimil Babka , kasan-dev@googlegroups.com, Evgenii Stepanov , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrey Konovalov Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 521A74000B X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: agzrn333heynwwkmqqksx613rdtz7i5t X-HE-Tag: 1693853178-315670 X-HE-Meta: U2FsdGVkX1/wvPLnVavMViab8DbA2dozlmxmN0dgrVN0nPCLu715LfIGjO8H3UB3a3KDgnjbCAzFUggWwvXpOqFgXCxxzs4t9WuymorKo4TA1JVoKbG/SPRr/okyDbNTZb+V0qZXJomohXfw/rrf3TYDVCD2FyQVTjp/N/JcSSNaLfbUDOHbSQfGJ+RUKeenDdjdYg8yetVd4suQMK16cVtg4E31rrq6kLhOnTy0HAQ29L/avUOi7PnS3WZaCVFQqFeqOJaIrqcFkdDblH2O/Vbpl1/Na4aVqGhRLfGyJYwAcGTN2Nl6Q3KHhGBR0u+UltHo3n7vRp+hSKVdQoY4CY+hh9tWxiF9T/e3woZM2AlmTGJf4v7xAZjmkr/tq7v46WcfP+iLSB6O/9G8egYulC2L1s9M23BmnCze7SoqaAoWtEeScC6NtHL2PyD/dcXC5HLGsR5pozqluZ4ePw8WBUYXM9uwBbRGy303urV7FnqHY/JKx9oRdEQx5X5MejdPCiHdj1YQ182y3Oszar7bJQvP+syZp2zEEhxUq8cY5qjddaZnQdUVvYNQp8Nktr3EtkY3dNQEXtDMGrUANWu3VRMP6CoPSbOADlQtPgkO+aXdLfMIwLRc751iQFQLezIXLSw4PuPQnAR4dxZCz0wMOEaFI6wf0F4niXFBq21qcZ1z1U2xLqEmVgKxSboXw3HPVaJegXjlqoC0cIB7T7Te3Z25OnyKsdCW/EFbr8DEJKxtJ9gvY6k3lDBi+VShVIyl+WMeDu5WXofqA4PTdj2wR+r+rx3lvPZ6eriIMU1jlvQLQMZ0Yxaz3Ou2iu0jjPimVf4PGSBvQB2RbN4xVyXg0RQ9gwvfAZqVtwJs7b9mmOv3j9Dd8xXcuFjB4+a1kS2eYJ1eiJXGfIcqaMK8kJlCicUZXwMi6UCaPYtUHX2DiLCn7GhRejb2a2FIPSlL8PdJYVmaYKabb+hiqrxMmsR e2VyAW0g cf6rkfvs+gkx/0syAnGqgpVrLua/OozR5AeZjJE5zuCJAwssFCAGtsN0+/vZd/c30oBx+1sZdnpHi1UXrPNUhv/MntfP843kHR97Yy6R8tDKHUP2YlvXwrS3r6k4ZLr1WX7sngmnrSGFeMjqhk+JPHNzRDLPJaS6USmkz21eVr7+zjYfmUQvlOZ8E5opFZDq3wFXH4SGv6++tRT7SRMhDwq/QbvlcBBUS6RPySielJat04Flbnl1bfYoLthgbPrCQEEALHLBdtp8cgfz757IJOIBo0y52kViqbKKghYZcIZfiyw50//uiGonHhG+PDvpOEBwT76ubXTZwEVwy8jsVUVmc0NYHyQmm+Z1QtJBi/yg7eb6ByqkZazAWxw== 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, Aug 30, 2023 at 11:13=E2=80=AFAM Marco Elver wro= te: > > > -static int new_pool_required =3D 1; > > +static bool new_pool_required =3D true; > > +/* Lock that protects the variables above. */ > > +static DEFINE_RWLOCK(pool_rwlock); > > Despite this being a rwlock, it'll introduce tons of (cache) contention > for the common case (stack depot entry exists). > > If creating new stack depot entries is only common during "warm-up" and > then becomes exceedingly rare, I think a percpu-rwsem (read-lock is a > CPU-local access, but write-locking is expensive) may be preferable. Good suggestion. I propose that we keep the rwlock for now, and I'll check whether the performance is better with percpu-rwsem once I get to implementing and testing the performance changes. I'll also check whether percpu-rwsem makes sense for stack ring in tag-based KASAN modes. > > @@ -262,10 +258,8 @@ static void depot_keep_new_pool(void **prealloc) > > /* > > * If a new pool is already saved or the maximum number of > > * pools is reached, do not use the preallocated memory. > > - * READ_ONCE is only used to mark the variable as atomic, > > - * there are no concurrent writes. > > */ > > - if (!READ_ONCE(new_pool_required)) > > + if (!new_pool_required) > > In my comment for the other patch I already suggested this change. Maybe > move it there. Will do in v2. > > > return; > > > > /* > > @@ -281,9 +275,8 @@ static void depot_keep_new_pool(void **prealloc) > > * At this point, either a new pool is kept or the maximum > > * number of pools is reached. In either case, take note that > > * keeping another pool is not required. > > - * smp_store_release pairs with smp_load_acquire in stack_depot_s= ave. > > */ > > - smp_store_release(&new_pool_required, 0); > > + new_pool_required =3D false; > > } > > > > /* Updates refences to the current and the next stack depot pools. */ > > @@ -300,7 +293,7 @@ static bool depot_update_pools(void **prealloc) > > > > /* Take note that we might need a new new_pool. */ > > if (pools_num < DEPOT_MAX_POOLS) > > - smp_store_release(&new_pool_required, 1); > > + new_pool_required =3D true; > > > > /* Try keeping the preallocated memory for new_pool. */ > > goto out_keep_prealloc; > > @@ -369,18 +362,13 @@ depot_alloc_stack(unsigned long *entries, int siz= e, u32 hash, void **prealloc) > > static struct stack_record *depot_fetch_stack(depot_stack_handle_t han= dle) > > { > > union handle_parts parts =3D { .handle =3D handle }; > > - /* > > - * READ_ONCE pairs with potential concurrent write in > > - * depot_init_pool. > > - */ > > - int pools_num_cached =3D READ_ONCE(pools_num); > > void *pool; > > size_t offset =3D parts.offset << DEPOT_STACK_ALIGN; > > struct stack_record *stack; > > I'd add lockdep assertions to check that the lock is held appropriately > when entering various helper functions that don't actually take the > lock. Similarly for places that should not have the lock held you could > assert the lock is not held. Will do in v2. Thanks!