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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 50A82C83F22 for ; Tue, 15 Jul 2025 17:44:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B6B056B009A; Tue, 15 Jul 2025 13:44:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B1B0D6B009B; Tue, 15 Jul 2025 13:44:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A0A4E6B009C; Tue, 15 Jul 2025 13:44:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 8D1266B009A for ; Tue, 15 Jul 2025 13:44:09 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 322C580122 for ; Tue, 15 Jul 2025 17:44:09 +0000 (UTC) X-FDA: 83667222618.01.1DEEB68 Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) by imf25.hostedemail.com (Postfix) with ESMTP id 87AB8A0002 for ; Tue, 15 Jul 2025 17:44:07 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=JAoCSJhR; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf25.hostedemail.com: domain of lossin@kernel.org designates 172.105.4.254 as permitted sender) smtp.mailfrom=lossin@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1752601447; a=rsa-sha256; cv=none; b=dl3+2NyVsLlovzm0Zuptk0zzzJq9ULC/hXC1FOkHOD92mSJYU+3k8Z7wHWN+xIyMw2hDeL ahlWjTTRtjcuJyYLc3vKPtV+PBYX1SekcQW+i7DFrYZ7PhM/JAupNCduF/jFmGAIVS1CID OlbBCYZlhLq2n2hywiWCQy7HhqkLO08= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=JAoCSJhR; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf25.hostedemail.com: domain of lossin@kernel.org designates 172.105.4.254 as permitted sender) smtp.mailfrom=lossin@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1752601447; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=POzmZVd60H4xw2zf0s896/tQr7gIBiE6dHNb5AJwq+o=; b=E7HNL2oftHDLuSJg2sk6jsq48mpXcUJpeTWJIg1rUbx4QMgRHLd6sj9f6tQNXI4/Dj2HMN i5ofrUWX6hRwRoEHs/p6HuTQf94ZfRCRoqO48jd0IbHfR1cr1oc+vBWFn2vBu919VsSJaN lopITuIX1veHwtp2CXtCi5e66SilN6s= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id AD0996145E; Tue, 15 Jul 2025 17:44:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1A1F0C4CEE3; Tue, 15 Jul 2025 17:44:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1752601446; bh=6sfqcOH8puA+a1sDQFn8QveHcgBcHzTz4Z3BgnTRXEE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JAoCSJhRYNhfcNTb74YcPzf+cYSmlo6gS/utgiFRBuTL0zNQXGhaF3/ReLuqe6lQo SE+SXaAP2LKFrAUXvYg6vJ8WNMmdY1lB5j95XvrCjXqQ3F2ca0eaCM1pbFrIAVR8RG 8axgAML+kcWNuGUs871wQSiWxfCs+pC5uep/+e5dbnPydjDYvzT0EVjD6qkc+3j/Wr 76JJA1aTKIy0UVYH9xkK40SrR+OSjErkUaHIu9rCMUCBqCpkfykHxE87DdSklzc1mg tn0pj2DusVlES8ADGV5eZgDtwmx9toMcX7TV1B3w/K0AVdw/rZwPujDQwv5ESqzIRr aWhWTpEdcQsng== Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 15 Jul 2025 19:44:01 +0200 Message-Id: From: "Benno Lossin" To: "Boqun Feng" Cc: "Mitchell Levy" , "Miguel Ojeda" , "Alex Gaynor" , "Gary Guo" , =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= , "Andreas Hindborg" , "Alice Ryhl" , "Trevor Gross" , "Andrew Morton" , "Dennis Zhou" , "Tejun Heo" , "Christoph Lameter" , "Danilo Krummrich" , , , Subject: Re: [PATCH v2 3/5] rust: percpu: add a rust per-CPU variable test X-Mailer: aerc 0.20.1 References: <20250712-rust-percpu-v2-0-826f2567521b@gmail.com> <20250712-rust-percpu-v2-3-826f2567521b@gmail.com> <68762e19.170a0220.33e203.a0b7@mx.google.com> In-Reply-To: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 87AB8A0002 X-Stat-Signature: 1i3ekgtfubwaz3unhc4x7zgm781pkrqp X-Rspam-User: X-HE-Tag: 1752601447-349678 X-HE-Meta: U2FsdGVkX18+xd6DNWOZPTlhTPdFh6yoN4tgCZD7sQIeBfbiq/vENjWlOFQ6Ks/qr+PCpDK850uDqvWklDvTjVm7+xR14QkcljUjSEzp0jLlv0wLwkBzokmokGtVnNwUbF3Zku1d9iDYyCtWnbyxku4nGjLq5jHtErfTuLqtDCDXpqlyafVVLHjCvO+nGAy/k/qlsqPXZZxUWgwh7qdBy9LcDNMaRyhUJoPnyi9GXlkz1l7k66OKoRF/KhTAFFPqGXjgDnzzBKcxKolFricdc9bkbSiQi188ZgvpPMlGl1tjmei8pvUX3sDySHkhTi5Y1oN6FpwMhnyGtnI+bSjbeBtCI7fQRPRQxM4TaBi6oPU+yes5D0wmZuorHaOZ7WsRUsgwRYXYzOQlorujkogoTuZe/Qcce+Zsg2QsSxlT0VlrmjhHwPRn+f6GjT5vAfSe4vKSKXyx6UMpcgVpY86w1y42YO3yQhozgzpBLPmikfiHCyvXWWNzTakdN/FCoNDjzAIU6a/nN0becY28Q4wOnxvUWAH996vxBiDsUijfntIWzUnBQGTftf3noMgJeDdmOfaBu5/t1trY+m/5ZSuCKs4E6fXmXhjuz5KUUCsEiN6hMn1Te1lTzzOJ+fBPH4LSd6Yt3NyGjpGcglQXUv7ITCcPKSeL75sJnqYNTbyx5LBWXypknIPoVPSlSdmppR9TA3eNx4jhmlgiJ2AQUDNnosNqrlsmgbaIQIeS1nXgx7Ql8/IpBZLCSe7oneMeJHV/XXvY7/cOwB2H/KV76WiIZgrCVIFRuK94/mtv4KpnMT4xlgaXmP5amaROWtGsTTTCVwNLAJcH1mfCWPQ4tgMoLVr1dwmhidP0r5tfxY3Y3Ke9sFXXy0G6Rf0nC8JDXE50K/yAXomDr7B+v4yvcapKAXnwoOJsxyW0KzHxeZr17IWQJgNLP3EvJTH2ypmJEgZuX/hIn0EHWAGd+unkAPg igvjBEAn 51r1iJUzZf9v8BAj5JMbYFPjw3yTqjEMy/bhY70O+9NH+gQEwUF28CoFx0JwHyZq25JBp770MXjDwN9g0nYJo9ou/ci06TNzPCavc4o+ajAH3uKUEGOcfULqMDZy9/xTqh5XSps84fnH6GPQYcDG0DcqlM8+ClTc3pbWTgCBXs3xxwT7eEGp+BZyGorz8Hq7ft5jN5in833/1LJorhFW0jEdOW77OB3qE0NApIx7ye/rXUJ+vKHjPIzvn55qXG+wsVMYvqikOww3OvTyZtxXlOcFHNopFbjugJJn6S1+8AIxzauFUO8KNVSM7H0PYcVM0eMZBbWE4Sx0keQ2zeMz0+CGhdC8t24yG64HUdLiWnToU3uNBuoLDU2LHyL+FlWWq/1TMlBJIEECigTtSenF1CR7lNCegsWfWx3Ta0EfgNKgXDmR5bJm5TxHyJrnsBsLUmWj7EzbxPX/3DuE7sO6QugtCbmwsELzakVnd5RgUPQf4GskRDjMeOgeNkOuW1Eiygf3t X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Tue Jul 15, 2025 at 6:31 PM CEST, Boqun Feng wrote: > On Tue, Jul 15, 2025 at 05:55:13PM +0200, Benno Lossin wrote: >> On Tue Jul 15, 2025 at 4:10 PM CEST, Boqun Feng wrote: >> > On Tue, Jul 15, 2025 at 01:31:06PM +0200, Benno Lossin wrote: >> > [...] >> >> >> > +impl kernel::Module for PerCpuTestModule { >> >> >> > + fn init(_module: &'static ThisModule) -> Result { >> >> >> > + pr_info!("rust percpu test start\n"); >> >> >> > + >> >> >> > + let mut native: i64 =3D 0; >> >> >> > + // SAFETY: PERCPU is properly defined >> >> >> > + let mut pcpu: StaticPerCpu =3D unsafe { unsafe_get= _per_cpu!(PERCPU) }; >> >> >>=20 >> >> >> I don't understand why we need unsafe here, can't we just create >> >> >> something specially in the `define_per_cpu` macro that is then con= firmed >> >> >> by the `get_per_cpu!` macro and thus it can be safe? >> >> > >> >> > As is, something like >> >> > define_per_cpu!(PERCPU: i32 =3D 0); >> >> > >> >> > fn func() { >> >> > let mut pcpu: StaticPerCpu =3D unsafe { unsafe_get_per= _cpu!(PERCPU) }; >> >> > } >> >> > will compile, but any usage of `pcpu` will be UB. This is because >> >> > `unsafe_get_per_cpu!` is just blindly casting pointers and, as far = as I >> >> > know, the compiler does not do any checking of pointer casts. If yo= u >> >> > have thoughts/ideas on how to get around this problem, I'd certainl= y >> >> > *like* to provide a safe API here :) >> >>=20 >> >> I haven't taken a look at your implementation, but you do have the ty= pe >> >> declared in `define_per_cpu!`, so it's a bit of a mystery to me why y= ou >> >> can't get that out in `unsafe_get_per_cpu!`... >> >>=20 >> >> Maybe in a few weeks I'll be able to take a closer look. >> >>=20 >> >> >> > + // SAFETY: We only have one PerCpu that points at PERCP= U >> >> >> > + unsafe { pcpu.get(CpuGuard::new()) }.with(|val: &mut i6= 4| { >> >> >>=20 >> >> >> Hmm I also don't like the unsafe part here... >> >> >>=20 >> >> >> Can't we use the same API that `thread_local!` in the standard lib= rary >> > >> > First of all, `thread_local!` has to be implemented by some sys-specif= ic >> > unsafe mechanism, right? For example on unix, I think it's using >> > pthread_key_t: >> > >> > https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key= _create.html >> > >> > what we are implementing (or wrapping) is the very basic unsafe >> > mechanism for percpu here. Surely we can explore the design for a safe >> > API, but the unsafe mechanism is probably necessary to look into at >> > first. >>=20 >> But this is intended to be used by drivers, right? If so, then we should > > Not necessarily only for drivers, we can also use it for implementing > other safe abstraction (e.g. hazard pointers, percpu counters etc) That's fair, but then it should be `pub(crate)`. >> do our usual due diligence and work out a safe abstraction. Only fall >> back to unsafe if it isn't possible. >>=20 > > All I'm saying is instead of figuring out a safe abstraction at first, > we should probably focus on identifying how to implement it and which > part is really unsafe and the safety requirement for that. Yeah. But then we should do that before merging :) >> I'm not familiar with percpu, but from the name I assumed that it's >> "just a variable for each cpu" so similar to `thread_local!`, but it's >> bound to the specific cpu instead of the thread. >>=20 >> That in my mind should be rather easy to support in Rust at least with >> the thread_local-style API. You just need to ensure that no reference >> can escape the cpu, so we can make it `!Send` & `!Sync` + rely on klint > > Not really, in kernel, we have plenty of use cases that we read the > other CPU's percpu variables. For example, each CPU keeps it's own > counter and we sum them other in another CPU. But then you need some sort of synchronization? > If we would like to model it conceptually, it's more like an array > that's index by CpuId to me. Gotcha, but this model is missing the access control/synchronization. So I'm not so sure how useful it is. (I think I asked this somewhere else, but the number of CPUs doesn't change, right?) >> to detect context switches. >>=20 >> >> >> has: >> >> >>=20 >> >> >> https://doc.rust-lang.org/std/macro.thread_local.html >> >> >>=20 >> >> >> So in this example you would store a `Cell` instead. >> >> >>=20 >> >> >> I'm not familiar with per CPU variables, but if you're usually sto= ring >> >> >> `Copy` types, then this is much better wrt not having unsafe code >> >> >> everywhere. >> >> >>=20 >> >> >> If one also often stores `!Copy` types, then we might be able to g= et >> >> >> away with `RefCell`, but that's a small runtime overhead -- which = is >> >> >> probably bad given that per cpu variables are most likely used for >> >> >> performance reasons? In that case the user might just need to stor= e >> >> >> `UnsafeCell` and use unsafe regardless. (or we invent something >> > >> > This sounds reasonable to me. >> > >> >> >> specifically for that case, eg tokens that are statically known to= be >> >> >> unique etc) >> >> > >> >> > I'm open to including a specialization for `T: Copy` in a similar v= ein >> >> > to what I have here for numeric types. Off the top of my head, that >> >> > shouldn't require any user-facing `unsafe`. But yes, I believe ther= e is >> >> > a significant amount of interest in having `!Copy` per-CPU variable= s. >> >> > (At least, I'm interested in having them around for experimenting w= ith >> >> > using Rust for HV drivers.) >> >>=20 >> >> What kinds of types would you like to store? Allocations? Just intege= rs >> >> in bigger structs? Mutexes? >> >>=20 >> > >> > In the VMBus driver, there is a percpu work_struct. >>=20 >> Do you have a link? Or better yet a Rust struct description of what you >> think it will look like :) >>=20 > > Not Rust code yet, but here is the corresponding C code: > > https://github.com/Rust-for-Linux/linux/blob/rust-next/drivers/hv/vmbus_= drv.c#L1396 Thanks! > But please note that we are not solely developing the abstraction for > this usage, but more for generally understand how to wrap percpu > functionality similar to the usage in C. Well, I have to start somewhere for looking at the use-cases :) If you have more, just let me see. (probably won't have enough time to look at them now, but maybe in a couple weeks) >> >> > I would definitely like to avoid *requiring* the use of `RefCell` s= ince, >> >> > as you mention, it does have a runtime overhead. Per-CPU variables = can >> >> > be used for "logical" reasons rather than just as a performance >> >> > optimization, so there might be some cases where paying the runtime >> >> > overhead is ok. But that's certainly not true in all cases. That sa= id, >> >> > perhaps there could be a safely obtainable token type that only pas= ses a >> >> > `&T` (rather than a `&mut T`) to its closure, and then if a user do= esn't >> >> > mind the runtime overhead, they can choose `T` to be a `RefCell`. >> >> > Thoughts? >> >>=20 >> >> So I think using an API similar to `thread_local!` will allow us to h= ave >> >> multiple other APIs that slot into that. `Cell` for `T: Copy`, >> >> `RefCell` for cases where you don't care about the runtime overhea= d, >> >> plain `T` for cases where you only need `&T`. For the case where you >> >> need `&mut T`, we could have something like a `TokenCell` that giv= es >> >> out a token that you need to mutably borrow in order to get `&mut T`. >> >> Finally for anything else that is too restricted by this, users can a= lso >> >> use `UnsafeCell` although that requires `unsafe`. >> >>=20 >> >> I think the advantage of this is that the common cases are all safe a= nd >> >> very idiomatic. In the current design, you *always* have to use unsaf= e. >> >>=20 >> > >> > I agree, but like I said, we need to figure out the unsafe interface >> > that C already uses and build API upon it. I think focusing on the >> > unsafe mechanism may be the way to start: you cannot implement somethi= ng >> > that cannot be implemented, and we don't have the magic pthread_key he= re >> > ;-) >>=20 >> Sure we can do some experimentation, but I don't think we should put >> unsafe abstractions upstream that we intend to replace with a safe >> abstraction later. Otherwise people are going to depend on it and it's > > I doubt we can replace the unsafe abstraction with a safe one, if users > really care the performance then they would really need to use some > unsafe API to build their safe abstraction. That sounds pretty pessimistic, why do you think that? >> going to be a mess. Do the experimenting out of tree and learn there. > > I disagree, Rust as a language its own should be able to do what C does > including being able to implement the percpu functionality same as C, > there is nothing wrong with a set of Rust primitives in the kernel that > provides fundamental percpu functionality the other core facilities can > rely on. The better part is that it will have all the safety requirement > documented well. Sure, but we haven't even tried to make it safe, so I don't think we should add them now in this state. --- Cheers, Benno