From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9D86C433E2 for ; Wed, 16 Sep 2020 19:04:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8046A206DB for ; Wed, 16 Sep 2020 19:04:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600283070; bh=7NyPhCsJFe+XnywOE0R6a34Zb8JkKakmIMyfHGRY4M4=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:List-ID: From; b=ZopQCms0mXWmUF9zo4VArBYhX+rcI2QmJLbSWunbmAeFYnODeaw5GFaCFuOtTLrnG 9LlcVomrpwu/FLEEryzA0LchuqSQoU3jevdUK0akekO8KeAszxp/6Ek2Pf8LQqLrzA xVhmpCZNM6t/7jYmOjGYoEiRc2ljJVc+uo1v76kA= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728170AbgIPTE3 (ORCPT ); Wed, 16 Sep 2020 15:04:29 -0400 Received: from mail.kernel.org ([198.145.29.99]:37590 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727435AbgIPRqV (ORCPT ); Wed, 16 Sep 2020 13:46:21 -0400 Received: from paulmck-ThinkPad-P72.home (unknown [50.45.173.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B64F12245B; Wed, 16 Sep 2020 15:29:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600270196; bh=7NyPhCsJFe+XnywOE0R6a34Zb8JkKakmIMyfHGRY4M4=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=RDn2wViv7vu9hmTmpsb1bbpxLvCuo/SWaPWp9V1mUJjTgcbrPb3R9uT93osWEIaEM 9afuPUPWLKufiQ8ziF9Odhp5i1jimAe0xZ86J+gr+sITpv+bXYsBTi2jsiSuqu37hP IQWviNGI8iEhKKuiZkBJY1pWiLjStFx1TIayKIP0= Received: by paulmck-ThinkPad-P72.home (Postfix, from userid 1000) id 4FBEC3522836; Wed, 16 Sep 2020 08:29:56 -0700 (PDT) Date: Wed, 16 Sep 2020 08:29:56 -0700 From: "Paul E. McKenney" To: Daniel Vetter Cc: Linus Torvalds , Thomas Gleixner , Ard Biesheuvel , Herbert Xu , LKML , linux-arch , Sebastian Andrzej Siewior , Valentin Schneider , Richard Henderson , Ivan Kokshaysky , Matt Turner , alpha , Jeff Dike , Richard Weinberger , Anton Ivanov , linux-um , Brian Cain , linux-hexagon@vger.kernel.org, Geert Uytterhoeven , linux-m68k , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Will Deacon , Andrew Morton , Linux-MM , Ingo Molnar , Russell King , Linux ARM , Chris Zankel , Max Filippov , linux-xtensa@linux-xtensa.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , intel-gfx , dri-devel , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , Shuah Khan , rcu@vger.kernel.org, "open list:KERNEL SELFTEST FRAMEWORK" Subject: Re: [patch 00/13] preempt: Make preempt count unconditional Message-ID: <20200916152956.GV29330@paulmck-ThinkPad-P72> Reply-To: paulmck@kernel.org References: <20200914204209.256266093@linutronix.de> <871rj4owfn.fsf@nanos.tec.linutronix.de> <87bli75t7v.fsf@nanos.tec.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 16, 2020 at 09:37:17AM +0200, Daniel Vetter wrote: > On Tue, Sep 15, 2020 at 7:35 PM Linus Torvalds > wrote: > > > > On Tue, Sep 15, 2020 at 1:39 AM Thomas Gleixner wrote: > > > > > > OTOH, having a working 'preemptible()' or maybe better named > > > 'can_schedule()' check makes tons of sense to make decisions about > > > allocation modes or other things. > > > > No. I think that those kinds of decisions about actual behavior are > > always simply fundamentally wrong. > > > > Note that this is very different from having warnings about invalid > > use. THAT is correct. It may not warn in all configurations, but that > > doesn't matter: what matters is that it warns in common enough > > configurations that developers will catch it. > > > > So having a warning in "might_sleep()" that doesn't always trigger, > > because you have a limited configuration that can't even detect the > > situation, that's fine and dandy and intentional. > > > > But having code like > > > > if (can_schedule()) > > .. do something different .. > > > > is fundamentally complete and utter garbage. > > > > It's one thing if you test for "am I in hardware interrupt context". > > Those tests aren't great either, but at least they make sense. > > > > But a driver - or some library routine - making a difference based on > > some nebulous "can I schedule" is fundamentally and basically WRONG. > > > > If some code changes behavior, it needs to be explicit to the *caller* > > of that code. > > > > So this is why GFP_ATOMIC is fine, but "if (!can_schedule()) > > do_something_atomic()" is pure shite. > > > > And I am not IN THE LEAST interested in trying to help people doing > > pure shite. We need to fix them. Like the crypto code is getting > > fixed. > > Just figured I'll throw my +1 in from reading too many (gpu) drivers. > Code that tries to cleverly adjust its behaviour depending upon the > context it's running in is harder to understand and blows up in more > interesting ways. We still have drm_can_sleep() and it's mostly just > used for debug code, and I've largely ended up just deleting > everything that used it because when you're driver is blowing up the > last thing you want is to realize your debug code and output can't be > relied upon. Or worse, that the only Oops you have is the one in the > debug code, because the real one scrolled away - the original idea > behind drm_can_sleep was to make all the modeset code work > automagically both in normal ioctl/kworker context and in the panic > handlers or kgdb callbacks. Wishful thinking at best. > > Also at least for me that extends to everything, e.g. I much prefer > explicit spin_lock and spin_lock_irq vs magic spin_lock_irqsave for > locks shared with interrupt handlers, since the former two gives me > clear information from which contexts such function can be called. > Other end is the memalloc_no*_save/restore functions, where I recently > made a real big fool of myself because I didn't realize how much that > impacts everything that's run within - suddenly "GFP_KERNEL for small > stuff never fails" is wrong everywhere. > > It's all great for debugging and sanity checks (and we run with all > that stuff enabled in our CI), but really semantic changes depending > upon magic context checks freak my out :-) All fair, but some of us need to write code that must handle being invoked from a wide variety of contexts. Now perhaps you like the idea of call_rcu() for schedulable contexts, call_rcu_nosched() when preemption is disabled, call_rcu_irqs_are_disabled() when interrupts are disabled, call_rcu_raw_atomic() from contexts where (for example) raw spinlocks are held, and so on. However, from what I can see, most people instead consistently prefer that the RCU API instead be consolidated. Some in-flight cache-efficiency work for kvfree_rcu() and call_rcu() needs to be able to allocate memory occasionally. It can do that when invoked from some contexts, but not when invoked from others. Right now, in !PREEMPT kernels, it cannot tell, and must either do things to the memory allocators that some of the MM hate or must unnecessarily invoke workqueues. Thomas's patches would allow the code to just allocate in the common case when these primitives are invoked from contexts where allocation is permitted. If we want to restrict access to the can_schedule() or whatever primitive, fine and good. We can add a check to checkpatch.pl, for example. Maybe we can go back to the old brlock approach of requiring certain people's review for each addition to the kernel. But there really are use cases that it would greatly help. Thanx, Paul From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 643CBC433E2 for ; Wed, 16 Sep 2020 15:30:00 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F2AE42245D for ; Wed, 16 Sep 2020 15:29:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="RDn2wViv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F2AE42245D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 99E626B006C; Wed, 16 Sep 2020 11:29:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 925E2900003; Wed, 16 Sep 2020 11:29:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7EF756B0072; Wed, 16 Sep 2020 11:29:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0242.hostedemail.com [216.40.44.242]) by kanga.kvack.org (Postfix) with ESMTP id 6A9BA6B006C for ; Wed, 16 Sep 2020 11:29:59 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 18F7D180AD806 for ; Wed, 16 Sep 2020 15:29:59 +0000 (UTC) X-FDA: 77269310118.30.jar45_350e60d2711b Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin30.hostedemail.com (Postfix) with ESMTP id D143D180B3AA7 for ; Wed, 16 Sep 2020 15:29:58 +0000 (UTC) X-HE-Tag: jar45_350e60d2711b X-Filterd-Recvd-Size: 8803 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Wed, 16 Sep 2020 15:29:57 +0000 (UTC) Received: from paulmck-ThinkPad-P72.home (unknown [50.45.173.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B64F12245B; Wed, 16 Sep 2020 15:29:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600270196; bh=7NyPhCsJFe+XnywOE0R6a34Zb8JkKakmIMyfHGRY4M4=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=RDn2wViv7vu9hmTmpsb1bbpxLvCuo/SWaPWp9V1mUJjTgcbrPb3R9uT93osWEIaEM 9afuPUPWLKufiQ8ziF9Odhp5i1jimAe0xZ86J+gr+sITpv+bXYsBTi2jsiSuqu37hP IQWviNGI8iEhKKuiZkBJY1pWiLjStFx1TIayKIP0= Received: by paulmck-ThinkPad-P72.home (Postfix, from userid 1000) id 4FBEC3522836; Wed, 16 Sep 2020 08:29:56 -0700 (PDT) Date: Wed, 16 Sep 2020 08:29:56 -0700 From: "Paul E. McKenney" To: Daniel Vetter Cc: Linus Torvalds , Thomas Gleixner , Ard Biesheuvel , Herbert Xu , LKML , linux-arch , Sebastian Andrzej Siewior , Valentin Schneider , Richard Henderson , Ivan Kokshaysky , Matt Turner , alpha , Jeff Dike , Richard Weinberger , Anton Ivanov , linux-um , Brian Cain , linux-hexagon@vger.kernel.org, Geert Uytterhoeven , linux-m68k , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Will Deacon , Andrew Morton , Linux-MM , Ingo Molnar , Russell King , Linux ARM , Chris Zankel , Max Filippov , linux-xtensa@linux-xtensa.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , intel-gfx , dri-devel , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , Shuah Khan , rcu@vger.kernel.org, "open list:KERNEL SELFTEST FRAMEWORK" Subject: Re: [patch 00/13] preempt: Make preempt count unconditional Message-ID: <20200916152956.GV29330@paulmck-ThinkPad-P72> Reply-To: paulmck@kernel.org References: <20200914204209.256266093@linutronix.de> <871rj4owfn.fsf@nanos.tec.linutronix.de> <87bli75t7v.fsf@nanos.tec.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Rspamd-Queue-Id: D143D180B3AA7 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Sep 16, 2020 at 09:37:17AM +0200, Daniel Vetter wrote: > On Tue, Sep 15, 2020 at 7:35 PM Linus Torvalds > wrote: > > > > On Tue, Sep 15, 2020 at 1:39 AM Thomas Gleixner wrote: > > > > > > OTOH, having a working 'preemptible()' or maybe better named > > > 'can_schedule()' check makes tons of sense to make decisions about > > > allocation modes or other things. > > > > No. I think that those kinds of decisions about actual behavior are > > always simply fundamentally wrong. > > > > Note that this is very different from having warnings about invalid > > use. THAT is correct. It may not warn in all configurations, but that > > doesn't matter: what matters is that it warns in common enough > > configurations that developers will catch it. > > > > So having a warning in "might_sleep()" that doesn't always trigger, > > because you have a limited configuration that can't even detect the > > situation, that's fine and dandy and intentional. > > > > But having code like > > > > if (can_schedule()) > > .. do something different .. > > > > is fundamentally complete and utter garbage. > > > > It's one thing if you test for "am I in hardware interrupt context". > > Those tests aren't great either, but at least they make sense. > > > > But a driver - or some library routine - making a difference based on > > some nebulous "can I schedule" is fundamentally and basically WRONG. > > > > If some code changes behavior, it needs to be explicit to the *caller* > > of that code. > > > > So this is why GFP_ATOMIC is fine, but "if (!can_schedule()) > > do_something_atomic()" is pure shite. > > > > And I am not IN THE LEAST interested in trying to help people doing > > pure shite. We need to fix them. Like the crypto code is getting > > fixed. > > Just figured I'll throw my +1 in from reading too many (gpu) drivers. > Code that tries to cleverly adjust its behaviour depending upon the > context it's running in is harder to understand and blows up in more > interesting ways. We still have drm_can_sleep() and it's mostly just > used for debug code, and I've largely ended up just deleting > everything that used it because when you're driver is blowing up the > last thing you want is to realize your debug code and output can't be > relied upon. Or worse, that the only Oops you have is the one in the > debug code, because the real one scrolled away - the original idea > behind drm_can_sleep was to make all the modeset code work > automagically both in normal ioctl/kworker context and in the panic > handlers or kgdb callbacks. Wishful thinking at best. > > Also at least for me that extends to everything, e.g. I much prefer > explicit spin_lock and spin_lock_irq vs magic spin_lock_irqsave for > locks shared with interrupt handlers, since the former two gives me > clear information from which contexts such function can be called. > Other end is the memalloc_no*_save/restore functions, where I recently > made a real big fool of myself because I didn't realize how much that > impacts everything that's run within - suddenly "GFP_KERNEL for small > stuff never fails" is wrong everywhere. > > It's all great for debugging and sanity checks (and we run with all > that stuff enabled in our CI), but really semantic changes depending > upon magic context checks freak my out :-) All fair, but some of us need to write code that must handle being invoked from a wide variety of contexts. Now perhaps you like the idea of call_rcu() for schedulable contexts, call_rcu_nosched() when preemption is disabled, call_rcu_irqs_are_disabled() when interrupts are disabled, call_rcu_raw_atomic() from contexts where (for example) raw spinlocks are held, and so on. However, from what I can see, most people instead consistently prefer that the RCU API instead be consolidated. Some in-flight cache-efficiency work for kvfree_rcu() and call_rcu() needs to be able to allocate memory occasionally. It can do that when invoked from some contexts, but not when invoked from others. Right now, in !PREEMPT kernels, it cannot tell, and must either do things to the memory allocators that some of the MM hate or must unnecessarily invoke workqueues. Thomas's patches would allow the code to just allocate in the common case when these primitives are invoked from contexts where allocation is permitted. If we want to restrict access to the can_schedule() or whatever primitive, fine and good. We can add a check to checkpatch.pl, for example. Maybe we can go back to the old brlock approach of requiring certain people's review for each addition to the kernel. But there really are use cases that it would greatly help. Thanx, Paul From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46A09C2BB84 for ; Wed, 16 Sep 2020 15:30:00 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EBC5922461 for ; Wed, 16 Sep 2020 15:29:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="RDn2wViv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EBC5922461 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CB6496EA2E; Wed, 16 Sep 2020 15:29:58 +0000 (UTC) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5CA246EA2E; Wed, 16 Sep 2020 15:29:57 +0000 (UTC) Received: from paulmck-ThinkPad-P72.home (unknown [50.45.173.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B64F12245B; Wed, 16 Sep 2020 15:29:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600270196; bh=7NyPhCsJFe+XnywOE0R6a34Zb8JkKakmIMyfHGRY4M4=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=RDn2wViv7vu9hmTmpsb1bbpxLvCuo/SWaPWp9V1mUJjTgcbrPb3R9uT93osWEIaEM 9afuPUPWLKufiQ8ziF9Odhp5i1jimAe0xZ86J+gr+sITpv+bXYsBTi2jsiSuqu37hP IQWviNGI8iEhKKuiZkBJY1pWiLjStFx1TIayKIP0= Received: by paulmck-ThinkPad-P72.home (Postfix, from userid 1000) id 4FBEC3522836; Wed, 16 Sep 2020 08:29:56 -0700 (PDT) Date: Wed, 16 Sep 2020 08:29:56 -0700 From: "Paul E. McKenney" To: Daniel Vetter Subject: Re: [patch 00/13] preempt: Make preempt count unconditional Message-ID: <20200916152956.GV29330@paulmck-ThinkPad-P72> References: <20200914204209.256266093@linutronix.de> <871rj4owfn.fsf@nanos.tec.linutronix.de> <87bli75t7v.fsf@nanos.tec.linutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: paulmck@kernel.org Cc: Juri Lelli , Peter Zijlstra , Sebastian Andrzej Siewior , Lai Jiangshan , dri-devel , Ben Segall , Linux-MM , "open list:KERNEL SELFTEST FRAMEWORK" , linux-hexagon@vger.kernel.org, Will Deacon , Ingo Molnar , Anton Ivanov , linux-arch , Vincent Guittot , Herbert Xu , Brian Cain , Richard Weinberger , Russell King , Ard Biesheuvel , David Airlie , Ingo Molnar , Geert Uytterhoeven , Mel Gorman , intel-gfx , Matt Turner , Valentin Schneider , linux-xtensa@linux-xtensa.org, Shuah Khan , Jeff Dike , linux-um , Josh Triplett , Steven Rostedt , rcu@vger.kernel.org, linux-m68k , Ivan Kokshaysky , Rodrigo Vivi , Thomas Gleixner , Dietmar Eggemann , Linux ARM , Richard Henderson , Chris Zankel , Max Filippov , Daniel Bristot de Oliveira , LKML , alpha , Mathieu Desnoyers , Andrew Morton , Linus Torvalds Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Wed, Sep 16, 2020 at 09:37:17AM +0200, Daniel Vetter wrote: > On Tue, Sep 15, 2020 at 7:35 PM Linus Torvalds > wrote: > > > > On Tue, Sep 15, 2020 at 1:39 AM Thomas Gleixner wrote: > > > > > > OTOH, having a working 'preemptible()' or maybe better named > > > 'can_schedule()' check makes tons of sense to make decisions about > > > allocation modes or other things. > > > > No. I think that those kinds of decisions about actual behavior are > > always simply fundamentally wrong. > > > > Note that this is very different from having warnings about invalid > > use. THAT is correct. It may not warn in all configurations, but that > > doesn't matter: what matters is that it warns in common enough > > configurations that developers will catch it. > > > > So having a warning in "might_sleep()" that doesn't always trigger, > > because you have a limited configuration that can't even detect the > > situation, that's fine and dandy and intentional. > > > > But having code like > > > > if (can_schedule()) > > .. do something different .. > > > > is fundamentally complete and utter garbage. > > > > It's one thing if you test for "am I in hardware interrupt context". > > Those tests aren't great either, but at least they make sense. > > > > But a driver - or some library routine - making a difference based on > > some nebulous "can I schedule" is fundamentally and basically WRONG. > > > > If some code changes behavior, it needs to be explicit to the *caller* > > of that code. > > > > So this is why GFP_ATOMIC is fine, but "if (!can_schedule()) > > do_something_atomic()" is pure shite. > > > > And I am not IN THE LEAST interested in trying to help people doing > > pure shite. We need to fix them. Like the crypto code is getting > > fixed. > > Just figured I'll throw my +1 in from reading too many (gpu) drivers. > Code that tries to cleverly adjust its behaviour depending upon the > context it's running in is harder to understand and blows up in more > interesting ways. We still have drm_can_sleep() and it's mostly just > used for debug code, and I've largely ended up just deleting > everything that used it because when you're driver is blowing up the > last thing you want is to realize your debug code and output can't be > relied upon. Or worse, that the only Oops you have is the one in the > debug code, because the real one scrolled away - the original idea > behind drm_can_sleep was to make all the modeset code work > automagically both in normal ioctl/kworker context and in the panic > handlers or kgdb callbacks. Wishful thinking at best. > > Also at least for me that extends to everything, e.g. I much prefer > explicit spin_lock and spin_lock_irq vs magic spin_lock_irqsave for > locks shared with interrupt handlers, since the former two gives me > clear information from which contexts such function can be called. > Other end is the memalloc_no*_save/restore functions, where I recently > made a real big fool of myself because I didn't realize how much that > impacts everything that's run within - suddenly "GFP_KERNEL for small > stuff never fails" is wrong everywhere. > > It's all great for debugging and sanity checks (and we run with all > that stuff enabled in our CI), but really semantic changes depending > upon magic context checks freak my out :-) All fair, but some of us need to write code that must handle being invoked from a wide variety of contexts. Now perhaps you like the idea of call_rcu() for schedulable contexts, call_rcu_nosched() when preemption is disabled, call_rcu_irqs_are_disabled() when interrupts are disabled, call_rcu_raw_atomic() from contexts where (for example) raw spinlocks are held, and so on. However, from what I can see, most people instead consistently prefer that the RCU API instead be consolidated. Some in-flight cache-efficiency work for kvfree_rcu() and call_rcu() needs to be able to allocate memory occasionally. It can do that when invoked from some contexts, but not when invoked from others. Right now, in !PREEMPT kernels, it cannot tell, and must either do things to the memory allocators that some of the MM hate or must unnecessarily invoke workqueues. Thomas's patches would allow the code to just allocate in the common case when these primitives are invoked from contexts where allocation is permitted. If we want to restrict access to the can_schedule() or whatever primitive, fine and good. We can add a check to checkpatch.pl, for example. Maybe we can go back to the old brlock approach of requiring certain people's review for each addition to the kernel. But there really are use cases that it would greatly help. Thanx, Paul _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 79969C433E2 for ; Wed, 16 Sep 2020 21:10:27 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 019DD206D4 for ; Wed, 16 Sep 2020 21:10:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="RDn2wViv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 019DD206D4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 212D16EB36; Wed, 16 Sep 2020 21:10:26 +0000 (UTC) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5CA246EA2E; Wed, 16 Sep 2020 15:29:57 +0000 (UTC) Received: from paulmck-ThinkPad-P72.home (unknown [50.45.173.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B64F12245B; Wed, 16 Sep 2020 15:29:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600270196; bh=7NyPhCsJFe+XnywOE0R6a34Zb8JkKakmIMyfHGRY4M4=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=RDn2wViv7vu9hmTmpsb1bbpxLvCuo/SWaPWp9V1mUJjTgcbrPb3R9uT93osWEIaEM 9afuPUPWLKufiQ8ziF9Odhp5i1jimAe0xZ86J+gr+sITpv+bXYsBTi2jsiSuqu37hP IQWviNGI8iEhKKuiZkBJY1pWiLjStFx1TIayKIP0= Received: by paulmck-ThinkPad-P72.home (Postfix, from userid 1000) id 4FBEC3522836; Wed, 16 Sep 2020 08:29:56 -0700 (PDT) Date: Wed, 16 Sep 2020 08:29:56 -0700 From: "Paul E. McKenney" To: Daniel Vetter Message-ID: <20200916152956.GV29330@paulmck-ThinkPad-P72> References: <20200914204209.256266093@linutronix.de> <871rj4owfn.fsf@nanos.tec.linutronix.de> <87bli75t7v.fsf@nanos.tec.linutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Mailman-Approved-At: Wed, 16 Sep 2020 21:10:25 +0000 Subject: Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: paulmck@kernel.org Cc: Juri Lelli , Peter Zijlstra , Sebastian Andrzej Siewior , Lai Jiangshan , dri-devel , Ben Segall , Linux-MM , "open list:KERNEL SELFTEST FRAMEWORK" , linux-hexagon@vger.kernel.org, Will Deacon , Ingo Molnar , Anton Ivanov , linux-arch , Herbert Xu , Brian Cain , Richard Weinberger , Russell King , Ard Biesheuvel , David Airlie , Ingo Molnar , Geert Uytterhoeven , Mel Gorman , intel-gfx , Matt Turner , Valentin Schneider , linux-xtensa@linux-xtensa.org, Shuah Khan , Jeff Dike , linux-um , Josh Triplett , Steven Rostedt , rcu@vger.kernel.org, linux-m68k , Ivan Kokshaysky , Thomas Gleixner , Dietmar Eggemann , Linux ARM , Richard Henderson , Chris Zankel , Max Filippov , Daniel Bristot de Oliveira , LKML , alpha , Mathieu Desnoyers , Andrew Morton , Linus Torvalds Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Wed, Sep 16, 2020 at 09:37:17AM +0200, Daniel Vetter wrote: > On Tue, Sep 15, 2020 at 7:35 PM Linus Torvalds > wrote: > > > > On Tue, Sep 15, 2020 at 1:39 AM Thomas Gleixner wrote: > > > > > > OTOH, having a working 'preemptible()' or maybe better named > > > 'can_schedule()' check makes tons of sense to make decisions about > > > allocation modes or other things. > > > > No. I think that those kinds of decisions about actual behavior are > > always simply fundamentally wrong. > > > > Note that this is very different from having warnings about invalid > > use. THAT is correct. It may not warn in all configurations, but that > > doesn't matter: what matters is that it warns in common enough > > configurations that developers will catch it. > > > > So having a warning in "might_sleep()" that doesn't always trigger, > > because you have a limited configuration that can't even detect the > > situation, that's fine and dandy and intentional. > > > > But having code like > > > > if (can_schedule()) > > .. do something different .. > > > > is fundamentally complete and utter garbage. > > > > It's one thing if you test for "am I in hardware interrupt context". > > Those tests aren't great either, but at least they make sense. > > > > But a driver - or some library routine - making a difference based on > > some nebulous "can I schedule" is fundamentally and basically WRONG. > > > > If some code changes behavior, it needs to be explicit to the *caller* > > of that code. > > > > So this is why GFP_ATOMIC is fine, but "if (!can_schedule()) > > do_something_atomic()" is pure shite. > > > > And I am not IN THE LEAST interested in trying to help people doing > > pure shite. We need to fix them. Like the crypto code is getting > > fixed. > > Just figured I'll throw my +1 in from reading too many (gpu) drivers. > Code that tries to cleverly adjust its behaviour depending upon the > context it's running in is harder to understand and blows up in more > interesting ways. We still have drm_can_sleep() and it's mostly just > used for debug code, and I've largely ended up just deleting > everything that used it because when you're driver is blowing up the > last thing you want is to realize your debug code and output can't be > relied upon. Or worse, that the only Oops you have is the one in the > debug code, because the real one scrolled away - the original idea > behind drm_can_sleep was to make all the modeset code work > automagically both in normal ioctl/kworker context and in the panic > handlers or kgdb callbacks. Wishful thinking at best. > > Also at least for me that extends to everything, e.g. I much prefer > explicit spin_lock and spin_lock_irq vs magic spin_lock_irqsave for > locks shared with interrupt handlers, since the former two gives me > clear information from which contexts such function can be called. > Other end is the memalloc_no*_save/restore functions, where I recently > made a real big fool of myself because I didn't realize how much that > impacts everything that's run within - suddenly "GFP_KERNEL for small > stuff never fails" is wrong everywhere. > > It's all great for debugging and sanity checks (and we run with all > that stuff enabled in our CI), but really semantic changes depending > upon magic context checks freak my out :-) All fair, but some of us need to write code that must handle being invoked from a wide variety of contexts. Now perhaps you like the idea of call_rcu() for schedulable contexts, call_rcu_nosched() when preemption is disabled, call_rcu_irqs_are_disabled() when interrupts are disabled, call_rcu_raw_atomic() from contexts where (for example) raw spinlocks are held, and so on. However, from what I can see, most people instead consistently prefer that the RCU API instead be consolidated. Some in-flight cache-efficiency work for kvfree_rcu() and call_rcu() needs to be able to allocate memory occasionally. It can do that when invoked from some contexts, but not when invoked from others. Right now, in !PREEMPT kernels, it cannot tell, and must either do things to the memory allocators that some of the MM hate or must unnecessarily invoke workqueues. Thomas's patches would allow the code to just allocate in the common case when these primitives are invoked from contexts where allocation is permitted. If we want to restrict access to the can_schedule() or whatever primitive, fine and good. We can add a check to checkpatch.pl, for example. Maybe we can go back to the old brlock approach of requiring certain people's review for each addition to the kernel. But there really are use cases that it would greatly help. Thanx, Paul _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Paul E. McKenney" Subject: Re: [patch 00/13] preempt: Make preempt count unconditional Date: Wed, 16 Sep 2020 08:29:56 -0700 Message-ID: <20200916152956.GV29330@paulmck-ThinkPad-P72> References: <20200914204209.256266093@linutronix.de> <871rj4owfn.fsf@nanos.tec.linutronix.de> <87bli75t7v.fsf@nanos.tec.linutronix.de> Reply-To: paulmck@kernel.org Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600270196; bh=7NyPhCsJFe+XnywOE0R6a34Zb8JkKakmIMyfHGRY4M4=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=RDn2wViv7vu9hmTmpsb1bbpxLvCuo/SWaPWp9V1mUJjTgcbrPb3R9uT93osWEIaEM 9afuPUPWLKufiQ8ziF9Odhp5i1jimAe0xZ86J+gr+sITpv+bXYsBTi2jsiSuqu37hP IQWviNGI8iEhKKuiZkBJY1pWiLjStFx1TIayKIP0= Content-Disposition: inline In-Reply-To: Sender: linux-alpha-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Daniel Vetter Cc: Linus Torvalds , Thomas Gleixner , Ard Biesheuvel , Herbert Xu , LKML , linux-arch , Sebastian Andrzej Siewior , Valentin Schneider , Richard Henderson , Ivan Kokshaysky , Matt Turner , alpha , Jeff Dike , Richard Weinberger , Anton Ivanov , linux-um , Brian Cain , linux-hexagon@vger.kernel.org, Geert Uytterhoeven On Wed, Sep 16, 2020 at 09:37:17AM +0200, Daniel Vetter wrote: > On Tue, Sep 15, 2020 at 7:35 PM Linus Torvalds > wrote: > > > > On Tue, Sep 15, 2020 at 1:39 AM Thomas Gleixner wrote: > > > > > > OTOH, having a working 'preemptible()' or maybe better named > > > 'can_schedule()' check makes tons of sense to make decisions about > > > allocation modes or other things. > > > > No. I think that those kinds of decisions about actual behavior are > > always simply fundamentally wrong. > > > > Note that this is very different from having warnings about invalid > > use. THAT is correct. It may not warn in all configurations, but that > > doesn't matter: what matters is that it warns in common enough > > configurations that developers will catch it. > > > > So having a warning in "might_sleep()" that doesn't always trigger, > > because you have a limited configuration that can't even detect the > > situation, that's fine and dandy and intentional. > > > > But having code like > > > > if (can_schedule()) > > .. do something different .. > > > > is fundamentally complete and utter garbage. > > > > It's one thing if you test for "am I in hardware interrupt context". > > Those tests aren't great either, but at least they make sense. > > > > But a driver - or some library routine - making a difference based on > > some nebulous "can I schedule" is fundamentally and basically WRONG. > > > > If some code changes behavior, it needs to be explicit to the *caller* > > of that code. > > > > So this is why GFP_ATOMIC is fine, but "if (!can_schedule()) > > do_something_atomic()" is pure shite. > > > > And I am not IN THE LEAST interested in trying to help people doing > > pure shite. We need to fix them. Like the crypto code is getting > > fixed. > > Just figured I'll throw my +1 in from reading too many (gpu) drivers. > Code that tries to cleverly adjust its behaviour depending upon the > context it's running in is harder to understand and blows up in more > interesting ways. We still have drm_can_sleep() and it's mostly just > used for debug code, and I've largely ended up just deleting > everything that used it because when you're driver is blowing up the > last thing you want is to realize your debug code and output can't be > relied upon. Or worse, that the only Oops you have is the one in the > debug code, because the real one scrolled away - the original idea > behind drm_can_sleep was to make all the modeset code work > automagically both in normal ioctl/kworker context and in the panic > handlers or kgdb callbacks. Wishful thinking at best. > > Also at least for me that extends to everything, e.g. I much prefer > explicit spin_lock and spin_lock_irq vs magic spin_lock_irqsave for > locks shared with interrupt handlers, since the former two gives me > clear information from which contexts such function can be called. > Other end is the memalloc_no*_save/restore functions, where I recently > made a real big fool of myself because I didn't realize how much that > impacts everything that's run within - suddenly "GFP_KERNEL for small > stuff never fails" is wrong everywhere. > > It's all great for debugging and sanity checks (and we run with all > that stuff enabled in our CI), but really semantic changes depending > upon magic context checks freak my out :-) All fair, but some of us need to write code that must handle being invoked from a wide variety of contexts. Now perhaps you like the idea of call_rcu() for schedulable contexts, call_rcu_nosched() when preemption is disabled, call_rcu_irqs_are_disabled() when interrupts are disabled, call_rcu_raw_atomic() from contexts where (for example) raw spinlocks are held, and so on. However, from what I can see, most people instead consistently prefer that the RCU API instead be consolidated. Some in-flight cache-efficiency work for kvfree_rcu() and call_rcu() needs to be able to allocate memory occasionally. It can do that when invoked from some contexts, but not when invoked from others. Right now, in !PREEMPT kernels, it cannot tell, and must either do things to the memory allocators that some of the MM hate or must unnecessarily invoke workqueues. Thomas's patches would allow the code to just allocate in the common case when these primitives are invoked from contexts where allocation is permitted. If we want to restrict access to the can_schedule() or whatever primitive, fine and good. We can add a check to checkpatch.pl, for example. Maybe we can go back to the old brlock approach of requiring certain people's review for each addition to the kernel. But there really are use cases that it would greatly help. Thanx, Paul