Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752296AbcJJWjK (ORCPT ); Mon, 10 Oct 2016 18:39:10 -0400 Received: from mail-lf0-f47.google.com ([209.85.215.47]:34240 "EHLO mail-lf0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752138AbcJJWjJ (ORCPT ); Mon, 10 Oct 2016 18:39:09 -0400 MIME-Version: 1.0 In-Reply-To: <1476133442-17757-1-git-send-email-dianders@chromium.org> References: <1476133442-17757-1-git-send-email-dianders@chromium.org> From: Doug Anderson Date: Mon, 10 Oct 2016 15:39:06 -0700 X-Google-Sender-Auth: TtMyYPhCITKMwkVRg_-PhXv0Tik Message-ID: Subject: Re: [PATCH v2] timers: Fix usleep_range() in the context of wake_up_process() To: Thomas Gleixner , John Stultz Cc: Andreas Mohr , Brian Norris , Tao Huang , Tony Xie , "open list:ARM/Rockchip SoC..." , Douglas Anderson , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2884 Lines: 82 Hi, On Mon, Oct 10, 2016 at 2:04 PM, Douglas Anderson wrote: > Users of usleep_range() expect that it will _never_ return in less time > than the minimum passed parameter. However, nothing in any of the code > ensures this. Specifically: > > usleep_range() => do_usleep_range() => schedule_hrtimeout_range() => > schedule_hrtimeout_range_clock() just ends up calling schedule() with an > appropriate timeout set using the hrtimer. If someone else happens to > wake up our task then we'll happily return from usleep_range() early. > > msleep() already has code to handle this case since it will loop as long > as there was still time left. usleep_range() had no such loop. > > The problem is is easily demonstrated with a small bit of test code: > > static int usleep_test_task(void *data) > { > atomic_t *done = data; > ktime_t start, end; > > start = ktime_get(); > usleep_range(50000, 100000); > end = ktime_get(); > pr_info("Requested 50000 - 100000 us. Actually slept for %llu us\n", > (unsigned long long)ktime_to_us(ktime_sub(end, start))); > atomic_set(done, 1); > > return 0; > } > > static void run_usleep_test(void) > { > struct task_struct *t; > atomic_t done; > > atomic_set(&done, 0); > > t = kthread_run(usleep_test_task, &done, "usleep_test_task"); > while (!atomic_read(&done)) { > wake_up_process(t); > udelay(1000); > } > kthread_stop(t); > } > > If you run the above code without this patch you get things like: > Requested 50000 - 100000 us. Actually slept for 967 us > > If you run the above code _with_ this patch, you get: > Requested 50000 - 100000 us. Actually slept for 50001 us > > Presumably this problem was not detected before because: > - It's not terribly common to use wake_up_process() directly. > - Other ways for processes to wake up are not typically mixed with > usleep_range(). > - There aren't lots of places that use usleep_range(), since many people > call either msleep() or udelay(). > > Reported-by: Tao Huang > Signed-off-by: Douglas Anderson > --- > Changes in v2: > - Fixed stupid bug that snuck in before posting > - Use ktime_before > - Remove delta from the loop > > NOTE: Tested against 4.4 tree w/ backports. I'm trying to get myself > up and running with mainline again to test there now but it might be a > little while. Hopefully this time I don't shoot myself in the foot. OK, I finally resolved all my issues and have officially tested this on mainline (not just in my private kernel) linux 4.8 on an rk3288-veyron-jerry based device. Problem still exists there and fix still works. Sorry for the noise earlier. Hopefully this version of the patch addresses all of Andreas's review feedback (and thank you very much for the review!) -Doug