| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <62dd026d-1290-49cb-a411-897f4d5f6ca7@suse.cz>
Date: Wed, 2 Apr 2025 11:02:51 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, bpf@...r.kernel.org,
daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org,
akpm@...ux-foundation.org, peterz@...radead.org, rostedt@...dmis.org,
shakeel.butt@...ux.dev, mhocko@...e.com, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locking/local_lock, mm: Replace localtry_ helpers with
local_trylock_t type
On 4/2/25 09:30, Sebastian Andrzej Siewior wrote:
> On 2025-03-31 17:51:34 [-0700], Alexei Starovoitov wrote:
>> From: Alexei Starovoitov <ast@...nel.org>
>>
>> Partially revert commit 0aaddfb06882 ("locking/local_lock: Introduce localtry_lock_t").
>> Remove localtry_*() helpers, since localtry_lock() name might
>> be misinterpreted as "try lock".
>
> So we back to what you suggested initially. I was more a fan of
> explicitly naming things but if this is misleading so be it. So
>
> Acked-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
>
> While at it, could you look at the hunk below and check if it worth it?
> The struct duplication and hoping that the first part remains the same,
> is hoping. This still relies that the first part remains the same but…
I've updated your fixups to v2
https://lore.kernel.org/all/20250401205245.70838-1-alexei.starovoitov@gmail.com/
and to support runtime local_trylock_init(), and it's at the end of my e-mail
But I also thought we could go all the way with removing casting in
that way and stop relying on the same layout implicitly.
So I rewrote this:
#define __local_lock_acquire(lock) \
do { \
local_trylock_t *tl; \
local_lock_t *l; \
\
_Generic((lock), \
local_lock_t *: ({ \
l = this_cpu_ptr(lock); \
}), \
local_trylock_t *: ({ \
tl = this_cpu_ptr(lock); \
l = &tl->llock; \
lockdep_assert(tl->acquired == 0); \
WRITE_ONCE(tl->acquired, 1); \
}), \
default:(void)0); \
local_lock_acquire(l); \
} while (0)
But I'm getting weird errors:
./include/linux/local_lock_internal.h:107:36: error: assignment to ‘local_trylock_t *’ from incompatible pointer type ‘local_lock_t *’ [-Wincompatible-pointer-types]
107 | tl = this_cpu_ptr(lock); \
coming from the guard expansions. I don't understand why it goes to the
_Generic() "branch" of local_trylock_t * with a local_lock_t *.
----8<----
>From eeeb928ccc6d86e93cb573fb93ce4f6aeb8576fb Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@...e.cz>
Date: Wed, 2 Apr 2025 10:13:28 +0200
Subject: [PATCH] fixup! locking/local_lock, mm: Replace localtry_ helpers with
local_trylock_t type
---
include/linux/local_lock.h | 5 +++++
include/linux/local_lock_internal.h | 23 +++++++++++++----------
mm/memcontrol.c | 2 +-
3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
index 7ac9385cd475..16a2ee4f8310 100644
--- a/include/linux/local_lock.h
+++ b/include/linux/local_lock.h
@@ -51,6 +51,11 @@
#define local_unlock_irqrestore(lock, flags) \
__local_unlock_irqrestore(lock, flags)
+/**
+ * local_lock_init - Runtime initialize a lock instance
+ */
+#define local_trylock_init(lock) __local_trylock_init(lock)
+
/**
* local_trylock - Try to acquire a per CPU local lock
* @lock: The lock variable
diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index 2389ae4f69a6..6ccb2c4ef86f 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -17,15 +17,8 @@ typedef struct {
/* local_trylock() and local_trylock_irqsave() only work with local_trylock_t */
typedef struct {
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
- struct lockdep_map dep_map;
- struct task_struct *owner;
-#endif
- /*
- * Same layout as local_lock_t with 'acquired' field at the end.
- * (local_trylock_t *) will be cast to (local_lock_t *).
- */
- int acquired;
+ local_lock_t llock;
+ int acquired;
} local_trylock_t;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -37,6 +30,9 @@ typedef struct {
}, \
.owner = NULL,
+# define LOCAL_TRYLOCK_DEBUG_INIT(lockname) \
+ .llock = { LOCAL_LOCK_DEBUG_INIT((lockname).llock) },
+
static inline void local_lock_acquire(local_lock_t *l)
{
lock_map_acquire(&l->dep_map);
@@ -64,6 +60,7 @@ static inline void local_lock_debug_init(local_lock_t *l)
}
#else /* CONFIG_DEBUG_LOCK_ALLOC */
# define LOCAL_LOCK_DEBUG_INIT(lockname)
+# define LOCAL_TRYLOCK_DEBUG_INIT(lockname)
static inline void local_lock_acquire(local_lock_t *l) { }
static inline void local_trylock_acquire(local_lock_t *l) { }
static inline void local_lock_release(local_lock_t *l) { }
@@ -71,6 +68,7 @@ static inline void local_lock_debug_init(local_lock_t *l) { }
#endif /* !CONFIG_DEBUG_LOCK_ALLOC */
#define INIT_LOCAL_LOCK(lockname) { LOCAL_LOCK_DEBUG_INIT(lockname) }
+#define INIT_LOCAL_TRYLOCK(lockname) { LOCAL_TRYLOCK_DEBUG_INIT(lockname) }
#define __local_lock_init(lock) \
do { \
@@ -80,9 +78,11 @@ do { \
lockdep_init_map_type(&(lock)->dep_map, #lock, &__key, \
0, LD_WAIT_CONFIG, LD_WAIT_INV, \
LD_LOCK_PERCPU); \
- local_lock_debug_init((local_lock_t *)lock); \
+ local_lock_debug_init(lock); \
} while (0)
+#define __local_trylock_init(lock) __local_lock_init(lock.llock)
+
#define __spinlock_nested_bh_init(lock) \
do { \
static struct lock_class_key __key; \
@@ -215,12 +215,15 @@ typedef spinlock_t local_lock_t;
typedef spinlock_t local_trylock_t;
#define INIT_LOCAL_LOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
+#define INIT_LOCAL_TRYLOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
#define __local_lock_init(l) \
do { \
local_spin_lock_init((l)); \
} while (0)
+#define __local_trylock_init(l) __local_lock_init(l)
+
#define __local_lock(__lock) \
do { \
migrate_disable(); \
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index bca86961754e..0401fb7b6c6a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1754,7 +1754,7 @@ struct memcg_stock_pcp {
#define FLUSHING_CACHED_CHARGE 0
};
static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock) = {
- .stock_lock = INIT_LOCAL_LOCK(stock_lock),
+ .stock_lock = INIT_LOCAL_TRYLOCK(stock_lock),
};
static DEFINE_MUTEX(percpu_charge_mutex);
--
2.49.0
Powered by blists - more mailing lists