[PATCH 1/5] rust: core abstractions for network device drivers

[PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

This patch adds very basic abstractions to implement network device
drivers, corresponds to the kernel's net_device and net_device_ops
structs with support for register_netdev/unregister_netdev functions.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/bindings/bindings_helper.h |   2 +
 rust/helpers.c                  |  16 ++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/net.rs              |   5 +
 rust/kernel/net/dev.rs          | 380 ++++++++++++++++++++++++++++++++
 5 files changed, 405 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/dev.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 50e7a76d5455..e3ebaaf9ab4f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,8 @@
  * Sorted alphabetically.
  */
 
+#include <linux/etherdevice.h>
+#include <linux/netdevice.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
 #include <linux/wait.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..a91d2a99035b 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -23,10 +23,26 @@
 #include <linux/err.h>
 #include <linux/refcount.h>
 #include <linux/mutex.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
 #include <linux/spinlock.h>
 #include <linux/sched/signal.h>
 #include <linux/wait.h>
 
+#ifdef CONFIG_NET
+void *rust_helper_netdev_priv(const struct net_device *dev)
+{
+	return netdev_priv(dev);
+}
+EXPORT_SYMBOL_GPL(rust_helper_netdev_priv);
+
+void rust_helper_skb_tx_timestamp(struct sk_buff *skb)
+{
+	skb_tx_timestamp(skb);
+}
+EXPORT_SYMBOL_GPL(rust_helper_skb_tx_timestamp);
+#endif
+
 __noreturn void rust_helper_BUG(void)
 {
 	BUG();
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 85b261209977..8e6926d965e1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -34,6 +34,8 @@
 pub mod error;
 pub mod init;
 pub mod ioctl;
+#[cfg(CONFIG_NET)]
+pub mod net;
 pub mod prelude;
 pub mod print;
 mod static_assert;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
new file mode 100644
index 000000000000..28fe8f398463
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking core.
+
+pub mod dev;
diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
new file mode 100644
index 000000000000..0ef8a78ad0bf
--- /dev/null
+++ b/rust/kernel/net/dev.rs
@@ -0,0 +1,380 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Network device.
+//!
+//! C headers: [`include/linux/etherdevice.h`](../../../../include/linux/etherdevice.h),
+//! [`include/linux/ethtool.h`](../../../../include/linux/ethtool.h),
+//! [`include/linux/netdevice.h`](../../../../include/linux/netdevice.h),
+//! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h),
+//! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h).
+
+use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable};
+use core::marker::PhantomData;
+
+/// Corresponds to the kernel's `struct net_device`.
+///
+/// # Safety
+///
+/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
+/// This object is passed to Rust callbacks while these functions are running.
+/// The C API guarantees that `net_device` isn't released while this function is running.
+pub struct Device(*mut bindings::net_device);
+
+impl Device {
+    /// Gets a pointer to network device private data.
+    ///
+    /// A driver in C uses contiguous memory `struct net_device` and its private data.
+    /// A driver in Rust contiguous memory `struct net_device` and a pointer to its private data.
+    /// So a driver in Rust pays extra cost to access to its private data.
+    /// A device driver passes an properly initialized private data and the kernel saves its pointer.
+    fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const core::ffi::c_void {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { *(bindings::netdev_priv(&mut *netdev) as *const usize) as _ }
+    }
+}
+
+/// Trait for device driver specific information.
+///
+/// This data structure is passed to a driver with the operations for `struct net_device`
+/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc.
+pub trait DriverData {
+    /// The object are stored in C object, `struct net_device`.
+    type Data: ForeignOwnable + Send + Sync;
+}
+
+/// Registration structure for a network device driver.
+///
+/// This allocates and owns a `struct net_device` object.
+/// Once the `net_device` object is registered via `register_netdev` function,
+/// the kernel calls various functions such as `struct net_device_ops` operations with
+/// the `net_device` object.
+///
+/// A driver must implement `struct net_device_ops` so the trait for it is tied.
+/// Other operations like `struct ethtool_ops` are optional.
+///
+/// # Safety
+///
+/// The pointer to the `net_device` object is guaranteed to be valid until
+/// the registration object is dropped.
+pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
+    dev: Device,
+    is_registered: bool,
+    _p: PhantomData<(D, T)>,
+}
+
+impl<D: DriverData, T: DeviceOperations<D>> Drop for Registration<T, D> {
+    fn drop(&mut self) {
+        // SAFETY: `dev` was allocated during initialization and guaranteed to be valid.
+        unsafe {
+            if self.is_registered {
+                bindings::unregister_netdev(self.dev.0);
+            }
+            let _ = D::Data::from_foreign(Device::priv_data_ptr(self.dev.0));
+            bindings::free_netdev(self.dev.0);
+        }
+    }
+}
+
+impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
+    /// Creates new instance of registration.
+    pub fn try_new(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> {
+        // SAFETY: FFI call.
+        match from_err_ptr(unsafe {
+            bindings::alloc_etherdev_mqs(
+                core::mem::size_of::<*const core::ffi::c_void>() as i32,
+                tx_queue_size,
+                rx_queue_size,
+            )
+        }) {
+            Ok(ptr) => {
+                // SAFETY: the kernel allocated the space for a pointer.
+                unsafe {
+                    let priv_ptr = bindings::netdev_priv(ptr) as *mut usize;
+                    core::ptr::write(priv_ptr, data.into_foreign() as usize);
+                }
+                Ok(Registration {
+                    dev: Device(ptr),
+                    is_registered: false,
+                    _p: PhantomData,
+                })
+            }
+            Err(err) => Err(err),
+        }
+    }
+
+    /// Returns a network device.
+    ///
+    /// A device driver normally configures the device before registration.
+    pub fn dev_get(&mut self) -> &mut Device {
+        &mut self.dev
+    }
+
+    /// Registers a network device.
+    pub fn register(&mut self) -> Result {
+        // SAFETY: `dev` was allocated during initialization and is guaranteed to be valid.
+        let ret = unsafe {
+            (*self.dev.0).netdev_ops = Self::build_device_ops();
+            bindings::register_netdev(self.dev.0)
+        };
+        if ret != 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            self.is_registered = true;
+            Ok(())
+        }
+    }
+
+    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
+        ndo_init: if <T>::HAS_INIT {
+            Some(Self::init_callback)
+        } else {
+            None
+        },
+        ndo_uninit: if <T>::HAS_UNINIT {
+            Some(Self::uninit_callback)
+        } else {
+            None
+        },
+        ndo_open: if <T>::HAS_OPEN {
+            Some(Self::open_callback)
+        } else {
+            None
+        },
+        ndo_stop: if <T>::HAS_STOP {
+            Some(Self::stop_callback)
+        } else {
+            None
+        },
+        ndo_start_xmit: if <T>::HAS_START_XMIT {
+            Some(Self::start_xmit_callback)
+        } else {
+            None
+        },
+        ndo_features_check: None,
+        ndo_select_queue: None,
+        ndo_change_rx_flags: None,
+        ndo_set_rx_mode: None,
+        ndo_set_mac_address: None,
+        ndo_validate_addr: None,
+        ndo_do_ioctl: None,
+        ndo_eth_ioctl: None,
+        ndo_siocbond: None,
+        ndo_siocwandev: None,
+        ndo_siocdevprivate: None,
+        ndo_set_config: None,
+        ndo_change_mtu: None,
+        ndo_neigh_setup: None,
+        ndo_tx_timeout: None,
+        ndo_get_stats64: None,
+        ndo_has_offload_stats: None,
+        ndo_get_offload_stats: None,
+        ndo_get_stats: None,
+        ndo_vlan_rx_add_vid: None,
+        ndo_vlan_rx_kill_vid: None,
+        #[cfg(CONFIG_NET_POLL_CONTROLLER)]
+        ndo_poll_controller: None,
+        #[cfg(CONFIG_NET_POLL_CONTROLLER)]
+        ndo_netpoll_setup: None,
+        #[cfg(CONFIG_NET_POLL_CONTROLLER)]
+        ndo_netpoll_cleanup: None,
+        ndo_set_vf_mac: None,
+        ndo_set_vf_vlan: None,
+        ndo_set_vf_rate: None,
+        ndo_set_vf_spoofchk: None,
+        ndo_set_vf_trust: None,
+        ndo_get_vf_config: None,
+        ndo_set_vf_link_state: None,
+        ndo_get_vf_stats: None,
+        ndo_set_vf_port: None,
+        ndo_get_vf_port: None,
+        ndo_get_vf_guid: None,
+        ndo_set_vf_guid: None,
+        ndo_set_vf_rss_query_en: None,
+        ndo_setup_tc: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_enable: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_disable: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_ddp_setup: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_ddp_done: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_ddp_target: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_get_hbainfo: None,
+        #[cfg(CONFIG_FCOE)]
+        ndo_fcoe_get_wwn: None,
+        #[cfg(CONFIG_RFS_ACCEL)]
+        ndo_rx_flow_steer: None,
+        ndo_add_slave: None,
+        ndo_del_slave: None,
+        ndo_get_xmit_slave: None,
+        ndo_sk_get_lower_dev: None,
+        ndo_fix_features: None,
+        ndo_set_features: None,
+        ndo_neigh_construct: None,
+        ndo_neigh_destroy: None,
+        ndo_fdb_add: None,
+        ndo_fdb_del: None,
+        ndo_fdb_del_bulk: None,
+        ndo_fdb_dump: None,
+        ndo_fdb_get: None,
+        ndo_mdb_add: None,
+        ndo_mdb_del: None,
+        ndo_mdb_dump: None,
+        ndo_bridge_setlink: None,
+        ndo_bridge_getlink: None,
+        ndo_bridge_dellink: None,
+        ndo_change_carrier: None,
+        ndo_get_phys_port_id: None,
+        ndo_get_port_parent_id: None,
+        ndo_get_phys_port_name: None,
+        ndo_dfwd_add_station: None,
+        ndo_dfwd_del_station: None,
+        ndo_set_tx_maxrate: None,
+        ndo_get_iflink: None,
+        ndo_fill_metadata_dst: None,
+        ndo_set_rx_headroom: None,
+        ndo_bpf: None,
+        ndo_xdp_xmit: None,
+        ndo_xdp_get_xmit_slave: None,
+        ndo_xsk_wakeup: None,
+        ndo_tunnel_ctl: None,
+        ndo_get_peer_dev: None,
+        ndo_fill_forward_path: None,
+        ndo_get_tstamp: None,
+    };
+
+    const fn build_device_ops() -> &'static bindings::net_device_ops {
+        &Self::DEVICE_OPS
+    }
+
+    unsafe extern "C" fn init_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            T::init(&mut dev, data)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn uninit_callback(netdev: *mut bindings::net_device) {
+        // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+        let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+        let mut dev = Device(netdev);
+        T::uninit(&mut dev, data);
+    }
+
+    unsafe extern "C" fn open_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            T::open(&mut dev, data)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn stop_callback(netdev: *mut bindings::net_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            T::stop(&mut dev, data)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn start_xmit_callback(
+        skb: *mut bindings::sk_buff,
+        netdev: *mut bindings::net_device,
+    ) -> bindings::netdev_tx_t {
+        // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+        let mut dev = Device(netdev);
+        let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+        // SAFETY: The C API guarantees that `sk_buff` isn't released while this function is running.
+        let skb = SkBuff(skb);
+        T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t
+    }
+}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {}
+// SAFETY: `Registration` is not restricted to a single thread,
+unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}
+
+/// Corresponds to the kernel's `enum netdev_tx`.
+#[repr(i32)]
+pub enum TxCode {
+    /// Driver took care of packet.
+    Ok = bindings::netdev_tx_NETDEV_TX_OK,
+    /// Driver tx path was busy.
+    Busy = bindings::netdev_tx_NETDEV_TX_BUSY,
+}
+
+/// Corresponds to the kernel's `struct net_device_ops`.
+///
+/// A device driver must implement this. Only very basic operations are supported for now.
+#[vtable]
+pub trait DeviceOperations<D: DriverData> {
+    /// Corresponds to `ndo_init` in `struct net_device_ops`.
+    fn init(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_uninit` in `struct net_device_ops`.
+    fn uninit(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) {}
+
+    /// Corresponds to `ndo_open` in `struct net_device_ops`.
+    fn open(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_stop` in `struct net_device_ops`.
+    fn stop(_dev: &mut Device, _data: <D::Data as ForeignOwnable>::Borrowed<'_>) -> Result {
+        Ok(())
+    }
+
+    /// Corresponds to `ndo_start_xmit` in `struct net_device_ops`.
+    fn start_xmit(
+        _dev: &mut Device,
+        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
+        _skb: SkBuff,
+    ) -> TxCode {
+        TxCode::Busy
+    }
+}
+
+/// Corresponds to the kernel's `struct sk_buff`.
+///
+/// A driver manages `struct sk_buff` in two ways. In both ways, the ownership is transferred
+/// between C and Rust. The allocation and release are done asymmetrically.
+///
+/// On the tx side (`ndo_start_xmit` operation in `struct net_device_ops`), the kernel allocates
+/// a `sk_buff' object and passes it to the driver. The driver is responsible for the release after transmission.
+/// On the rx side, the driver allocates a `sk_buff` object then passes it to the kernel after receiving data.
+pub struct SkBuff(*mut bindings::sk_buff);
+
+impl SkBuff {
+    /// Provides a time stamp.
+    pub fn tx_timestamp(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            bindings::skb_tx_timestamp(self.0);
+        }
+    }
+}
+
+impl Drop for SkBuff {
+    fn drop(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            bindings::kfree_skb_reason(
+                self.0,
+                bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED,
+            )
+        }
+    }
+}
-- 
2.34.1

