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

[Steven Rostedt <rostedt@goodmis.org>]

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

-
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/