| Message ID | db0166341ce824c157d0c58c240b3efc6aec6f6e.1738832118.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
On 10-02-25, 19:02, Yury Norov wrote: > On Thu, Feb 06, 2025 at 02:58:25PM +0530, Viresh Kumar wrote: > > diff --git a/MAINTAINERS b/MAINTAINERS > > index ee6709599df5..bfc1bf2ebd77 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -4021,6 +4021,7 @@ F: lib/cpumask_kunit.c > > F: lib/find_bit.c > > F: lib/find_bit_benchmark.c > > F: lib/test_bitmap.c > > +F: rust/helpers/cpumask.c > > Sorry what? I never committed to maintain this thing, and will > definitely not do it for free. > > NAK. Okay. I will add a separate entry for Rust cpumask stuff. > > +#ifndef CONFIG_CPUMASK_OFFSTACK > > +void rust_helper_free_cpumask_var(cpumask_var_t mask) > > +{ > > + free_cpumask_var(mask); > > +} > > +#endif > > This is most likely wrong because free_cpumask_var() is declared > unconditionally, and I suspect the rust helper should be as well. Non-trivial C macros and inlined C functions cannot be used directly in the Rust code and are used via functions ("helpers") that wrap those so that they can be called from Rust. The free_cpumask_var() function is defined as inline only for the CONFIG_CPUMASK_OFFSTACK=n case and that's where we need this wrapper. For the other case (CONFIG_CPUMASK_OFFSTACK=y), Rust code can directly call free_cpumask_var() from the C code and we don't need this helper. > Please for the next iteration keep me CCed for the whole series. I > want to make sure you'll not make me a rust maintainer accidentally. Sure.
On Tue, Feb 11, 2025 at 11:24:55AM -0500, Yury Norov wrote: > I'm particularly concerned with the following comment from Jason > Gunthorpe: > > All PRs to Linus must not break the rust build and the responsibilty > for that falls to all the maintainers. If the Rust team is not quick > enough to resolve any issues during the development window then > patches must be dropped before sending PRs, or Linus will refuse the > PR. > > https://lore.kernel.org/lkml/20250130154646.GA2298732@nvidia.com/ > > And that happened at least once, right? Yes, 6 patches were dropped from the last merge window by Andrew and Linus because of rust build breakage. There has since been some clarification posted: https://lore.kernel.org/all/CANiq72m-R0tOakf=j7BZ78jDHdy=9-fvZbAT8j91Je2Bxy0sFg@mail.gmail.com/ Quoting: Who is responsible if a C change breaks a build with Rust enabled? The usual kernel policy applies. So, by default, changes should not be introduced if they are known to break the build, including Rust. However, exceptionally, for Rust, a subsystem may allow to temporarily break Rust code. The intention is to facilitate friendly adoption of Rust in a subsystem without introducing a burden to existing maintainers who may be working on urgent fixes for the C side. The breakage should nevertheless be fixed as soon as possible, ideally before the breakage reaches Linus. For instance, this approach was chosen by the block laye they called it "stage 1" in their Rust integration plan. We believe this approach is reasonable as long as the kernel does not have way too many subsystems doing that (because otherwise it would be very hard to build e.g. linux-next). However, it is unclear how this "temporarily break Rust code" will work in practice. We do not know under what conditions Linus will accept a PR that forces him to go to CONFIG_RUST=n, or even if Linus has agreed to this exception policy. I suggest the safe thing for any maintainer is to not send Linus PRs that break rust, otherwise they get to be a test case to see what happens.. Jason
Hi Yury, On Tue, Feb 11, 2025 at 11:24:55AM -0500, Yury Norov wrote: > + Jason, Christoph > > On Tue, Feb 11, 2025 at 09:59:08AM +0530, Viresh Kumar wrote: > > On 10-02-25, 19:02, Yury Norov wrote: > > > On Thu, Feb 06, 2025 at 02:58:25PM +0530, Viresh Kumar wrote: > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > index ee6709599df5..bfc1bf2ebd77 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -4021,6 +4021,7 @@ F: lib/cpumask_kunit.c > > > > F: lib/find_bit.c > > > > F: lib/find_bit_benchmark.c > > > > F: lib/test_bitmap.c > > > > +F: rust/helpers/cpumask.c > > > > > > Sorry what? I never committed to maintain this thing, and will > > > definitely not do it for free. > > > > > > NAK. > > > > Okay. > > No, not Okay. > > To begin with, this is the 8th version of the same patch, but you > only now bothered to CC someone who is listed in MAINTAINERS. This is > not how the community works. I'm answering, since I was involved in the discussion you refer to below, but please also let me add a few other thoughts from my side. First of all, I think you are right here. AFAICS, the cpumask abstraction was added to this series in v6, and you were CC'd in v8, which is *not* OK. I also agree that this definitely deserves it's own series for you to be properly involved. > > I will add a separate entry for Rust cpumask stuff. > > That would make things even worse. Before you wanted me to maintain > rust linkage. Now you want me to get approval from someone else who > maintains rust linkage. In case I need to change something, I want to > be able to just change. I think the decision is up to you, whether 1) You want to maintain it on your own. 2) You want a co-maintainer / reviewer that takes care of the Rust parts. 3) You want nothing to do with it and have it maintained as a separate component. In case you prefer option 3), please do *not* see it as "you need to get approval from someone else for code changes in your subsystem", because no ones wants to impose this on you. Please see it as just another driver or another component that makes use of the API you maintain. If you are running into API changes that affect the Rust abstraction, that's where you can refer to the maintainer of the Rust abstraction to help out. Just like with every other driver or component that uses a core API, which isn't trivial to adjust. > > I went deeper into the subject, and found that rust team has similar > problems with other maintainers: > > https://lore.kernel.org/lkml/20250108122825.136021-3-abdiel.janulgue@gmail.com/ I don't think that this case is similar to the one you linked in. I think you were indeed a bit bypassed here, plus you seem to have a real concern with maintainership, which I think is fair to be addressed and resolved. I hope my reply already helps with that. - Danilo
On Tue, Feb 11, 2025 at 5:24 PM Yury Norov <yury.norov@gmail.com> wrote: > > No, not Okay. > > To begin with, this is the 8th version of the same patch, but you > only now bothered to CC someone who is listed in MAINTAINERS. This is > not how the community works. Yeah, that is not good. For what it is worth, we try to make this very clear: https://rust-for-linux.com/contributing#submitting-new-abstractions-and-modules i.e. that maintainers need to be Cc'd and contacted as soon as possible (possibly even before writing the code). > You also made it a patch bomb that touches multiple critical and very > sensitive subsystems. You link them to an experimental and unstable > project, and do it in a way that makes it really easy to slip through > maintainers' attention. Not sure what you mean by "unstable project", but I agree that the patch series, unless Viresh is the maintainer of the C side of everything added, it should be discussed and maintenance discussed accordingly before merging anything. This is what we have done for everything else, and that has not changed. I try to spot cases where this is not done, which is why I Cc'd you in v7 and told Viresh to please do so, and he did -- I don't think he was trying to bypass on purpose: https://lore.kernel.org/rust-for-linux/CANiq72=o+uc3ZnNrdkuoSGSL8apNE4z4QwpvsiLfGzXFywSLrQ@mail.gmail.com/ > That would make things even worse. Before you wanted me to maintain > rust linkage. Now you want me to get approval from someone else who > maintains rust linkage. In case I need to change something, I want to > be able to just change. Like Danilo mentions, there are several ways to go forward here. For some ideas/context, please see: https://rust-for-linux.com/rust-kernel-policy#how-is-rust-introduced-in-a-subsystem (Thanks Jason for linking the page) And, yeah, whoever ends up maintaining this, then they should of course be testing it properly with Rust enabled with a proper config for that and so on, just like one would do for anything else. By the way, it is possible to request Intel's 0day to build with Rust enabled. (Side-note: to clarify, there are different parties involved here -- "Rust team" is fairly ambiguous.) Cheers, Miguel
diff --git a/MAINTAINERS b/MAINTAINERS index ee6709599df5..bfc1bf2ebd77 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4021,6 +4021,7 @@ F: lib/cpumask_kunit.c F: lib/find_bit.c F: lib/find_bit_benchmark.c F: lib/test_bitmap.c +F: rust/helpers/cpumask.c F: tools/include/linux/bitfield.h F: tools/include/linux/bitmap.h F: tools/include/linux/bits.h diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index fda1e0d8f376..59b4bc49d039 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -11,6 +11,7 @@ #include <linux/blk_types.h> #include <linux/blkdev.h> #include <linux/cpu.h> +#include <linux/cpumask.h> #include <linux/cred.h> #include <linux/errname.h> #include <linux/ethtool.h> diff --git a/rust/helpers/cpumask.c b/rust/helpers/cpumask.c new file mode 100644 index 000000000000..49267ad33b3c --- /dev/null +++ b/rust/helpers/cpumask.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/cpumask.h> + +void rust_helper_cpumask_set_cpu(unsigned int cpu, struct cpumask *dstp) +{ + cpumask_set_cpu(cpu, dstp); +} + +void rust_helper_cpumask_clear_cpu(int cpu, struct cpumask *dstp) +{ + cpumask_clear_cpu(cpu, dstp); +} + +void rust_helper_cpumask_setall(struct cpumask *dstp) +{ + cpumask_setall(dstp); +} + +unsigned int rust_helper_cpumask_weight(struct cpumask *srcp) +{ + return cpumask_weight(srcp); +} + +void rust_helper_cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp) +{ + cpumask_copy(dstp, srcp); +} + +bool rust_helper_zalloc_cpumask_var(cpumask_var_t *mask, gfp_t flags) +{ + return zalloc_cpumask_var(mask, flags); +} + +#ifndef CONFIG_CPUMASK_OFFSTACK +void rust_helper_free_cpumask_var(cpumask_var_t mask) +{ + free_cpumask_var(mask); +} +#endif diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index 0640b7e115be..de2341cfd917 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -11,6 +11,7 @@ #include "bug.c" #include "build_assert.c" #include "build_bug.c" +#include "cpumask.c" #include "cred.c" #include "device.c" #include "err.c"
In order to prepare for adding Rust abstractions for cpumask, this patch adds cpumask helpers. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- MAINTAINERS | 1 + rust/bindings/bindings_helper.h | 1 + rust/helpers/cpumask.c | 40 +++++++++++++++++++++++++++++++++ rust/helpers/helpers.c | 1 + 4 files changed, 43 insertions(+) create mode 100644 rust/helpers/cpumask.c