[PATCH 3/5] rust: add support for get_stats64 in struct net_device_ops

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

get_stats64() is used to return the stats for user-space like the
number of packets, which network device drivers are supposed to
support.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/dev.rs | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index 535b0e6f6424..7681de529682 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -178,7 +178,11 @@ pub fn set_ether_operations<U>(&mut self, adapter: EtherOperationsAdapter<U, D>)
         ndo_change_mtu: None,
         ndo_neigh_setup: None,
         ndo_tx_timeout: None,
-        ndo_get_stats64: None,
+        ndo_get_stats64: if <T>::HAS_GET_STATS64 {
+            Some(Self::get_stats64_callback)
+        } else {
+            None
+        },
         ndo_has_offload_stats: None,
         ndo_get_offload_stats: None,
         ndo_get_stats: None,
@@ -311,6 +315,16 @@ const fn build_device_ops() -> &'static bindings::net_device_ops {
         let skb = SkBuff(skb);
         T::start_xmit(&mut dev, data, skb) as bindings::netdev_tx_t
     }
+
+    unsafe extern "C" fn get_stats64_callback(
+        netdev: *mut bindings::net_device,
+        stats: *mut bindings::rtnl_link_stats64,
+    ) {
+        // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+        let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+        let mut dev = Device(netdev);
+        T::get_stats64(&mut dev, data, &mut RtnlLinkStats64(stats));
+    }
 }
 
 // SAFETY: `Registration` does not expose any of its state across threads.
@@ -358,6 +372,14 @@ fn start_xmit(
     ) -> TxCode {
         TxCode::Busy
     }
+
+    /// Corresponds to `ndo_get_stats64` in `struct net_device_ops`.
+    fn get_stats64(
+        _dev: &mut Device,
+        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
+        _stats: &mut RtnlLinkStats64,
+    ) {
+    }
 }
 
 /// Corresponds to the kernel's `struct sk_buff`.
@@ -392,6 +414,22 @@ fn drop(&mut self) {
     }
 }
 
+/// Corresponds to the kernel's `struct rtnl_link_stats64`.
+pub struct RtnlLinkStats64(*mut bindings::rtnl_link_stats64);
+
+impl RtnlLinkStats64 {
+    /// Updates TX stats.
+    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).tx_packets = packets;
+            (*self.0).tx_bytes = bytes;
+            (*self.0).tx_errors = errors;
+            (*self.0).tx_dropped = dropped;
+        }
+    }
+}
+
 /// Builds the kernel's `struct ethtool_ops`.
 pub struct EtherOperationsAdapter<D, T> {
     _p: PhantomData<(D, T)>,
-- 
2.34.1

[PATCH 2/5] rust: add support for ethernet operations

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

This improves abstractions for network device drivers to implement
struct ethtool_ops, the majority of ethernet device drivers need to
do.

struct ethtool_ops also needs to access to device private data like
struct net_devicve_ops.

Currently, only get_ts_info operation is supported. The following
patch adds the Rust version of the dummy network driver thats uses the
feature.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/net/dev.rs          | 163 ++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e3ebaaf9ab4f..71db7ab4d92d 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -7,6 +7,7 @@
  */
 
 #include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/netdevice.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index 0ef8a78ad0bf..535b0e6f6424 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -124,6 +124,19 @@ pub fn register(&mut self) -> Result {
         }
     }
 
+    /// Sets `ethtool_ops` of `net_device`.
+    pub fn set_ether_operations<U>(&mut self, adapter: EtherOperationsAdapter<U, D>) -> Result
+    where
+        U: EtherOperations<D>,
+    {
+        if self.is_registered {
+            Err(code::EINVAL)
+        } else {
+            adapter.set_ether_ops(&mut self.dev);
+            Ok(())
+        }
+    }
+
     const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
         ndo_init: if <T>::HAS_INIT {
             Some(Self::init_callback)
@@ -378,3 +391,153 @@ fn drop(&mut self) {
         }
     }
 }
+
+/// Builds the kernel's `struct ethtool_ops`.
+pub struct EtherOperationsAdapter<D, T> {
+    _p: PhantomData<(D, T)>,
+}
+
+impl<D, T> EtherOperationsAdapter<T, D>
+where
+    D: DriverData,
+    T: EtherOperations<D>,
+{
+    /// Creates a new instance.
+    pub fn new() -> Self {
+        EtherOperationsAdapter { _p: PhantomData }
+    }
+
+    unsafe extern "C" fn get_ts_info_callback(
+        netdev: *mut bindings::net_device,
+        info: *mut bindings::ethtool_ts_info,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
+            let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
+            let mut dev = Device(netdev);
+            let mut info = EthtoolTsInfo(info);
+            T::get_ts_info(&mut dev, data, &mut info)?;
+            Ok(0)
+        })
+    }
+
+    const ETHER_OPS: bindings::ethtool_ops = bindings::ethtool_ops {
+        _bitfield_1: bindings::__BindgenBitfieldUnit::new([0; 1]),
+        supported_coalesce_params: 0,
+        supported_ring_params: 0,
+        get_drvinfo: None,
+        get_regs_len: None,
+        get_regs: None,
+        get_wol: None,
+        set_wol: None,
+        get_msglevel: None,
+        set_msglevel: None,
+        nway_reset: None,
+        get_link: None,
+        get_link_ext_state: None,
+        get_link_ext_stats: None,
+        get_eeprom_len: None,
+        get_eeprom: None,
+        set_eeprom: None,
+        get_coalesce: None,
+        set_coalesce: None,
+        get_ringparam: None,
+        set_ringparam: None,
+        get_pause_stats: None,
+        get_pauseparam: None,
+        set_pauseparam: None,
+        self_test: None,
+        get_strings: None,
+        set_phys_id: None,
+        get_ethtool_stats: None,
+        begin: None,
+        complete: None,
+        get_priv_flags: None,
+        set_priv_flags: None,
+        get_sset_count: None,
+        get_rxnfc: None,
+        set_rxnfc: None,
+        flash_device: None,
+        reset: None,
+        get_rxfh_key_size: None,
+        get_rxfh_indir_size: None,
+        get_rxfh: None,
+        set_rxfh: None,
+        get_rxfh_context: None,
+        set_rxfh_context: None,
+        get_channels: None,
+        set_channels: None,
+        get_dump_flag: None,
+        get_dump_data: None,
+        set_dump: None,
+        get_ts_info: if <T>::HAS_GET_TS_INFO {
+            Some(Self::get_ts_info_callback)
+        } else {
+            None
+        },
+        get_module_info: None,
+        get_module_eeprom: None,
+        get_eee: None,
+        set_eee: None,
+        get_tunable: None,
+        set_tunable: None,
+        get_per_queue_coalesce: None,
+        set_per_queue_coalesce: None,
+        get_link_ksettings: None,
+        set_link_ksettings: None,
+        get_fec_stats: None,
+        get_fecparam: None,
+        set_fecparam: None,
+        get_ethtool_phy_stats: None,
+        get_phy_tunable: None,
+        set_phy_tunable: None,
+        get_module_eeprom_by_page: None,
+        get_eth_phy_stats: None,
+        get_eth_mac_stats: None,
+        get_eth_ctrl_stats: None,
+        get_rmon_stats: None,
+        get_module_power_mode: None,
+        set_module_power_mode: None,
+        get_mm: None,
+        set_mm: None,
+        get_mm_stats: None,
+    };
+
+    const fn build_ether_ops() -> &'static bindings::ethtool_ops {
+        &Self::ETHER_OPS
+    }
+
+    fn set_ether_ops(&self, dev: &mut Device) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*dev.0).ethtool_ops = Self::build_ether_ops();
+        }
+    }
+}
+
+/// Corresponds to the kernel's `struct ethtool_ops`.
+#[vtable]
+pub trait EtherOperations<D: DriverData> {
+    /// Corresponds to `get_ts_info` in `struct ethtool_ops`.
+    fn get_ts_info(
+        _dev: &mut Device,
+        _data: <D::Data as ForeignOwnable>::Borrowed<'_>,
+        _info: &mut EthtoolTsInfo,
+    ) -> Result {
+        Err(Error::from_errno(bindings::EOPNOTSUPP as i32))
+    }
+}
+
+/// Corresponds to the kernel's `struct ethtool_ts_info`.
+pub struct EthtoolTsInfo(*mut bindings::ethtool_ts_info);
+
+/// Sets up the info for software timestamping.
+///
+/// This can be [`Device`] method, but there are non ethernet devices.
+pub fn ethtool_op_get_ts_info(dev: &mut Device, info: &mut EthtoolTsInfo) -> Result {
+    // SAFETY: The safety requirement ensures that the pointer is valid.
+    unsafe {
+        bindings::ethtool_op_get_ts_info(dev.0, info.0);
+    }
+    Ok(())
+}
-- 
2.34.1

[PATCH 4/5] rust: add methods for configure net_device

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

adds methods to net::Device for the basic configurations of
net_device.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers.c         |  7 ++++
 rust/kernel/net/dev.rs | 77 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index a91d2a99035b..7c280c552d64 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -21,6 +21,7 @@
 #include <linux/bug.h>
 #include <linux/build_bug.h>
 #include <linux/err.h>
+#include <linux/etherdevice.h>
 #include <linux/refcount.h>
 #include <linux/mutex.h>
 #include <linux/netdevice.h>
@@ -30,6 +31,12 @@
 #include <linux/wait.h>
 
 #ifdef CONFIG_NET
+void rust_helper_eth_hw_addr_random(struct net_device *dev)
+{
+	eth_hw_addr_random(dev);
+}
+EXPORT_SYMBOL_GPL(rust_helper_eth_hw_addr_random);
+
 void *rust_helper_netdev_priv(const struct net_device *dev)
 {
 	return netdev_priv(dev);
diff --git a/rust/kernel/net/dev.rs b/rust/kernel/net/dev.rs
index 7681de529682..4fafd7e73112 100644
--- a/rust/kernel/net/dev.rs
+++ b/rust/kernel/net/dev.rs
@@ -8,7 +8,7 @@
 //! [`include/linux/skbuff.h`](../../../../include/linux/skbuff.h),
 //! [`include/uapi/linux/if_link.h`](../../../../include/uapi/linux/if_link.h).
 
-use crate::{bindings, error::*, prelude::vtable, types::ForeignOwnable};
+use crate::{bindings, error::*, prelude::vtable, str::CStr, types::ForeignOwnable};
 use core::marker::PhantomData;
 
 /// Corresponds to the kernel's `struct net_device`.
@@ -31,6 +31,81 @@ fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const core::ffi::c_void
         // SAFETY: The safety requirement ensures that the pointer is valid.
         unsafe { *(bindings::netdev_priv(&mut *netdev) as *const usize) as _ }
     }
+
+    /// Sets the name of a device.
+    pub fn set_name(&mut self, name: &'static CStr) -> Result {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            if name.len() > (*self.0).name.len() {
+                return Err(code::EINVAL);
+            }
+            for i in 0..name.len() {
+                (*self.0).name[i] = name[i] as i8;
+            }
+        }
+        Ok(())
+    }
+
+    /// Sets carrier.
+    pub fn netif_carrier_on(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { bindings::netif_carrier_on(self.0) }
+    }
+
+    /// Clears carrier.
+    pub fn netif_carrier_off(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { bindings::netif_carrier_off(self.0) }
+    }
+
+    /// Sets the max mtu of the device.
+    pub fn set_max_mtu(&mut self, max_mtu: u32) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).max_mtu = max_mtu;
+        }
+    }
+
+    /// Sets the minimum mtu of the device.
+    pub fn set_min_mtu(&mut self, min_mtu: u32) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).min_mtu = min_mtu;
+        }
+    }
+
+    /// Returns the flags of the device.
+    pub fn get_flags(&self) -> u32 {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { (*self.0).flags }
+    }
+
+    /// Sets the flags of the device.
+    pub fn set_flags(&mut self, flags: u32) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe {
+            (*self.0).flags = flags;
+        }
+    }
+
+    /// Returns the priv_flags of the device.
+    pub fn get_priv_flags(&self) -> u64 {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { (*self.0).priv_flags }
+    }
+
+    /// Sets the priv_flags of the device.
+    pub fn set_priv_flags(&mut self, flags: u64) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { (*self.0).priv_flags = flags }
+    }
+
+    /// Generate a random Ethernet address (MAC) to be used by a net device
+    /// and set addr_assign_type.
+    pub fn set_random_eth_hw_addr(&mut self) {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        unsafe { bindings::eth_hw_addr_random(self.0) }
+    }
 }
 
 /// Trait for device driver specific information.
-- 
2.34.1

[PATCH 5/5] samples: rust: add dummy network driver sample

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

This is a simpler version of drivers/net/dummy.c.

This demonstrates the usage of abstractions for network device drivers.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 samples/rust/Kconfig       | 11 +++++
 samples/rust/Makefile      |  1 +
 samples/rust/rust_dummy.rs | 98 ++++++++++++++++++++++++++++++++++++++
 scripts/Makefile.build     |  2 +-
 4 files changed, 111 insertions(+), 1 deletion(-)
 create mode 100644 samples/rust/rust_dummy.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..0b528c0bed3e 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,17 @@ config SAMPLE_RUST_PRINT
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DUMMY
+	tristate "Dummy network driver"
+	help
+	  This is the simpler version of drivers/net/dummy.c. No intention to replace it. This provides educational information for Rust
+	  abstractions for network drivers.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_minimal.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 03086dabbea4..c01994baaf8a 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,5 +2,6 @@
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_DUMMY)			+= rust_dummy.o
 
 subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS)		+= hostprogs
diff --git a/samples/rust/rust_dummy.rs b/samples/rust/rust_dummy.rs
new file mode 100644
index 000000000000..b551e7d3331e
--- /dev/null
+++ b/samples/rust/rust_dummy.rs
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+//! Rust dummy netdev.
+
+use core::sync::atomic::{AtomicU64, Ordering};
+use kernel::{
+    bindings, c_str,
+    net::dev::{
+        ethtool_op_get_ts_info, Device, DeviceOperations, DriverData, EtherOperations,
+        EtherOperationsAdapter, EthtoolTsInfo, Registration, RtnlLinkStats64, SkBuff, TxCode,
+    },
+    prelude::*,
+};
+
+struct DevOps {}
+
+#[vtable]
+impl<D: DriverData<Data = Box<Stats>>> DeviceOperations<D> for DevOps {
+    fn init(_dev: &mut Device, data: &Stats) -> Result {
+        data.packets.store(0, Ordering::Relaxed);
+        data.bytes.store(0, Ordering::Relaxed);
+        Ok(())
+    }
+
+    fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
+        data.packets.fetch_add(1, Ordering::Relaxed);
+        skb.tx_timestamp();
+        TxCode::Ok
+    }
+
+    fn get_stats64(_dev: &mut Device, data: &Stats, stats: &mut RtnlLinkStats64) {
+        stats.set_tx_stats(data.packets.load(Ordering::Relaxed), 0, 0, 0);
+    }
+}
+
+#[allow(dead_code)]
+struct Stats {
+    packets: AtomicU64,
+    bytes: AtomicU64,
+}
+
+impl DriverData for Stats {
+    type Data = Box<Stats>;
+}
+
+struct DummyNetdev {
+    _r: Registration<DevOps, Stats>,
+}
+
+struct EtherOps {}
+
+#[vtable]
+impl<D: DriverData<Data = Box<Stats>>> EtherOperations<D> for EtherOps {
+    fn get_ts_info(dev: &mut Device, _data: &Stats, info: &mut EthtoolTsInfo) -> Result {
+        ethtool_op_get_ts_info(dev, info)
+    }
+}
+
+impl kernel::Module for DummyNetdev {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        const NAME: &'static CStr = c_str!("dummy%d");
+        let data = Box::try_new(Stats {
+            packets: AtomicU64::new(0),
+            bytes: AtomicU64::new(0),
+        })?;
+        let mut r = Registration::<DevOps, Stats>::try_new(1, 1, data)?;
+        r.set_ether_operations(EtherOperationsAdapter::<EtherOps, Stats>::new())?;
+
+        let netdev = r.dev_get();
+        netdev.set_name(NAME)?;
+
+        netdev.set_flags(
+            netdev.get_flags()
+                | bindings::net_device_flags_IFF_NOARP & !bindings::net_device_flags_IFF_MULTICAST,
+        );
+        netdev.set_priv_flags(
+            netdev.get_priv_flags()
+                | bindings::netdev_priv_flags_IFF_LIVE_ADDR_CHANGE
+                | bindings::netdev_priv_flags_IFF_NO_QUEUE,
+        );
+        netdev.set_random_eth_hw_addr();
+        netdev.set_min_mtu(0);
+        netdev.set_max_mtu(0);
+
+        r.register()?;
+
+        pr_info!("Hello Rust dummy netdev!");
+        Ok(DummyNetdev { _r: r })
+    }
+}
+
+module! {
+    type: DummyNetdev,
+    name: "rust_dummy",
+    author: "Rust for Linux Contributors",
+    description: "Rust dummy netdev",
+    license: "GPL v2",
+}
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 78175231c969..1404967e908e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := new_uninit
+rust_allowed_features := allocator_api,new_uninit
 
 rust_common_cmd = \
 	RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \
-- 
2.34.1

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Alice Ryhl]

FUJITA Tomonori writes:
> +/// Corresponds to the kernel's `struct net_device`.
> +///
> +/// # Safety
> +///
> +/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
> +/// This object is passed to Rust callbacks while these functions are running.
> +/// The C API guarantees that `net_device` isn't released while this function is running.
> +pub struct Device(*mut bindings::net_device);

You don't implement `Send` or `Sync` for this type. But maybe that's
intentional since you don't know what kind of private data it stores?

> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {}
> +// SAFETY: `Registration` is not restricted to a single thread,
> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}

This does expose the `D::Data` object across threads as far as I can
tell. That type is required to be Send/Sync, so this is still ok, but I
would mention that here.

> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
> +    /// Creates new instance of registration.
> +    pub fn try_new(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> {
> +        // SAFETY: FFI call.
> +        match from_err_ptr(unsafe {
> +            bindings::alloc_etherdev_mqs(
> +                core::mem::size_of::<*const core::ffi::c_void>() as i32,
> +                tx_queue_size,
> +                rx_queue_size,
> +            )
> +        }) {
> +            Ok(ptr) => {
> +                // SAFETY: the kernel allocated the space for a pointer.
> +                unsafe {
> +                    let priv_ptr = bindings::netdev_priv(ptr) as *mut usize;
> +                    core::ptr::write(priv_ptr, data.into_foreign() as usize);
> +                }
> +                Ok(Registration {
> +                    dev: Device(ptr),
> +                    is_registered: false,
> +                    _p: PhantomData,
> +                })
> +            }
> +            Err(err) => Err(err),
> +        }
> +    }

This method can be simplified using the question mark operator (also
known as the try operator).

```
pub fn try_new(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> {
    // SAFETY: FFI call.
    let ptr = from_err_ptr(unsafe {
        bindings::alloc_etherdev_mqs(
            core::mem::size_of::<*const core::ffi::c_void>() as i32,
            tx_queue_size,
            rx_queue_size,
        )
    })?;

    // SAFETY: the kernel allocated the space for a pointer.
    unsafe {
        let priv_ptr = bindings::netdev_priv(ptr) as *mut usize;
        core::ptr::write(priv_ptr, data.into_foreign() as usize);
    }
    Ok(Registration {
        dev: Device(ptr),
        is_registered: false,
        _p: PhantomData,
    })
}
```

Also, it is unclear to me why you are converting the pointers into an
usize. I would recommend keeping them as void pointers instead.
Especially considering that you're using a void pointer for the
`sizeof_priv` argument. You can do that like this:

```
use core::ffi::c_void;

pub fn try_new(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> {
    // SAFETY: FFI call.
    let ptr = from_err_ptr(unsafe {
        bindings::alloc_etherdev_mqs(
            core::mem::size_of::<*const c_void>() as i32,
            tx_queue_size,
            rx_queue_size,
        )
    })?;

    // SAFETY: the kernel allocated the space for a pointer.
    unsafe {
        let priv_ptr = bindings::netdev_priv(ptr) as *mut *const c_void;
        core::ptr::write(priv_ptr, data.into_foreign());
    }
    Ok(Registration {
        dev: Device(ptr),
        is_registered: false,
        _p: PhantomData,
    })
}

impl Device {
    /// Gets a pointer to network device private data.
    ///
    /// A driver in C uses contiguous memory `struct net_device` and its private data.
    /// A driver in Rust contiguous memory `struct net_device` and a pointer to its private data.
    /// So a driver in Rust pays extra cost to access to its private data.
    /// A device driver passes an properly initialized private data and the kernel saves its pointer.
    fn priv_data_ptr(netdev: *mut bindings::net_device) -> *const c_void {
        // SAFETY: The safety requirement ensures that the pointer is valid.
        unsafe { core::ptr::read(bindings::netdev_priv(&mut *netdev) as *const *const c_void) }
    }
}
```

> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
> +        ndo_init: if <T>::HAS_INIT {
> +            Some(Self::init_callback)
> +        } else {
> +            None
> +        },
> +        ndo_uninit: if <T>::HAS_UNINIT {
> +            Some(Self::uninit_callback)
> +        } else {
> +            None
> +        },
> +        ndo_open: if <T>::HAS_OPEN {
> +            Some(Self::open_callback)
> +        } else {
> +            None
> +        },
> +        ndo_stop: if <T>::HAS_STOP {
> +            Some(Self::stop_callback)
> +        } else {
> +            None
> +        },
> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
> +            Some(Self::start_xmit_callback)
> +        } else {
> +            None
> +        },
> +        ndo_features_check: None,
> +        ndo_select_queue: None,
> +        ndo_change_rx_flags: None,
> +        ndo_set_rx_mode: None,
> +        ndo_set_mac_address: None,

