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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E56E4C433EF for ; Sun, 7 Nov 2021 17:16:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CE78E60EBD for ; Sun, 7 Nov 2021 17:16:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235930AbhKGRSw (ORCPT ); Sun, 7 Nov 2021 12:18:52 -0500 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:41051 "EHLO out2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235923AbhKGRSw (ORCPT ); Sun, 7 Nov 2021 12:18:52 -0500 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id C4FEC5C00C8; Sun, 7 Nov 2021 12:16:08 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Sun, 07 Nov 2021 12:16:08 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=Liz9He z2pQ3I/11xvMmZRFFwaYA7f2I/IxstpB3jmBo=; b=nyDBEMPKwtXB60WUGE1W4K ubQ51UbRauaMLJstx3QvCnDyRuYeqf0557P8yrWJFhy8iwgCBlE28Py4aVah8o/V /bsR4KBR35JMhnzVSsUeRX4/15DIUfMKfbDKnnTErtuD+D2KENwl92D1CHNb2Na+ lf4cX7FE20W+x/D/nMCVhOhuEXj+ZP0pPkOp5ZtSxheRlwaD3TJF2l3hbZKzHGlm +oboLiz9Iv0PEkiFX6LC31qBHoSNbZhfJYhXZu+IFBJZaIzJvdZH3i4Hux1n52dD Mt3gFvuqc5b0WbPdFwrdPTmZgsUFDS6iJSfvZ6b3SHT3rd8wxxrHT/1oKpH572pw == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddruddtgdeliecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesthdtredttddtvdenucfhrhhomhepkfguohcuufgt hhhimhhmvghluceoihguohhstghhsehiughoshgthhdrohhrgheqnecuggftrfgrthhtvg hrnhepgfevgfevueduueffieffheeifffgjeelvedtteeuteeuffekvefggfdtudfgkeev necuffhomhgrihhnpehkvghrnhgvlhdrohhrghenucevlhhushhtvghrufhiiigvpedtne curfgrrhgrmhepmhgrihhlfhhrohhmpehiughoshgthhesihguohhstghhrdhorhhg X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 7 Nov 2021 12:16:07 -0500 (EST) Date: Sun, 7 Nov 2021 19:16:05 +0200 From: Ido Schimmel To: Jakub Kicinski Cc: Jiri Pirko , Leon Romanovsky , "David S . Miller" , Leon Romanovsky , Jiri Pirko , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, edwin.peer@broadcom.com Subject: Re: [PATCH net-next] devlink: Require devlink lock during device reload Message-ID: References: <9716f9a13e217a0a163b745b6e92e02d40973d2c.1635701665.git.leonro@nvidia.com> <20211101161122.37fbb99d@kicinski-fedora-PC1C0HJN> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211101161122.37fbb99d@kicinski-fedora-PC1C0HJN> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Nov 01, 2021 at 04:11:22PM -0700, Jakub Kicinski wrote: > On Mon, 1 Nov 2021 22:52:19 +0200 Ido Schimmel wrote: > > > >Signed-off-by: Leon Romanovsky > > > > > > Looks fine to me. > > > > > > Reviewed-by: Jiri Pirko > > > > Traces from mlxsw / netdevsim below: > > Thanks a lot for the testing Ido! > > Would you mind giving my RFC a spin as well on your syzbot machinery? Sorry for the delay. I didn't have a lot of time last week. I tried to apply your set [1] on top of net-next, but I'm getting a conflict with patch #5. Can you send me (here / privately) a link to a git tree that has the patches on top of net-next? TBH, if you ran the netdevsim selftests with a debug config and nothing exploded, then I don't expect syzkaller to find anything (major). [1] https://lore.kernel.org/netdev/20211030231254.2477599-1-kuba@kernel.org/ > > Any input on the three discussion points there? > > (1) should we have a "locked" and "unlocked" API or use lock nesting? Judging by the netdevsim conversion, it seems that for the vast majority of APIs (if not all) we will only have an "unlocked" API. Drivers will hold the devlink instance lock on probe / remove and devlink itself will hold the lock when calling into drivers (e.g., on reload, port split). > > (2) should we expose devlink lock so that drivers can use devlink > as a framework for their locking needs? It is better than dropping locks (e.g., DEVLINK_NL_FLAG_NO_LOCK, which I expect will go away after the conversion). With the asserts you put in place, misuses will be caught early. > > (3) should we let drivers take refs on the devlink instance? I think it's fine mainly because I don't expect it to be used by too many drivers other than netdevsim which is somewhat special. Looking at the call sites of devlink_get() in netdevsim, it is only called from places (debugfs and trap workqueue) that shouldn't be present in real drivers. The tl;dr is that your approach makes sense to me. I was initially worried that we will need to propagate a "reload" argument everywhere in drivers, but you wrote "The expectation is that driver will take the devlink instance lock on its probe and remove paths", which avoids that. Thanks for working on that