[PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Lai Jiangshan]

The size of struct rcu_head may be changed. When it becomes larger,
it will pollute the page array.

We reserve some some bytes for struct rcu_head when a slab
is allocated in this situation.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 slub.c |   30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index ad545b2..ceb135a 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1254,21 +1254,38 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	__free_pages(page, order);
 }
 
+#define need_reserve_slab_rcu						\
+	(sizeof(((struct page *)NULL)->lru) < sizeof(struct rcu_head))
+
 static void rcu_free_slab(struct rcu_head *h)
 {
 	struct page *page;
 
-	page = container_of((struct list_head *)h, struct page, lru);
+	if (need_reserve_slab_rcu)
+		page = virt_to_head_page(h);
+	else
+		page = container_of((struct list_head *)h, struct page, lru);
+
 	__free_slab(page->slab, page);
 }
 
 static void free_slab(struct kmem_cache *s, struct page *page)
 {
 	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) {
-		/*
-		 * RCU free overloads the RCU head over the LRU
-		 */
-		struct rcu_head *head = (void *)&page->lru;
+		struct rcu_head *head;
+
+		if (need_reserve_slab_rcu) {
+			int order = compound_order(page);
+			int offset = (PAGE_SIZE << order) - s->reserved;
+
+			BUG_ON(s->reserved != sizeof(*head));
+			head = page_address(page) + offset;
+		} else {
+			/*
+			 * RCU free overloads the RCU head over the LRU
+			 */
+			head = (void *)&page->lru;
+		}
 
 		call_rcu(head, rcu_free_slab);
 	} else
@@ -2356,6 +2373,9 @@ static int kmem_cache_open(struct kmem_cache *s,
 	s->flags = kmem_cache_flags(size, flags, name, ctor);
 	s->reserved = 0;
 
+	if (need_reserve_slab_rcu && (s->flags & SLAB_DESTROY_BY_RCU))
+		s->reserved = sizeof(struct rcu_head);
+
 	if (!calculate_sizes(s, -1))
 		goto error;
 	if (disable_higher_order_debug) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Pekka Enberg]

On Tue, Mar 1, 2011 at 10:03 AM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> The size of struct rcu_head may be changed. When it becomes larger,
> it will pollute the page array.
>
> We reserve some some bytes for struct rcu_head when a slab
> is allocated in this situation.
>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

The SLAB and SLUB patches are fine by me if there are going to be real
users for this. Christoph, Paul?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Christoph Lameter]

On Tue, 1 Mar 2011, Pekka Enberg wrote:

> The SLAB and SLUB patches are fine by me if there are going to be real
> users for this. Christoph, Paul?

The solution is a bit overkill. It would be much simpler to add a union to
struct page that has lru and the rcu in there similar things can be done
for SLAB and the network layer. A similar issue already exists for the
spinlock in struct page. Lets follow the existing way of handling this.

Struct page may be larger for debugging purposes already because of the
need for extended spinlock data.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Lai Jiangshan]

On 03/01/2011 11:11 PM, Christoph Lameter wrote:
> On Tue, 1 Mar 2011, Pekka Enberg wrote:
> 
>> The SLAB and SLUB patches are fine by me if there are going to be real
>> users for this. Christoph, Paul?
> 
> The solution is a bit overkill. It would be much simpler to add a union to
> struct page that has lru and the rcu in there similar things can be done
> for SLAB and the network layer. A similar issue already exists for the
> spinlock in struct page. Lets follow the existing way of handling this.

I don't want to impact the whole system too much to
touch struct page. The solution changes existed things little,
and the reversed data may just make use of the pad data.

> 
> Struct page may be larger for debugging purposes already because of the
> need for extended spinlock data.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Hugh Dickins]

On Tue, Mar 1, 2011 at 7:11 AM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 1 Mar 2011, Pekka Enberg wrote:
>
> > The SLAB and SLUB patches are fine by me if there are going to be real
> > users for this. Christoph, Paul?
>
> The solution is a bit overkill. It would be much simpler to add a union to
> struct page that has lru and the rcu in there similar things can be done
> for SLAB and the network layer. A similar issue already exists for the
> spinlock in struct page. Lets follow the existing way of handling this.
>
> Struct page may be larger for debugging purposes already because of the
> need for extended spinlock data.

That was so for a long time, but I stopped it just over a year ago
with commit a70caa8ba48f21f46d3b4e71b6b8d14080bbd57a, stop ptlock
enlarging struct page.

Partly out of shame at how large struct page was growing when lockdep
is on, but also a subtle KSM reason which might apply here too: KSM
relies on the content of page->mapping to be kernel pointer to a
relevant structure, NULLed when the page is freed.