Bindgen is set up to generate `Default` implementations for these types,
so you can do this to omit the remaining fields:
```
const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
    ndo_init: if <T>::HAS_INIT {
        Some(Self::init_callback)
    } else {
        None
    },
    ndo_uninit: if <T>::HAS_UNINIT {
        Some(Self::uninit_callback)
    } else {
        None
    },
    ndo_open: if <T>::HAS_OPEN {
        Some(Self::open_callback)
    } else {
        None
    },
    ndo_stop: if <T>::HAS_STOP {
        Some(Self::stop_callback)
    } else {
        None
    },
    ndo_start_xmit: if <T>::HAS_START_XMIT {
        Some(Self::start_xmit_callback)
    } else {
        None
    },
    ..Default::default()
};
```

> +impl Drop for SkBuff {
> +    fn drop(&mut self) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            bindings::kfree_skb_reason(
> +                self.0,
> +                bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED,
> +            )
> +        }
> +    }
> +}

What are these drop reasons used for?

Alice

Re: [PATCH 2/5] rust: add support for ethernet operations

[Alice Ryhl]

FUJITA Tomonori writes:
> +    /// Sets `ethtool_ops` of `net_device`.
> +    pub fn set_ether_operations<U>(&mut self, adapter: EtherOperationsAdapter<U, D>) -> Result
> +    where
> +        U: EtherOperations<D>,
> +    {
> +        if self.is_registered {
> +            Err(code::EINVAL)
> +        } else {
> +            adapter.set_ether_ops(&mut self.dev);
> +            Ok(())
> +        }
> +    }

The `EtherOperationsAdapter` struct seems unnecessary here. How about
using this signature instead?

pub fn set_ether_operations<U: EtherOperations<D>>(&mut self) -> Result

Or even

pub fn set_ether_operations(&mut self) -> Result
where
    D: EtherOperations

Re: [PATCH 3/5] rust: add support for get_stats64 in struct net_device_ops

[Alice Ryhl]

FUJITA Tomonori writes:
> +/// Corresponds to the kernel's `struct rtnl_link_stats64`.
> +pub struct RtnlLinkStats64(*mut bindings::rtnl_link_stats64);
> +
> +impl RtnlLinkStats64 {
> +    /// Updates TX stats.
> +    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            (*self.0).tx_packets = packets;
> +            (*self.0).tx_bytes = bytes;
> +            (*self.0).tx_errors = errors;
> +            (*self.0).tx_dropped = dropped;
> +        }
> +    }
> +}

It seems like this type is always used behind a reference. Your current
approach has the disadvantage that you're introducing an extra layer of
indirection. You can avoid that like this:

```
/// Corresponds to the kernel's `struct rtnl_link_stats64`.
#[repr(transparent)]
pub struct RtnlLinkStats64(Opaque<bindings::rtnl_link_stats64>);

impl RtnlLinkStats64 {
    /// # Safety
    ///
    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
    /// may read or write to the `rtnl_link_stats64` object.
    pub unsafe fn from_raw<'a>(ptr: *mut bindings::rtnl_link_stats64) -> &'a mut Self {
    	unsafe { &mut *(ptr as *mut Self) }
    }

    /// 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 = tx_packets;
	    (*inner).tx_bytes = tx_bytes;
	    (*inner).tx_errors = tx_errors;
	    (*inner).tx_dropped = tx_dropped;
        }
    }
}

unsafe extern "C" fn get_stats64_callback(
    netdev: *mut bindings::net_device,
    stats: *mut bindings::rtnl_link_stats64,
) {
    // SAFETY: The C API guarantees that `net_device` isn't released while this function is running.
    let data = unsafe { D::Data::borrow(Device::priv_data_ptr(netdev)) };
    // SAFETY: For the duration of this call to `get_stats64_callback`, the `stats` pointer is valid
    // for writing and nobody else will read or write to it.
    let stats = unsafe { RtnlLinkStats64::from_raw(stats) };
    let mut dev = Device(netdev);
    T::get_stats64(&mut dev, data, stats);
}
```

Alice

Re: [PATCH 4/5] rust: add methods for configure net_device

[Alice Ryhl]

FUJITA Tomonori writes:
> +    /// Sets the name of a device.
> +    pub fn set_name(&mut self, name: &'static CStr) -> Result {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        unsafe {
> +            if name.len() > (*self.0).name.len() {
> +                return Err(code::EINVAL);
> +            }
> +            for i in 0..name.len() {
> +                (*self.0).name[i] = name[i] as i8;
> +            }
> +        }
> +        Ok(())
> +    }

You should use `name: &CStr` here instead because you don't need the
provided string to stay valid after the call.

Alice

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[Alice Ryhl]

FUJITA Tomonori writes:
> +config SAMPLE_RUST_DUMMY
> +	tristate "Dummy network driver"
> +	help
> +	  This is the simpler version of drivers/net/dummy.c. No intention to replace it. This provides educational information for Rust
> +	  abstractions for network drivers.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_minimal.
> +
> +	  If unsure, say N.

Perhaps a better name would be `SAMPLE_RUST_NET_DUMMY`?

