From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932188AbeCIQDc (ORCPT ); Fri, 9 Mar 2018 11:03:32 -0500 Received: from mail-oi0-f65.google.com ([209.85.218.65]:39806 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751188AbeCIQD2 (ORCPT ); Fri, 9 Mar 2018 11:03:28 -0500 X-Google-Smtp-Source: AG47ELuBKuoepyXLNo78V+lycfCn8gE3vWNPqGZ6yy1JXbNzmyoBm9Py2qxgdejuB1tm8G04Hb6y7A== Subject: Re: Warning from swake_up_all in 4.14.15-rt13 non-RT To: Sebastian Andrzej Siewior Cc: Peter Zijlstra , Thomas Gleixner , Steven Rostedt , linux-rt-users , linux-kernel , Tejun Heo References: <20180306174604.nta5rcvfvrfdfftz@linutronix.de> <1704d817-8fb9-ce8f-1aa1-fe6e8b0c3919@mvista.com> <20180308174103.mduy5qq2ttlcvig3@linutronix.de> <20180309110418.lwtennjqwqcxh422@linutronix.de> <08aa9f8a-afe5-e1d3-5301-d196beb665b5@mvista.com> <20180309145819.af2546nfa4z6qqxm@linutronix.de> From: Corey Minyard Message-ID: Date: Fri, 9 Mar 2018 10:03:22 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180309145819.af2546nfa4z6qqxm@linutronix.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/09/2018 08:58 AM, Sebastian Andrzej Siewior wrote: > On 2018-03-09 07:29:31 [-0600], Corey Minyard wrote: >> From what I can tell, wake_up_q() is unbounded, and you have undone what >> the previous code had tried to accomplish.  In the scenario I'm talking >> about, >> interrupts are still disabled here.  That's why I was asking about where to >> put >> wake_up_q(), I knew you could put it here, but it didn't seem to me to help >> at all. > So you are worried about unbound latencies on !RT. Okay. So for !RT this > does not help but it is not worse then before (before the RT patch was > applied and changed things). > In fact it is better now (with RT patch and this one) because before > that patch you would not only open interrupts between the wake up but you > would leave the function with interrupts open which is wrong. Any > interrupt (or a context switch due to need-resched() that would invoke > percpu_ref_put() would freeze the CPU/system. > Also, every user that invoked swake_up_all() with enabled interrupts > will still perform the wake up with enabled interrupts. So nothing > changes here. Ah, ok, that makes sense.  Sorry, I was mixing things up.  Yes. on RT this should fix the unbounded time issue, and it should also solve the interrupts disabled issue on !RT. I'll try this out. -corey >>>> I had another idea.  This is only occurring if RT is not enabled, because >>>> with >>>> RT all the irq disable things go away and you are generally running in task >>>> context.  So why not have a different version of swake_up_all() for non-RT >>>> that does work from irqs-off context? >>> With the patch above I have puzzle part which would allow to use swait >>> based completions upstream. That ifdef would probably not help. >> I agree that having a bounded time way to wake up a bunch of threads while >> interrupts are disabled would solve a bunch of issues.  I just don't see how >> it >> can be done without pushing it off to a softirq or workqueue. > true but this is a different story. We started with a WARN_ON() which > triggered correctly and the problem it pointed to looks solved to me. > > This "unbounded runtime during the wake up of many tasks with interrupts > disabled via percpu_ref_kill() -> blk_queue_usage_counter_release()" > thing exists already in the vanilla kernel and does not exist > with the RT patch applied and RT enabled. If you are affected by this > and you don't like it - fine. Using a workqueue is one way of getting > around it (the softirq is not preemptible in !RT so it wouldn't change > much). However, I see no benefit in carrying such a patch because as I > said only !RT is affected by this. > >> -corey > Sebastian