From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753865AbbBAXJl (ORCPT ); Sun, 1 Feb 2015 18:09:41 -0500 Received: from mail-ig0-f171.google.com ([209.85.213.171]:48816 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753124AbbBAXJi (ORCPT ); Sun, 1 Feb 2015 18:09:38 -0500 MIME-Version: 1.0 In-Reply-To: <20150201221458.GN2974@kvack.org> References: <20150201144058.GM2974@kvack.org> <20150201221458.GN2974@kvack.org> Date: Sun, 1 Feb 2015 15:09:37 -0800 X-Google-Sender-Auth: v1e9Gy4W6a2FT97oOjbxNk3K7pc Message-ID: Subject: Re: [GIT PULL] aio: fix sleeping while TASK_INTERRUPTIBLE From: Linus Torvalds To: Benjamin LaHaise Cc: linux-aio@kvack.org, Linux Kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 1, 2015 at 2:14 PM, Benjamin LaHaise wrote: > > It's ugly, but it actually is revealing a bug. If so, that patch is *still* not the right thing to do. Leave the warning. It's better than horrible crap code that doesn't warn, but also doesn't make any *sense*. Because code like this is just crap: + int running = current->state == TASK_RUNNING; really. It's just crazy. It makes no sense. It's all just avoiding the warning, it's not making the code any better. In fact, it's actively making it worse, because now the code is literally designed to give random different behavior depending on timing. For all I know, there might be multiple concurrent waiters etc that got woken up on the same wait-queue, afaik, so the "am I runnable" question is fundamentally nonsensical, since it isn't necessarily the same as testing the state of the ring that we're waiting for anyway. And if it *is* the same thing (because maybe the locking does end up guaranteeing that "running" here ends up always matching some particular state of the aio ring), then it should still be rewritten in terms of that ring state, not some random "am I runnable" question. So *clearly* the code has explicitly been written for the debug test, not to make sense. Seriously. It's the only possible reason for why it would test "current->state". If the code relies on some 1:1 relationship with the wakeups (*despite* having comments currently explicitly saying that it doesn't), then use the new "wait_woken()" interface instead, which actually does exactly that. That one doesn't test some random state, it tests "was I explicitly woken". Then the whole "oh, but the mutex changed the task state" doesn't even *matter*. Which may in the end be the same thing in practice, but at least the code makes *sense*. In ways that that "current->state == TASK_RUNNING" test does not. Linus