If a union leads to "random junk" overwriting the page->mapping field
when the page is reused, and that junk could resemble the pointer in
question, then KSM would mistakenly think it still owned the page.
Very remote chance, and maybe it amounts to no more than a leak.  But
I'd still prefer we keep page->mapping for pointers (sometimes with
lower bits set as flags).

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Christoph Lameter]

On Tue, 1 Mar 2011, Hugh Dickins wrote:

> > Struct page may be larger for debugging purposes already because of the
> > need for extended spinlock data.
>
> That was so for a long time, but I stopped it just over a year ago
> with commit a70caa8ba48f21f46d3b4e71b6b8d14080bbd57a, stop ptlock
> enlarging struct page.

Strange. I just played around with in in January and the page struct size
changes when I build kernels with full debugging. I have some
cmpxchg_double patches here that depend on certain alignment in the page
struct. Debugging causes all that stuff to get out of whack so that I had
to do some special patches to make sure fields following the spinlock are
properly aligned when the sizes change.

> If a union leads to "random junk" overwriting the page->mapping field
> when the page is reused, and that junk could resemble the pointer in
> question, then KSM would mistakenly think it still owned the page.
> Very remote chance, and maybe it amounts to no more than a leak.  But
> I'd still prefer we keep page->mapping for pointers (sometimes with
> lower bits set as flags).

DESTROY BY RCU uses the lru field which follows the mapping field in page
struct. Why would random junk overwrite the mapping field?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Hugh Dickins]

On Wed, Mar 2, 2011 at 4:32 AM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 1 Mar 2011, Hugh Dickins wrote:
>
>> > Struct page may be larger for debugging purposes already because of the
>> > need for extended spinlock data.
>>
>> That was so for a long time, but I stopped it just over a year ago
>> with commit a70caa8ba48f21f46d3b4e71b6b8d14080bbd57a, stop ptlock
>> enlarging struct page.
>
> Strange. I just played around with in in January and the page struct size
> changes when I build kernels with full debugging. I have some
> cmpxchg_double patches here that depend on certain alignment in the page
> struct. Debugging causes all that stuff to get out of whack so that I had
> to do some special patches to make sure fields following the spinlock are
> properly aligned when the sizes change.

That puzzles me, it's not my experience and I don't have an
explanation: do you have time to investigate?

Uh oh, you're going to tell me you're working on an out-of-tree
architecture with a million cpus ;)  In that case, yes, I'm afraid
I'll have to update the SPLIT_PTLOCK_CPUS defaulting (for a million -
1 even).

>
>> If a union leads to "random junk" overwriting the page->mapping field
>> when the page is reused, and that junk could resemble the pointer in
>> question, then KSM would mistakenly think it still owned the page.
>> Very remote chance, and maybe it amounts to no more than a leak.  But
>> I'd still prefer we keep page->mapping for pointers (sometimes with
>> lower bits set as flags).
>
> DESTROY BY RCU uses the lru field which follows the mapping field in page
> struct. Why would random junk overwrite the mapping field?

Random junk does not overwrite the mapping field with the current
implementation of DESTROY_BY_RCU.  But you and Jiangshan were
discussing how to change it, so I was warning of this issue with
page->mapping.

But I would anyway agree with Jiangshan, that it's preferable not to
bloat struct page size just for this DESTROY_BY_RCU issue, even if it
is only an issue when debugging.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Lai Jiangshan]

On 03/07/2011 03:39 AM, Hugh Dickins wrote:
> On Wed, Mar 2, 2011 at 4:32 AM, Christoph Lameter <cl@linux.com> wrote:
>> On Tue, 1 Mar 2011, Hugh Dickins wrote:
>>
>>>> Struct page may be larger for debugging purposes already because of the
>>>> need for extended spinlock data.
>>>
>>> That was so for a long time, but I stopped it just over a year ago
>>> with commit a70caa8ba48f21f46d3b4e71b6b8d14080bbd57a, stop ptlock
>>> enlarging struct page.
>>
>> Strange. I just played around with in in January and the page struct size
>> changes when I build kernels with full debugging. I have some
>> cmpxchg_double patches here that depend on certain alignment in the page
>> struct. Debugging causes all that stuff to get out of whack so that I had
>> to do some special patches to make sure fields following the spinlock are
>> properly aligned when the sizes change.
> 
> That puzzles me, it's not my experience and I don't have an
> explanation: do you have time to investigate?
> 
> Uh oh, you're going to tell me you're working on an out-of-tree
> architecture with a million cpus ;)  In that case, yes, I'm afraid
> I'll have to update the SPLIT_PTLOCK_CPUS defaulting (for a million -
> 1 even).
> 
>>
>>> If a union leads to "random junk" overwriting the page->mapping field
>>> when the page is reused, and that junk could resemble the pointer in
>>> question, then KSM would mistakenly think it still owned the page.
>>> Very remote chance, and maybe it amounts to no more than a leak.  But
>>> I'd still prefer we keep page->mapping for pointers (sometimes with
>>> lower bits set as flags).
>>
>> DESTROY BY RCU uses the lru field which follows the mapping field in page
>> struct. Why would random junk overwrite the mapping field?
> 
> Random junk does not overwrite the mapping field with the current
> implementation of DESTROY_BY_RCU.  But you and Jiangshan were
> discussing how to change it, so I was warning of this issue with
> page->mapping.
> 
> But I would anyway agree with Jiangshan, that it's preferable not to
> bloat struct page size just for this DESTROY_BY_RCU issue, even if it
> is only an issue when debugging.
> 

