From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 918CD26AD9 for ; Wed, 5 Mar 2025 07:30:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741159841; cv=none; b=nj24JYNzENewQ5CBFkPjjpGx8OO05t224NfhDvdCwvyygheK8HXtdPbqt2qiLQu1J6Vr908dl5IBQ8NPUHDtc23WNNgTh0pOSa866E4Qa2idte7AGxpjo7gsQPS6DBE5WCqwVu8kLLNKjv6CtaNxLunioYaF4VrhaM4nMfEyQEk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741159841; c=relaxed/simple; bh=E9d/ZYRDweWef9J6nm+N37n6i90JPaipsMFkzvOxEHw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=cMx6vM9L2gYPnp0kTbjM1AzV39JNr+w6WBOE1wx8nWzdajWAuxMMW9fwg8r2ROwr3rnx0Sye77PxRnDtcl7digCciFB5AGpjUzoxe4/Ap4JO2jQn4otV+vvv7hWszIDjAMbR1iG4ytaGfTJo5UhPei9ogVvuijW6InNOYS4iDPs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch; spf=none smtp.mailfrom=ffwll.ch; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b=jRTO0s8y; arc=none smtp.client-ip=209.85.128.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="jRTO0s8y" Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-438a39e659cso43586985e9.2 for ; Tue, 04 Mar 2025 23:30:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; t=1741159838; x=1741764638; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=6eIPjoywVHzslmjO8vs+eTHuxI6qCJTL5FwNFg9NfBw=; b=jRTO0s8yBID2JL6Q4Ibt7Q6zqZv72n+v6fKAaEP9vsvcRdv302j81GRZfFA+NPMhGX NCbYYSbxnZSmPL6VRdHgqgxuIXEvyImCfhQz6hV95IKiyy3FJuRuCTyXxKKDVUqR0OVX JEtEM7smY6vqKMfGmZYjODH5b4UbIDtoltkk0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741159838; x=1741764638; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=6eIPjoywVHzslmjO8vs+eTHuxI6qCJTL5FwNFg9NfBw=; b=p2Kuo5qlgQKfWc+njRqv16X+xK6jtvwIJpTTF10cHXAQLjvlD3Rt5BUQKS+owcbGKV XqbSur5kq4uh6gAjGJjsUpVYqu6tS7420j6ZsW/3f21Oxh7Tpg6RfZ9IaQXEN9lJO764 MI83/69K7YSsb56nVjaeQnTQLh1uvMTB3TgPnXFtpZPsQBgS7KGIOvCUbflJ7TDfLyJu VdqNv8U7SQcYrrruF+U1dXW1TV2ARJTP9WAWXVcIMvsBLGYfdnZvkVZTzQZV2+jsKOFJ czOPA65a2x+VBwNXtpNY8sLsPMrYfT6n1Jw7dHTh5/PzGhnhRUsL9G0T2p2zHxKh+tUC ijzA== X-Forwarded-Encrypted: i=1; AJvYcCWJmScF8MVRxkXR2zGN6HMfgvq+w5rMVDhnUModGatNrZHL9tlTSBTO4p47Kfd2yNyPu86pScq/FheeXfQbnw==@vger.kernel.org X-Gm-Message-State: AOJu0YwBQa6tbf9tSxkhXBUyBJezhyfteQrrfT6qUm4uGRb7tRdT6Jg4 j/wLnYHbvKV+DgODa5sRm01p4FRp19jtRWea2piNChc9n/7ebqLCb8pisAebfJA= X-Gm-Gg: ASbGnctcgxvbcCPBIz5YcjYGkPN46mLfmg8+Ab5oHzcwHZ0wei2Xl2U+nMPT7ZWWg4h DPlVl0B5zug4Qmub/JoVlJX1CARYhKTlKY3QohWXGHwzqZehA6xTeBOYvkwSvg34AzcRXP0tm96 aa9YEZY6o9tRRBePaPQdWAC/NHECUbBWVXHGllRIt01BxggI7N+XB4Nl5BIuBxl9wxmFvRuCF7l E558xWT2CK6DZzqjwDfUmki6zSaQwNBdtT9awUp9mrTS3CbZylQqTj0MpX5SA8XtMeziWtERpXa 0RI1Ho2GBj9+9ssUwaCcxLrwYNit+P4HuG7X45WaXyqyoLAj+Ox3GVPu X-Google-Smtp-Source: AGHT+IGLEHkW+RyaKKz2KrNndnwh4djhORTNBKpw376Kno8bWHtJQuS7EruIxE2wip3vv6xuWjSI3w== X-Received: by 2002:a05:600c:1d0f:b0:43b:d203:da18 with SMTP id 5b1f17b1804b1-43bd295494bmr11723085e9.13.1741159837590; Tue, 04 Mar 2025 23:30:37 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:5485:d4b2:c087:b497]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-43bd426cbc2sm8898505e9.4.2025.03.04.23.30.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Mar 2025 23:30:37 -0800 (PST) Date: Wed, 5 Mar 2025 08:30:34 +0100 From: Simona Vetter To: Jason Gunthorpe Cc: John Hubbard , Greg KH , Danilo Krummrich , Joel Fernandes , Alexandre Courbot , Dave Airlie , Gary Guo , Joel Fernandes , Boqun Feng , Ben Skeggs , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, paulmck@kernel.org Subject: Re: [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation Message-ID: Mail-Followup-To: Jason Gunthorpe , John Hubbard , Greg KH , Danilo Krummrich , Joel Fernandes , Alexandre Courbot , Dave Airlie , Gary Guo , Joel Fernandes , Boqun Feng , Ben Skeggs , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, paulmck@kernel.org References: <20250226172120.GD28425@nvidia.com> <20250226234730.GC39591@nvidia.com> <2025022644-fleshed-petite-a944@gregkh> <20250228184013.GF39591@nvidia.com> <20250304164201.GN133783@nvidia.com> Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250304164201.GN133783@nvidia.com> X-Operating-System: Linux phenom 6.12.11-amd64 On Tue, Mar 04, 2025 at 12:42:01PM -0400, Jason Gunthorpe wrote: > On Tue, Mar 04, 2025 at 05:10:45PM +0100, Simona Vetter wrote: > > On Fri, Feb 28, 2025 at 02:40:13PM -0400, Jason Gunthorpe wrote: > > > On Fri, Feb 28, 2025 at 11:52:57AM +0100, Simona Vetter wrote: > > > > > > > - Nuke the driver binding manually through sysfs with the unbind files. > > > > - Nuke all userspace that might beholding files and other resources open. > > > > - At this point the module refcount should be zero and you can unload it. > > > > > > > > Except developers really don't like the manual unbind step, and so we're > > > > missing try_module_get() in a bunch of places where it really should be. > > > > > > IMHO they are not missing, we just have a general rule that if a > > > cleanup function, required to be called prior to module exit, revokes > > > any .text pointers then you don't need to hold the module refcount. > > > > > > file_operations doesn't have such a cleanup function which is why it > > > takes the refcount. > > > > > > hrtimer does have such a function which is why it doesn't take the > > > refcount. > > > > I was talking about a bunch of other places, where it works like > > file_operations, except we don't bother with the module reference count. > > I've seen patches fly by where people "fix" these things because module > > unload is "broken". > > Sure, but there are only two correct API approaches, either you > require the user to make a cancel call that sanitizes the module > references, or you manage them internally. > > Hope and pray isn't an option :) > > > gpu drivers can hog console_lock (yes we're trying to get away from that > > as much as possible), at that point a cavalier attitude of "you can just > > wait" isn't very appreciated. > > What are you trying to solve here? If the system is already stuck > infinitely on the console lock why is module remove even being > considered? > > module remove shouldn't be a remedy for a crashed driver... I mean hotunplug here, and trying to make that correct. This confusion is is why this is so hard, because there's really two main users for all this: - developers who want to quickly test new driver versions without full reboot. They're often preferring convenience over correctness, like with the removal of module refcounting that's strictly needed but means they first have to unbind drivers in sysfs before they can unload the driver. Another one is that this use-case prefers that the hw is cleanly shut down, so that you can actually load the new driver from a well-known state. And it's entirely ok if this all fails occasionally, it's just for development and testing. - hotunplug as an actual use-case. Bugs are not ok. The hw can go away at any moment. And it might happen while you're holding console_lock. You generally do not remove the actual module here, which is why for the actual production use-case getting that part right isn't really required. But getting the lifetimes of all the various structs/objects/resources perfectly right is required. So the "stuck on console_lock" is the 2nd case, not the first. Module unload doesn't even come into play on that one. > > > But so is half removing the driver while it is doing *anything* and > > > trying to mitigate that with a different kind of hard to do locking > > > fix. *shrug* > > > > The thing is that rust helps you enormously with implementing revocable > > resources and making sure you're not cheating with all the bail-out paths. > > Assuming a half alive driver with MMIO and interrupts ripped away > doesn't lock up. Rust's drop takes care of that for you. It's not guaranteed, but it's a case of "the minimal amount of typing yields correct code", unlike C, where that just blows up for sure. > Assuming all your interrupt triggered sleeps have gained a shootdown > mechanism. Hence why I want revocable to only be rcu, not srcu. > Assuming all the new extra error paths this creates don't corrupt the > internal state of the driver and cause it to lockup. Yeah this one is a bit scary. Corrupting the state is doable, locking up is much less likely I think, it seems to be more leaks that you get if rust goes wrong. > Meh. It doesn't seem like such an obvious win to me. Personally I'm > terrified of the idea of a zombie driver half sitting around in a > totally untestable configuration working properly.. Yeah agreed. I might really badly regret this all. But I'm not sold that switching to message passing design is really going to be better, while it's definitely going to be a huge amount of work. > > It cannot help you with making sure you have interruptible/abortable > > sleeps in all the right places. > > :( > > > > Like, I see a THIS_MODULE in driver->fops == amdgpu_driver_kms_fops ? > > > > Yeah it's there, except only for the userspace references and not for the > > kernel internal ones. Because developers get a bit prickle about adding > > those unfortunately due to "it breaks module unload". Maybe we just should > > add them, at least for rust. > > Yeah, I think such obviously wrong things should be pushed back > against. We don't want EAF bugs in the kernel, we want security... Maybe the two different use-cases above help explain why I'm a bit more pragmatic here. As long as the hotunplug case does not gain bugs (or gets some fixed) I'm fairly lax with hacks for the driver developer use-case of reloading modules. > > You've missed the "it will upset developers part". I've seen people remove > > module references that are needed, to "fix" driver unloading. > > When done properly the module can be unloaded. Most rdma driver > modules are unloadable, live, while FDs are open. > > > The third part is that I'm not aware of anything in rust that would > > guarantee that the function pointer and the module reference actually > > belong to each another. Which means another runtime check most likely, and > > hence another thing that shouldn't fail which kinda can now. > > I suspect it has to come from the C code API contracts, which leak > into the binding design. > > If the C API handles module refcounting internally then rust is fine > so long as it enforces THIS_MODULE. You could do contrived stuff and pass function pointers around, so that THIS_MODULE doesn't actually match up with the function pointer. Sure it's really stupid, but the idea with rust is that for memory safety stuff like this, it's not just stupid, but impossible and the compiler will catch you. So we need a tad more for rust. > If the C API requires cancel then rust is fine so long as the binding > guarantees cancel before module unload. Yeah this is again where I think rust needs a bit more, because the compiler can't always nicely proof this for you in all the "obvious" cases. -Sima -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch