[PATCH -rt] scheduling while atomic in fs/file.c

[PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

In free_fdtable_rcu() it does the following,

        } else {
                fddef = &get_cpu_var(fdtable_defer_list);
                spin_lock(&fddef->lock);
		...
                spin_unlock(&fddef->lock);
                put_cpu_var(fdtable_defer_list);
        }

I've never seen a scheduling while atomic, but seems like
it could happen . Not much contention on this lock though.

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

Index: linux-2.6.16/fs/file.c
===================================================================
--- linux-2.6.16.orig/fs/file.c
+++ linux-2.6.16/fs/file.c
@@ -137,7 +137,7 @@ static void free_fdtable_rcu(struct rcu_
 		kfree(fdt->fd);
 		kfree(fdt);
 	} else {
-		fddef = &get_cpu_var(fdtable_defer_list);
+		fddef = &__get_cpu_var(fdtable_defer_list);
 		spin_lock(&fddef->lock);
 		fdt->next = fddef->next;
 		fddef->next = fdt;
@@ -149,7 +149,6 @@ static void free_fdtable_rcu(struct rcu_
 		if (!schedule_work(&fddef->wq))
 			mod_timer(&fddef->timer, 5);
 		spin_unlock(&fddef->lock);
-		put_cpu_var(fdtable_defer_list);
 	}
 }
 

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Ingo Molnar]

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

> -		fddef = &get_cpu_var(fdtable_defer_list);
> +		fddef = &__get_cpu_var(fdtable_defer_list);

ok, the bug you found is real - but i dont like the fix: now we will be 
using smp_processor_id() in preemptible code, which will trip up under 
DEBUG_PREEMPT. Since in this particular case any CPU is fine, 
per_cpu(...,raw_smp_processor_id()) ought to do the trick. Mind to 
update the patch?

	Ingo

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

On Wed, 2006-05-03 at 22:45 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > -		fddef = &get_cpu_var(fdtable_defer_list);
> > +		fddef = &__get_cpu_var(fdtable_defer_list);
> 
> ok, the bug you found is real - but i dont like the fix: now we will be 
> using smp_processor_id() in preemptible code, which will trip up under 
> DEBUG_PREEMPT. Since in this particular case any CPU is fine, 
> per_cpu(...,raw_smp_processor_id()) ought to do the trick. Mind to 
> update the patch?

Sure I can do it, not exactly high priority considering it's never been
observed ..

Daniel 

[PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

Quite the smp_processor_id() wanrings. I don't see any SMP
concerns here . It just adds to a percpu list, so it shouldn't
matter if it switches after sampling fdtable_defer_list .

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

Index: linux-2.6.16/fs/file.c
===================================================================
--- linux-2.6.16.orig/fs/file.c
+++ linux-2.6.16/fs/file.c
@@ -138,6 +138,8 @@ static void free_fdtable_rcu(struct rcu_
 		kfree(fdt);
 	} else {
 		fddef = &get_cpu_var(fdtable_defer_list);
+		put_cpu_var(fdtable_defer_list);
+
 		spin_lock(&fddef->lock);
 		fdt->next = fddef->next;
 		fddef->next = fdt;
@@ -149,7 +151,6 @@ static void free_fdtable_rcu(struct rcu_
 		if (!schedule_work(&fddef->wq))
 			mod_timer(&fddef->timer, 5);
 		spin_unlock(&fddef->lock);
-		put_cpu_var(fdtable_defer_list);
 	}
 }
 

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Steven Rostedt]

On Sun, 14 May 2006, Daniel Walker wrote:

> Quite the smp_processor_id() wanrings. I don't see any SMP
> concerns here . It just adds to a percpu list, so it shouldn't
> matter if it switches after sampling fdtable_defer_list .

I'm not so sure that there isn't SMP concerns here. I have to catch a
train in a few minutes, otherwise I would look deeper into this. But this
might be a candidate to turn fdtable_defer_list into a per_cpu_locked.

-- Steve

>
> Signed-Off-By: Daniel Walker <dwalker@mvista.com>
>
> Index: linux-2.6.16/fs/file.c
> ===================================================================
> --- linux-2.6.16.orig/fs/file.c
> +++ linux-2.6.16/fs/file.c
> @@ -138,6 +138,8 @@ static void free_fdtable_rcu(struct rcu_
>  		kfree(fdt);
>  	} else {
>  		fddef = &get_cpu_var(fdtable_defer_list);
> +		put_cpu_var(fdtable_defer_list);
> +
>  		spin_lock(&fddef->lock);
>  		fdt->next = fddef->next;
>  		fddef->next = fdt;
> @@ -149,7 +151,6 @@ static void free_fdtable_rcu(struct rcu_
>  		if (!schedule_work(&fddef->wq))
>  			mod_timer(&fddef->timer, 5);
>  		spin_unlock(&fddef->lock);
> -		put_cpu_var(fdtable_defer_list);
>  	}
>  }
>
> -
> 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/
>

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

