[PATCH] rust: workqueue: define built-in bh queues

[PATCH] rust: workqueue: define built-in bh queues

[Hamza Mahfooz]

We provide these methods because it lets us access these queues from
Rust without using unsafe code.

Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
---
 rust/kernel/workqueue.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 0cd100d2aefb..68ce70d94f2d 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -703,3 +703,21 @@ pub fn system_freezable_power_efficient() -> &'static Queue {
     // SAFETY: `system_freezable_power_efficient_wq` is a C global, always available.
     unsafe { Queue::from_raw(bindings::system_freezable_power_efficient_wq) }
 }
+
+/// Returns the system bottom halves work queue (`system_bh_wq`).
+///
+/// It is similar to the one returned by [`system`] but for work items which
+/// need to run from a softirq context.
+pub fn system_bh() -> &'static Queue {
+    // SAFETY: `system_bh_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_bh_wq) }
+}
+
+/// Returns the system bottom halves high-priority work queue (`system_bh_highpri_wq`).
+///
+/// It is similar to the one returned by [`system_bh`] but for work items which
+/// require higher scheduling priority.
+pub fn system_bh_highpri() -> &'static Queue {
+    // SAFETY: `system_bh_highpri_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_bh_highpri_wq) }
+}
-- 
2.47.1

Re: [PATCH] rust: workqueue: define built-in bh queues

[Miguel Ojeda]

On Fri, Feb 21, 2025 at 11:36 PM Hamza Mahfooz
<hamzamahfooz@linux.microsoft.com> wrote:
>
> We provide these methods because it lets us access these queues from
> Rust without using unsafe code.
>
> Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>

Cc'ing WORKQUEUE -- thanks!

Cheers,
Miguel

Re: [PATCH] rust: workqueue: define built-in bh queues

[Jarkko Sakkinen]

On Fri, Feb 21, 2025 at 05:35:31PM -0500, Hamza Mahfooz wrote:
> We provide these methods because it lets us access these queues from
> Rust without using unsafe code.
> 
> Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>

Just getting familiar how code testing and reviews go with Rust code,
as I might put out even a driver at some point of time, so let's give
this a shot :-)

Using 1st person plural is usually almost a cardinal sin almost and is
somewhat exhausting to read. "These methods" refer to nothing and the
commit message does not educate me why the accessors are needed.

It should be IMHO language agnostic that commit message is more
important than the source code. It weights now even more than ever
before because it is also AI acid test. The current commit message
is as good as it did not exist at all.

Based on these conclusions I'd start the commit message by saying
that "Provide safe accessors to the workqueue bottom-halves."

Then add another sentence on why they are required (i.e. who will
be  the caller for these).

BR, Jarkko 

Re: [PATCH] rust: workqueue: define built-in bh queues

[Jarkko Sakkinen]

On Fri, Feb 21, 2025 at 11:45:38PM +0100, Miguel Ojeda wrote:
> On Fri, Feb 21, 2025 at 11:36 PM Hamza Mahfooz
> <hamzamahfooz@linux.microsoft.com> wrote:
> >
> > We provide these methods because it lets us access these queues from
> > Rust without using unsafe code.
> >
> > Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
> 
> Cc'ing WORKQUEUE -- thanks!

Not meaning to complain but it by practical has no commit message.

Does not meet any quality standards in that area tbh. In order to
make Rust more appealing you really should pay more attention to
this. 

I learned absolutely nothing from this.

> 
> Cheers,
> Miguel
> 

BR, Jarkko

Re: [PATCH] rust: workqueue: define built-in bh queues

[Jarkko Sakkinen]

On Sat, Feb 22, 2025 at 06:17:41AM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 21, 2025 at 11:45:38PM +0100, Miguel Ojeda wrote:
> > On Fri, Feb 21, 2025 at 11:36 PM Hamza Mahfooz
> > <hamzamahfooz@linux.microsoft.com> wrote:
> > >
> > > We provide these methods because it lets us access these queues from
> > > Rust without using unsafe code.
> > >
> > > Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
> > 
> > Cc'ing WORKQUEUE -- thanks!
> 
> Not meaning to complain but it by practical has no commit message.

oops, sorry, "... but by practical means it ..."

Anyway I hope my message was received ;-) Leaves me wonder tho why
this was queued because it apparently is not even part of a patch
set. "zero callers" should never be merged to mainline...

If however such patch is merged, the commit message should probably
address this exceptional condition.

R, Jarkko

Re: [PATCH] rust: workqueue: define built-in bh queues

[Boqun Feng]

