DRAFT: [RFC]: Add rust net_device wrappers #439

Kloenk · 2021-07-10T10:38:51Z

Second attempt of #208

I will try to add the dummyrs driver in the following day as second commit.

rust/kernel/bindings_helper.h

rust/macros/module.rs

bjorn3

no obvious problems on a quick glance

Kloenk · 2021-07-10T13:35:58Z

Could somebody restart the CI, I don't think risc does not have the symbol kfree_skb

ksquirrel · 2021-07-10T14:34:22Z

Review of 9a650833f0ca:

⚠️ This PR adds/deletes more than 500 lines.
✔️ Commit 9a65083: Looks fine!

tivrfoa · 2021-07-10T20:37:35Z

Hi! I'm a Rust newbie, and I'm just curious if all those unsafe are really necessary.

I thought unsafe would be used basically when interacting with C and probably with hardware too.

It's because the Linux kernel folks will probably notice and be suspicious about it, like: "ok, it promises to bring safety, but there's unsafe everywhere."

The SAFETY comments are good, but they are just comments in the end right? Or are they evaluated somehow?

I saw many unsafe code (in general, not in this PR) that says things like: "this is safe because some other code behaves like x"
But then it has a really tight coupling. What if "x" changes to "y"?

Will kernel driver developers have to use a lot of unsafe too if they want to use Rust?

Kloenk · 2021-07-10T21:16:15Z

I a perfect world Drivers will need no unsafe at all. Sadly at this time my only idea on how to implement the rtnl_ops is via a static mut. Static mut is usafe by design, so there unsafe is needed for every access.

In the abstraction of this pr unsafe is used quite often, because I'm only storing a pointer which I got from the C side. This is not optimal, but that is the current state of network in rust (from my side).

rust/kernel/net/ethtool.rs

rust/macros/module.rs

Kloenk · 2021-07-12T17:41:36Z

Let's hope the pipeline succeeds. I don't now anything else I would like to change for this pr.

nbdd0121 · 2021-07-12T17:56:47Z

rust/helpers.c

+
+int rust_helper_net_device_set_new_lstats(struct net_device *dev)
+{
+	dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats);


Is it possible to have rust_helper_netdev_alloc_pcpu_stats and move the other logic to Rust side? Ideally rust_helper_foo should be functionally equivalent to foo.

I wasn't able do to that. because of the union stuff.

I think I have to rework this as part of my e1000 driver that I want to write. Really don't want to that before that :-)

I agree with Gary -- logic should not be in rust_helper_*s.

If it cannot be done for some reason, please add a comment explaining why.

rust/kernel/error.rs

rust/kernel/net/device.rs

nbdd0121 · 2021-07-12T18:08:19Z

rust/kernel/net/device.rs