> +struct DevOps {}
> +
> +#[vtable]
> +impl<D: DriverData<Data = Box<Stats>>> DeviceOperations<D> for DevOps {
> +    fn init(_dev: &mut Device, data: &Stats) -> Result {
> +        data.packets.store(0, Ordering::Relaxed);
> +        data.bytes.store(0, Ordering::Relaxed);
> +        Ok(())
> +    }

Do we not have mutable access to the data during initialization?

> +impl kernel::Module for DummyNetdev {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        const NAME: &'static CStr = c_str!("dummy%d");
> +        let data = Box::try_new(Stats {
> +            packets: AtomicU64::new(0),
> +            bytes: AtomicU64::new(0),
> +        })?;
> +        let mut r = Registration::<DevOps, Stats>::try_new(1, 1, data)?;
> +        r.set_ether_operations(EtherOperationsAdapter::<EtherOps, Stats>::new())?;
> +
> +        let netdev = r.dev_get();
> +        netdev.set_name(NAME)?;

Should you not replace the `%d` part here with a counter?

Alice

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Thanks a lot for reviewing!

On Sun,  4 Jun 2023 12:47:35 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori writes:
>> +/// Corresponds to the kernel's `struct net_device`.
>> +///
>> +/// # Safety
>> +///
>> +/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
>> +/// This object is passed to Rust callbacks while these functions are running.
>> +/// The C API guarantees that `net_device` isn't released while this function is running.
>> +pub struct Device(*mut bindings::net_device);
> 
> You don't implement `Send` or `Sync` for this type. But maybe that's
> intentional since you don't know what kind of private data it stores?

I totally forgot. The type of private data is required to be Send and Sync
so it's ok to implement Send and Sync for Device, right?


>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {}
>> +// SAFETY: `Registration` is not restricted to a single thread,
>> +unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}
> 
> This does expose the `D::Data` object across threads as far as I can
> tell. That type is required to be Send/Sync, so this is still ok, but I
> would mention that here.

I should have updated the comments.

// SAFETY: `Registration` expose sthe `D::Data` object across threads
// but that type is required to be Send and Sync.
unsafe impl<D: DriverData, T: DeviceOperations<D>> Send for Registration<T, D> {}
unsafe impl<D: DriverData, T: DeviceOperations<D>> Sync for Registration<T, D> {}


>> +impl<D: DriverData, T: DeviceOperations<D>> Registration<T, D> {
>> +    /// Creates new instance of registration.
>> +    pub fn try_new(tx_queue_size: u32, rx_queue_size: u32, data: D::Data) -> Result<Self> {
>> +        // SAFETY: FFI call.
>> +        match from_err_ptr(unsafe {
>> +            bindings::alloc_etherdev_mqs(
>> +                core::mem::size_of::<*const core::ffi::c_void>() as i32,
>> +                tx_queue_size,
>> +                rx_queue_size,
>> +            )
>> +        }) {
>> +            Ok(ptr) => {
>> +                // SAFETY: the kernel allocated the space for a pointer.
>> +                unsafe {
>> +                    let priv_ptr = bindings::netdev_priv(ptr) as *mut usize;
>> +                    core::ptr::write(priv_ptr, data.into_foreign() as usize);
>> +                }
>> +                Ok(Registration {
>> +                    dev: Device(ptr),
>> +                    is_registered: false,
>> +                    _p: PhantomData,
>> +                })
>> +            }
>> +            Err(err) => Err(err),
>> +        }
>> +    }
> 
> This method can be simplified using the question mark operator (also
> known as the try operator).

Oops, it will be fixed in the next version.


> Also, it is unclear to me why you are converting the pointers into an
> usize. I would recommend keeping them as void pointers instead.
> Especially considering that you're using a void pointer for the
> `sizeof_priv` argument. You can do that like this:

Ah, it will be fixed in the next version.


>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>> +        ndo_init: if <T>::HAS_INIT {
>> +            Some(Self::init_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_uninit: if <T>::HAS_UNINIT {
>> +            Some(Self::uninit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_open: if <T>::HAS_OPEN {
>> +            Some(Self::open_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_stop: if <T>::HAS_STOP {
>> +            Some(Self::stop_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>> +            Some(Self::start_xmit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_features_check: None,
>> +        ndo_select_queue: None,
>> +        ndo_change_rx_flags: None,
>> +        ndo_set_rx_mode: None,
>> +        ndo_set_mac_address: None,
> 
> Bindgen is set up to generate `Default` implementations for these types,
> so you can do this to omit the remaining fields:

It can work with const?


>> +impl Drop for SkBuff {
>> +    fn drop(&mut self) {
>> +        // SAFETY: The safety requirement ensures that the pointer is valid.
>> +        unsafe {
>> +            bindings::kfree_skb_reason(
>> +                self.0,
>> +                bindings::skb_drop_reason_SKB_DROP_REASON_NOT_SPECIFIED,
>> +            )
>> +        }
>> +    }
>> +}
> 
> What are these drop reasons used for?

ndo_start_xmit() passes a sk_buff object to a driver. The driver is
responsible for freeing the object after sending the data in the
sk_buff. I expect a device driver to keep the SkBuff object until the
completion of transfer then drops the object (kfree_skb is called).

Re: [PATCH 4/5] rust: add methods for configure net_device

[FUJITA Tomonori]

Hi,

On Sun,  4 Jun 2023 12:48:37 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori writes:
>> +    /// Sets the name of a device.
>> +    pub fn set_name(&mut self, name: &'static CStr) -> Result {
>> +        // SAFETY: The safety requirement ensures that the pointer is valid.
>> +        unsafe {
>> +            if name.len() > (*self.0).name.len() {
>> +                return Err(code::EINVAL);
>> +            }
>> +            for i in 0..name.len() {
>> +                (*self.0).name[i] = name[i] as i8;
>> +            }
>> +        }
>> +        Ok(())
>> +    }
> 
> You should use `name: &CStr` here instead because you don't need the
> provided string to stay valid after the call.

Oops, it will be fixed in the next version.

Thanks a lot!

Re: [PATCH 3/5] rust: add support for get_stats64 in struct net_device_ops

[FUJITA Tomonori]

Hi,

On Sun,  4 Jun 2023 12:48:29 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori writes:
>> +/// Corresponds to the kernel's `struct rtnl_link_stats64`.
>> +pub struct RtnlLinkStats64(*mut bindings::rtnl_link_stats64);
>> +
>> +impl RtnlLinkStats64 {
>> +    /// Updates TX stats.
>> +    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
>> +        // SAFETY: The safety requirement ensures that the pointer is valid.
>> +        unsafe {
>> +            (*self.0).tx_packets = packets;
>> +            (*self.0).tx_bytes = bytes;
>> +            (*self.0).tx_errors = errors;
>> +            (*self.0).tx_dropped = dropped;
>> +        }
>> +    }
>> +}
> 
> It seems like this type is always used behind a reference. Your current
> approach has the disadvantage that you're introducing an extra layer of
> indirection. You can avoid that like this:
> 
> ```
> /// Corresponds to the kernel's `struct rtnl_link_stats64`.
> #[repr(transparent)]
> pub struct RtnlLinkStats64(Opaque<bindings::rtnl_link_stats64>);
> 
> impl RtnlLinkStats64 {
>     /// # Safety
>     ///
>     /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>     /// may read or write to the `rtnl_link_stats64` object.
>     pub unsafe fn from_raw<'a>(ptr: *mut bindings::rtnl_link_stats64) -> &'a mut Self {
>     	unsafe { &mut *(ptr as *mut Self) }
>     }

I was wondering which I should use here, a pointer or Opaque. Can you
give guidelines on which to use?

The condition in the above commment is met, Opaque should be used?

Re: [PATCH 2/5] rust: add support for ethernet operations

[FUJITA Tomonori]

Hi,

On Sun,  4 Jun 2023 12:48:21 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori writes:
>> +    /// Sets `ethtool_ops` of `net_device`.
>> +    pub fn set_ether_operations<U>(&mut self, adapter: EtherOperationsAdapter<U, D>) -> Result
>> +    where
>> +        U: EtherOperations<D>,
>> +    {
>> +        if self.is_registered {
>> +            Err(code::EINVAL)
>> +        } else {
>> +            adapter.set_ether_ops(&mut self.dev);
>> +            Ok(())
>> +        }
>> +    }
> 
> The `EtherOperationsAdapter` struct seems unnecessary here. How about
> using this signature instead?
> 
> pub fn set_ether_operations<U: EtherOperations<D>>(&mut self) -> Result
> 
> Or even
> 
> pub fn set_ether_operations(&mut self) -> Result
> where
>     D: EtherOperations

How can I get *const bindings::ethtool_ops with the above approach?

Re: [PATCH 3/5] rust: add support for get_stats64 in struct net_device_ops

[Boqun Feng]

On Mon, Jun 05, 2023 at 01:57:36PM +0000, FUJITA Tomonori wrote:
> Hi,
> 
> On Sun,  4 Jun 2023 12:48:29 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > FUJITA Tomonori writes:
> >> +/// Corresponds to the kernel's `struct rtnl_link_stats64`.
> >> +pub struct RtnlLinkStats64(*mut bindings::rtnl_link_stats64);
> >> +
> >> +impl RtnlLinkStats64 {
> >> +    /// Updates TX stats.
> >> +    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
> >> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> >> +        unsafe {
> >> +            (*self.0).tx_packets = packets;
> >> +            (*self.0).tx_bytes = bytes;
> >> +            (*self.0).tx_errors = errors;
> >> +            (*self.0).tx_dropped = dropped;
> >> +        }
> >> +    }
> >> +}
> > 
> > It seems like this type is always used behind a reference. Your current
> > approach has the disadvantage that you're introducing an extra layer of
> > indirection. You can avoid that like this:
> > 
> > ```
> > /// Corresponds to the kernel's `struct rtnl_link_stats64`.
> > #[repr(transparent)]
> > pub struct RtnlLinkStats64(Opaque<bindings::rtnl_link_stats64>);
> > 
> > impl RtnlLinkStats64 {
> >     /// # Safety
> >     ///
> >     /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >     /// may read or write to the `rtnl_link_stats64` object.
> >     pub unsafe fn from_raw<'a>(ptr: *mut bindings::rtnl_link_stats64) -> &'a mut Self {
> >     	unsafe { &mut *(ptr as *mut Self) }
> >     }
> 
> I was wondering which I should use here, a pointer or Opaque. Can you
> give guidelines on which to use?
> 

Always avoid raw pointers in data structures, unless there is no
dereference on the pointers (i.e. use the pointer types as plain data).

The reason is as Alice mentioned above.

Regards,
Boqun

> The condition in the above commment is met, Opaque should be used?

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[FUJITA Tomonori]

Hi,

On Sun,  4 Jun 2023 12:48:44 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> FUJITA Tomonori writes:
>> +config SAMPLE_RUST_DUMMY
>> +	tristate "Dummy network driver"
>> +	help
>> +	  This is the simpler version of drivers/net/dummy.c. No intention to replace it. This provides educational information for Rust
>> +	  abstractions for network drivers.
>> +
>> +	  To compile this as a module, choose M here:
>> +	  the module will be called rust_minimal.
>> +
>> +	  If unsure, say N.
> 
> Perhaps a better name would be `SAMPLE_RUST_NET_DUMMY`?

Sure. drivers/net/dummy.c uses CONFIG_DUMMY so I used
SAMPLE_RUST_DUMMY but SAMPLE_RUST_NET_DUMMY works for me.


>> +struct DevOps {}
>> +
>> +#[vtable]
>> +impl<D: DriverData<Data = Box<Stats>>> DeviceOperations<D> for DevOps {
>> +    fn init(_dev: &mut Device, data: &Stats) -> Result {
>> +        data.packets.store(0, Ordering::Relaxed);
>> +        data.bytes.store(0, Ordering::Relaxed);
>> +        Ok(())
>> +    }
> 
> Do we not have mutable access to the data during initialization?

I guess that most of operations in DeviceOperations need mutable
access. For example, start_xmit() likely needs to update the stats
(the counts of packets, etc). Also the private data was initialized
when the device is registered. So I don't think that we need to handle
only init() in a different way.


>> +impl kernel::Module for DummyNetdev {
>> +    fn init(_module: &'static ThisModule) -> Result<Self> {
>> +        const NAME: &'static CStr = c_str!("dummy%d");
>> +        let data = Box::try_new(Stats {
>> +            packets: AtomicU64::new(0),
>> +            bytes: AtomicU64::new(0),
>> +        })?;
>> +        let mut r = Registration::<DevOps, Stats>::try_new(1, 1, data)?;
>> +        r.set_ether_operations(EtherOperationsAdapter::<EtherOps, Stats>::new())?;
>> +
>> +        let netdev = r.dev_get();
>> +        netdev.set_name(NAME)?;
> 
> Should you not replace the `%d` part here with a counter?

I think that the kernel assigns an appropriate number to '%d' part
when a device is registered.

Again, thanks a lot! Your comments really help me.

Re: [PATCH 2/5] rust: add support for ethernet operations

[FUJITA Tomonori]

On Mon, 5 Jun 2023 14:02:00 +0000
FUJITA Tomonori <tomo@exabit.dev> wrote:

> Hi,
> 
> On Sun,  4 Jun 2023 12:48:21 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>> FUJITA Tomonori writes:
>>> +    /// Sets `ethtool_ops` of `net_device`.
>>> +    pub fn set_ether_operations<U>(&mut self, adapter: EtherOperationsAdapter<U, D>) -> Result
>>> +    where
>>> +        U: EtherOperations<D>,
>>> +    {
>>> +        if self.is_registered {
>>> +            Err(code::EINVAL)
>>> +        } else {
>>> +            adapter.set_ether_ops(&mut self.dev);
>>> +            Ok(())
>>> +        }
>>> +    }
>> 
>> The `EtherOperationsAdapter` struct seems unnecessary here. How about
>> using this signature instead?
>> 
>> pub fn set_ether_operations<U: EtherOperations<D>>(&mut self) -> Result
>> 
>> Or even
>> 
>> pub fn set_ether_operations(&mut self) -> Result
>> where
>>     D: EtherOperations
> 
> How can I get *const bindings::ethtool_ops with the above approach?

Sorry, I successfully get it without EtherOperationsAdapter arg in the following way.

pub fn set_ether_operations<U>(&mut self) -> Result
where
    U: EtherOperations<D>,
{
    if self.is_registered {
        Err(code::EINVAL)
    } else {
        EtherOperationsAdapter::<U, D>::new().set_ether_ops(&mut self.dev);
        Ok(())
    }
}

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Andrew Lunn]

> +/// The pointer to the `net_device` object is guaranteed to be valid until
> +/// the registration object is dropped.
> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
> +    dev: Device,
> +    is_registered: bool,
> +    _p: PhantomData<(D, T)>,
> +}


> +    pub fn register(&mut self) -> Result {
> +        // SAFETY: `dev` was allocated during initialization and is guaranteed to be valid.
> +        let ret = unsafe {
> +            (*self.dev.0).netdev_ops = Self::build_device_ops();
> +            bindings::register_netdev(self.dev.0)
> +        };
> +        if ret != 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            self.is_registered = true;
> +            Ok(())
> +        }
> +    }

I knew ~0 about rust, but i do know the network stack.

You need to be careful with the value of self.is_registered. The
network stack will start using the interface during the call to
register_netdev(). It can call any of the netdev ops or ethtool ops
before register_netdev() has returned. You can see this in particular
with NFS root, but there are other use cases as well.

However, i do see value in self.is_registered. One of the common bugs
in drivers caught at review time is not completely setting up the
device before calling register_netdev(). The way you have added
setters for the netdev ops and ethtool ops we want it to error out if
these setters are called after the interface is registered.

> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
> +        ndo_init: if <T>::HAS_INIT {
> +            Some(Self::init_callback)
> +        } else {
> +            None
> +        },
> +        ndo_uninit: if <T>::HAS_UNINIT {
> +            Some(Self::uninit_callback)
> +        } else {
> +            None
> +        },
> +        ndo_open: if <T>::HAS_OPEN {
> +            Some(Self::open_callback)
> +        } else {
> +            None
> +        },
> +        ndo_stop: if <T>::HAS_STOP {
> +            Some(Self::stop_callback)
> +        } else {
> +            None
> +        },
> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
> +            Some(Self::start_xmit_callback)
> +        } else {
> +            None
> +        },
> +        ndo_features_check: None,
> +        ndo_select_queue: None,
> +        ndo_change_rx_flags: None,
> +        ndo_set_rx_mode: None,
> +        ndo_set_mac_address: None,
...
> +        ndo_validate_addr: None,
> +        ndo_fill_forward_path: None,
> +        ndo_get_tstamp: None,
> +    };

Is it necessary to list all these members, particularly those with
None? New members do get added, so it would be nice if either the
default would be None, or the compiler exited with an error if there
is a new member and it is not listed here.

The same is true for the ethtool ops.

    Andrew

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[Andrew Lunn]

> +    fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
> +        data.packets.fetch_add(1, Ordering::Relaxed);
> +        skb.tx_timestamp();
> +        TxCode::Ok
> +    }
> +
> +    fn get_stats64(_dev: &mut Device, data: &Stats, stats: &mut RtnlLinkStats64) {
> +        stats.set_tx_stats(data.packets.load(Ordering::Relaxed), 0, 0, 0);
> +    }
> +}
> +
> +#[allow(dead_code)]
> +struct Stats {
> +    packets: AtomicU64,
> +    bytes: AtomicU64,
> +}

start_xmit() and get_stats64() can be called in parallel. So
statistics needs some sort of synchronisation, particularly on 32 bit
systems with 64 bit counters. start_xmit() is on the hot path and used
very often, 10 million times a second, but get_stats64() is a cold
path, probably less than once per second. So the network stack tries
to push as much of the cost to get_stats64(). Atomic operations are
very expensive, and got more and more expensive as the number of CPUs
go up. So in general, you won't see atomics used in networking hot
path.

What we don't want is this code copied into a real driver. I see three
paths forward:

1) Remove statistics for the moment.
2) Add a big comment that Atomics are inefficient and need replacing
3) Wrap u64_stats_update_begin(), u64_stats_update_end() etc.

   Andrew

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,
Thanks a lot for reviewing!

On Tue, 6 Jun 2023 18:20:15 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +/// The pointer to the `net_device` object is guaranteed to be valid until
>> +/// the registration object is dropped.
>> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
>> +    dev: Device,
>> +    is_registered: bool,
>> +    _p: PhantomData<(D, T)>,
>> +}
> 
> 
>> +    pub fn register(&mut self) -> Result {
>> +        // SAFETY: `dev` was allocated during initialization and is guaranteed to be valid.
>> +        let ret = unsafe {
>> +            (*self.dev.0).netdev_ops = Self::build_device_ops();
>> +            bindings::register_netdev(self.dev.0)
>> +        };
>> +        if ret != 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            self.is_registered = true;
>> +            Ok(())
>> +        }
>> +    }
> 
> I knew ~0 about rust, but i do know the network stack.
> 
> You need to be careful with the value of self.is_registered. The
> network stack will start using the interface during the call to
> register_netdev(). It can call any of the netdev ops or ethtool ops
> before register_netdev() has returned. You can see this in particular
> with NFS root, but there are other use cases as well.
> 
> However, i do see value in self.is_registered. One of the common bugs
> in drivers caught at review time is not completely setting up the
> device before calling register_netdev(). The way you have added
> setters for the netdev ops and ethtool ops we want it to error out if
> these setters are called after the interface is registered.

Yeah, the current API guarantees that netdev ops and ethtool ops are
set up before calling register_netdev().

However, register() could be misused, register_netdev() might called
multiple times. It will be fixed in the next version.


>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>> +        ndo_init: if <T>::HAS_INIT {
>> +            Some(Self::init_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_uninit: if <T>::HAS_UNINIT {
>> +            Some(Self::uninit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_open: if <T>::HAS_OPEN {
>> +            Some(Self::open_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_stop: if <T>::HAS_STOP {
>> +            Some(Self::stop_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>> +            Some(Self::start_xmit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_features_check: None,
>> +        ndo_select_queue: None,
>> +        ndo_change_rx_flags: None,
>> +        ndo_set_rx_mode: None,
>> +        ndo_set_mac_address: None,
> ...
>> +        ndo_validate_addr: None,
>> +        ndo_fill_forward_path: None,
>> +        ndo_get_tstamp: None,
>> +    };
> 
> Is it necessary to list all these members, particularly those with
> None? New members do get added, so it would be nice if either the
> default would be None, or the compiler exited with an error if there
> is a new member and it is not listed here.
> 
> The same is true for the ethtool ops.

With the current code, the compiler exited with an error if there is a
new member and it is not listed here.

However, it would be nicer if the default would be set
automatically. As far as I know, we can't do yet.

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[FUJITA Tomonori]

Hi,

On Tue, 6 Jun 2023 18:34:57 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +    fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
>> +        data.packets.fetch_add(1, Ordering::Relaxed);
>> +        skb.tx_timestamp();
>> +        TxCode::Ok
>> +    }
>> +
>> +    fn get_stats64(_dev: &mut Device, data: &Stats, stats: &mut RtnlLinkStats64) {
>> +        stats.set_tx_stats(data.packets.load(Ordering::Relaxed), 0, 0, 0);
>> +    }
>> +}
>> +
>> +#[allow(dead_code)]
>> +struct Stats {
>> +    packets: AtomicU64,
>> +    bytes: AtomicU64,
>> +}
> 
> start_xmit() and get_stats64() can be called in parallel. So
> statistics needs some sort of synchronisation, particularly on 32 bit
> systems with 64 bit counters. start_xmit() is on the hot path and used
> very often, 10 million times a second, but get_stats64() is a cold
> path, probably less than once per second. So the network stack tries
> to push as much of the cost to get_stats64(). Atomic operations are
> very expensive, and got more and more expensive as the number of CPUs
> go up. So in general, you won't see atomics used in networking hot
> path.
> 
> What we don't want is this code copied into a real driver. I see three
> paths forward:
> 
> 1) Remove statistics for the moment.
> 2) Add a big comment that Atomics are inefficient and need replacing
> 3) Wrap u64_stats_update_begin(), u64_stats_update_end() etc.

I'll go with the second becuase

- it would be nice to show a way to access to the private data of a
driver in this example.

- to keep this patchset simple, I prefer to avoid adding more
  appropriate such as per_cpu, etc.

Thanks!

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Alice Ryhl]

> Thanks a lot for reviewing!
> 
> On Sun,  4 Jun 2023 12:47:35 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>> FUJITA Tomonori writes:
>>> +/// Corresponds to the kernel's `struct net_device`.
>>> +///
>>> +/// # Safety
>>> +///
>>> +/// The kernel uses a `net_device` object with various functions like `struct net_device_ops`.
>>> +/// This object is passed to Rust callbacks while these functions are running.
>>> +/// The C API guarantees that `net_device` isn't released while this function is running.
>>> +pub struct Device(*mut bindings::net_device);
>> 
>> You don't implement `Send` or `Sync` for this type. But maybe that's
>> intentional since you don't know what kind of private data it stores?
> 
> I totally forgot. The type of private data is required to be Send and Sync
> so it's ok to implement Send and Sync for Device, right?

Yes, it's probably fine.

>>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>>> +        ndo_init: if <T>::HAS_INIT {
>>> +            Some(Self::init_callback)
>>> +        } else {
>>> +            None
>>> +        },
>>> +        ndo_uninit: if <T>::HAS_UNINIT {
>>> +            Some(Self::uninit_callback)
>>> +        } else {
>>> +            None
>>> +        },
>>> +        ndo_open: if <T>::HAS_OPEN {
>>> +            Some(Self::open_callback)
>>> +        } else {
>>> +            None
>>> +        },
>>> +        ndo_stop: if <T>::HAS_STOP {
>>> +            Some(Self::stop_callback)
>>> +        } else {
>>> +            None
>>> +        },
>>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>>> +            Some(Self::start_xmit_callback)
>>> +        } else {
>>> +            None
>>> +        },
>>> +        ndo_features_check: None,
>>> +        ndo_select_queue: None,
>>> +        ndo_change_rx_flags: None,
>>> +        ndo_set_rx_mode: None,
>>> +        ndo_set_mac_address: None,
>> 
>> Bindgen is set up to generate `Default` implementations for these types,
>> so you can do this to omit the remaining fields:
> 
> It can work with const?

Ah, no, good point.

However, you could use `unsafe { mem::zeroed() }` instead.

Re: [PATCH 3/5] rust: add support for get_stats64 in struct net_device_ops

[Alice Ryhl]

FUJITA Tomonori writes:
> Hi,
> 
> On Sun,  4 Jun 2023 12:48:29 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>> FUJITA Tomonori writes:
>>> +/// Corresponds to the kernel's `struct rtnl_link_stats64`.
>>> +pub struct RtnlLinkStats64(*mut bindings::rtnl_link_stats64);
>>> +
>>> +impl RtnlLinkStats64 {
>>> +    /// Updates TX stats.
>>> +    pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) {
>>> +        // SAFETY: The safety requirement ensures that the pointer is valid.
>>> +        unsafe {
>>> +            (*self.0).tx_packets = packets;
>>> +            (*self.0).tx_bytes = bytes;
>>> +            (*self.0).tx_errors = errors;
>>> +            (*self.0).tx_dropped = dropped;
>>> +        }
>>> +    }
>>> +}
>> 
>> It seems like this type is always used behind a reference. Your current
>> approach has the disadvantage that you're introducing an extra layer of
>> indirection. You can avoid that like this:
>> 
>> ```
>> /// Corresponds to the kernel's `struct rtnl_link_stats64`.
>> #[repr(transparent)]
>> pub struct RtnlLinkStats64(Opaque<bindings::rtnl_link_stats64>);
>> 
>> impl RtnlLinkStats64 {
>>     /// # Safety
>>     ///
>>     /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>>     /// may read or write to the `rtnl_link_stats64` object.
>>     pub unsafe fn from_raw<'a>(ptr: *mut bindings::rtnl_link_stats64) -> &'a mut Self {
>>         unsafe { &mut *(ptr as *mut Self) }
>>     }
> 
> I was wondering which I should use here, a pointer or Opaque. Can you
> give guidelines on which to use?
> 
> The condition in the above commment is met, Opaque should be used?

An alternative to what you're doing here would be a type that wraps a
raw pointer:

```
pub struct RtnlLinkStats64Ptr<'a> {
    ptr: *mut bindings::rtnl_link_stats64,
    _lifetime: PhantomData<&'a mut bindings::rtnl_link_stats64>,
}

impl RtnlLinkStats64Ptr<'_> {
    /// # Safety
    ///
    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
    /// may read or write to the `rtnl_link_stats64` object.
    pub unsafe fn from_raw(ptr: *mut bindings::rtnl_link_stats64) -> Self {
        Self {
            ptr,
            _lifetime: PhantomData,
        }
    }

    /// 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 {
            (*self.ptr).tx_packets = tx_packets;
            (*self.ptr).tx_bytes = tx_bytes;
            (*self.ptr).tx_errors = tx_errors;
            (*self.ptr).tx_dropped = tx_dropped;
        }
    }
}
```

With the above, you replace instances of `&mut RtnlLinkStats64` with
`RtnlLinkStats64Ptr`. That is, the wrapper struct is itself a pointer,
so it does not need to be behind a mutable reference.

This approach also eliminates the double indirection, and I think both
approaches are fine. The mutable reference is probably preferable
because it is more convenient for the user, but there are some cases
where you have to use the `RtnlLinkStats64Ptr` solution, e.g. if the
value behind the pointer is dynamically sized (can happen with tagged
unions).

Alice

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Alice Ryhl]

>> +/// The pointer to the `net_device` object is guaranteed to be valid until
>> +/// the registration object is dropped.
>> +pub struct Registration<T: DeviceOperations<D>, D: DriverData> {
>> +    dev: Device,
>> +    is_registered: bool,
>> +    _p: PhantomData<(D, T)>,
>> +}
> 
> 
>> +    pub fn register(&mut self) -> Result {
>> +        // SAFETY: `dev` was allocated during initialization and is guaranteed to be valid.
>> +        let ret = unsafe {
>> +            (*self.dev.0).netdev_ops = Self::build_device_ops();
>> +            bindings::register_netdev(self.dev.0)
>> +        };
>> +        if ret != 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            self.is_registered = true;
>> +            Ok(())
>> +        }
>> +    }
> 
> I knew ~0 about rust, but i do know the network stack.
> 
> You need to be careful with the value of self.is_registered. The
> network stack will start using the interface during the call to
> register_netdev(). It can call any of the netdev ops or ethtool ops
> before register_netdev() has returned. You can see this in particular
> with NFS root, but there are other use cases as well.
> 
> However, i do see value in self.is_registered. One of the common bugs
> in drivers caught at review time is not completely setting up the
> device before calling register_netdev(). The way you have added
> setters for the netdev ops and ethtool ops we want it to error out if
> these setters are called after the interface is registered.

Another possibility is to use two different types:

 * `UnregisteredDevice`
 * `RegisteredDevice`

Then, the methods that need to be called before registration can be
defined on the first type, and the other methods can be defined on the
second type. Using Rust's move semantics, we can prevent people from
using the `UnregisteredDevice` after calling `register`. This means that
the following will not compile:

device.register();
device.setup_step_that_should_happen_before_register();

This is called the "typestate pattern". [1]

Now, the above only helps if the setup operations don't also make sense
after registration, because in that case you would need to add the
method to both types, meaning that you're no longer prevented from
calling the setup step after registration. I don't know the network
stack well enough to tell whether this is the case here.

>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>> +        ndo_init: if <T>::HAS_INIT {
>> +            Some(Self::init_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_uninit: if <T>::HAS_UNINIT {
>> +            Some(Self::uninit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_open: if <T>::HAS_OPEN {
>> +            Some(Self::open_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_stop: if <T>::HAS_STOP {
>> +            Some(Self::stop_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>> +            Some(Self::start_xmit_callback)
>> +        } else {
>> +            None
>> +        },
>> +        ndo_features_check: None,
>> +        ndo_select_queue: None,
>> +        ndo_change_rx_flags: None,
>> +        ndo_set_rx_mode: None,
>> +        ndo_set_mac_address: None,
> ...
>> +        ndo_validate_addr: None,
>> +        ndo_fill_forward_path: None,
>> +        ndo_get_tstamp: None,
>> +    };
> 
> Is it necessary to list all these members, particularly those with
> None? New members do get added, so it would be nice if either the
> default would be None, or the compiler exited with an error if there
> is a new member and it is not listed here.
> 
> The same is true for the ethtool ops.

With the code in the current patchset, the compiler will exit with an
error if you add a new member.

However, it is possible to avoid listing all of them, see [2].

Alice

[1]: http://cliffle.com/blog/rust-typestate/
[2]: https://lore.kernel.org/all/20230608073807.1353371-1-aliceryhl@google.com/

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[Alice Ryhl]

>> +    fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
>> +        data.packets.fetch_add(1, Ordering::Relaxed);
>> +        skb.tx_timestamp();
>> +        TxCode::Ok
>> +    }
>> +
>> +    fn get_stats64(_dev: &mut Device, data: &Stats, stats: &mut RtnlLinkStats64) {
>> +        stats.set_tx_stats(data.packets.load(Ordering::Relaxed), 0, 0, 0);
>> +    }
>> +}
>> +
>> +#[allow(dead_code)]
>> +struct Stats {
>> +    packets: AtomicU64,
>> +    bytes: AtomicU64,
>> +}
> 
> start_xmit() and get_stats64() can be called in parallel. So
> statistics needs some sort of synchronisation, particularly on 32 bit
> systems with 64 bit counters. start_xmit() is on the hot path and used
> very often, 10 million times a second, but get_stats64() is a cold
> path, probably less than once per second. So the network stack tries
> to push as much of the cost to get_stats64(). Atomic operations are
> very expensive, and got more and more expensive as the number of CPUs
> go up. So in general, you won't see atomics used in networking hot
> path.
> 
> What we don't want is this code copied into a real driver. I see three
> paths forward:
> 
> 1) Remove statistics for the moment.
> 2) Add a big comment that Atomics are inefficient and need replacing
> 3) Wrap u64_stats_update_begin(), u64_stats_update_end() etc.
> 
>    Andrew

I'm guessing that the `dev` pointers to the device will point to the
same device if they are called in parallel? In that case, the type will
need to be changed to `&Device`. We can only use `&mut T` references
when we are the only one accessing the value for the duration of the
function call.

Anyway, how do other parts of the network stack usually synchronize the
access to this data? If there can only be one call to `start_xmit` at
the time, then you could rewrite the code to this without introducing a
race:

fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
    let cur_packets = data.packets.load(Ordering::Relaxed);
    data.packets.store(cur_packets + 1, Ordering::Relaxed);
    skb.tx_timestamp();
    TxCode::Ok
}

My understanding is that (unlike `fetch_add`) relaxed loads/stores just
compile down to non-atomic mov instructions on most architectures. (But
I could be wrong.)

Of course, this assumes that `start_xmit` can only race with
`get_stats64` and not with itself. Otherwise you will lose updates.

Alice

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[FUJITA Tomonori]

Hi,

On Thu,  8 Jun 2023 08:22:25 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

>> start_xmit() and get_stats64() can be called in parallel. So
>> statistics needs some sort of synchronisation, particularly on 32 bit
>> systems with 64 bit counters. start_xmit() is on the hot path and used
>> very often, 10 million times a second, but get_stats64() is a cold
>> path, probably less than once per second. So the network stack tries
>> to push as much of the cost to get_stats64(). Atomic operations are
>> very expensive, and got more and more expensive as the number of CPUs
>> go up. So in general, you won't see atomics used in networking hot
>> path.
>> 
>> What we don't want is this code copied into a real driver. I see three
>> paths forward:
>> 
>> 1) Remove statistics for the moment.
>> 2) Add a big comment that Atomics are inefficient and need replacing
>> 3) Wrap u64_stats_update_begin(), u64_stats_update_end() etc.
>> 
>>    Andrew
> 
> I'm guessing that the `dev` pointers to the device will point to the
> same device if they are called in parallel? In that case, the type will
> need to be changed to `&Device`. We can only use `&mut T` references
> when we are the only one accessing the value for the duration of the
> function call.

Ah, yes. ndo_start_xmit() could run on the same device in parallel, I
think.

> Anyway, how do other parts of the network stack usually synchronize the
> access to this data? If there can only be one call to `start_xmit` at
> the time, then you could rewrite the code to this without introducing a
> race:

I guess that a typical way is using sorta per-CPU data. For example,
let's assume that NIC supports multiple TX queues. The driver
allocates data structure for stats per TX queue. Only one CPU can
access to a queue at one time (runs ndo_start_xmit() to send data via
a queue).


> fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
>     let cur_packets = data.packets.load(Ordering::Relaxed);
>     data.packets.store(cur_packets + 1, Ordering::Relaxed);
>     skb.tx_timestamp();
>     TxCode::Ok
> }
> 
> My understanding is that (unlike `fetch_add`) relaxed loads/stores just
> compile down to non-atomic mov instructions on most architectures. (But
> I could be wrong.)
> 
> Of course, this assumes that `start_xmit` can only race with
> `get_stats64` and not with itself. Otherwise you will lose updates.

Hmm, the official doc [1] says on the Relaxed ordering:

"No ordering constraints, only atomic operations."

so I expect that fetch_add() always gives an atomic operation. I
assume that the ordering argument is about the ordering between this
atomic operation and other operations on different variables.

I tried to compile fetch_add() with Ordering::Relaxed and looks ok,
got `lock inc` operation on x86_64 and ldadd on aarch64.

[1] https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Relaxed

Thanks!

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Thu,  8 Jun 2023 07:38:07 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

>>>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>>>> +        ndo_init: if <T>::HAS_INIT {
>>>> +            Some(Self::init_callback)
>>>> +        } else {
>>>> +            None
>>>> +        },
>>>> +        ndo_uninit: if <T>::HAS_UNINIT {
>>>> +            Some(Self::uninit_callback)
>>>> +        } else {
>>>> +            None
>>>> +        },
>>>> +        ndo_open: if <T>::HAS_OPEN {
>>>> +            Some(Self::open_callback)
>>>> +        } else {
>>>> +            None
>>>> +        },
>>>> +        ndo_stop: if <T>::HAS_STOP {
>>>> +            Some(Self::stop_callback)
>>>> +        } else {
>>>> +            None
>>>> +        },
>>>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>>>> +            Some(Self::start_xmit_callback)
>>>> +        } else {
>>>> +            None
>>>> +        },
>>>> +        ndo_features_check: None,
>>>> +        ndo_select_queue: None,
>>>> +        ndo_change_rx_flags: None,
>>>> +        ndo_set_rx_mode: None,
>>>> +        ndo_set_mac_address: None,
>>> 
>>> Bindgen is set up to generate `Default` implementations for these types,
>>> so you can do this to omit the remaining fields:
>> 
>> It can work with const?
> 
> Ah, no, good point.
> 
> However, you could use `unsafe { mem::zeroed() }` instead.

I can't figure out a way to make mem::zeroed() with const but looks
like the following similar way works.

const DEVICE_OPS: bindings::net_device_ops = {
    let mut ops = unsafe {
        core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() };

    if <T>::HAS_UNINIT {
        ops.ndo_init = Some(Self::init_callback);
    }
    ops
};

thanks,

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Thu,  8 Jun 2023 08:10:23 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

>>> +    pub fn register(&mut self) -> Result {
>>> +        // SAFETY: `dev` was allocated during initialization and is guaranteed to be valid.
>>> +        let ret = unsafe {
>>> +            (*self.dev.0).netdev_ops = Self::build_device_ops();
>>> +            bindings::register_netdev(self.dev.0)
>>> +        };
>>> +        if ret != 0 {
>>> +            Err(Error::from_errno(ret))
>>> +        } else {
>>> +            self.is_registered = true;
>>> +            Ok(())
>>> +        }
>>> +    }
>> 
>> I knew ~0 about rust, but i do know the network stack.
>> 
>> You need to be careful with the value of self.is_registered. The
>> network stack will start using the interface during the call to
>> register_netdev(). It can call any of the netdev ops or ethtool ops
>> before register_netdev() has returned. You can see this in particular
>> with NFS root, but there are other use cases as well.
>> 
>> However, i do see value in self.is_registered. One of the common bugs
>> in drivers caught at review time is not completely setting up the
>> device before calling register_netdev(). The way you have added
>> setters for the netdev ops and ethtool ops we want it to error out if
>> these setters are called after the interface is registered.
> 
> Another possibility is to use two different types:
> 
>  * `UnregisteredDevice`
>  * `RegisteredDevice`
> 
> Then, the methods that need to be called before registration can be
> defined on the first type, and the other methods can be defined on the
> second type. Using Rust's move semantics, we can prevent people from
> using the `UnregisteredDevice` after calling `register`. This means that
> the following will not compile:
> 
> device.register();
> device.setup_step_that_should_happen_before_register();
> 
> This is called the "typestate pattern". [1]
> 
> Now, the above only helps if the setup operations don't also make sense
> after registration, because in that case you would need to add the
> method to both types, meaning that you're no longer prevented from
> calling the setup step after registration. I don't know the network
> stack well enough to tell whether this is the case here.

Looks like works nicely for the netdev ops or ethtool ops (and other
ops), must be set before registration. However, there are several
members on netdev that can be changed both before and after
registration like mtu, features, etc. So this pattern might not be
fit. I'll investigate.

Thanks again for reviewing!

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[Alice Ryhl]

FUJITA Tomonori writes:
> On Thu,  8 Jun 2023 08:22:25 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>> fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
>>     let cur_packets = data.packets.load(Ordering::Relaxed);
>>     data.packets.store(cur_packets + 1, Ordering::Relaxed);
>>     skb.tx_timestamp();
>>     TxCode::Ok
>> }
>> 
>> My understanding is that (unlike `fetch_add`) relaxed loads/stores just
>> compile down to non-atomic mov instructions on most architectures. (But
>> I could be wrong.)
>> 
>> Of course, this assumes that `start_xmit` can only race with
>> `get_stats64` and not with itself. Otherwise you will lose updates.
> 
> Hmm, the official doc [1] says on the Relaxed ordering:
> 
> "No ordering constraints, only atomic operations."
> 
> so I expect that fetch_add() always gives an atomic operation. I
> assume that the ordering argument is about the ordering between this
> atomic operation and other operations on different variables.
> 
> I tried to compile fetch_add() with Ordering::Relaxed and looks ok,
> got `lock inc` operation on x86_64 and ldadd on aarch64.
> 
> [1] https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Relaxed

Yes, `fetch_add` definitely gives an atomic instruction. I was referring
to the `load` and `store` operations.

Alice

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[Alice Ryhl]

FUJITA Tomonori writes:
> On Thu,  8 Jun 2023 07:38:07 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>>>>> +    const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>>>>> +        ndo_init: if <T>::HAS_INIT {
>>>>> +            Some(Self::init_callback)
>>>>> +        } else {
>>>>> +            None
>>>>> +        },
>>>>> +        ndo_uninit: if <T>::HAS_UNINIT {
>>>>> +            Some(Self::uninit_callback)
>>>>> +        } else {
>>>>> +            None
>>>>> +        },
>>>>> +        ndo_open: if <T>::HAS_OPEN {
>>>>> +            Some(Self::open_callback)
>>>>> +        } else {
>>>>> +            None
>>>>> +        },
>>>>> +        ndo_stop: if <T>::HAS_STOP {
>>>>> +            Some(Self::stop_callback)
>>>>> +        } else {
>>>>> +            None
>>>>> +        },
>>>>> +        ndo_start_xmit: if <T>::HAS_START_XMIT {
>>>>> +            Some(Self::start_xmit_callback)
>>>>> +        } else {
>>>>> +            None
>>>>> +        },
>>>>> +        ndo_features_check: None,
>>>>> +        ndo_select_queue: None,
>>>>> +        ndo_change_rx_flags: None,
>>>>> +        ndo_set_rx_mode: None,
>>>>> +        ndo_set_mac_address: None,
>>>> 
>>>> Bindgen is set up to generate `Default` implementations for these types,
>>>> so you can do this to omit the remaining fields:
>>> 
>>> It can work with const?
>> 
>> Ah, no, good point.
>> 
>> However, you could use `unsafe { mem::zeroed() }` instead.
> 
> I can't figure out a way to make mem::zeroed() with const but looks
> like the following similar way works.
> 
> const DEVICE_OPS: bindings::net_device_ops = {
>     let mut ops = unsafe {
>         core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() };
> 
>     if <T>::HAS_UNINIT {
>         ops.ndo_init = Some(Self::init_callback);
>     }
>     ops
> };
> 
> thanks,

You can also write:

```
const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
    ndo_init: if <T>::HAS_INIT {
        Some(Self::init_callback)
    } else {
        None
    },
    ndo_uninit: if <T>::HAS_UNINIT {
        Some(Self::uninit_callback)
    } else {
        None
    },
    ndo_open: if <T>::HAS_OPEN {
        Some(Self::open_callback)
    } else {
        None
    },
    ndo_stop: if <T>::HAS_STOP {
        Some(Self::stop_callback)
    } else {
        None
    },
    ndo_start_xmit: if <T>::HAS_START_XMIT {
        Some(Self::start_xmit_callback)
    } else {
        None
    },
    ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() }
};
```

Alice

Re: [PATCH 1/5] rust: core abstractions for network device drivers

[FUJITA Tomonori]

Hi,

On Fri,  9 Jun 2023 13:15:00 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> You can also write:
> 
> ```
> const DEVICE_OPS: bindings::net_device_ops = bindings::net_device_ops {
>     ndo_init: if <T>::HAS_INIT {
>         Some(Self::init_callback)
>     } else {
>         None
>     },
>     ndo_uninit: if <T>::HAS_UNINIT {
>         Some(Self::uninit_callback)
>     } else {
>         None
>     },
>     ndo_open: if <T>::HAS_OPEN {
>         Some(Self::open_callback)
>     } else {
>         None
>     },
>     ndo_stop: if <T>::HAS_STOP {
>         Some(Self::stop_callback)
>     } else {
>         None
>     },
>     ndo_start_xmit: if <T>::HAS_START_XMIT {
>         Some(Self::start_xmit_callback)
>     } else {
>         None
>     },
>     ..unsafe { core::mem::MaybeUninit::<bindings::net_device_ops>::zeroed().assume_init() }
> };
> ```

Looks good! I'll go with this in the next version.

Thanks,

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[Andrew Lunn]

> >> What we don't want is this code copied into a real driver. I see three
> >> paths forward:
> >> 
> >> 1) Remove statistics for the moment.
> >> 2) Add a big comment that Atomics are inefficient and need replacing
> >> 3) Wrap u64_stats_update_begin(), u64_stats_update_end() etc.
> >> 
> >>    Andrew
> > 
> > I'm guessing that the `dev` pointers to the device will point to the
> > same device if they are called in parallel? In that case, the type will
> > need to be changed to `&Device`. We can only use `&mut T` references
> > when we are the only one accessing the value for the duration of the
> > function call.
> 
> Ah, yes. ndo_start_xmit() could run on the same device in parallel, I
> think.
> 
> > Anyway, how do other parts of the network stack usually synchronize the
> > access to this data? If there can only be one call to `start_xmit` at
> > the time, then you could rewrite the code to this without introducing a
> > race:

In general, you should assume that all packet handling functions can
run in parallel. This is typical with devices with multiple
queues. Each queue has its own interrupt, and generally that interrupt
is tied to a CPU. The hardware will use hashing algorithms to spread
packets over queues, and hence over CPUs.

> I guess that a typical way is using sorta per-CPU data.

Not sorta, but actual per-CPU data. 

> For example,
> let's assume that NIC supports multiple TX queues. The driver
> allocates data structure for stats per TX queue. Only one CPU can
> access to a queue at one time (runs ndo_start_xmit() to send data via
> a queue).
 
There can be two different sorts of statistics in play here. One is
per queue statistics, and the other is global. The per queue
statistics should be safe because only one CPU should service a queue
at once.

For the global statistics, each CPU has a per-CPU set of counters. The
get_stats call then need to iterate over all these per-CPU statistics
and add them together. 

You also need a different implementation between 32 bit CPUs and 64
bit CPUs. A 64 bit counter on a 32 bit CPU has tearing problems. When
the lower 32 bits overflows and wraps to 0 and the upper 32bits get
incremented by 1. There is a point in that process where the lower
word can be 0, but the upper word has not incremented yet. Or
possible, the other way around, the upper word has incremented, but
the lower is not wrapped. This does not happen with 64 bit machines.
So the implementation takes care to ensure you either see the old
value, or the new value. And it does this as cheaply as possible,
pushing the cost to the reader.

> > fn start_xmit(_dev: &mut Device, data: &Stats, mut skb: SkBuff) -> TxCode {
> >     let cur_packets = data.packets.load(Ordering::Relaxed);
> >     data.packets.store(cur_packets + 1, Ordering::Relaxed);
> >     skb.tx_timestamp();
> >     TxCode::Ok
> > }
> > 
> > My understanding is that (unlike `fetch_add`) relaxed loads/stores just
> > compile down to non-atomic mov instructions on most architectures. (But
> > I could be wrong.)
> > 
> > Of course, this assumes that `start_xmit` can only race with
> > `get_stats64` and not with itself. Otherwise you will lose updates.
> 
> Hmm, the official doc [1] says on the Relaxed ordering:
> 
> "No ordering constraints, only atomic operations."
> 
> so I expect that fetch_add() always gives an atomic operation. I
> assume that the ordering argument is about the ordering between this
> atomic operation and other operations on different variables.
> 
> I tried to compile fetch_add() with Ordering::Relaxed and looks ok,
> got `lock inc` operation on x86_64 and ldadd on aarch64.

So be very careful with atomics, they are very expensive, and you
don't want to use them unless you really must.

Incriminating statistics is done in a transaction. You have clear
start and end markers:

An example for a real driver. Here is increment:

        u64_stats_update_begin(&stats->syncp);
        stats->es.ps.rx_packets += ps->rx_packets;
        stats->es.ps.rx_bytes += ps->rx_bytes;
        stats->es.ps.xdp_redirect += ps->xdp_redirect;
        stats->es.ps.xdp_pass += ps->xdp_pass;
        stats->es.ps.xdp_drop += ps->xdp_drop;
        u64_stats_update_end(&stats->syncp);

Because of the _begin/_end, we don't need to worry about 32/64 bit
issues. The reader either sees all the old values, or all the new
values. And none of the counters are atomic.

The reader is:

       for_each_possible_cpu(cpu) {
                struct mvneta_pcpu_stats *cpu_stats;
                u64 rx_packets;
                u64 rx_bytes;
                u64 rx_dropped;
                u64 rx_errors;
                u64 tx_packets;
                u64 tx_bytes;

                cpu_stats = per_cpu_ptr(pp->stats, cpu);
                do {
                        start = u64_stats_fetch_begin(&cpu_stats->syncp);
                        rx_packets = cpu_stats->es.ps.rx_packets;
                        rx_bytes   = cpu_stats->es.ps.rx_bytes;
                        rx_dropped = cpu_stats->rx_dropped;
                        rx_errors  = cpu_stats->rx_errors;
                        tx_packets = cpu_stats->es.ps.tx_packets;
                        tx_bytes   = cpu_stats->es.ps.tx_bytes;
                } while (u64_stats_fetch_retry(&cpu_stats->syncp, start));

                stats->rx_packets += rx_packets;
                stats->rx_bytes   += rx_bytes;
                stats->rx_dropped += rx_dropped;
                stats->rx_errors  += rx_errors;
                stats->tx_packets += tx_packets;
                stats->tx_bytes   += tx_bytes;
        }

The expensive magic is in the do while loop. If the syncp has a
different value between the beginning and the end, it means the values
have been changed in parallel. So it does it again, until it has
consistent values.

You need to look at how syncp actually works. It is probably not an
atomic. All you need it a guarantee that other CPUs see syncp change
before a counter changes.

There is a lot of highly optimised code here. Either you need to
really understand it if you want to implement it in rust, or you just
wrap the high level interface.

If you look around at papers talking about high performance
networking, you often see that you have around 200 instructions to
handle a packet. You need to avoid any sort of contention with other
CPUs, you don't want cache lines bouncing between CPUs, you don't want
to be signalling between CPUs which atomics will do, take any locks
you really don't need. Everything should be local to the CPU.

	Andrew

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[FUJITA Tomonori]

Hi,

On Fri,  9 Jun 2023 13:11:58 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> Yes, `fetch_add` definitely gives an atomic instruction. I was referring
> to the `load` and `store` operations.

Sorry, I misunderstood what you meant.

Anyway, start_xmit() can run in paralle so fetch_add() is necessary
here until adding Rust support for per-CPU data.

Thanks,

Re: [PATCH 5/5] samples: rust: add dummy network driver sample

[FUJITA Tomonori]

Hi,

On Fri, 9 Jun 2023 15:53:43 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> There is a lot of highly optimised code here. Either you need to
> really understand it if you want to implement it in rust, or you just
> wrap the high level interface.
> 
> If you look around at papers talking about high performance
> networking, you often see that you have around 200 instructions to
> handle a packet. You need to avoid any sort of contention with other
> CPUs, you don't want cache lines bouncing between CPUs, you don't want
> to be signalling between CPUs which atomics will do, take any locks
> you really don't need. Everything should be local to the CPU.

Surely, understood.

I'll remove the atomic counter stuff in the sample code in the next
version. Even with big comments, it could be controversial under
review of netdev ml, I guess.

Likely per-CPU data can't be simply wrapped in Rust because of its way
to use macros. Let me try to push the minimum abstractions for
upstream first. Once it's done, I'll work on more Rust abstrations for
real network drivers.

Thanks,