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 22310C021B5 for ; Mon, 24 Feb 2025 08:20:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8E9D86B007B; Mon, 24 Feb 2025 03:20:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 872E26B0083; Mon, 24 Feb 2025 03:20:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7136A6B0085; Mon, 24 Feb 2025 03:20:02 -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 524AE6B007B for ; Mon, 24 Feb 2025 03:20:02 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id C02171A0E37 for ; Mon, 24 Feb 2025 08:20:01 +0000 (UTC) X-FDA: 83154140202.18.532941D Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by imf30.hostedemail.com (Postfix) with ESMTP id DDCAD8000B for ; Mon, 24 Feb 2025 08:19:59 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b=BllI6CKN; dkim=pass header.d=linutronix.de header.s=2020e header.b=hxL6KaR9; spf=pass (imf30.hostedemail.com: domain of bigeasy@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=bigeasy@linutronix.de; dmarc=pass (policy=none) header.from=linutronix.de ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740385200; 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=9b6JI1PAYyIfk1s7vLaQh6uto3DYX7c5B+kYCp5fkFk=; b=6SfVGSXXlN+eohpI6Tj7HLfJxhTltivGgxaeiDgP3nVVbMpVxAZj3+/ZHcD4W0mgAFVsjQ aMFUjIh2NSYqfe33NWg40zIBfgHF0wLnKOm9QoetwZ5Fuh0rw2FtmnMpP4zxWLTuleDk40 2pEG9GBvFgZvI2JTMBehjoB+ivwpoYc= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b=BllI6CKN; dkim=pass header.d=linutronix.de header.s=2020e header.b=hxL6KaR9; spf=pass (imf30.hostedemail.com: domain of bigeasy@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=bigeasy@linutronix.de; dmarc=pass (policy=none) header.from=linutronix.de ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740385200; a=rsa-sha256; cv=none; b=prhEsO8e7krw9fKRsBHnfbKaLKfufCLS+rz1dKxFo6i6fO4rLqggB0gccoKH5NdVQtbeT2 Hl5TRlUwUFhuY+b4Sy6bw5vBs4DBnC/oJVGRNz1yBSaPgbBqy2XhOjz2oTnx/sjvVQMQ1b TQG6yRUMk67eDUTcd/a4uAHRkF84SVM= Date: Mon, 24 Feb 2025 09:19:56 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1740385197; h=from:from: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; bh=9b6JI1PAYyIfk1s7vLaQh6uto3DYX7c5B+kYCp5fkFk=; b=BllI6CKNnuo6xYkmJqFOsIkmEJBxivlt8tjDn5uFY+6g134dkoSaguOBHNnAxowunfWhBE aXFnPZc0evUNstXD3Fd5T45Rgoqk24MOzlRTjiOfLawj1tguU9U0ltbNOZXOoJJyleAjn4 Su/yTOFoJRLTfho8iLKwOVuKJv+aQRTdzuTPu5nSAbpt/FivLo4sRR8uTxRk4H1AwpbR+W W2ip5t0b7EVmxIaky9mPDZUtHesrTKGbeRkrA+JfNSpb9+XtRuc2uFqyLjcI8aub3eJ/SR XrEgWVMWHsCnA3g40ikMm8uAUt4zmyVuauBnQMbyZfYuaps/RJ3sG3kNXjuarQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1740385197; h=from:from: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; bh=9b6JI1PAYyIfk1s7vLaQh6uto3DYX7c5B+kYCp5fkFk=; b=hxL6KaR9KJV4d90jn/TKI1wfgV8r0ZUB+HQv6e1PgA5wrDKroIBz05Ju6ojvT3XNPY6Xag GbzWlV2BumnsSQAA== From: Sebastian Andrzej Siewior To: Sergey Senozhatsky Cc: Andrew Morton , Yosry Ahmed , Hillf Danton , Kairui Song , Minchan Kim , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v8 01/17] zram: sleepable entry locking Message-ID: <20250224081956.knanS8L_@linutronix.de> References: <20250221222958.2225035-1-senozhatsky@chromium.org> <20250221222958.2225035-2-senozhatsky@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <20250221222958.2225035-2-senozhatsky@chromium.org> X-Rspamd-Queue-Id: DDCAD8000B X-Stat-Signature: gm5uy7rnqmfxtctboke57iqfr6qy3i7r X-Rspam-User: X-Rspamd-Server: rspam01 X-HE-Tag: 1740385199-743725 X-HE-Meta: U2FsdGVkX183StVWNBHD7aplxZ3EEyeI7U0Xpqb1joLCePWPMFBLhXLyz31sGwoHqyTQAzYDSP4az5YBI6ygCje56e/v4hUxkvZr2DM+mB5ulz9sMRZbpIZWuEduz0gY19F1B9CPcORq/BPtGuUaZyyCcQUqeJxwU6t8ThCEtXk/JL2lg26Za0N9IJ8Ru0iRhdtELNQuUjuzSulFJcCcqDGF9fEiWwwOrGaIsud2B9JUZPrB5rpIj/OFQPlcZOFWqjxcHBXW/AdB+yngtg2/YT+FN0SR8YocdVvU1q3jMgU4QjmT4LC/TSy8pj45I0tV7zYOknkTQANJMRdK7BWG/FliXvGGMkPhF/Ykz1Ql6qVKWhkUBZbfK569EuspzhuXRrFsxj0Ko2DdAl4hDOqkB9/cpDImsxNipqVR72oSVDvQD7dU2fdA371tKnLS/M7Ra+PLZVDcxrevNqWFGVxnh71DpvTi+3rLsVoRpGP+C93Cy1SU6GutcLV5kuVB3+b8JxsP6P+2RdP+lVlZVmsdrQjWx2SCtRZ94T1Gn4DDsq5/2Y4OGCPgZ2NtL4vgWUoPAXbGzpShki4inbiPo0atv/ruyGle1dHM+VYNBQyuUeW8f4AVFSCeuje1ITDZ1/K3ENKJ7CYCnx3HYscD4c9kmXMNpup3O1kaye14cPBj1bz9/H8h6OPx7smtvz8NRF2mMjQkOrMMrVg36kYqNhM1LmqTbEMVHPbldNPZd1Aw6QU+l8G7ApsQos3fkvotaemKY+lN71KMpqDiLQIrV0vVe8ZAqcnTKt1mBE+fVkR1U7Ya5ZjXzSZpZIQzN1s6kb7unxTtMBvddOVe0d9kjTya7PEwXmXA5doFHiQA+cTj0iwhsCWqgLgFi9Mg46FNsOQSl0UtQXmSqaCobjRZzxDPdUIydhI5y2G+D2COFP6A/AmITA0kvmhimGsSrfdAV308jisPykoE/574NDNNxI6 xa8M4CbN U+KP0odyume3iqj8XZSSkHJCWmmKacyGWmXYvxLDRfTjDOGXBPBqlZn5ZVoP8TS+/nWcl/JMMyRqgrSALHyS0u5MtcYlQgtmF2Kd8zvKiGjov6r9sr1KQck+9GQcLgTcf6FlZAcvaaDOuRNcv1MEfJsqALPZ0964rBNU9rggdFDneTnsdb1KT1cByH079ImzJpa/7 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 2025-02-22 07:25:32 [+0900], Sergey Senozhatsky wrote: =E2=80=A6 > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 9f5020b077c5..37c5651305c2 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -58,19 +58,62 @@ static void zram_free_page(struct zram *zram, size_t = index); > static int zram_read_from_zspool(struct zram *zram, struct page *page, > u32 index); > =20 > -static int zram_slot_trylock(struct zram *zram, u32 index) > +#ifdef CONFIG_DEBUG_LOCK_ALLOC > +#define slot_dep_map(zram, index) (&(zram)->table[(index)].dep_map) > +#define zram_lock_class(zram) (&(zram)->lock_class) > +#else > +#define slot_dep_map(zram, index) NULL > +#define zram_lock_class(zram) NULL > +#endif That CONFIG_DEBUG_LOCK_ALLOC here is not needed because dep_map as well as lock_class goes away in !CONFIG_DEBUG_LOCK_ALLOC case. > +static void zram_slot_lock_init(struct zram *zram, u32 index) > { > - return spin_trylock(&zram->table[index].lock); > + lockdep_init_map(slot_dep_map(zram, index), > + "zram->table[index].lock", > + zram_lock_class(zram), 0); > +} Why do need zram_lock_class and slot_dep_map? As far as I can tell, you init both in the same place and you acquire both in the same place. Therefore it looks like you tell lockdep that you acquire two locks while it would be enough to do it with one. > +/* > + * entry locking rules: > + * > + * 1) Lock is exclusive > + * > + * 2) lock() function can sleep waiting for the lock > + * > + * 3) Lock owner can sleep > + * > + * 4) Use TRY lock variant when in atomic context > + * - must check return value and handle locking failers > + */ > +static __must_check bool zram_slot_trylock(struct zram *zram, u32 index) > +{ > + unsigned long *lock =3D &zram->table[index].flags; > + > + if (!test_and_set_bit_lock(ZRAM_ENTRY_LOCK, lock)) { > + mutex_acquire(slot_dep_map(zram, index), 0, 1, _RET_IP_); > + lock_acquired(slot_dep_map(zram, index), _RET_IP_); > + return true; > + } > + > + lock_contended(slot_dep_map(zram, index), _RET_IP_); > + return false; > } > =20 > static void zram_slot_lock(struct zram *zram, u32 index) > { > - spin_lock(&zram->table[index].lock); > + unsigned long *lock =3D &zram->table[index].flags; > + > + mutex_acquire(slot_dep_map(zram, index), 0, 0, _RET_IP_); > + wait_on_bit_lock(lock, ZRAM_ENTRY_LOCK, TASK_UNINTERRUPTIBLE); > + lock_acquired(slot_dep_map(zram, index), _RET_IP_); This looks odd. The first mutex_acquire() can be invoked twice by two threads, right? The first thread gets both (mutex_acquire() and lock_acquired()) while, the second gets mutex_acquire() and blocks on wait_on_bit_lock()). > } > =20 > static void zram_slot_unlock(struct zram *zram, u32 index) > { > - spin_unlock(&zram->table[index].lock); > + unsigned long *lock =3D &zram->table[index].flags; > + > + mutex_release(slot_dep_map(zram, index), _RET_IP_); > + clear_and_wake_up_bit(ZRAM_ENTRY_LOCK, lock); > } > =20 > static inline bool init_done(struct zram *zram) =E2=80=A6 > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h > index db78d7c01b9a..794c9234e627 100644 > --- a/drivers/block/zram/zram_drv.h > +++ b/drivers/block/zram/zram_drv.h > @@ -58,13 +58,18 @@ enum zram_pageflags { > __NR_ZRAM_PAGEFLAGS, > }; > =20 > -/*-- Data structures */ > - > -/* Allocated for each disk page */ > +/* > + * Allocated for each disk page. We use bit-lock (ZRAM_ENTRY_LOCK bit > + * of flags) to save memory. There can be plenty of entries and standard > + * locking primitives (e.g. mutex) will significantly increase sizeof() > + * of each entry and hence of the meta table. > + */ > struct zram_table_entry { > unsigned long handle; > - unsigned int flags; > - spinlock_t lock; > + unsigned long flags; > +#ifdef CONFIG_DEBUG_LOCK_ALLOC > + struct lockdep_map dep_map; > +#endif > #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME > ktime_t ac_time; > #endif > @@ -137,5 +142,8 @@ struct zram { > struct dentry *debugfs_dir; > #endif > atomic_t pp_in_progress; > +#ifdef CONFIG_DEBUG_LOCK_ALLOC > + struct lock_class_key lock_class; > +#endif As mentioned earlier, no need for CONFIG_DEBUG_LOCK_ALLOC. > }; > #endif > --=20 > 2.48.1.601.g30ceb7b040-goog >=20