Date: Wed, 30 Aug 2017 11:09:53 +0900
From: Byungchul Park <byungchul.park@lge.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: mingo@kernel.org, tj@kernel.org, boqun.feng@gmail.com,
        david@fromorbit.com, johannes@sipsolutions.net, oleg@redhat.com,
        linux-kernel@vger.kernel.org, kernel-team@lge.com
Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation
Message-ID: <20170830020953.GE3240@X58A-UD3R>
References: <20170823115843.662056844@infradead.org>
 <20170823121432.990701317@infradead.org>
 <20170824021840.GC6772@X58A-UD3R>
 <20170824140240.t4imrpvussebfimm@hirez.programming.kicks-ass.net>
 <20170825011114.GA3858@X58A-UD3R>
 <20170829085939.ggmb6xiohw67micb@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170829085939.ggmb6xiohw67micb@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2052
Lines: 45

On Tue, Aug 29, 2017 at 10:59:39AM +0200, Peter Zijlstra wrote:
> Subject: lockdep: Untangle xhlock history save/restore from task independence
> 
> Where XHLOCK_{SOFT,HARD} are save/restore points in the xhlocks[] to
> ensure the temporal IRQ events don't interact with task state, the
> XHLOCK_PROC is a fundament different beast that just happens to share
> the interface.
> 
> The purpose of XHLOCK_PROC is to annotate independent execution inside
> one task. For example workqueues, each work should appear to run in its
> own 'pristine' 'task'.
> 
> Remove XHLOCK_PROC in favour of its own interface to avoid confusion.

Much better to me than the patch you did previously, but, see blow.

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c0331891dec1..ab3c0dc8c7ed 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2107,14 +2107,14 @@ __acquires(&pool->lock)
>  	 * Which would create W1->C->W1 dependencies, even though there is no
>  	 * actual deadlock possible. There are two solutions, using a
>  	 * read-recursive acquire on the work(queue) 'locks', but this will then
> -	 * hit the lockdep limitation on recursive locks, or simly discard
> +	 * hit the lockdep limitation on recursive locks, or simply discard
>  	 * these locks.
>  	 *
>  	 * AFAICT there is no possible deadlock scenario between the
>  	 * flush_work() and complete() primitives (except for single-threaded
>  	 * workqueues), so hiding them isn't a problem.
>  	 */
> -	crossrelease_hist_start(XHLOCK_PROC, true);
> +	lockdep_invariant_state(true);

This is what I am always curious about. It would be ok if you agree with
removing this work-around after fixing acquire things in wq. But, you
keep to say this is essencial.

You should focus on what dependencies actually are, than saparating
contexts unnecessarily. Of course, we have to do it for each work, _BUT_
not between outside of work and each work since there might be
dependencies between them certainly.