On 01/29, Christoph Lameter wrote:
>
> On Mon, 29 Jan 2007, Oleg Nesterov wrote:
>
> > > > This is wrong. Suppose we have a CPU_UP,CPU_DOWN,CPU_UP sequence. The
> > > > last
> > > > CPU_UP will not restart a per-cpu "cache_reap timer".
> > >
> > > Why?
> >
> > Because the last CPU_UP calls start_cpu_timer(), but since ->work.func !=
> > NULL
> > we don't do schedule_delayed_work_on(). I think (if I am right) this is a
> > slab's
> > bug.
>
> The CPU_DOWN would need to set work.func == NULL for this to work. But
> then the slab does not shut down the work queues for the processor. Isnt
> this another issue with workqueues?
I think no, please see below. Actually, this is more related to timers for
this particular case.
> The slab would need a notification
> that the workqueue for a processor was shutdown in order to set work.func
> = NULL.
The slab has a notification: CPU_XXX events. It should cancel a pending per
cpu "reap_work timer".
> > I think cache_reap() is not alone, and this is not its fault.
> >
> > But please note another minor problem,
> >
> > void cache_reap(struct work_struct *unused)
> > {
> > ...
> >
> > schedule_delayed_work(&__get_cpu_var(reap_work), ...);
> > }
> >
> > Even if smp_processor_id() was stable during the execution of cache_reap(),
> > this work_struct can be moved to another CPU if CPU_DEAD happens. We can't
> > avoid this, and this is correct.
>
> Uhh.... This may not be correct in terms of how the slab operates.
But this is practically impossible to avoid. We can't delay CPU_DOWN until all
workqueues flush their cwq->worklist. This is livelockable, the work can
re-queue
itself, and new works can be added since the dying CPU is still on
cpu_online_map.
This means that some pending works will be processed on another CPU.
delayed_work is even worse, the timer can migrate as well.
The first problem (smp_processor_id() is not stable) could be solved if we
use freezer or with the help of not-yet-implemented scalable lock_cpu_hotplug.
> > This means that __get_cpu_var(reap_work) returns a "wrong" struct
> > delayed_work.
> > This is absolutely harmless right now, but may be it's better to use
> > container_of(unused, struct delayed_work, work).
>
> Well seems that we have a set of unresolved issues with workqueues and cpu
> hotplug.
Yes.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
