From: Tejun Heo <tj@kernel.org>
To: Peter Hurley <peter@hurleysoftware.com>
Cc: Roman Pen <roman.penyaev@profitbricks.com>,
Jens Axboe <axboe@kernel.dk>,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
David Howells <dhowells@redhat.com>
Subject: Re: [PATCH 1/1] [RFC] workqueue: fix ghost PENDING flag while doing MQ IO
Date: Tue, 26 Apr 2016 11:15:07 -0400 [thread overview]
Message-ID: <20160426151507.GK7822@mtj.duckdns.org> (raw)
In-Reply-To: <571EC2B9.6000802@hurleysoftware.com>
Hello, Peter.
On Mon, Apr 25, 2016 at 06:22:01PM -0700, Peter Hurley wrote:
> This is the same bug I wrote about 2 yrs ago (but with the wrong fix).
>
> http://lkml.iu.edu/hypermail/linux/kernel/1402.2/04697.html
>
> Unfortunately I didn't have a reproducer at all :/
Ah, bummer.
> The atomic_long_xchg() patch has several benefits over the naked barrier:
>
> 1. set_work_pool_and_clear_pending() has the same requirements as
> clear_work_data(); note that both require write barrier before and
> full barrier after.
clear_work_data() is only used by __cancel_work_timer() and there's no
following execution or anything where rescheduling memory loads can
cause any issue.
> 2. xchg() et al imply full barrier before and full barrier after.
>
> 3. The naked barriers could be removed, while improving efficiency.
> On x86, mov + mfence => xchg
It's unlikely to make any measureable difference. Is xchg() actually
cheaper than store + rmb?
> 4. Maybe fixes other hidden bugs.
> For example, I'm wondering if reordering with set_work_pwq/list_add_tail
> would be a problem; ie., what if work is visible on the worklist _before_
> data is initialized by set_work_pwq()?
Worklist is always accessed under the pool lock. The barrier comes
into play only because we're using bare PENDING bit for
synchronization. I'm not necessarily against making all clearings of
PENDING to be followed by a rmb or use xhcg. Reasons 2-4 are pretty
weak tho.
Thanks.
--
tejun
next prev parent reply other threads:[~2016-04-26 15:15 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-25 15:22 [PATCH 1/1] [RFC] workqueue: fix ghost PENDING flag while doing MQ IO Roman Pen
2016-04-25 15:48 ` Tejun Heo
2016-04-25 16:00 ` Tejun Heo
2016-04-25 16:40 ` Roman Penyaev
2016-04-25 16:34 ` Roman Penyaev
2016-04-25 17:03 ` Tejun Heo
2016-04-25 17:39 ` Roman Penyaev
2016-04-25 17:51 ` Tejun Heo
2016-04-26 1:22 ` Peter Hurley
2016-04-26 15:15 ` Tejun Heo [this message]
2016-04-26 17:27 ` Peter Hurley
2016-04-26 17:45 ` Tejun Heo
2016-04-26 20:07 ` Peter Hurley
2016-04-27 5:50 ` Hannes Reinecke
2016-04-27 19:05 ` 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=20160426151507.GK7822@mtj.duckdns.org \
--to=tj@kernel.org \
--cc=axboe@kernel.dk \
--cc=dhowells@redhat.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peter@hurleysoftware.com \
--cc=roman.penyaev@profitbricks.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.