Date: Tue, 26 Apr 2016 11:15:07 -0400
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
Message-ID: <20160426151507.GK7822@mtj.duckdns.org>
References: <1461597771-25352-1-git-send-email-roman.penyaev@profitbricks.com>
 <20160425154847.GZ7822@mtj.duckdns.org>
 <571EC2B9.6000802@hurleysoftware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <571EC2B9.6000802@hurleysoftware.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1491
Lines: 44

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