rust: add initial netdevice support by fujita · Pull Request #908 · Rust-for-Linux/linux

fujita · 2022-10-07T10:08:43Z

Added minimum abstration APIs for network device drivers.

Signed-off-by: FUJITA Tomonori fujita.tomonori@gmail.com

fujita · 2022-10-07T11:04:49Z

The CI system failure? The same code successfully passed the CIs on my repository:
https://github.com/fujita/linux/actions/runs/3203834264

Richardhongyu · 2022-10-10T13:01:24Z

+ ndo_get_tstamp: None,
+ };
+
+ const unsafe fn build_device_ops() -> &'static bindings::net_device_ops {


The unsafe function needs a specification about the condition when it's safe to use by comment Safety.

Richardhongyu · 2022-10-10T13:11:12Z

+ let ptr = unsafe { bindings::dev_get_drvdata(&mut (*netdev).dev) };
+ let dev = unsafe { Device::from_ptr(netdev) };
+ let data = unsafe { T::Data::borrow(ptr) };


It lacks the SAFETY comment. There are many other places also lacking a SAFETY comment.
Besides, It is better to use from_kernel_result to wrap a rust crate::error::Result to a Linux kernel result.
For 'net_device', I find it's more common to use netdev_priv to get data from the device.

Richardhongyu · 2022-10-10T13:33:28Z

I'm not a reviewer. Just have a look because I also wrap net_device in #886:)

fujita · 2022-10-11T10:41:23Z

I'm not a reviewer. Just have a look because I also wrap net_device in #886:)

Thanks for the pointer.

How do a device driver initialize the driver-defined data (that netdev_priv() points to)?
How free_netdev() is called?

Richardhongyu · 2022-10-14T14:01:10Z

How do a device driver initialize the driver-defined data (that netdev_priv() points to)?
How free_netdev() is called?

It seems that I lack a Registration of 'bindings::net_device'. The driver-defined data can be initialized in the init function of the Registration.

BTW, our PRs both wrap the net_device and have similarities and differences. I wonder whether you're willing to merge them and make the net_device wrapped more complete. If not, it's totally okay:)

fujita · 2022-10-16T23:17:29Z

It seems that I lack a Registration of 'bindings::net_device'. The driver-defined data can be initialized in the init function of the Registration.

How it can be with netdev_priv()? Passing uninitialized memory to a driver?

BTW, our PRs both wrap the net_device and have similarities and differences. I wonder whether you're willing to merge them and make the net_device wrapped more complete. If not, it's totally okay:)

Surely. Your PR includes most of net_device_ops and ethtool_ops. I don't think that there isn't much to discuss about how to handle them. Once the registration/unregistration is merged with this PR, I'll add them.

Richardhongyu · 2022-10-17T04:07:22Z

It seems that I lack a Registration of 'bindings::net_device'. The driver-defined data can be initialized in the init function of the Registration.
How it can be with netdev_priv()? Passing uninitialized memory to a driver?

For virtio-net, the driver-defined data is initialized in the probe function. We could use the try_new function to create a net_device at the beginning of the probe function. In the try_new function, the private data size of alloc_etherdev_mq is a pointer size. After all the driver-defined data is prepared, we use register to add the address of the driver-defined data to the pointer where netdev_priv returns.

The above procedure can be constrainted in the # Safety comment of the build_foo_ops.

fujita · 2022-10-17T05:00:57Z

For virtio-net, the driver-defined data is initialized in the probe function. We could use the try_new function to create a net_device at the beginning of the probe function. In the try_new function, the private data size of alloc_etherdev_mq is a pointer size. After all the driver-defined data is prepared, we use register to add the address of the driver-defined data to the pointer where netdev_priv returns.

In the probe function, the driver calculates a pointer to the driver-defined data by using netdev_priv()? Then the driver initialized the uninitialized memory area and registers the address to the core (the core knows the address though)?

You have your virtio-net code somewhere? Still not sure how your registration works.

btw, updated the code; adds SAFETY comments and uses from_kernel_result! for net_device_ops.

Richardhongyu · 2022-10-17T06:36:18Z

I just merge part of the Registration code for explanation and it cannot compile yet.

The net_device is created in the probe function. The device has a private data of a pointer size in the try_new function. The private data can be added to the net_device in the probe_callback function later.

fujita · 2022-10-17T06:51:47Z

virtio abstraction should not use netdev_priv()? What about other virtio drivers such as block?
https://github.com/Richardhongyu/linux/blob/30692ea965af5fee933db0571c1d6eb625ad4507/rust/kernel/virtio.rs#L90

Richardhongyu · 2022-10-17T07:13:08Z

The previous version has a problem.

The private data should be added to the net_device in the probe function. This is done by storing the pointer where netdev_privreturns.

The virtio_device should have its this private data by the probe_callback function.

fujita · 2022-10-17T07:26:02Z

I see what you try to do with netdev_priv() but it's not an expected way to use netdev_priv(); allocating the size of a pointer with net_device struct. An expected way is allocating the size of driver-defined data so a driver can efficiently access to the driver-defined data with net_device object.

Richardhongyu · 2022-10-17T12:26:28Z

I see what you try to do with netdev_priv() but it's not an expected way to use netdev_priv(); allocating the size of a pointer with net_device struct. An expected way is allocating the size of driver-defined data so a driver can efficiently access to the driver-defined data with net_device object.

Yes. This will lose the efficience of cache the data in the close area, making it similar to using 'dev->driver_data' in performance.

fujita · 2022-10-17T12:34:32Z

I think that using netdev_priv() in an expected way is the best option but as I said before I'm not sure how to do cleanly for Rust. If it can't, I prefer dev_set_drvdata because it's designed to save a pointer to driver-defined data. netdev_priv() isn't designed for that.

