archive mirror
 help / color / mirror / Atom feed
From: Johannes Berg <>
To: Bart Van Assche <>, Tejun Heo <>
Cc: "" <>,
	Christoph Hellwig <>, Sagi Grimberg <>,
	"" <>
Subject: Re: [PATCH 3/3] kernel/workqueue: Suppress a false positive lockdep complaint
Date: Thu, 25 Oct 2018 21:59:38 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (sfid-20181025_175553_256485_FAC088A9)

On Thu, 2018-10-25 at 08:55 -0700, Bart Van Assche wrote:

> Please have a look at the call trace in the description of this patch and also
> at the direct I/O code. The lockdep complaint in the description of this patch
> really is a false positive. 

I agree. I just don't agree that it's a false positive because of the
lockdep tracking of workqueues (which you're essentially saying since
you want to change it), I think it's a false positive because the user
isn't telling lockdep properly what it's doing.

> What I think needs further discussion is on how to
> address this false positive - track whether or not a work queue has been used
> or follow Tejun's proposal that I became aware of after I posted this patch,
> namely introduce a new function for destroying a work queue that skips draining,
> e.g. destroy_workqueue_skip_drain() (

Agree. Ted's suggestion is basically what I meant in my other email when
I said:

> So, thinking about this more, can you guarantee (somehow) that the
> workqueue is empty at this point?

(I hadn't looked at the code then - obviously that's guaranteed)

> Perhaps then we should encode that
> guarantee into the API, and actually make it WARN_ON() if something is
> scheduled on it at that point? And then skip lockdep in this case.

So that API I'm talking about is what Ted suggests with
destroy_never_used_workqueue() or Tejun's suggestion of
destroy_workqueue_skip_drain(). Either is fine with me, though I'd
probably think it would make sense to actually check that it really was
never used, or at least is actually empty right now and thus doesn't
need the train.

However, I think more generally useful would be the
destroy_workqueue_nested() API I've proposed in my other reply just
moments ago, since that could be used in other code with non-empty
workqueues, as long as you can ensure that there's a guaranteed layering
of them. Then you could even have two objects A and B of the same class,
but separate instances, and flush B's workqueue from a work executing on
A's workqueue, which may come in handy in other places to address false
positives similar to this one.


  reply	other threads:[~2018-10-25 20:00 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25 15:05 [PATCH 0/3] Suppress false positives triggered by workqueue lockdep annotations Bart Van Assche
2018-10-25 15:05 ` [PATCH 1/3] kernel/workqueue: Remove lockdep annotation from __flush_work() Bart Van Assche
2018-10-25 15:31   ` Johannes Berg
2018-10-25 15:57     ` Johannes Berg
2018-10-25 16:01     ` Bart Van Assche
2018-10-25 15:05 ` [PATCH 2/3] kernel/workqueue: Surround work execution with shared lock annotations Bart Van Assche
2018-10-25 16:53   ` Johannes Berg
2018-10-25 17:22     ` Bart Van Assche
2018-10-25 19:17       ` Johannes Berg
2018-10-25 15:05 ` [PATCH 3/3] kernel/workqueue: Suppress a false positive lockdep complaint Bart Van Assche
2018-10-25 15:34   ` Johannes Berg
2018-10-25 15:55     ` Bart Van Assche
2018-10-25 19:59       ` Johannes Berg [this message]
2018-10-25 20:21         ` Theodore Y. Ts'o
2018-10-25 20:26           ` Johannes Berg
2018-10-25 15:36   ` Tejun Heo
2018-10-25 15:37     ` Tejun Heo
2018-10-25 20:13     ` Johannes Berg
2018-10-25 15:40   ` Theodore Y. Ts'o
2018-10-25 17:02   ` Johannes Berg
2018-10-25 17:11     ` Bart Van Assche
2018-10-25 19:51       ` Johannes Berg
2018-10-25 20:39         ` Bart Van Assche
2018-10-25 20:47           ` Johannes Berg
2018-10-25 15:27 ` [PATCH 0/3] Suppress false positives triggered by workqueue lockdep annotations Johannes Berg
2018-10-25 15:47   ` Bart Van Assche

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 3/3] kernel/workqueue: Suppress a false positive lockdep complaint' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).