Re: [GIT PULL] Block changes for 4.21-rc

[Jens Axboe <axboe@kernel.dk>]

On 12/28/18 2:48 PM, Linus Torvalds wrote:
> On Thu, Dec 20, 2018 at 8:05 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Jens Axboe (108):
>>       block: avoid ordered task state change for polled IO
> 
> This one seems *very* questionable.
> 
> The commit message is misleading:
> 
>     For the core poll helper, the task state setting don't need to imply any
>     atomics, as it's the current task itself that is being modified and
>     we're not going to sleep.
> 
>     For IRQ driven, the wakeup path have the necessary barriers to not need
>     us using the heavy handed version of the task state setting.
> 
> What? Barriers on one side have *no* meaning or point if the *other*
> side doesn't have any barriers.  That's completely magical thinking.
> 
> But it's also not at all the case that such barriers even exist. The
> wakup side does:
> 
>                         struct task_struct *waiter = dio->submit.waiter;
>                         WRITE_ONCE(dio->submit.waiter, NULL);
>                         blk_wake_io_task(waiter);
> 
> and here the sleeping side now does:
> 
>                         __set_current_state(TASK_UNINTERRUPTIBLE);
> 
>                         if (!READ_ONCE(dio->submit.waiter))
>                                 break;
> 
> which is entirely unordered. So just what protects against this:
> 
>   Sleeper:              Waker (different cpu)
> 
>   read submit.waker
>   (sees non-NULL)
> 
>                         WRITE_ONCE(dio->submit.waiter, NULL);
>                         blk_wake_io_task(waiter);
> 
>   write TASK_UNINTERRUPTIBLE
>   io_schedule()
> 
> and now the sleeper sleeps forever, because there will be nobody who
> ever wakes it up (the wakeup happened before the write of
> TASK_UNINTERRUPTIBLE).
> 
> Notice how it does not matter AT ALL what barriers that other CPU is
> doing when waking things up. The problem is the sleeping CPU having
> reordered the check for "oh, there's a waiter", and "tell people I'm
> going to sleep".
> 
> Those memory barriers matter. You can't just remove them randomly and
> claim they don't.
> 
> Why would you even remove it in the first place?
> 
> Guys, if you don't understand memory ordering, then you have
> absolutely no business using __set_current_state(). And talking about
> barriers on the other side of the equation clearly shows that you
> don't seem to understand memory ordering.
> 
> Barriers are only *ever* meaningful if they exist on both sides
> (although sometimes said barriers can be implicit: an address
> dependency or similar in RCU, now that we've decided to not care abotu
> the crazy alpha read-barrier-depends)
> 
> Maybe I'm  missing something, but this really looks like a completely
> invalid "optimization" to me.  And it's entirely bogus too. If that
> memory barrier matters, you're almost certainly doing something wrong
> (most likely benchmarking something pointless).

I think what went wrong here is that the patch morphed through
multiple iterations, and I agree the IRQ side does not look correct.
The polled part is fine.

Sorry for the late reply, in and out this time of year. I'll get
something posted and queued up for the later pull I was planning
for this merge window, for the other stragglers.


-- 
Jens Axboe