Richardhongyu · 2022-10-17T12:46:55Z

I think that using netdev_priv() in an expected way is the best option but as I said before I'm not sure how to do cleanly for Rust. If it can't, I prefer dev_set_drvdata because it's designed to save a pointer to driver-defined data. netdev_priv() isn't designed for that.

One thing I'm worried is that I don't know whether the data pointed by netdev_priv and dev->driver_data can be used at the same time. If this happens, where should we put the data that was originally in dev->driver_data?

fujita · 2022-10-19T00:57:18Z

Why you needs to use both netdev_priv and dev->driver_data? dev->driver_data should be enough for drivers.

If this happens, where should we put the data that was originally in dev->driver_data?

What do you mean? net_device.dev->driver_data is NULL.

Richardhongyu · 2022-10-19T08:30:58Z

If this happens, where should we put the data that was originally in dev->driver_data?
What do you mean? net_device.dev->driver_data is NULL.

Are you sure that net_device.dev->driver_data is NULL for all the net_device?

fujita · 2022-10-19T10:44:50Z

Yeah, I can't find any places where networking core touches dev->driver_data of net_device that a driver allocates. A driver allocates a net_device object then its dev->driver_data is for the driver, I think.

Richardhongyu · 2022-10-19T12:51:09Z

I may find an example. This net_device use the address returned by netdev_priv first. Then it uses dev_set_drvdata on its dev later.

fujita · 2022-10-19T13:04:16Z

Hmm, this driver doesn't use dev_get_drvdata? It uses netdev_priv everywhere.
Anyway, this driver just shows that a driver can safely use net_device.dev->driver_data.

Richardhongyu · 2022-10-19T14:35:31Z

That driver does only use dev_set_drvdata but not use dev_get_drvdata. It seems that dev_set_drvdata is redundant in this case. Thus, it is very likely Linux drivers don't use netdev_priv(net_device) and net_device.dev->driver_data at the same time.

I will check whether there is a way to use netdev_priv() in a standard way. If not, the dev_get_drvdata is a better way. Thanks for your patient explanation!

Lymkwi · 2023-03-01T14:03:56Z

Your code looks quite promising! The only issue I see is a couple missing SAFETY notices here and there.

EDIT: Actually I noticed you're still using PointerWrapper, I forgot it was renamed to ForeignOwnable. You should probably reflect that change in your code.

I am also not a reviewer, just yet another person eager to develop network drivers for Linux in Rust. The kernel is in dire need of an API on the Rust side to write network drivers so that every person who wants to write one doesn't have to reinvent the wheel so to speak. As I see it right now, this PR is a very good base that doesn't break the way things like net::Device have been designed so far, while still giving very good fundamental tools. Regardless of its merge status, I will base the work I need to get done right now on yours.

I have a question however, wouldn't it be time to split everything related to Device off into its own little module within kernel::net? The Device, its operations, registration, flags, etc, are going to take a lot of space (especially everything related to flags if they end up being handled similarly to #439 ). Should everything about devices stay at the root of net for now because of how central it is to the module?
Maybe I missed a decision regarding module organization, I've only been looking from the sidelines for a while.
I hope there can be more work to properly enable network driver writing in Rust in the near future.

Added minimum abstration APIs for network device drivers. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

fujita · 2023-03-07T15:02:13Z

Thanks for the comments! I've just update the code against the latest linux-rust branch.

I have a question however, wouldn't it be time to split everything related to Device off into its own little module within kernel::net?

It was just easier for me to submit a single module for initial submission. But it will be split in the future.

Richardhongyu reviewed Oct 10, 2022
View reviewed changes

rust: add initial netdevice support
da5da5d
Added minimum abstration APIs for network device drivers. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

fujita force-pushed the rust-netdev branch from 06db922 to da5da5dCompare March 7, 2023 14:51

fujita closed this Jun 14, 2023

fujita deleted the rust-netdev branch June 14, 2023 23:58

Uh oh!

Conversation

fujita commented Oct 7, 2022

Uh oh!

fujita commented Oct 7, 2022

Uh oh!

RichardhongyuOct 10, 2022

Choose a reason for hiding this comment

Uh oh!

RichardhongyuOct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Richardhongyu commented Oct 10, 2022

Uh oh!

fujita commented Oct 11, 2022

Uh oh!

Richardhongyu commented Oct 14, 2022

Uh oh!

fujita commented Oct 16, 2022

Uh oh!

Richardhongyu commented Oct 17, 2022

Uh oh!

fujita commented Oct 17, 2022

Uh oh!

Richardhongyu commented Oct 17, 2022

Uh oh!

fujita commented Oct 17, 2022

Uh oh!

Richardhongyu commented Oct 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujita commented Oct 17, 2022

Uh oh!

Richardhongyu commented Oct 17, 2022

Uh oh!

fujita commented Oct 17, 2022

Uh oh!

Richardhongyu commented Oct 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujita commented Oct 19, 2022

Uh oh!

Richardhongyu commented Oct 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujita commented Oct 19, 2022

Uh oh!

Richardhongyu commented Oct 19, 2022

Uh oh!

fujita commented Oct 19, 2022

Uh oh!

Richardhongyu commented Oct 19, 2022

Uh oh!

Lymkwi commented Mar 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujita commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Richardhongyu commented Oct 17, 2022•
edited
Loading

Richardhongyu commented Oct 17, 2022•
edited
Loading

Richardhongyu commented Oct 19, 2022•
edited
Loading

Lymkwi commented Mar 1, 2023•
edited
Loading