+        }
+    }
+
+    impl Add for NetIF {


Any reason to use Add and Sub? Why not implement BitOr, BitAnd, Not?

I find it more intuitive to read flagA + flagB. That's all. So I'm open to both. What should I do? both of them?

I personally tend to use flagA | flagB because flagA + flagA = flagA may seem a bit counter-intuitive for C users. But someflag - flagA does seem nicer than someflag & !flagA.

I think implementing both would be a real mess. Also implementing BitOR BitAnd and Sub is also not nice. so? I find this much more intuitive, and rust is also with borrow checking and so on quite different to c

rust/kernel/net/device.rs

rust/kernel/net/mod.rs

nbdd0121 · 2021-07-12T18:14:49Z

rust/kernel/net/mod.rs

+/// Please note: addr must be aligned to u16.
+#[cfg(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)]
+pub unsafe fn is_zero_ether_addr(addr: *const u8) -> bool {
+    // SAFETY: function already unsafe


This is not correct. You should explain why the function is unsafe on the safety requirement above, e.g. say "addr must point to a valid ethernet address and 16-bit aligned, and then use the safety requirement to justify why the function is safe.

Is this better now?

I wonder if an encapsulation for a known-to-be-valid ethernet address pointer would help, so that we don't need these unsafe-to-call functions.

Or maybe we should just bind directly to C implementations via helpers for now, and we can come back for performance optimisations later.

Yes I would like an Wrapper for that in the long run. for now I think it's a bit much, It's already +4k. Keep it like this until I probably need to change it for e1000?

rust/macros/net.rs

Kloenk · 2021-07-12T20:16:01Z

Whey features (plural) and Flag (singular)? Because Flag is alway just one, und Features is a bit mask?

Kloenk · 2021-07-12T20:47:19Z

Moved code a bit around. I have the mod features internal, but it is private. @nbdd0121 can you take a look?

rust/kernel/net/device/mod.rs

rust/kernel/net/rtnl.rs

rust/kernel/net/skbuff.rs

rust/macros/lib.rs

rust/macros/net.rs

rust/kernel/net/device/flags.rs

Kloenk · 2021-07-12T21:28:53Z

@nbdd0121 all should have been applied now

Signed-off-by: Finn Behrens <me@kloenk.de>

Enables networking support and network device drivers support required by net_device wrappers and dummy_rs module. Signed-off-by: Dariusz Sosnowski <dsosnowski@dsosnowski.pl> Signed-off-by: Finn Behrens <me@kloenk.de>

Signed-off-by: Finn Behrens <me@kloenk.de>

Kloenk · 2021-07-13T12:15:19Z

@ojeda I only got one review of @ksquirrel . Is this correct behaviour? I thought it would look at every force push?

ksquirrel · 2021-07-13T17:43:03Z

Review of 5d64fb90f526:

⚠️ This PR changes more than 20 files.
⚠️ This PR adds/deletes more than 500 lines.
✔️ Commit efda5a1: Looks fine!
✔️ Commit e2dbf35: Looks fine!
⚠️ Commit 0dc000d: The commit committer does not match the commit author.
❌ Commit 0dc000d: The last line of the commit message must be a Signed-off-by and must match the submitter of the PR.
✔️ Commit 5d64fb9: Looks fine!

ojeda · 2021-07-13T17:44:10Z

@ojeda I only got one review of @ksquirrel . Is this correct behaviour? I thought it would look at every force push?

I am manually running it now as needed, so you may see delays.

ojeda · 2021-07-13T17:49:45Z

rust/helpers.c

+
+int rust_helper_net_device_set_new_lstats(struct net_device *dev)
+{
+	dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats);


I agree with Gary -- logic should not be in rust_helper_*s.

If it cannot be done for some reason, please add a comment explaining why.

ojeda · 2021-07-13T17:51:07Z

rust/kernel/error.rs

@@ -276,7 +279,7 @@ where
 #[macro_export]
 macro_rules! from_kernel_result {
    ($($tt:tt)*) => {{
-        $crate::error::from_kernel_result_helper((|| {
+        $crate::from_kernel_result_helper((|| {


Why this change? Should it be separate?

The rtnl macro uses this

ojeda · 2021-07-13T17:53:00Z

rust/kernel/net/device/flags.rs

+        Self(0)
+    }
+
+    /// Add flag to Self.


Suggested change

/// Add flag to Self.

/// Inserts flag into the set.

ojeda · 2021-07-13T17:53:44Z

rust/kernel/net/device/flags.rs

+    /// Remove the given flag from Self.
+    #[inline]
+    pub fn remove(&mut self, flag: u64) {
+        self.0 &= !(flag);


Are the parenthesis needed? Is there no warning about it?

Suggested change

self.0 &= !(flag);

self.0 &= !flag;

ojeda · 2021-07-13T17:54:10Z

rust/kernel/net/device/flags.rs

+        self.0 |= flag;
+    }
+
+    /// Remove the given flag from Self.


Suggested change

/// Remove the given flag from Self.

/// Removes a flag from the set.

ojeda · 2021-07-13T18:18:39Z

rust/kernel/net/skbuff.rs

+    /// Noop if `CONFIG_NETWORK_PHY_TIMESTAMPING` is not enabled.
+    #[cfg(CONFIG_NETWORK_PHY_TIMESTAMPING)]
+    pub fn clone_tx_timestamp(&mut self) {
+        // SAFETY: self.ptr is valid if self is valid.


This should say self.ptr is always valid by the type invariant.

ojeda · 2021-07-13T18:19:38Z

rust/kernel/net/skbuff.rs

+        self.get_internal().len
+    }
+
+    /// Get the Shared info for this SKBuffer.


I would avoid repeating "for this SKBuffer", "for Self", etc. if they are methods.

Also, not sure why Shared is capitalized.

Suggested change

/// Get the Shared info for this SKBuffer.

/// Get shared information.

ojeda · 2021-07-13T18:20:03Z

rust/kernel/net/skbuff.rs

+        self.end_pointer() as *mut bindings::skb_shared_info
+    }
+
+    // NET_SKBUFF_DATA_USES_OFFSET


If this is a CONFIG, perhaps we can be more clear:

Suggested change

// NET_SKBUFF_DATA_USES_OFFSET

// CONFIG_NET_SKBUFF_DATA_USES_OFFSET

We should also explain why this is done with target_pointer_width instead of the CONFIG itself.

ojeda · 2021-07-13T18:21:46Z

rust/kernel/types.rs

@@ -247,3 +247,53 @@ impl<T: FnOnce()> Drop for ScopeGuard<T> {
        }
    }
 }
+
+/// Trait for wrappers which are holding a direct pointer that they are getting from a C side function.


Is this a new PointerWrapper?

Yes, types implementing this trait only have a pointer, and are not Owned like the PointerWrapper.

ojeda · 2021-07-13T18:22:48Z

rust/macros/lib.rs

+/// helper functions
+mod helpers;
+
+/// generic module/driver macros


Please capitalize like the rest of the code, end with period, etc.

Also, all these refactors should probably go first in another PR.

This refectory is there, because of need created here. It's needed now, as the rtnl macro is not a module macro. What about not a pr, but a commit?

Finn, I think you need to break this whole thing up into separate a bunch of PRs. This is so that we can carefully review this in increments rather than in one huge go.

For example, and as Miguel has already suggested, changes to existing code should be factored out so that they can be reviewed separately. In fact, pure refactors without functional changes should also have their own PRs. Even if they are not immediately needed, you can say something to the effect of "This is in preparation for XXX because XXX" -- see for example #386, where there are no users yet of an enhancement, they come in a subsequent PR. See also #239 as an example of a pure refactor. They (and many others) were broken into different PRs to make reviewing easier.

Ok, I will try to add all open reviews on this I to this one, then keep this one open, but without the intention to merge as a meta, and create small prs

Thanks Finn -- I know it takes time on the submitter's side, but it is best for everyone and reduces the overall time to get things merged.

No problem. But I think I can't do it until this weekend

Kloenk · 2021-07-16T10:29:10Z

I created to prs emerging from this one. I think I will wait until those 2 at least got some reviews, as all others would depend on at least on of them

rust/kernel/lib.rs

`rustfmt` (in its default config, at least) does not add a comma in the last feature of the list, which means diffs are bigger at times. In addition, this is how the standard library does it, and one may argue it allows for easier `grep`ing too. Link: Rust-for-Linux#439 (comment). Indirectly-suggested-by: Benoit de Chezelles <benoit.dechezelles@gmail.com> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Kloenk force-pushed the rust-netdev-2 branch 2 times, most recently from 419ef04 to 07cd559 Compare July 10, 2021 10:48

bjorn3 reviewed Jul 10, 2021

View reviewed changes

rust/kernel/bindings_helper.h Outdated Show resolved Hide resolved

bjorn3 reviewed Jul 10, 2021

View reviewed changes

rust/macros/module.rs Outdated Show resolved Hide resolved

Kloenk force-pushed the rust-netdev-2 branch from 07cd559 to 06b9497 Compare July 10, 2021 11:59

Kloenk requested a review from bjorn3 July 10, 2021 11:59

bjorn3 reviewed Jul 10, 2021

View reviewed changes

Kloenk force-pushed the rust-netdev-2 branch 3 times, most recently from 3878bda to 9a65083 Compare July 10, 2021 13:25

Kloenk force-pushed the rust-netdev-2 branch from 9a65083 to 33d6562 Compare July 10, 2021 14:45

sodar reviewed Jul 10, 2021

View reviewed changes

rust/kernel/net/ethtool.rs Outdated Show resolved Hide resolved

nbdd0121 reviewed Jul 11, 2021

View reviewed changes

rust/macros/module.rs Outdated Show resolved Hide resolved

Kloenk force-pushed the rust-netdev-2 branch 3 times, most recently from 4b0adf7 to 60e7f47 Compare July 12, 2021 17:26

Kloenk mentioned this pull request Jul 12, 2021

doc_cfg #442

Closed

Kloenk force-pushed the rust-netdev-2 branch from 60e7f47 to 3fb2acb Compare July 12, 2021 17:35

Kloenk requested a review from nbdd0121 July 12, 2021 17:35

Kloenk force-pushed the rust-netdev-2 branch from 3fb2acb to 9c60c1c Compare July 12, 2021 17:52

nbdd0121 suggested changes Jul 12, 2021

View reviewed changes

Kloenk force-pushed the rust-netdev-2 branch 2 times, most recently from b8216b4 to 3a1cb66 Compare July 12, 2021 18:53

Kloenk requested a review from nbdd0121 July 12, 2021 18:54

Kloenk force-pushed the rust-netdev-2 branch from 3a1cb66 to b4960c6 Compare July 12, 2021 19:31

Kloenk force-pushed the rust-netdev-2 branch from 95d4b03 to cc4942a Compare July 12, 2021 20:46

nbdd0121 reviewed Jul 12, 2021

View reviewed changes

rust/kernel/net/device/flags.rs Outdated Show resolved Hide resolved

Kloenk force-pushed the rust-netdev-2 branch from cc4942a to f6fb1d6 Compare July 12, 2021 21:27

Kloenk and others added 4 commits July 12, 2021 23:44

Add rust net_device wrappers

efda5a1

Signed-off-by: Finn Behrens <me@kloenk.de>

Fix dead_code in rust KParamGuard.

e2dbf35

Signed-off-by: Finn Behrens <me@kloenk.de>

rust/CI: Enables networking support

0dc000d

Enables networking support and network device drivers support required by net_device wrappers and dummy_rs module. Signed-off-by: Dariusz Sosnowski <dsosnowski@dsosnowski.pl> Signed-off-by: Finn Behrens <me@kloenk.de>

add dummy_rs driver

5d64fb9

Signed-off-by: Finn Behrens <me@kloenk.de>

Kloenk force-pushed the rust-netdev-2 branch from f6fb1d6 to 5d64fb9 Compare July 12, 2021 21:45

ojeda reviewed Jul 13, 2021

View reviewed changes

Kloenk added • drivers Related to the example drivers in `drivers/`. • lib Related to the `rust/` library. labels Jul 13, 2021

This was referenced Jul 16, 2021

Fix dead_code in rust KParamGuard. #448

Open

Move macro helpers into helpers submodule #449

Merged

Kloenk changed the title ~~[RFC]: Add rust net_device wrappers~~ DRAFT: [RFC]: Add rust net_device wrappers Jul 23, 2021

Kloenk mentioned this pull request Jul 23, 2021

Add EADDRNOTAVAIL error to rust #455

Merged

Kloenk marked this pull request as draft July 23, 2021 12:21

Kloenk mentioned this pull request Jul 23, 2021

Add SavedAsPointer and it's mutable part as trait #456

Closed

sodar mentioned this pull request Jul 23, 2021

rust/CI: Enables networking support #457

Open

bew reviewed Jan 19, 2022

View reviewed changes

rust/kernel/lib.rs Show resolved Hide resolved

ojeda mentioned this pull request Jan 20, 2022

rust: kernel: use feature individually #636

Merged

Lymkwi mentioned this pull request Mar 1, 2023

rust: add initial netdevice support #908

Closed

	/// Remove the given flag from Self.
	/// Removes a flag from the set.

	/// Get the Shared info for this SKBuffer.
	/// Get shared information.

	// NET_SKBUFF_DATA_USES_OFFSET
	// CONFIG_NET_SKBUFF_DATA_USES_OFFSET

DRAFT: [RFC]: Add rust net_device wrappers #439

Are you sure you want to change the base?

DRAFT: [RFC]: Add rust net_device wrappers #439

Conversation

Kloenk commented Jul 10, 2021

bjorn3 left a comment

Choose a reason for hiding this comment

Kloenk commented Jul 10, 2021

ksquirrel commented Jul 10, 2021

tivrfoa commented Jul 10, 2021

Kloenk commented Jul 10, 2021

Kloenk commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kloenk commented Jul 12, 2021

Kloenk commented Jul 12, 2021

Kloenk commented Jul 12, 2021

Kloenk commented Jul 13, 2021

ksquirrel commented Jul 13, 2021

ojeda commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kloenk commented Jul 16, 2021