Received-SPF: pass (google.com: domain of linux-kernel+bounces-213552-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 13 Jun 2024 17:36:16 +0200
From: Larysa Zaremba <larysa.zaremba@intel.com>
To: Jakub Kicinski <kuba@kernel.org>
CC: <intel-wired-lan@lists.osuosl.org>, Jesse Brandeburg
	<jesse.brandeburg@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>,
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
	Paolo Abeni <pabeni@redhat.com>, Alexei Starovoitov <ast@kernel.org>, "Daniel
 Borkmann" <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>, Maciej Fijalkowski
	<maciej.fijalkowski@intel.com>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>,
	<magnus.karlsson@intel.com>, Michal Kubiak <michal.kubiak@intel.com>
Subject: Re: [PATCH iwl-net 0/3] ice: fix synchronization between .ndo_bpf()
 and reset
Message-ID: <ZmsR8F9GFgxgBXfV@lzaremba-mobl.ger.corp.intel.com>
References: <20240610153716.31493-1-larysa.zaremba@intel.com>
 <20240611193837.4ffb2401@kernel.org>
 <ZmlGppe04yuGHvPx@lzaremba-mobl.ger.corp.intel.com>
 <20240612140935.54981c49@kernel.org>
 <ZmqztPo6UDIC6gKx@lzaremba-mobl.ger.corp.intel.com>
 <20240613071343.019e7dca@kernel.org>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240613071343.019e7dca@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?eyfcm9ViYICJwCXqe3UHmmkgwbqi78s0MUEmS015D/lQDen7IISy+FYRjk5+?=
 =?us-ascii?Q?5S5Fa4VPAs37DzVsuvk+OeeztigIRi1w/sh1el1pW287pb0y88NFpIvxOLJO?=
 =?us-ascii?Q?UptOzo45jAxciyWrq35uQ2gc9H4dwMvpZFrzHtG5ibcAMuyhPGljrmMzPu5R?=
 =?us-ascii?Q?5Il8X7EJz/vCNbeIhi+kcaUYhe/RVZEmm01cUe3q/dc/IeWlMZpXavuby2Vm?=
 =?us-ascii?Q?cuQ75oOB5S0WI7mLoLw+5g3/5Zzp6wtkV2e9J7Q6vVhNk51OreZX01zHvkul?=
 =?us-ascii?Q?O9449Fq6QjFoId0lmwoXkD45vL7xKSUqj7jP87nLUpWZPp65mGtJaFtW+RRm?=
 =?us-ascii?Q?CMixqRggVl35nuhWxzschJ6depzZomvK7LCYkvhvyPREXOuP2f1/wzd8J2xz?=
 =?us-ascii?Q?eSiRvQRYwasPA/VZYb0vEidqq+T4zHa1c9/j87mSvls31EVLTIJRelClYLR1?=
 =?us-ascii?Q?uIb5y4DB4d3oYVr7uzgYbDnn8ptZgalCcsPWmyS5arH32+hdfo/kY27bKGHB?=
 =?us-ascii?Q?gbssqZByNrnATQ5Vq7X3ZJuJbjN/4ATflbo4jn4n24sySKgTl1NYyA6wBosp?=
 =?us-ascii?Q?mYtV7RVJP4Y160AUd5efGpgTPoU+zrWxfSC6uJKrbbhNcuXdRfaTJacUiGMF?=
 =?us-ascii?Q?wJVW3/8oFuNr2fjuvUflA9joPuF+0TAOG52OMG13S2RsUdGU36kwj8HvjRZR?=
 =?us-ascii?Q?D4hXwVzKmhy3kZhdQ1EENWhQGYEbqEfUHP+4fXO1mYobiriMyFi55N2EgcCS?=
 =?us-ascii?Q?6xFwEU0R25Hhqh4cs/2scpcP0Ch4mUAH5+1IjWU3DvLW948J5+ZWQWJIccw6?=
 =?us-ascii?Q?lgdo4ZVOxhx6Ou7ewkouY42F+PLK33NHztY8l/Hr9QuiK6ZbagnWPPvYYW6Y?=
 =?us-ascii?Q?AeoxT/ejIv/IjHdFOmvDUIZU7zZLDkrGfRWcewYeQAJIhhi09ZQcvun8m1YV?=
 =?us-ascii?Q?pqA+gWC5ATXfgqnjMtxHbrYJXvzBte3626HOuKKjUMOMsQO3Jcrxvc6D39PF?=
 =?us-ascii?Q?dlmsKvrfIxoxuQlM5axgKOV3g7BqQ0/NhPpM6f1XEMofp4M0QHEGhuMha2NX?=
 =?us-ascii?Q?Cz744pTGVzjrAzhIM/sO5BEe/P160qXz5n5aKooA+qCBcGUfOMrCt1NxwnI0?=
 =?us-ascii?Q?f0cp6AVAO0DvhAIoUypKoLCa0kpOqgczqLqKC0x2sGeYEmCobMHAUUrVrw1Y?=
 =?us-ascii?Q?Ckc3jFj7lwLqAUq87nkINHMA43k+6pQe9iFqXTcDe/4gmLYDijrXRMB00Chq?=
 =?us-ascii?Q?YrayZamw/xp1ttuEzuPNtnwxAa+AvsbrA9DcD6GXLHVcxECmb3eONTrBXOCV?=
 =?us-ascii?Q?QwI3Jl7F465CCrp8BNP+gxQNdk/q6VBLAZsqvy05lhYHHRUHeO2kXm7q1tEE?=
 =?us-ascii?Q?3QoMBLauxCntiNtoUDdhx5aNl8ln3pzpKgjdsT9AMibuEk8pIqnRzyArnqgC?=
 =?us-ascii?Q?RxtNqSdVn6T+XtBrBJZRAmCscVFpBXQZru1AXgsvYm1pmrXgFR5ER/zmcpU1?=
 =?us-ascii?Q?MBm10Z/mwIeM8dyW/eurgwFU63NXbGETjKEmJ2CysK/PE9YqsQ0IERL2SQRZ?=
 =?us-ascii?Q?9kshLagF0NT1uq2SCrhEIRN9IChy+Lfip+9HDUQGTbyeOsGe2qG52hm491gV?=
 =?us-ascii?Q?Bg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 262b516d-8e4d-4978-7c71-08dc8bbe95f0
X-MS-Exchange-CrossTenant-AuthSource: SN7PR11MB7540.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 15:36:25.7235
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CoBxkWFm8eUdNYVGQwIUAVgzqprQbX3CxRxXZaEp48z6axxj8ExIXj9Ne8qWtYiKTPooG15Qtauwl/t6bVMW2N8XQBc+Aplaz4aRLANCdOw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4704
X-OriginatorOrg: intel.com

On Thu, Jun 13, 2024 at 07:13:43AM -0700, Jakub Kicinski wrote:
> 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.
>

Ok, will do this in v2. Thanks for the discussion.
 
> > > > 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.

This does not seem that simple. In cases of program/pool detachment, 
.ndo_bpf() does not accept 'no' as an answer, so there is no easy existing way 
of handling !netif_device_present() either. And we have to notify the driver 
that pool/program is no longer needed no matter what. So what is left is somehow 
postpone pool/prog removal to after the netdev gets attached again.