From: Tejun Heo <tj@kernel.org>
To: Lai Jiangshan <laijs@cn.fujitsu.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/10] workqueue: restructure flush_workqueue() and start all flusher at the same time
Date: Wed, 26 Sep 2012 09:49:58 -0700 [thread overview]
Message-ID: <20120926164958.GR16296@google.com> (raw)
In-Reply-To: <5062893B.70908@cn.fujitsu.com>
Hello, Lai.
On Wed, Sep 26, 2012 at 12:48:59PM +0800, Lai Jiangshan wrote:
> > Hmmm... so, that's a lot simpler. flush_workqueue() isn't a super-hot
> > code path and I don't think grabbing mutex twice is too big a deal. I
> > haven't actually reviewed the code but if it can be much simpler and
> > thus easier to understand and verify, I might go for that.
>
> I updated it. it is attached, it forces flush_workqueue() to grab
> mutex twice(no other forcing). overflow queue is implemented in a
> different way. This new algorithm may become our choice likely,
> please review this one.
Will do shortly.
> I did not know this history, thank you.
>
> But the number of colors is not essential.
> "Does the algorithm chain flushers" is essential.
>
> If we can have multiple flushers for each color. It is not chained.
> If we have only one flusher for one color. It is chained. Even we
> have multiple color, it is still partially chained(image we have
> very high frequent flush_workqueue()).
If you have very few colors, you can end up merging flushes of a lot
of work items which in turn delays the next flush and thus merging
more of them. This was what Linus was worried about.
> The initial implementation of flush_workqueue() is "chained" algorithm.
I don't know what you mean by "chained" here. The current mainline
implementation has enough colors for most use cases and don't assign a
color to single work item. It's specifically designed to avoid
chained latencies.
> The initial implementation of SRCU is also "chained" algorithm.
> but the current SRCU which was implemented by me is not "chained"
> (I don't propose to use SRCU for flush_workqueue(), I just discuss it)
So, you lost me. The current implementation doesn't have a problem on
that front.
> The simple version of flush_workqueue() which I sent yesterday is "chained",
> because it forces overflow flushers wait for free color and forces only one
> flusher for one color.
>
> Since "not chaining" is important/essential. I sent a new draft implement today.
> it uses multiple queues, one for each color(like SRCU).
> this version is also simple, it remove 90 LOC.
I'll review your patch but the current implementation is enough on
that regard. I was trying to advise against going for two-color
scheme.
Thanks.
--
tejun
next prev parent reply other threads:[~2012-09-26 16:50 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-24 10:07 [PATCH 00/10] workqueue: restructure flush_workqueue() and start all flusher at the same time Lai Jiangshan
2012-09-24 10:07 ` [PATCH 01/10] workqueue: always pass flush responsibility to next Lai Jiangshan
2012-09-24 10:07 ` [PATCH 02/10] workqueue: remove unneeded check Lai Jiangshan
2012-09-24 10:07 ` [PATCH 03/10] workqueue: remove while(true) Lai Jiangshan
2012-09-24 10:07 ` [PATCH 04/10] workqueue: use nr_cwqs_to_flush array Lai Jiangshan
2012-09-24 10:07 ` [PATCH 05/10] workqueue: add wq_dec_flusher_ref() Lai Jiangshan
2012-09-24 10:07 ` [PATCH 06/10] workqueue: start all flusher at the same time Lai Jiangshan
2012-09-24 10:07 ` [PATCH 07/10] workqueue: simplify flush_workqueue_prep_cwqs() Lai Jiangshan
2012-09-24 10:07 ` [PATCH 08/10] workqueue: assign overflowed flushers's flush color when queued Lai Jiangshan
2012-09-24 10:07 ` [PATCH 09/10] workqueue: remove flusher_overflow Lai Jiangshan
2012-09-24 10:07 ` [PATCH 10/10] workqueue: remove wq->flush_color Lai Jiangshan
2012-09-24 20:39 ` [PATCH 00/10] workqueue: restructure flush_workqueue() and start all flusher at the same time Tejun Heo
2012-09-25 9:02 ` Lai Jiangshan
2012-09-25 20:14 ` Tejun Heo
2012-09-25 9:02 ` Lai Jiangshan
2012-09-25 20:24 ` Tejun Heo
2012-09-26 4:48 ` Lai Jiangshan
2012-09-26 16:49 ` Tejun Heo [this message]
2012-09-26 17:04 ` Tejun Heo
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120926164958.GR16296@google.com \
--to=tj@kernel.org \
--cc=laijs@cn.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox