On 8/25/22 09:50, Vlastimil Babka wrote:
>
> On 8/25/22 09:13, Vlastimil Babka wrote:
>> On 8/25/22 04:56, Joel Fernandes wrote:
>>> Hi,
>>> I am trying to add some debug information to rcu_head for some patches, and need
>>> your help! At least this used to work some time ago (> 1 year).
>>
>> Hmm, got a patch of how it worked back then? Because what I see (in v5.12)
>> struct page definition:
>>
>> struct rcu_head rcu_head;
>>
>> is in union with the whole of
>>
>> struct { /* slab, slob and slub */ ... }
>>
>> which starts with
>>
>> union { struct list_head slab_list; ... }
>> struct kmem_cache *slab_cache; /* not slob */
>>
>> and there's e.g. kmem_rcu_free() in mm/slab.c that does
>>
>> page = container_of(head, struct page, rcu_head);
>> cachep = page->slab_cache;
>>
>> and very similar thing in mm/slub.c rcu_free_slab() - we assume that the
>> rcu_head and slab_cache fields can coexist.
>>
>> So this looks silently fragile to me? In case rcu_head becomes larger than
>> list_head, e.g. with your buf[4], it would start overlaping the
>> page->slab_cache field, no?
>>
>> AFAICS the way it's structured now in struct slab in mm/slab.h, it makes the
>> overlap impossible, thus the struct slab will grow as rcu_head grows, and
>> offsets of the later fields stop matching struct page counterparts, which
>> doesn't grow. Hmm.
>
> The following has made things compile for both SLUB and SLAB (didn't adjust
> SLOB), the idea is to define more explicitly that everything can overlap
> with rcu_head except slab_cache. So the structures don't even grow in the
> end. Which requires moving slab_cache field around. Which would be fine
> except for SLUB and its cmpxchg_double usage, which requires the
> freelist+counter fields to be 128bit aligned, which means we can't fit it
> all with these constraints. So that's broken with the patch and I don't see
> an easy way to fix this.
Ah maybe I found a way. It requires that rcu_head in struct slab would be at
different offset in struct page, and we drop the particular SLAB_MATCH
check. I think it should be fine as we don't cast struct slab to struct page
in the rcu calls/callbacks so it was probably there just for intermediate
steps of the struct slab series, and can be dropped now.
So this should allow you to use up to 32 bytes of rcu_head.
----8<----
diff --git a/include/linux/types.h b/include/linux/types.h
index ea8cf60a8a79..daf7682a7a3b 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -220,6 +220,7 @@ struct ustat {
struct callback_head {
struct callback_head *next;
void (*func)(struct callback_head *head);
+ char buf[4];
} __attribute__((aligned(sizeof(void *))));
#define rcu_head callback_head
diff --git a/mm/slab.h b/mm/slab.h
index 4ec82bec15ec..dd49851f9814 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -12,37 +12,45 @@ struct slab {
#if defined(CONFIG_SLAB)
union {
- struct list_head slab_list;
+ struct {
+ struct list_head slab_list;
+ void *freelist; /* array of free object indexes */
+ void *s_mem; /* first object */
+ };
struct rcu_head rcu_head;
};
struct kmem_cache *slab_cache;
- void *freelist; /* array of free object indexes */
- void *s_mem; /* first object */
unsigned int active;
#elif defined(CONFIG_SLUB)
- union {
- struct list_head slab_list;
- struct rcu_head rcu_head;
-#ifdef CONFIG_SLUB_CPU_PARTIAL
- struct {
- struct slab *next;
- int slabs; /* Nr of slabs left */
- };
-#endif
- };
struct kmem_cache *slab_cache;
- /* Double-word boundary */
- void *freelist; /* first free object */
+
union {
- unsigned long counters;
struct {
- unsigned inuse:16;
- unsigned objects:15;
- unsigned frozen:1;
+ union {
+ struct list_head slab_list;
+#ifdef CONFIG_SLUB_CPU_PARTIAL
+ struct {
+ struct slab *next;
+ int slabs; /* Nr of slabs left */
+ };
+#endif
+ };
+ /* Double-word boundary */
+ void *freelist; /* first free object */
+ union {
+ unsigned long counters;
+ struct {
+ unsigned inuse:16;
+ unsigned objects:15;
+ unsigned frozen:1;
+ };
+ };
};
+ struct rcu_head rcu_head;
};
+
unsigned int __unused;
#elif defined(CONFIG_SLOB)
@@ -66,9 +74,13 @@ struct slab {
#define SLAB_MATCH(pg, sl) \
static_assert(offsetof(struct page, pg) == offsetof(struct slab, sl))
SLAB_MATCH(flags, __page_flags);
+#ifdef CONFIG_SLUB
+SLAB_MATCH(compound_head, slab_cache); /* Ensure bit 0 is clear */
+#else
SLAB_MATCH(compound_head, slab_list); /* Ensure bit 0 is clear */
+#endif
#ifndef CONFIG_SLOB
-SLAB_MATCH(rcu_head, rcu_head);
+// SLAB_MATCH(rcu_head, rcu_head); /* not necessary, hopefully? */
#endif
SLAB_MATCH(_refcount, __page_refcount);
#ifdef CONFIG_MEMCG
