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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 88CD7C43217 for ; Thu, 13 Oct 2022 13:19:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229989AbiJMNTz (ORCPT ); Thu, 13 Oct 2022 09:19:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36694 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229658AbiJMNTx (ORCPT ); Thu, 13 Oct 2022 09:19:53 -0400 Received: from vps0.lunn.ch (vps0.lunn.ch [185.16.172.187]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CCC4D1EC6B for ; Thu, 13 Oct 2022 06:19:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Transfer-Encoding:Content-Disposition: Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:From: Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version:Content-Type: Content-Transfer-Encoding:Content-ID:Content-Description:Content-Disposition: In-Reply-To:References; bh=7n7Lfasr/Gsjpvedt/7abRR+55tCsTTHCgaqJ2vWxBs=; b=Ty Ck+3lbfoL6XLVNB6XxdsJLZ/MwQDAUcx7dhECE0Kgt3njxAL1BWgwN5N0E+tYogKymBcgSYFwH//P xvwJZ539SaaUQE8yPHvNlIIjVwuafLm/4xFpa26YMqAwupd4ddhJqxLffAX6MOb57JefKenf6ALc8 8r6wCgOMQ3AMPlc=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1oiy7h-001tDd-HD; Thu, 13 Oct 2022 15:19:41 +0200 Date: Thu, 13 Oct 2022 15:19:41 +0200 From: Andrew Lunn To: Thorsten Glaser Cc: Eric Dumazet , Dave Taht , netdev@vger.kernel.org Subject: Re: RFH, where did I go wrong? Message-ID: References: <42776059-242c-cf49-c3ed-31e311b91f1c@tarent.de> <1a1214b6-fc29-1e11-ec21-682684188513@tarent.de> <8051fcd-4b5-7b32-887e-7df7a779be1b@tarent.de> <3660fc5b-5cb3-61ee-a10c-0f541282eba4@gmail.com> <93ce7034-a26f-b68d-f27f-ef90b6b01bf8@tarent.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <93ce7034-a26f-b68d-f27f-ef90b6b01bf8@tarent.de> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Oct 12, 2022 at 11:56:11PM +0200, Thorsten Glaser wrote: > On Wed, 12 Oct 2022, Andrew Lunn wrote: > > > > Ooh! Will try! That’s what I get for getting, ahem, inspiration > > > from other qdiscs. > > > > Are other qdiscs also missing RTNL, or are you just using the > > inspiration in a different context? > > I think I was probably confused between which of the functions can > be used when. Eric explained the why. What I was missing was… well, > basically what I asked for weeks ago — what functions I need to > provide when writing a qdisc, and which guarantees and expectations > these have. That rtnl is held for… apparently all but enqueue/dequeue… > was one of these. I doubt other qdiscs miss it, or their users would > also run into this crash or so :/ Not all broken code, specially with locks, causes a simple to reproduce crash. So i was hoping that now you have gained an understanding of what you did wrong, maybe you can see other places which make the same error, possibly from where you cut/pasted, which might not yet of crashed. Thats another way of say, if you have found a bug, look around and see if the same bug exists somewhere else nearby. > The thing I did first was to add ASSERT_RTNL(); directly before the > rtnl_* call, just like it was in the other place. That, of course, > crashed immediately. Now *this* could be done systematically. > > In OpenBSD, things like that are often hidden behind #if DIAGNOSTIC > which is a global option, disabled in “prod” or space-constrained > (installer) kernels but enabled for the “generic” one for wide testing. > Something to think about? The network stack generally falls into two parts. The fast path tries its best to avoid anything expensive, like mutexes. It uses spinlocks if it needs any sort of locking. And there is the rest, which is mostly the control plain, which is the slow path. It is protected by RTNL. As you said above, "That rtnl is held for… apparently all but enqueue/dequeue…" enqueue/dequeue are the fast path, so RTNL is not required, the rest is mostly control plan, so RTNL is probably required. Also, since it is the slow path, adding ASSERT_RTNL() is fine, and it does not need to be hidden behind #if DIAGNOSTIC. Linux does have some debug code hidden behind ifdefs, see the kernel hacking section of make menuconfig. The sleep in atomic context test is a good example of this. While doing development work, you want to turn on most of the checks in this section. That sleep in atomic check will probably help you find cases where you hold RTNL but should not. The lock checking will help you find potential deadlocks with RTNL and any other locks you might use, etc. Andrew