On Sat, Feb 22, 2025 at 06:37:06AM +0200, Jarkko Sakkinen wrote:
> On Sat, Feb 22, 2025 at 06:17:41AM +0200, Jarkko Sakkinen wrote:
> > On Fri, Feb 21, 2025 at 11:45:38PM +0100, Miguel Ojeda wrote:
> > > On Fri, Feb 21, 2025 at 11:36 PM Hamza Mahfooz
> > > <hamzamahfooz@linux.microsoft.com> wrote:
> > > >
> > > > We provide these methods because it lets us access these queues from
> > > > Rust without using unsafe code.
> > > >
> > > > Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
> > > 
> > > Cc'ing WORKQUEUE -- thanks!
> > 
> > Not meaning to complain but it by practical has no commit message.
> 
> oops, sorry, "... but by practical means it ..."
> 
> Anyway I hope my message was received ;-) Leaves me wonder tho why
> this was queued because it apparently is not even part of a patch

What do you mean by "queued"? IIUC, Miguel was just copying workqueue
maintainers, since they should be in the review process.

Thanks for your review in another email anyway!

Regards,
Boqun

> set. "zero callers" should never be merged to mainline...
> 
> If however such patch is merged, the commit message should probably
> address this exceptional condition.
> 
> R, Jarkko

Re: [PATCH] rust: workqueue: define built-in bh queues

[Jarkko Sakkinen]

On Fri, 2025-02-21 at 20:41 -0800, Boqun Feng wrote:
> On Sat, Feb 22, 2025 at 06:37:06AM +0200, Jarkko Sakkinen wrote:
> > On Sat, Feb 22, 2025 at 06:17:41AM +0200, Jarkko Sakkinen wrote:
> > > On Fri, Feb 21, 2025 at 11:45:38PM +0100, Miguel Ojeda wrote:
> > > > On Fri, Feb 21, 2025 at 11:36 PM Hamza Mahfooz
> > > > <hamzamahfooz@linux.microsoft.com> wrote:
> > > > > 
> > > > > We provide these methods because it lets us access these
> > > > > queues from
> > > > > Rust without using unsafe code.
> > > > > 
> > > > > Signed-off-by: Hamza Mahfooz
> > > > > <hamzamahfooz@linux.microsoft.com>
> > > > 
> > > > Cc'ing WORKQUEUE -- thanks!
> > > 
> > > Not meaning to complain but it by practical has no commit
> > > message.
> > 
> > oops, sorry, "... but by practical means it ..."
> > 
> > Anyway I hope my message was received ;-) Leaves me wonder tho why
> > this was queued because it apparently is not even part of a patch
> 
> What do you mean by "queued"? IIUC, Miguel was just copying workqueue
> maintainers, since they should be in the review process.
> 
> Thanks for your review in another email anyway!

Ah sorry! I have dyslexia (for real) and I did read that the patch
was queued :-) Thanks for correcting and please do also the next
time!

I spent also last 48h awake setting up my personal Rust development
and QA environment... [1]

Anyway, to soften my feedback a bit I'll say this: everything else was
great:

1. Code was great
2. It was commented like you'd expect.

And I don't have much to complain about code quality in any other
patches I've seen so far.

I've been silently following this list for some time and this is a
common problem that the commit messages are quite bad. Thus, I even
wrote a public post about the topic now that I had this in mind:

https://social.kernel.org/notice/ArMk27Ism4Citb90K0

There's two great arguments to improve across the board:

1. Education
2. Integrity. E.g. AI is actually acceptable for short a patch, if
   you can in detail unmask the implementation (as is e.g. Windows
   notepad or really anything that produces text). But AI can be also
   used to engineer harmless looking changes that are actually vulns.
   By being punctual in expalanations we also contribute to the 
   governance of our ecosystem.

I'm personally reading this list exactly for learning purposes because I
have a roadmap to Rust driver ongoing, and explanations even in trivial
things help to gather bits and pieces... So please help me to succeed
by doing just a tiny bit more effort ;-)

> Regards,
> Boqun

[1] https://codeberg.org/jarkko/linux-tpmdd-nixos

BR, Jarkko

Re: [PATCH] rust: workqueue: define built-in bh queues

[Miguel Ojeda]

On Sat, Feb 22, 2025 at 5:15 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> Using 1st person plural is usually almost a cardinal sin almost and is
> somewhat exhausting to read.

Using "we" is far from a "cardinal sin" -- even key maintainers use it
sometimes.

Yes, commits should be generally written using the imperative,
especially for the sentence about the actual change itself, but it is
more natural in some cases to use "we".

> "These methods" refer to nothing

"These methods" refer to the ones added in the commit -- that seems clear to me.

They are not "methods", though (that is wrong), but apart from that, I
am not sure what the issue is with those two words.

To be clear, this does not mean the commit message is good -- I agree
that it should provide more justification.

Cheers,
Miguel

Re: [PATCH] rust: workqueue: define built-in bh queues

[Miguel Ojeda]

On Sat, Feb 22, 2025 at 7:04 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> Ah sorry! I have dyslexia (for real) and I did read that the patch
> was queued :-) Thanks for correcting and please do also the next
> time!
>
> I spent also last 48h awake setting up my personal Rust development
> and QA environment... [1]

Dyslexia and lack of sleep do not excuse yourself claiming anything
about "quality standards" of others and asking them to "really pay
more attention" as you did in the other message.

Even if you thought the patch was queued, you could have easily
checked whether it was merged or not before sending these messages.

Moreover, even if it was queued, the commit message could have been
fixed by the maintainer.

Finally, even if a maintainer applied it without fixing the message,
your message is uncalled for, both in tone and the in the fact that it
could have been a particular instance.

> I've been silently following this list for some time and this is a
> common problem that the commit messages are quite bad. Thus, I even
> wrote a public post about the topic now that I had this in mind:
>
> https://social.kernel.org/notice/ArMk27Ism4Citb90K0

This is, again, uncalled for.

We get a lot of new kernel contributors in the Rust for Linux mailing
list -- it is to be expected that their first few patches are not
great.

We do not control who posts to the mailing list, nor should we, and I
hope you are not trying to imply that the actual commit messages in
the branch are bad.

We have commit messages that are _pages_ long sometimes. We try to
follow the rules for every tag every single time. Same for tags sent
to mainline. So what are you talking about?

You suggest "Education" -- we have spent a *ton* of time teaching
newcomers how to contribute, how to write commit messages, how tags
work, etc. Not just in the mailing list, but in our Zulip and in
private too. Others can attest to this.

Moreover, I do take issue with your social media post. You claim:

    "Rust kernel patches should really level up on commit messages and
not merging random code with zero callers."

We do _not_ merge random code. In fact, my message above was Cc'ing
workqueue precisely because we do not just randomly merge code.

Not just that -- if you had actually checked the Git log, you would
have seen that Tejun himself merged the bulk of the content in that
file. So it seems now you have just blamed two different subsystems
entirely.

Regarding "zero callers": that is the usual rule, yes, but it can
happen when there are expected users in the future, and in the end it
is up to the judgement of the maintainers. For instance, in this file,
there are other queues that do not have users yet.

As for the rest of your social media post, I will ignore it before I rant more.

Thank you.

Cheers,
Miguel

Re: [PATCH] rust: workqueue: define built-in bh queues

[Tejun Heo]

On Fri, Feb 21, 2025 at 05:35:31PM -0500, Hamza Mahfooz wrote:
> We provide these methods because it lets us access these queues from
> Rust without using unsafe code.
> 
> Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>

Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route it through rust tree. If you'd prefer it to be
routed to workqueue tree, please let me know.

Thanks.

-- 
tejun

Re: [PATCH] rust: workqueue: define built-in bh queues

[Tejun Heo]

On Sat, Feb 22, 2025 at 12:57:03PM +0100, Miguel Ojeda wrote:
...
> Moreover, I do take issue with your social media post. You claim:
> 
>     "Rust kernel patches should really level up on commit messages and
> not merging random code with zero callers."
> 
> We do _not_ merge random code. In fact, my message above was Cc'ing
> workqueue precisely because we do not just randomly merge code.
> 
> Not just that -- if you had actually checked the Git log, you would
> have seen that Tejun himself merged the bulk of the content in that
> file. So it seems now you have just blamed two different subsystems
> entirely.
> 
> Regarding "zero callers": that is the usual rule, yes, but it can
> happen when there are expected users in the future, and in the end it
> is up to the judgement of the maintainers. For instance, in this file,
> there are other queues that do not have users yet.

FWIW, the commit message could be better but at the same time I'm not sure
commit message bar is any higher for C patches for something this trivial.
As for no-immediate-user policy, yes, generally true but again it's sometims
a necessity or just more convenient to merge these API patches separately -
e.g. features straddling across subsystems, straightforward prep patches and
so on. So, that's the *general* rule but rules without flexibility are often
silly things and it's not like culling these trivial wrappers afterwards is
difficult.

Thanks.

-- 
tejun