On Wed, Aug 16, 2017 at 02:05:06PM +0900, Byungchul Park wrote:
> On Wed, Aug 16, 2017 at 12:05:31PM +0800, Boqun Feng wrote:
> > > I see...
> > >
> > > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > > Task B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > > Task C : acquired of lock#3 -> wait for completion of barr->done
> >
> > >From the stack trace below, this barr->done is for flush_work() in
> > lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> >
> > > Worker D : wait for wfc.work to be released -> will complete barr->done
> >
> > and this barr->done is for work "wfc.work".
> >
> > So those two barr->done could not be the same instance, IIUC. Therefore
> > the deadlock case is not possible.
> >
> > The problem here is all barr->done instances are initialized at
> > insert_wq_barrier() and they belongs to the same lock class, to fix
>
> I'm not sure this caused the lockdep warning but, if they belongs to the
> same class even though they couldn't be the same instance as you said, I
> also think that is another problem and should be fixed.
>
My point was more like this is a false positive case, which we should
avoid as hard as we can, because this very case doesn't look like a
deadlock to me.
Maybe the pattern above does exist in current kernel, but we need to
guide/adjust lockdep to find the real case showing it's happening.
Regards,
Boqun
> > this, we need to differ barr->done with different lock classes based on
> > the corresponding works.
> >
> > How about the this(only compilation test):
> >
> > ----------------->8
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index e86733a8b344..d14067942088 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -2431,6 +2431,27 @@ struct wq_barrier {
> > struct task_struct *task; /* purely informational */
> > };
> >
> > +#ifdef CONFIG_LOCKDEP_COMPLETE
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target) \
> > +do { \
> > + INIT_WORK_ONSTACK(&(barr)->work, func); \
> > + __set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work)); \
> > + lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map, \
> > + "(complete)" #barr, \
> > + (target)->lockdep_map.key, 1); \
> > + __init_completion(&barr->done); \
> > + barr->task = current; \
> > +} while (0)
> > +#else
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target) \
> > +do { \
> > + INIT_WORK_ONSTACK(&(barr)->work, func); \
> > + __set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work)); \
> > + init_completion(&barr->done); \
> > + barr->task = current; \
> > +} while (0)
> > +#endif
> > +
> > static void wq_barrier_func(struct work_struct *work)
> > {
> > struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> > @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
> > * checks and call back into the fixup functions where we
> > * might deadlock.
> > */
> > - INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> > - __set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> > - init_completion(&barr->done);
> > - barr->task = current;
> > + INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
> >
> > /*
> > * If @target is currently being executed, schedule the
Attachment:
signature.asc
Description: PGP signature