A union with rcu_head does not cause overwriting, But the problem is
only one minority use of the page (as a DESTROY_BY_RCU slab) needs to
fit a rcu_head and to bloat the struct page size.

Except for preparing for debugging or adding priority information for rcu_head,
this patch also does a de-coupling work.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Christoph Lameter]

On Tue, 1 Mar 2011, Lai Jiangshan wrote:

> -		 * RCU free overloads the RCU head over the LRU
> -		 */
> -		struct rcu_head *head = (void *)&page->lru;
> +		struct rcu_head *head;
> +
> +		if (need_reserve_slab_rcu) {
> +			int order = compound_order(page);
> +			int offset = (PAGE_SIZE << order) - s->reserved;
> +
> +			BUG_ON(s->reserved != sizeof(*head));

VM_BUG_ON is sufficient here I think.

Otherwise

Acked-by: Christoph Lameter <cl@linux.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Christoph Lameter]

On Sun, 6 Mar 2011, Hugh Dickins wrote:

> >> That was so for a long time, but I stopped it just over a year ago
> >> with commit a70caa8ba48f21f46d3b4e71b6b8d14080bbd57a, stop ptlock
> >> enlarging struct page.
> >
> > Strange. I just played around with in in January and the page struct size
> > changes when I build kernels with full debugging. I have some
> > cmpxchg_double patches here that depend on certain alignment in the page
> > struct. Debugging causes all that stuff to get out of whack so that I had
> > to do some special patches to make sure fields following the spinlock are
> > properly aligned when the sizes change.
>
> That puzzles me, it's not my experience and I don't have an
> explanation: do you have time to investigate?
>
> Uh oh, you're going to tell me you're working on an out-of-tree
> architecture with a million cpus ;)  In that case, yes, I'm afraid
> I'll have to update the SPLIT_PTLOCK_CPUS defaulting (for a million -
> 1 even).

No I am not working on any out of tree structure. Just regular dual socket
server boxes with 24 processors (which is a normal business machine
configuration these days).

But then there is also CONFIG_GENERIC_LOCKBREAK. That does not affect
things?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] slub,rcu: don't assume the size of struct rcu_head

[Hugh Dickins]

On Thu, 17 Mar 2011, Christoph Lameter wrote:
> On Sun, 6 Mar 2011, Hugh Dickins wrote:
> 
> > >> That was so for a long time, but I stopped it just over a year ago
> > >> with commit a70caa8ba48f21f46d3b4e71b6b8d14080bbd57a, stop ptlock
> > >> enlarging struct page.
> > >
> > > Strange. I just played around with in in January and the page struct size
> > > changes when I build kernels with full debugging. I have some
> > > cmpxchg_double patches here that depend on certain alignment in the page
> > > struct. Debugging causes all that stuff to get out of whack so that I had
> > > to do some special patches to make sure fields following the spinlock are
> > > properly aligned when the sizes change.
> >
> > That puzzles me, it's not my experience and I don't have an
> > explanation: do you have time to investigate?
> >
> > Uh oh, you're going to tell me you're working on an out-of-tree
> > architecture with a million cpus ;)  In that case, yes, I'm afraid
> > I'll have to update the SPLIT_PTLOCK_CPUS defaulting (for a million -
> > 1 even).
> 
> No I am not working on any out of tree structure. Just regular dual socket
> server boxes with 24 processors (which is a normal business machine
> configuration these days).
> 
> But then there is also CONFIG_GENERIC_LOCKBREAK. That does not affect
> things?

CONFIG_GENERIC_LOCKBREAK adds an unsigned int break_lock after the
int-sized arch_spinlock_t: which would make no difference on 64-bit
anyway (the two ints fitting into one long), and makes no difference
on 32-bit because we have put
	struct {
		unsigned long private;
		struct address_space *mapping;
	};
into the union with spinlock_t ptl - the arch_spinlock_t then
overlays private and the break_lock overlays mapping.

I'd much rather have had simple elements in that union, but it's
precisely because of 32-bit CONFIG_GENERIC_LOCKBREAK that we need
that structure in there.

(It is important to the KSM assumption about page->mapping that
what goes into break_lock is either 0 or 1: in neither case could
page->mapping look like a kmem address + 3.)

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>