Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751569AbdHPHPq (ORCPT ); Wed, 16 Aug 2017 03:15:46 -0400 Received: from LGEAMRELO11.lge.com ([156.147.23.51]:58049 "EHLO lgeamrelo11.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751212AbdHPHPp (ORCPT ); Wed, 16 Aug 2017 03:15:45 -0400 X-Original-SENDERIP: 156.147.1.125 X-Original-MAILFROM: byungchul.park@lge.com X-Original-SENDERIP: 10.177.222.33 X-Original-MAILFROM: byungchul.park@lge.com Date: Wed, 16 Aug 2017 16:14:21 +0900 From: Byungchul Park To: Boqun Feng Cc: Ingo Molnar , Thomas Gleixner , peterz@infradead.org, walken@google.com, kirill@shutemov.name, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, willy@infradead.org, npiggin@gmail.com, kernel-team@lge.com Subject: Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Message-ID: <20170816071421.GT20323@X58A-UD3R> References: <1502089981-21272-1-git-send-email-byungchul.park@lge.com> <20170815082020.fvfahxwx2zt4ps4i@gmail.com> <20170816001637.GN20323@X58A-UD3R> <20170816035842.p33z5st3rr2gwssh@tardis> <20170816050506.GR20323@X58A-UD3R> <20170816055808.GB11771@tardis> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170816055808.GB11771@tardis> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3326 Lines: 93 On Wed, Aug 16, 2017 at 01:58:08PM +0800, Boqun Feng wrote: > > 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. As long as they are initialized as a same class, there's no way to distinguish between them within lockdep. And I also think we should avoid false positive cases. Do you think there are many places where completions are initialized in a same place even though they could never be the same instance? If no, it would be better to fix it whenever we face it, as you did. If yes, we have to change it for completion, for example: 1. Do not apply crossrelease into completions initialized on stack. or 2. Use the full call path instead of a call site as a lockdep_map key. or 3. So on. Could you let me know your opinion about it? Thanks, Byungchul > 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