From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932884AbcHDIpt (ORCPT ); Thu, 4 Aug 2016 04:45:49 -0400 Received: from www381.your-server.de ([78.46.137.84]:48611 "EHLO www381.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932351AbcHDIpq (ORCPT ); Thu, 4 Aug 2016 04:45:46 -0400 Subject: Re: [PATCH] iio: fix sched WARNING "do not call blocking ops when !TASK_RUNNING" To: Brian Norris References: <20160802011244.GA54171@google.com> <37ea974c-9ac6-5a40-0f0e-ee34ef605a08@metafoo.de> <20160802165732.GA3310@localhost> <9a6a7f54-0343-db74-57a2-9f747ae659cc@metafoo.de> <20160804082621.GA11331@localhost> Cc: Jonathan Cameron , Hartmut Knaack , Peter Meerwald-Stadler , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Guenter Roeck , Brian Norris , Peter Zijlstra , Ingo Molnar From: Lars-Peter Clausen X-Enigmail-Draft-Status: N1110 Message-ID: <489632a0-9aaf-e63b-4b08-0fa724a06e28@metafoo.de> Date: Thu, 4 Aug 2016 10:45:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160804082621.GA11331@localhost> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Authenticated-Sender: lars@metafoo.de Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/04/2016 10:26 AM, Brian Norris wrote: > When using CONFIG_DEBUG_ATOMIC_SLEEP, the scheduler nicely points out > that we're calling sleeping primitives within the wait_event loop, which > means we might clobber the task state: > > [ 10.831289] do not call blocking ops when !TASK_RUNNING; state=1 set at [] > [ 10.845531] ------------[ cut here ]------------ > [ 10.850161] WARNING: at kernel/sched/core.c:7630 > ... > [ 12.164333] ---[ end trace 45409966a9a76438 ]--- > [ 12.168942] Call trace: > [ 12.171391] [] __might_sleep+0x64/0x90 > [ 12.176699] [] mutex_lock_nested+0x50/0x3fc > [ 12.182440] [] iio_kfifo_buf_data_available+0x28/0x4c > [ 12.189043] [] iio_buffer_ready+0x60/0xe0 > [ 12.194608] [] iio_buffer_read_first_n_outer+0x108/0x1a8 > [ 12.201474] [] __vfs_read+0x58/0x114 > [ 12.206606] [] vfs_read+0x94/0x118 > [ 12.211564] [] SyS_read+0x64/0xb4 > [ 12.216436] [] el0_svc_naked+0x24/0x28 > > To avoid this, we should (a la https://lwn.net/Articles/628628/) use the > wait_woken() function, which avoids the nested sleeping while still > handling races between waiting / wake-events. > > Signed-off-by: Brian Norris Thanks for taking care of this. Looks good, just one thing. > drivers/iio/industrialio-buffer.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index 90462fcf5436..2ad10e0190d8 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -107,6 +107,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf, > { > struct iio_dev *indio_dev = filp->private_data; > struct iio_buffer *rb = indio_dev->buffer; > + DEFINE_WAIT_FUNC(wait, woken_wake_function); > size_t datum_size; > size_t to_wait; > int ret; > @@ -132,10 +133,13 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf, > to_wait = min_t(size_t, n / datum_size, rb->watermark); > > do { > - ret = wait_event_interruptible(rb->pollq, > - iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)); > - if (ret) > - return ret; > + add_wait_queue(&rb->pollq, &wait); > + while (!iio_buffer_ready(indio_dev, rb, to_wait, > + n / datum_size)) { > + wait_woken(&wait, TASK_INTERRUPTIBLE, > + MAX_SCHEDULE_TIMEOUT); We loose the ability to break out from this loop by sending a signal to the task. This needs something like if (signal_pending(current)) { ret = -ERESTARTSYS; break; } before the wait_woken() And as a minor improvement I'd also move the add_wait_queue()/remove_wait_queue() outside of the outer loop. And then just if (!iio_buffer_ready(...)) continue; rather than having the inner loop. This should slightly simplify the flow. Just make sure to replace the returns in the loop with a break so remove_wait_queue() has a chance to run. > + } > + remove_wait_queue(&rb->pollq, &wait); > > if (!indio_dev->info) > return -ENODEV; >