Rust-for-Linux / linux Public
forked from torvalds/linuxNew issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DRAFT: [RFC]: Add rust net_device wrappers #439
base: rust
Are you sure you want to change the base?
Conversation
419ef04
to
07cd559
3878bda
to
9a65083
Could somebody restart the CI, I don't think risc does not have the symbol |
Review of
|
Hi! I'm a Rust newbie, and I'm just curious if all those I thought 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 The I saw many Will kernel driver developers have to use a lot of |
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 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). |
4b0adf7
to
60e7f47
Let's hope the pipeline succeeds. I don't now anything else I would like to change for this pr. |
|
||
int rust_helper_net_device_set_new_lstats(struct net_device *dev) | ||
{ | ||
dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able do to that. because of the union stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/net/device.rs
Outdated
} | ||
} | ||
|
||
impl Add for NetIF { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to use Add
and Sub
? Why not implement BitOr, BitAnd, Not
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/mod.rs
Outdated
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this better now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
b8216b4
to
3a1cb66
Hmm, what the rationale to a module containing a single item? If you want use module to organize code, you can make the Anyway, |
What about renaming the mod to flags, renaming NetIF to Feature, and also moving Iff and IffPriv into that mod? |
Since they're companions to |
Whey features (plural) and Flag (singular)? Because Flag is alway just one, und Features is a bit mask? |
Moved code a bit around. I have the |
@nbdd0121 all should have been applied now |
Signed-off-by: Finn Behrens <me@kloenk.de>
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>
@ojeda I only got one review of @ksquirrel . Is this correct behaviour? I thought it would look at every force push? |
Review of
|
I am manually running it now as needed, so you may see delays. |
|
||
int rust_helper_net_device_set_new_lstats(struct net_device *dev) | ||
{ | ||
dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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((|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? Should it be separate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rtnl macro uses this
Self(0) | ||
} | ||
|
||
/// Add flag to Self. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Add flag to Self. | |
/// Inserts flag into the set. |
/// Remove the given flag from Self. | ||
#[inline] | ||
pub fn remove(&mut self, flag: u64) { | ||
self.0 &= !(flag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the parenthesis needed? Is there no warning about it?
self.0 &= !(flag); | |
self.0 &= !flag; |
self.0 |= flag; | ||
} | ||
|
||
/// Remove the given flag from Self. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Remove the given flag from Self. | |
/// Removes a flag from the set. |
/// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say self.ptr
is always valid by the type invariant.
self.get_internal().len | ||
} | ||
|
||
/// Get the Shared info for this SKBuffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid repeating "for this SKBuffer", "for Self", etc. if they are methods.
Also, not sure why Shared
is capitalized.
/// Get the Shared info for this SKBuffer. | |
/// Get shared information. |
self.end_pointer() as *mut bindings::skb_shared_info | ||
} | ||
|
||
// NET_SKBUFF_DATA_USES_OFFSET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a CONFIG
, perhaps we can be more clear:
// 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.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new PointerWrapper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, types implementing this trait only have a pointer, and are not Owned like the PointerWrapper.
/// helper functions | ||
mod helpers; | ||
|
||
/// generic module/driver macros |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please capitalize like the rest of the code, end with period, etc.
Also, all these refactors should probably go first in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. But I think I can't do it until this weekend
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 |
Second attempt of #208
I will try to add the dummyrs driver in the following day as second commit.
The text was updated successfully, but these errors were encountered: