Message-ID: <20230615191931.4e4751ac@kernel.org>
Date: Thu, 15 Jun 2023 19:19:31 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, aliceryhl@...gle.com, andrew@...n.ch
Subject: Re: [PATCH 0/5] Rust abstractions for network device drivers

On Thu, 15 Jun 2023 10:58:50 +0200 Miguel Ojeda wrote:
> On Thu, Jun 15, 2023 at 8:01 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > I was hoping someone from the Rust side is going to review this.
> > We try to review stuff within 48h at netdev, and there's no review :S  
> 
> I think the version number got reset, but Tomonori had a couple
> versions on the rust-for-linux@...r list [2][3].
> 
> Andrew Lunn was taking a look, and there were some other comments going on, too.
> 
> The email threading is broken in [2][3], though, so it may be easiest
> to use a query like "f:lunn" [4] to find those.
> 
> [2] https://lore.kernel.org/rust-for-linux/01010188843258ec-552cca54-4849-4424-b671-7a5bf9b8651a-000000@us-west-2.amazonses.com/
> [3] https://lore.kernel.org/rust-for-linux/01010188a42d5244-fffbd047-446b-4cbf-8a62-9c036d177276-000000@us-west-2.amazonses.com/
> [4] https://lore.kernel.org/rust-for-linux/?q=f%3Alunn
> 
> > My immediate instinct is that I'd rather not merge toy implementations
> > unless someone within the netdev community can vouch for the code.  
> 
> Yes, in general, the goal is that maintainers actually understand what
> is getting merged, get involved, etc. So patch submitters of Rust
> code, at this time, should be expected/ready to explain Rust if
> needed. We can also help from the Rust subsystem side on that.
> 
> But, yeah, knowledgeable people should review the code.

All sounds pretty reasonable, thanks for the pointers.

TBH I was hoping that the code will be more like reading "modern C++"
for a C developer. I can't understand much of what's going on.

Taking an example of what I have randomly on the screen as I'm writing
this email:

+    /// Updates TX stats.
+    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
+        // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay.
+        unsafe {
+            let inner = Opaque::get(&self.0);
+            (*inner).tx_packets = packets;
+            (*inner).tx_bytes = bytes;
+            (*inner).tx_errors = errors;
+            (*inner).tx_dropped = dropped;
+        }
+    }

What is this supposed to be doing? Who needs to _set_ unrelated
statistics from a function call? Yet no reviewer is complaining
which either means I don't understand, or people aren't really 
paying attention :(

> > You seem to create a rust/net/ directory without adding anything
> > to MAINTAINERS. Are we building a parallel directory structure?
> > Are the maintainers also different?  
> 
> The plan is to split the `kernel` crate and move the files to their
> proper subsystems if the experiment goes well.

I see.

> But, indeed, it is best if a `F:` entry is added wherever you think it
> is best. Some subsystems may just add it to their entry (e.g. KUnit
> wants to do that). Others may decide to split the Rust part into
> another entry, so that maintainers may be a subset (or a different set
> -- sometimes this could be done, for instance, if a new maintainer
> shows up that wants to take care of the Rust abstractions).

I think F: would work for us.

Are there success stories in any subsystem for getting a driver for
real HW supported? I think the best way to focus the effort would be 
to set a target on a relatively simple device.

Actually Andrew is interested, and PHY drivers seem relatively simple..
/me runs away

Powered by blists - more mailing lists