On Sun, 2006-05-14 at 12:44 -0400, Steven Rostedt wrote:
> On Sun, 14 May 2006, Daniel Walker wrote:
> 
> > Quite the smp_processor_id() wanrings. I don't see any SMP
> > concerns here . It just adds to a percpu list, so it shouldn't
> > matter if it switches after sampling fdtable_defer_list .
> 
> I'm not so sure that there isn't SMP concerns here. I have to catch a
> train in a few minutes, otherwise I would look deeper into this. But this
> might be a candidate to turn fdtable_defer_list into a per_cpu_locked.

I reviewed it again, and it looks like these percpu structures have a
spinlock to protect the list from being emptied by a work queue while
things are being added to the list . The lock appears to be used
properly .  The work queue frees struct fdtable pointers added to the
list , the only place these structures are added is in the block I've
modified .

I think making this a locked percpu would just be overkill ..

Daniel

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Steven Rostedt]

On Sun, 14 May 2006, Daniel Walker wrote:

> On Sun, 2006-05-14 at 12:44 -0400, Steven Rostedt wrote:
> > On Sun, 14 May 2006, Daniel Walker wrote:
> >
> > > Quite the smp_processor_id() wanrings. I don't see any SMP
> > > concerns here . It just adds to a percpu list, so it shouldn't
> > > matter if it switches after sampling fdtable_defer_list .
> >
> > I'm not so sure that there isn't SMP concerns here. I have to catch a
> > train in a few minutes, otherwise I would look deeper into this. But this
> > might be a candidate to turn fdtable_defer_list into a per_cpu_locked.
>
> I reviewed it again, and it looks like these percpu structures have a
> spinlock to protect the list from being emptied by a work queue while
> things are being added to the list . The lock appears to be used
> properly .  The work queue frees struct fdtable pointers added to the
> list , the only place these structures are added is in the block I've
> modified .
>
> I think making this a locked percpu would just be overkill ..
>

It seems that the timer is percpu. So it has a timer for each cpu.  If you
switch CPUs after the put, the modtimer might put the fddef->timer onto
another CPU, and thus have more than one going off on the same CPU.

-- Steve

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Steven Rostedt]

On Mon, 15 May 2006, Steven Rostedt wrote:

> On Sun, 14 May 2006, Daniel Walker wrote:
>
> > On Sun, 2006-05-14 at 12:44 -0400, Steven Rostedt wrote:
> > > On Sun, 14 May 2006, Daniel Walker wrote:
> > >
> > > > Quite the smp_processor_id() wanrings. I don't see any SMP
> > > > concerns here . It just adds to a percpu list, so it shouldn't
> > > > matter if it switches after sampling fdtable_defer_list .
> > >
> > > I'm not so sure that there isn't SMP concerns here. I have to catch a
> > > train in a few minutes, otherwise I would look deeper into this. But this
> > > might be a candidate to turn fdtable_defer_list into a per_cpu_locked.
> >
> > I reviewed it again, and it looks like these percpu structures have a
> > spinlock to protect the list from being emptied by a work queue while
> > things are being added to the list . The lock appears to be used
> > properly .  The work queue frees struct fdtable pointers added to the
> > list , the only place these structures are added is in the block I've
> > modified .
> >
> > I think making this a locked percpu would just be overkill ..
> >
>
> It seems that the timer is percpu. So it has a timer for each cpu.  If you
> switch CPUs after the put, the modtimer might put the fddef->timer onto
> another CPU, and thus have more than one going off on the same CPU.
>

Just to clarify, although fdtable_defer_list_init(int cpu) creates a timer
for each CPU but sets them to the same CPU.  The mod_timer in the changed
function is what is used to spread the timers out.

Although your patch wont actually break anything, since it is unlikely
that the timer would be moved, and if it was, it would probably be put
back again. The design is just not clean.  It's best to keep the timer
where it is.

So I'm NACKing the current patch.

-- Steve

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

