From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 352DC145321; Thu, 13 Jun 2024 14:13:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718288026; cv=none; b=CKLtDKWHuRYUjbOAlJ1CsMolvil5ER4TVGwQnrq0HIXL2wIIVAlVqyisrLGbpaKPsR1cUcbNR7qYGZlUvKQa1dyLaHI17cLrnQiTP4NPpNpmI/qAXvuzRt0L2LayuTdyBxAGVh5mM/DRs2yDzTvuf2W6rCcKeDZOhJRk315D6jM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718288026; c=relaxed/simple; bh=miLeVMM2w5yAcnT3uzAx4GpVzV/Y9CzVC17jsvEBUfU=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=LFrRW8EsNPzlLdlfDrJ98UN7hAf2zD27vz4a+wAsvDSARhauN1EmjMqobC1P0FqC+2/8s6UcDvaKrrIdcoMepodiM1CjAkTZGmSRXqo1TpwB2h9QBwr8qnhRDAll1A5cmDqcskeC4h9QVYwayQ0ZkvGb8lnBwU7ZqUE0DxuEwpI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ogjDdw34; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ogjDdw34" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0C59BC2BBFC; Thu, 13 Jun 2024 14:13:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1718288024; bh=miLeVMM2w5yAcnT3uzAx4GpVzV/Y9CzVC17jsvEBUfU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ogjDdw34aMYbnVFXO1TQJ6gxcFOElu+kQ1l9kcwCvX/5TJXX6Y46YnhOfBue9+SfZ KpPZTJi+ymmNr/Fe68Gb+Dj57jzeUz4dXcdurC3Y09XSnhkysiQrzzFouyiUXmPG+h 0Kw32Ke9rEiUSd1lqkOMQXvXVpp7YdjaLVT8YQdUjqxJpeUE/hzDrskqDgJmQaAtLh 37Y3FO250POxonskubWav59Lxb9Vj8N2COWpo2ifg80tyiSjgFNLU5YuwuFpltcUAS pavfyYFer7rPxxkOfzw+IPQUsivZ0OlfTrFK3ejk/7EUpbu2SoYv2OJdtoIY5zQygp rp62AoeqnUtCw== Date: Thu, 13 Jun 2024 07:13:43 -0700 From: Jakub Kicinski To: Larysa Zaremba Cc: , Jesse Brandeburg , Tony Nguyen , "David S. Miller" , Eric Dumazet , Paolo Abeni , Alexei Starovoitov , "Daniel Borkmann" , Jesper Dangaard Brouer , John Fastabend , Maciej Fijalkowski , , , , , Michal Kubiak Subject: Re: [PATCH iwl-net 0/3] ice: fix synchronization between .ndo_bpf() and reset Message-ID: <20240613071343.019e7dca@kernel.org> In-Reply-To: References: <20240610153716.31493-1-larysa.zaremba@intel.com> <20240611193837.4ffb2401@kernel.org> <20240612140935.54981c49@kernel.org> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 13 Jun 2024 10:54:12 +0200 Larysa Zaremba wrote: > > > The locking mechanisms I use here do not look pretty, but if I am not missing > > > anything, the synchronization they provide must be robust. > > > > Robust as in they may be correct here, but you lose lockdep and all > > other infra normal mutex would give you. > > I know, but __netif_queue_set_napi() requires rtnl_lock() inside the potential > critical section and creates a deadlock this way. However, after reading > patches that introduce this function, I think it is called too early in the > configuration. Seems like it should be called somewhere right after > netif_set_real_num_rx/_tx_queues(), much later in the configuration where we > already hold the rtnl_lock(). In such way, ice_vsi_rebuild() could be protected > with an internal mutex. WDYT? On a quick look I think that may work. For setting the NAPI it makes sense - netif_set_real_num_rx/_tx_queues() and netif_queue_set_napi() both inform netdev about the queue config, so its logical to keep them together. I was worried there may be an inconveniently placed netif_queue_set_napi() call which is clearing the NAPI pointer. But I don't see one. > > > A prettier way of protecting the same critical sections would be replacing > > > ICE_CFG_BUSY around ice_vsi_rebuild() with rtnl_lock(), this would eliminate > > > locking code from .ndo_bpf() altogether, ice_rebuild_pending() logic will have > > > to stay. > > > > > > At some point I have decided to avoid using rtnl_lock(), if I do not have to. I > > > think this is a goal worth pursuing? > > > > Is the reset for failure recovery, rather than reconfiguration? > > If so netif_device_detach() is generally the best way of avoiding > > getting called (I think I mentioned it to someone @intal recently). > > AFAIK, netif_device_detach() does not affect .ndo_bpf() calls. We were trying > such approach with idpf and it does work for ethtool, but not for XDP. I reckon that's an unintentional omission. In theory XDP is "pure software" but if the device is running driver will likely have to touch HW to reconfigure. So, if you're willing, do send a ndo_bpf patch to add a detached check. 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 smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 17B1AC27C6E for ; Thu, 13 Jun 2024 14:13:48 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id B51AF60ECC; Thu, 13 Jun 2024 14:13:48 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id 4i1pUWgKqO5z; Thu, 13 Jun 2024 14:13:48 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=140.211.166.34; helo=ash.osuosl.org; envelope-from=intel-wired-lan-bounces@osuosl.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 0C97760EFC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1718288028; bh=9ImLj4yEnuP3KxiO9Uc2yulYAdCMZJddf0VtYCT8SmA=; h=Date:From:To:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: Cc:From; b=oT6EkFIWk/dstbvtSvGH3j6MWz4mpnV8LHzrKJK5Ptn3/MyNgTcL/B8z7ho3O+PaK lpuuDLTFCtYS4jd3l1t9Jn3ZMCe0hVVDQ17CYvfvgG1nY39Beigj0Sh0f8zFv7JmBr kPmqavV5+IcNnQ1W6v0vP15JGn3xSnq0fVNIQqxRIFLiD/mXG3r8cmigJ0923jet6V DgndxYoJk9BcdbXu3e+JOofMUH6vneM9VFLD+s0BNCGpukEqBBSfBLJHnhPV+0qQ+N 8a319HqvfPws/+h48x2Cs6U/gTdFz5olhdLykMVeGi7B91cG1OXljWM1yeX//5SVbz CMnX7oBaU15mw== Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp3.osuosl.org (Postfix) with ESMTP id 0C97760EFC; Thu, 13 Jun 2024 14:13:48 +0000 (UTC) Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by ash.osuosl.org (Postfix) with ESMTP id DC6A11BF34B for ; Thu, 13 Jun 2024 14:13:46 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id C938E60EFC for ; Thu, 13 Jun 2024 14:13:46 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id Q1CWkLzLeK9E for ; Thu, 13 Jun 2024 14:13:46 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=2604:1380:4641:c500::1; helo=dfw.source.kernel.org; envelope-from=kuba@kernel.org; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp3.osuosl.org D43CE60ECC DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org D43CE60ECC Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by smtp3.osuosl.org (Postfix) with ESMTPS id D43CE60ECC for ; Thu, 13 Jun 2024 14:13:45 +0000 (UTC) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 0994B61797; Thu, 13 Jun 2024 14:13:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0C59BC2BBFC; Thu, 13 Jun 2024 14:13:43 +0000 (UTC) Date: Thu, 13 Jun 2024 07:13:43 -0700 From: Jakub Kicinski To: Larysa Zaremba Message-ID: <20240613071343.019e7dca@kernel.org> In-Reply-To: References: <20240610153716.31493-1-larysa.zaremba@intel.com> <20240611193837.4ffb2401@kernel.org> <20240612140935.54981c49@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1718288024; bh=miLeVMM2w5yAcnT3uzAx4GpVzV/Y9CzVC17jsvEBUfU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ogjDdw34aMYbnVFXO1TQJ6gxcFOElu+kQ1l9kcwCvX/5TJXX6Y46YnhOfBue9+SfZ KpPZTJi+ymmNr/Fe68Gb+Dj57jzeUz4dXcdurC3Y09XSnhkysiQrzzFouyiUXmPG+h 0Kw32Ke9rEiUSd1lqkOMQXvXVpp7YdjaLVT8YQdUjqxJpeUE/hzDrskqDgJmQaAtLh 37Y3FO250POxonskubWav59Lxb9Vj8N2COWpo2ifg80tyiSjgFNLU5YuwuFpltcUAS pavfyYFer7rPxxkOfzw+IPQUsivZ0OlfTrFK3ejk/7EUpbu2SoYv2OJdtoIY5zQygp rp62AoeqnUtCw== X-Mailman-Original-Authentication-Results: smtp3.osuosl.org; dmarc=pass (p=none dis=none) header.from=kernel.org X-Mailman-Original-Authentication-Results: smtp3.osuosl.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=ogjDdw34 Subject: Re: [Intel-wired-lan] [PATCH iwl-net 0/3] ice: fix synchronization between .ndo_bpf() and reset X-BeenThere: intel-wired-lan@osuosl.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Wired Ethernet Linux Kernel Driver Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Maciej Fijalkowski , Jesper Dangaard Brouer , Daniel Borkmann , netdev@vger.kernel.org, John Fastabend , Alexei Starovoitov , Eric Dumazet , Michal Kubiak , Tony Nguyen , magnus.karlsson@intel.com, intel-wired-lan@lists.osuosl.org, bpf@vger.kernel.org, Paolo Abeni , "David S. Miller" , linux-kernel@vger.kernel.org Errors-To: intel-wired-lan-bounces@osuosl.org Sender: "Intel-wired-lan" On Thu, 13 Jun 2024 10:54:12 +0200 Larysa Zaremba wrote: > > > The locking mechanisms I use here do not look pretty, but if I am not missing > > > anything, the synchronization they provide must be robust. > > > > Robust as in they may be correct here, but you lose lockdep and all > > other infra normal mutex would give you. > > I know, but __netif_queue_set_napi() requires rtnl_lock() inside the potential > critical section and creates a deadlock this way. However, after reading > patches that introduce this function, I think it is called too early in the > configuration. Seems like it should be called somewhere right after > netif_set_real_num_rx/_tx_queues(), much later in the configuration where we > already hold the rtnl_lock(). In such way, ice_vsi_rebuild() could be protected > with an internal mutex. WDYT? On a quick look I think that may work. For setting the NAPI it makes sense - netif_set_real_num_rx/_tx_queues() and netif_queue_set_napi() both inform netdev about the queue config, so its logical to keep them together. I was worried there may be an inconveniently placed netif_queue_set_napi() call which is clearing the NAPI pointer. But I don't see one. > > > A prettier way of protecting the same critical sections would be replacing > > > ICE_CFG_BUSY around ice_vsi_rebuild() with rtnl_lock(), this would eliminate > > > locking code from .ndo_bpf() altogether, ice_rebuild_pending() logic will have > > > to stay. > > > > > > At some point I have decided to avoid using rtnl_lock(), if I do not have to. I > > > think this is a goal worth pursuing? > > > > Is the reset for failure recovery, rather than reconfiguration? > > If so netif_device_detach() is generally the best way of avoiding > > getting called (I think I mentioned it to someone @intal recently). > > AFAIK, netif_device_detach() does not affect .ndo_bpf() calls. We were trying > such approach with idpf and it does work for ethtool, but not for XDP. I reckon that's an unintentional omission. In theory XDP is "pure software" but if the device is running driver will likely have to touch HW to reconfigure. So, if you're willing, do send a ndo_bpf patch to add a detached check.