Re: [PATCH v9 01/13] rust: hrtimer: introduce hrtimer support

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Feb 24, 2025 at 08:52:35PM +0100, Andreas Hindborg wrote:
> "Boqun Feng" <boqun.feng@gmail.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@gmail.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.

> 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?

> >
> >> than having to disable rust entirely - or going and commenting out lines
> >> in the kernel crate - I think it is better to provide an option to just
> >> disable building these particular bindings.
> >>
> >> This has nothing to do with general policies related to breakage between
> >> Rust and C code, and how to fix such breakage in a timely manner. It is
> >> simply a useful switch for disabling part of the build so that people
> >> can move on with their business, while someone else scrambles to fix
> >> whatever needs fixing on the Rust side.
> >>
> >
> > It's of course up to hrtimer maintainers. But I personally nack this
> > kconfig, because it's not necessary, and hrtimer API has been stable for
> > a while.
> 
> Having the switch is fine for me, removing it is fine as well. It's just
> an added convenience that might come in handy. But having this kconfig
> very close to zero overhead, so I do not really understand your
> objection. I would like to better understand your reasoning.
> 

Hope my explanation above is helpful.

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
>