On Mon, 2006-05-15 at 03:04 -0400, Steven Rostedt wrote:
> On Mon, 15 May 2006, Steven Rostedt wrote:
> 
> > On Sun, 14 May 2006, Daniel Walker wrote:
> >
> > > On Sun, 2006-05-14 at 12:44 -0400, Steven Rostedt wrote:
> > > > On Sun, 14 May 2006, Daniel Walker wrote:
> > > >
> > > > > Quite the smp_processor_id() wanrings. I don't see any SMP
> > > > > concerns here . It just adds to a percpu list, so it shouldn't
> > > > > matter if it switches after sampling fdtable_defer_list .
> > > >
> > > > I'm not so sure that there isn't SMP concerns here. I have to catch a
> > > > train in a few minutes, otherwise I would look deeper into this. But this
> > > > might be a candidate to turn fdtable_defer_list into a per_cpu_locked.
> > >
> > > I reviewed it again, and it looks like these percpu structures have a
> > > spinlock to protect the list from being emptied by a work queue while
> > > things are being added to the list . The lock appears to be used
> > > properly .  The work queue frees struct fdtable pointers added to the
> > > list , the only place these structures are added is in the block I've
> > > modified .
> > >
> > > I think making this a locked percpu would just be overkill ..
> > >
> >
> > It seems that the timer is percpu. So it has a timer for each cpu.  If you
> > switch CPUs after the put, the modtimer might put the fddef->timer onto
> > another CPU, and thus have more than one going off on the same CPU.
> >
> 
> Just to clarify, although fdtable_defer_list_init(int cpu) creates a timer
> for each CPU but sets them to the same CPU.  The mod_timer in the changed
> function is what is used to spread the timers out.

The timer is able able to migrate CPU's , also the work queue will
easily switch cpu's . That was true before .

> Although your patch wont actually break anything, since it is unlikely
> that the timer would be moved, and if it was, it would probably be put
> back again. The design is just not clean.  It's best to keep the timer
> where it is.

Why would it be a problem if the timer moved ? Or the work queue? both
are protected under a spinlock which is consistently held . 

Daniel

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Steven Rostedt]

On Mon, 15 May 2006, Daniel Walker wrote:

>
> Why would it be a problem if the timer moved ? Or the work queue? both
> are protected under a spinlock which is consistently held .
>

fine, but the patch is still wrong.  If it really doesn't matter
which one it's on, instead of moving the put, just get the cpu var with
per_cpu(fdtable_defer_list, raw_smp_processor_id()) and get rid of the put
altogether.

Ingo,

Didn't John have a patch that did something with get_percpu_var() that
made this simple?

-- Steve

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

On Mon, 2006-05-15 at 03:04 -0400, Steven Rostedt wrote:

> 
> Just to clarify, although fdtable_defer_list_init(int cpu) creates a timer
> for each CPU but sets them to the same CPU.  The mod_timer in the changed
> function is what is used to spread the timers out.

Your right , it could migrate with my change only .. But I don't think
that a problem .. It's probably better ..

Daniel

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Steven Rostedt]

On Mon, 15 May 2006, Daniel Walker wrote:

> On Mon, 2006-05-15 at 03:04 -0400, Steven Rostedt wrote:
>
> >
> > Just to clarify, although fdtable_defer_list_init(int cpu) creates a timer
> > for each CPU but sets them to the same CPU.  The mod_timer in the changed
> > function is what is used to spread the timers out.
>
> Your right , it could migrate with my change only .. But I don't think
> that a problem .. It's probably better ..

My only concern is that this makes things less deterministic, but I'm
probably being paranoid here.

-- Steve

[PATCH -rt] scheduling while atomic in fs/file.c

[Daniel Walker]

Quite the smp_processor_id() warnings. The timer was pinned
before, but the spinlock protection is such that the timer
can migrate with out issues. Allowing the timers to 
migrate (although not often) will allow them to
move off busy cpu's , and potentially follow the
work queue that they wake up. 

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

Index: linux-2.6.16/fs/file.c
===================================================================
--- linux-2.6.16.orig/fs/file.c
+++ linux-2.6.16/fs/file.c
@@ -137,7 +137,9 @@ static void free_fdtable_rcu(struct rcu_
 		kfree(fdt->fd);
 		kfree(fdt);
 	} else {
-		fddef = &get_cpu_var(fdtable_defer_list);
+
+		fddef = &per_cpu(fdtable_defer_list, raw_smp_processor_id());
+
 		spin_lock(&fddef->lock);
 		fdt->next = fddef->next;
 		fddef->next = fdt;
@@ -149,7 +151,6 @@ static void free_fdtable_rcu(struct rcu_
 		if (!schedule_work(&fddef->wq))
 			mod_timer(&fddef->timer, 5);
 		spin_unlock(&fddef->lock);
-		put_cpu_var(fdtable_defer_list);
 	}
 }
 

Re: [PATCH -rt] scheduling while atomic in fs/file.c

[Ingo Molnar]

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

> Quite the smp_processor_id() warnings. The timer was pinned before, 
> but the spinlock protection is such that the timer can migrate with 
> out issues. Allowing the timers to migrate (although not often) will 
> allow them to move off busy cpu's , and potentially follow the work 
> queue that they wake up.

thanks, applied.

	Ingo