[PATCH] release quicklist before free_page

[PATCH] release quicklist before free_page

[Daniel Walker]

Resolves,

BUG: sleeping function called from invalid context cc1(29651) at kernel/rtmutex.c:636
in_atomic():1 [00000001], irqs_disabled():0
 [<c0119f50>] __might_sleep+0xf3/0xf9
 [<c031600e>] __rt_spin_lock+0x21/0x3c
 [<c014102c>] get_zone_pcp+0x20/0x29
 [<c0141a40>] free_hot_cold_page+0xdc/0x167
 [<c013a3f4>] add_preempt_count+0x12/0xcc
 [<c0110d92>] pgd_dtor+0x0/0x1
 [<c015d865>] quicklist_trim+0xb7/0xe3
 [<c0111025>] check_pgt_cache+0x19/0x1c
 [<c0148df5>] free_pgtables+0x54/0x12c
 [<c013a3f4>] add_preempt_count+0x12/0xcc
 [<c014e5be>] unmap_region+0xeb/0x13b


It looks like the quicklist isn't used after a few variables are evaluated.
So no need to keep preemption disabled over the whole function.

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

---
 mm/quicklist.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6.22.1/mm/quicklist.c
===================================================================
--- linux-2.6.22.1.orig/mm/quicklist.c
+++ linux-2.6.22.1/mm/quicklist.c
@@ -55,6 +55,7 @@ void quicklist_trim(int nr, void (*dtor)
 	q = &get_cpu_var(quicklist)[nr];
 	if (q->nr_pages > min_pages) {
 		pages_to_free = min_pages_to_free(q, min_pages, max_free);
+		put_cpu_var(quicklist);
 
 		while (pages_to_free > 0) {
 			/*
@@ -68,8 +69,8 @@ void quicklist_trim(int nr, void (*dtor)
 			free_page((unsigned long)p);
 			pages_to_free--;
 		}
-	}
-	put_cpu_var(quicklist);
+	} else
+		put_cpu_var(quicklist);
 }
 
 unsigned long quicklist_total_size(void)
-- 

-- 

Re: [PATCH] release quicklist before free_page

[Daniel Walker]

On Mon, 2007-07-23 at 18:32 +0200, Peter Zijlstra wrote:
> On Mon, 2007-07-23 at 08:21 -0700, Daniel Walker wrote:
> > Resolves,
> > 
> > BUG: sleeping function called from invalid context cc1(29651) at kernel/rtmutex.c:636
> > in_atomic():1 [00000001], irqs_disabled():0
> >  [<c0119f50>] __might_sleep+0xf3/0xf9
> >  [<c031600e>] __rt_spin_lock+0x21/0x3c
> >  [<c014102c>] get_zone_pcp+0x20/0x29
> >  [<c0141a40>] free_hot_cold_page+0xdc/0x167
> >  [<c013a3f4>] add_preempt_count+0x12/0xcc
> >  [<c0110d92>] pgd_dtor+0x0/0x1
> >  [<c015d865>] quicklist_trim+0xb7/0xe3
> >  [<c0111025>] check_pgt_cache+0x19/0x1c
> >  [<c0148df5>] free_pgtables+0x54/0x12c
> >  [<c013a3f4>] add_preempt_count+0x12/0xcc
> >  [<c014e5be>] unmap_region+0xeb/0x13b
> > 
> > 
> > It looks like the quicklist isn't used after a few variables are evaluated.
> > So no need to keep preemption disabled over the whole function.
> 
> Not quite, it uses preempt_disable() to avoid migration and stick to a
> cpu. Without that it might end up freeing pages from another quicklist.
> 
> How about this - compile tested only

How about a comment to go with it ?  Which says something like what's
above, notes on how the locking is getting used..

Daniel

Re: [PATCH] release quicklist before free_page

[Peter Zijlstra]

On Mon, 2007-07-23 at 08:21 -0700, Daniel Walker wrote:
> Resolves,
> 
> BUG: sleeping function called from invalid context cc1(29651) at kernel/rtmutex.c:636
> in_atomic():1 [00000001], irqs_disabled():0
>  [<c0119f50>] __might_sleep+0xf3/0xf9
>  [<c031600e>] __rt_spin_lock+0x21/0x3c
>  [<c014102c>] get_zone_pcp+0x20/0x29
>  [<c0141a40>] free_hot_cold_page+0xdc/0x167
>  [<c013a3f4>] add_preempt_count+0x12/0xcc
>  [<c0110d92>] pgd_dtor+0x0/0x1
>  [<c015d865>] quicklist_trim+0xb7/0xe3
>  [<c0111025>] check_pgt_cache+0x19/0x1c
>  [<c0148df5>] free_pgtables+0x54/0x12c
>  [<c013a3f4>] add_preempt_count+0x12/0xcc
>  [<c014e5be>] unmap_region+0xeb/0x13b
> 
> 
> It looks like the quicklist isn't used after a few variables are evaluated.
> So no need to keep preemption disabled over the whole function.

Not quite, it uses preempt_disable() to avoid migration and stick to a
cpu. Without that it might end up freeing pages from another quicklist.

How about this - compile tested only
---

We cannot call the page allocator with preemption-disabled, use the
per_cpu_locked construct to allow preemption while guarding the per cpu
data.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/quicklist.h |   19 +++++++++++++++----
 mm/quicklist.c            |    9 +++++----
 2 files changed, 20 insertions(+), 8 deletions(-)

Index: linux-2.6/include/linux/quicklist.h
===================================================================
--- linux-2.6.orig/include/linux/quicklist.h
+++ linux-2.6/include/linux/quicklist.h
@@ -18,7 +18,7 @@ struct quicklist {
 	int nr_pages;
 };
 
-DECLARE_PER_CPU(struct quicklist, quicklist)[CONFIG_NR_QUICK];
+DECLARE_PER_CPU_LOCKED(struct quicklist, quicklist)[CONFIG_NR_QUICK];
 
 /*
  * The two key functions quicklist_alloc and quicklist_free are inline so
@@ -30,19 +30,30 @@ DECLARE_PER_CPU(struct quicklist, quickl
  * The fast patch in quicklist_alloc touched only a per cpu cacheline and
  * the first cacheline of the page itself. There is minmal overhead involved.
  */
-static inline void *quicklist_alloc(int nr, gfp_t flags, void (*ctor)(void *))
+static inline void *__quicklist_alloc(int cpu, int nr, gfp_t flags, void (*ctor)(void *))
 {
 	struct quicklist *q;
 	void **p = NULL;
 
-	q =&get_cpu_var(quicklist)[nr];
+	q = &__get_cpu_var_locked(quicklist, cpu)[nr];
 	p = q->page;
 	if (likely(p)) {
 		q->page = p[0];
 		p[0] = NULL;
 		q->nr_pages--;
 	}
-	put_cpu_var(quicklist);
+	return p;
+}
+
+static inline void *quicklist_alloc(int nr, gfp_t flags, void (*ctor)(void *))
+{
+	struct quicklist *q;
+	void **p = NULL;
+	int cpu;
+
+	(void)get_cpu_var_locked(quicklist, &cpu)[nr];
+	p = __quicklist_alloc(cpu, nr, flags, ctor);
+	put_cpu_var_locked(quicklist, cpu);
 	if (likely(p))
 		return p;
 
Index: linux-2.6/mm/quicklist.c
===================================================================
--- linux-2.6.orig/mm/quicklist.c
+++ linux-2.6/mm/quicklist.c
@@ -19,7 +19,7 @@
 #include <linux/module.h>
 #include <linux/quicklist.h>
 
-DEFINE_PER_CPU(struct quicklist, quicklist)[CONFIG_NR_QUICK];
+DEFINE_PER_CPU_LOCKED(struct quicklist, quicklist)[CONFIG_NR_QUICK];
 
 #define FRACTION_OF_NODE_MEM	16
 
@@ -51,8 +51,9 @@ void quicklist_trim(int nr, void (*dtor)
 {
 	long pages_to_free;
 	struct quicklist *q;
+	int cpu;
 
-	q = &get_cpu_var(quicklist)[nr];
+	q = &get_cpu_var_locked(quicklist, &cpu)[nr];
 	if (q->nr_pages > min_pages) {
 		pages_to_free = min_pages_to_free(q, min_pages, max_free);
 
@@ -61,7 +62,7 @@ void quicklist_trim(int nr, void (*dtor)
 			 * We pass a gfp_t of 0 to quicklist_alloc here
 			 * because we will never call into the page allocator.
 			 */
-			void *p = quicklist_alloc(nr, 0, NULL);
+			void *p = __quicklist_alloc(cpu, nr, 0, NULL);
 
 			if (dtor)
 				dtor(p);
@@ -69,7 +70,7 @@ void quicklist_trim(int nr, void (*dtor)
 			pages_to_free--;
 		}
 	}
-	put_cpu_var(quicklist);
+	put_cpu_var_locked(quicklist, cpu);
 }
 
 unsigned long quicklist_total_size(void)

Re: [PATCH] release quicklist before free_page

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> > Not quite, it uses preempt_disable() to avoid migration and stick to 
> > a cpu. Without that it might end up freeing pages from another 
> > quicklist.

i.e. the patch hides a debug warning and there's possibly silent data 
corruption - not good.

> > How about this - compile tested only
> 
> How about a comment to go with it ?  Which says something like what's 
> above, notes on how the locking is getting used..

how about: "if you've got some time then please also add a few comments, 
because the code was quite non-obvious to me and I misunderstood it when 
I tried to fix it. Thanks."

ok?

	Ingo

Re: [PATCH] release quicklist before free_page

[Daniel Walker]

On Mon, 2007-07-23 at 20:23 +0200, Ingo Molnar wrote:
> 
> how about: "if you've got some time then please also add a few comments, 
> because the code was quite non-obvious to me and I misunderstood it when 
> I tried to fix it. Thanks."

Not exactly what I was thinking of .. More like,

"LOCKING NOTE: This code uses the fact that get_cpu_var disables
preemption to maintain cpu affinity during the call to free_page()
otherwise it might free pages on another cpu."

(btw, your comment above comes off very snide, which I don't appreciate.
I haven't done anything that remotely warrants that.)

Daniel

Re: [PATCH] release quicklist before free_page

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> (btw, your comment above comes off very snide, which I don't 
> appreciate. I haven't done anything that remotely warrants that.)

That snideness is simply a reflection, to make you realize how it feels. 
I tried to point this issue out to you in the past, but here we go 
again, back to square one. But i'm not giving up on you :-)
 
Peter sent a patch to a subtly buggy patch of yours (which introduces a 
bug worse than it fixes), with this comment:

 | | How about this - compile tested only

and you replied in this almost flagrant tone:

 | How about a comment to go with it ?  Which says something like what's 
 | above, notes on how the locking is getting used.. "

that kind of tone can be offensive, in such a context. I _know_ you 
(still) dont realize it as offensive and that you dont accept my 
characterisation of it, but nevertheless it's a fact and i'm going to 
complain about it to you when i see you do it.

A proper polite answer to the helpful patch of Peter would have been 
what i suggested:

> > how about: "if you've got some time then please also add a few 
> > comments, because the code was quite non-obvious to me and I 
> > misunderstood it when I tried to fix it. Thanks."

the basis of writing such replies is a certain level of humility and 
fundamental respect towards the capabilities of other kernel developers. 
Do you have it? If yes, are you willing to express it? If yes then 
please do so.

	Ingo

Re: [PATCH] release quicklist before free_page

[Daniel Walker]

On Mon, 2007-07-23 at 20:58 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > (btw, your comment above comes off very snide, which I don't 
> > appreciate. I haven't done anything that remotely warrants that.)
> 
> That snideness is simply a reflection, to make you realize how it feels. 
> I tried to point this issue out to you in the past, but here we go 
> again, back to square one. But i'm not giving up on you :-)

I could say the same thing ".. here we go again .." 
 
> Peter sent a patch to a subtly buggy patch of yours (which introduces a 
> bug worse than it fixes), with this comment:

I'm with you up to this point ..

>  | | How about this - compile tested only
> 
> and you replied in this almost flagrant tone:
> 
>  | How about a comment to go with it ?  Which says something like what's 
>  | above, notes on how the locking is getting used.. "

before I sent this I must have accidentally delete the "Ok." which
originally proceeded the above.

> that kind of tone can be offensive, in such a context. I _know_ you 
> (still) dont realize it as offensive and that you dont accept my 
> characterisation of it, but nevertheless it's a fact and i'm going to 
> complain about it to you when i see you do it.

I do disagree with the way your accepting this .. I wouldn't say "It's a
fact" either, since I don't accept similar comments from other people
the way your accepting it .. English generally is pretty loose. I could
say "That car is bad" which could literally mean the car is good or
nice.. There is a whole cornucopia of things that need to be considered.

> A proper polite answer to the helpful patch of Peter would have been 
> what i suggested:
> 
> > > how about: "if you've got some time then please also add a few 
> > > comments, because the code was quite non-obvious to me and I 
> > > misunderstood it when I tried to fix it. Thanks."
> 
> the basis of writing such replies is a certain level of humility and 
> fundamental respect towards the capabilities of other kernel developers. 
> Do you have it? If yes, are you willing to express it? If yes then 
> please do so.

I didn't argue with Peter , which means I agree with what he said .. He
was right, I was wrong and I accepted what he said in it's entirety ..
Certainly there is a level of respect there.. Had I argued with him it
would be different .

I'm a minimalist .. I don't say more than I need to, and it's not about
respect or superiority .. For example, I don't like saying "I think" or
"My opinion is" since that's implicit in my comments. If I'm writing
something clearly that's my opinion .. And a question mark alone
represents "Do you have it? If yes, are you willing to express it? If
yes then please do so"

Daniel