Message-ID: <875xkyzi7q.fsf@kernel.org>
Date: Tue, 25 Feb 2025 06:50:01 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
Cc: "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com>,  "Frederic Weisbecker"
 <frederic@...nel.org>,  "Anna-Maria Behnsen" <anna-maria@...utronix.de>,
  "Thomas Gleixner" <tglx@...utronix.de>,  "Danilo Krummrich"
 <dakr@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno
 Lossin" <benno.lossin@...ton.me>,  "Alice Ryhl" <aliceryhl@...gle.com>,
  "Trevor Gross" <tmgross@...ch.edu>,  "Lyude Paul" <lyude@...hat.com>,
  "Guangbo Cui" <2407018371@...com>,  "Dirk Behme" <dirk.behme@...il.com>,
  "Daniel Almeida" <daniel.almeida@...labora.com>,  "Tamir Duberstein"
 <tamird@...il.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>,  "Miguel Ojeda" <ojeda@...nel.org>
Subject: Re: [PATCH v9 01/13] rust: hrtimer: introduce hrtimer support

"Boqun Feng" <boqun.feng@...il.com> writes:

> On Mon, Feb 24, 2025 at 08:52:35PM +0100, Andreas Hindborg wrote:
>> "Boqun Feng" <boqun.feng@...il.com> writes:
>>
>> > On Mon, Feb 24, 2025 at 07:58:04PM +0100, Andreas Hindborg wrote:
>> >> > On Mon, Feb 24, 2025 at 05:45:03PM +0100, Miguel Ojeda wrote:
>> >> >> On Mon, Feb 24, 2025 at 5:31 PM Boqun Feng <boqun.feng@...il.com> wrote:
>> >> >> >
>> >> >> > On Mon, Feb 24, 2025 at 05:23:59PM +0100, Miguel Ojeda wrote:
>> >> >> > >
>> >> >> > > side -- Andreas and I discussed it the other day. The description of
>> >> >> > > the issue has some lines, but perhaps the commit message could
>> >> >> >
>> >> >> > Do you have a link to the issue?
>> >> >>
>> >> >> Sorry, I meant "description of the symbol", i.e. the description field
>> >> >> in the patch.
>> >> >>
>> >> >
>> >> > Oh, I see. Yes, the patch description should provide more information
>> >> > about what the kconfig means for hrtimer maintainers' development.
>> >>
>> >> Right, I neglected to update the commit message. I will do that if we
>> >> have another version.
>> >>
>> >> >
>> >> >> > I asked because hrtimer API is always available regardless of the
>> >> >> > configuration, and it's such a core API, so it should always be there
>> >> >> > (Rust or C).
>> >> >>
>> >> >> It may not make sense for something that is always built on the C
>> >> >> side, yeah. I think the intention here may be that one can easily
>> >> >> disable it while "developing" a change on the C side. I am not sure
>> >> >> what "developing" means here, though, and we need to be careful --
>> >> >> after all, Kconfig options are visible to users and they do not care
>> >> >> about that.
>> >> >>
>> >> >
>> >> > Personally, I don't think CONFIG_RUST_HRTIMER is necessarily because as
>> >> > you mentioned below, people can disable Rust entirely during
>> >> > "developing".
>> >> >
>> >> > And if I understand the intention correctly, the CONFIG_RUST_HRTIMER
>> >> > config provides hrtimer maintainers a way that they could disable Rust
>> >> > hrtimer abstraction (while enabling other Rust component) when they're
>> >> > developing a change on the C side, right? If so, it's hrtimer
>> >> > maintainers' call, and this patch should provide more information on
>> >> > this.
>> >> >
>> >> > Back to my personal opinion, I don't think this is necessary ;-)
>> >> > Particularly because I can fix if something breaks Rust side, and I'm
>> >> > confident and happy to do so for hrtimer ;-)
>> >>
>> >> As Miguel said, the idea for this came up in the past week in one of the
>> >> mega threads discussing rust in general. We had a lot of "what happens
>> >> if I change something in my subsystem and that breaks rust" kind of
>> >> discussions.
>> >>
>> >
>> > So far we haven't heard such a question from hrtimer maintainers, I
>> > would only add such a kconfig if explicitly requested.
>>
>> It gives flexibility and has no negative side effects. Of course, if it
>
> The negative side effects that I can think of:
>
> * It doubles the work for testing, it's a Kconfig after all, so every
>   reasonable test run will have to run at least one build with it and
>   one build without it combined with other configs.
>
> * It may compelicate other component. For example, if I would like
>   use hrtimer in a doc test of a lock component (the component itself
>   doesn't depend on hrtimer, so it exists with CONFIG_RUST_HRTIMER=n),
>   because I would like to unlock something after a certain time. Now
>   since CONFIG_RUST_HRTIMER can be unset, how would I write the test?
>
>   #[cfg(CONFIG_RUST_HRTIMER)]
>   <use the Rust timer>
>   #[cfg(not(CONFIG_RUST_HRTIMER))]
>   <use the C timer? with unsafe??>
>
> A new kconfig is not something free. We will need to cope with it in
> multiple places.

Alright, those are valid arguments.

>
>> is unwanted, we can just remove it. But I would like to understand the
>> deeper rationale.
>>
>>
>> >
>> >> For subsystems where the people maintaining the C subsystem is not the
>> >> same people maintaining the Rust abstractions, this switch might be
>> >> valuable. It would allow making breaking changes to the C code of a
>> >> subsystem without refactoring the Rust code in the same sitting. Rather
>> >
>> > That's why I asked Frederic to be a reviewer of Rust hrtimer API. In
>> > longer-term, more and more people will get more or less Rust knowledge,
>> > and I'd argue that's the direction we should head to. So my vision is a
>> > significant amount of core kernel developers would be able to make C and
>> > Rust changes at the same time. It's of course not mandatory, but it's
>> > better collaboration.
>>
>> Having this switch does not prevent longer term plans or change
>> directions of anything. It's simply a convenience feature made
>> available. I also expect the future you envision. But it is an
>> envisioned _future_. It is not the present reality.
>>
>
> The reality is: we haven't heard hrtimer maintainers ask for this,
> right? I know you're trying to do something nice, I do appreciate your
> intention, but if hrtimer maintainers haven't asked for this, maybe it
> implies that they can handle or trust that wouldn't be a problem?

Thanks for explaining.

For reference, we do not have this feature in block, and it was not a
problem yet.

Let's await hrtimer maintainers and follow their lead.



Best regards,
Andreas Hindborg

Powered by blists - more mailing lists