Re: [RFC] add Rust API for basic port operations

["Etelson, Gregory" <getelson@nvidia.com>]

Hello Harry,

>
> Thanks for posting this RFC. I'll share my API RFC in the coming days too, for now
> some feedback (which is already integrated/existing in the API I've been working on)
>
> Bigger picture; the APIs below implement "DPDK C API thinking" in Rust. I'd like to rework the APIs to be "DPDK with Rust Ergonomics" instead:
>
> Example:
> 1) DpdkPortConf::new_from(0, c_dev_conf_struct, c_rx_conf_struct, c_tx_conf_struct, 8, 8, 512, 512, mempool (or None));
>    - the API here is not use case focussed. The values passed in are documented, but not easy to explore/understand.
>
> 2) let ports = dpdk::get_ports(); // returns a Vec<DpdkPort>
>   let p = ports.get(0);
>   p.rxqs(8, rx_mempool)?;
>   p.rx_crc_strip(true);
>   p.set_rss_hash(&[0,1,2,3..N])?;
>
>   // Notes: configure RX values: note the type-safe, and english-syntax functions.
>   // These functions prompt in modern IDEs, making "exploring" APIs easy/fast/intuitive (struct member bit-fields are not as easy to explore)
>   // The "rxqs()" function takes the parameters it needs (u16 count, and a Arc<Dpdk::mempool> object)
>   //   - this avoids exposing an "Option<Arc<Mempool>>" publically, hiding impl detail, giving better ergonomics
>   // Notice the "?" operator, this can be used "inline" to early-return and error (e.g. rss_hash array has incorrect length)
>

Agree.
That approach is much better.

>
> The ability to error check individual functions/APIs allow better error reporting to the user,
> specifically at the point where the error occurred. As these are not datapath APIs, allocations and
> overhead in the failure case should not be a problem - user-experience is the goal:
>
> 1) on error: -EINVAL returned from rte_eth_rxq_configure(), difficult to see what parameter caused the -EINVAL.
> 2) on error: "rss_hash has invalid lenght of 42" (or whatever we make the Debug/std::error::Error impl say)
>
> To summarize, "use-case focused APIs" are easier to read, debug, and maintain.
> The work here is to design/build these Rust ergonomic APIs: it takes DPDK knowledge, and some Rust API
> best-practices to figure these out. There's probably a "playbook" of like 3 rules/strategies, which will
> cover 90%+ of the API design.
>
> Hopefully this email is a starting point; some resources for future reader's reference;
> - https://burntsushi.net/rust-error-handling/
> - https://corrode.dev/blog/idiomatic-rust-resources/ (look for "error-handling" tag)
>

I'll definitely check these out

>
>> ```rust
>> /// Configuration details for a DPDK port.
>> ///
>> /// # Overview
>> ///
>> /// `DpdkPortConf` is used to initialize and configure ports in a DPDK environment. It includes:
>> /// - Device information (`dev_info`) pulled from DPDK.
>> /// - Transmit and receive configurations, including queue settings and memory pools.
>> /// - Details about the number of queues and descriptors for transmission and reception.
>> ///
>> /// This struct is typically instantiated using the [`DpdkPortConf::new_from`] method.
>> ///
>> /// # Example
>> ///
>> /// ```
>> /// let conf = DpdkPortConf::new_from(
>> ///     port_id,
>> ///     dev_conf,
>> ///     tx_conf,
>> ///     rx_conf,
>> ///     8,               // Number of RX queues
>> ///     8,               // Number of TX queues
>> ///     512,             // RX descriptors
>> ///     512,             // TX descriptors
>> ///     0,               // RX queue socket ID
>> ///     0,               // TX queue socket ID
>> ///     Some(rx_mempool) // RX queue mempool
>> /// ).unwrap();
>
> A few observations; The DpdkPortConf itself is a Rust struct, but internally it exposes a number
> of "pub" fields (rte_eth_dev_info, _eth_conf, rte_eth_rxconf, ...), which are directly bindgen-ed
> from the DPDK C API. While this works, it will not provide the ergonomics we'd like for Rust code.
>
> For example, the eth_dev_info->driver_name is a CString (aka, const char *), which needs to be
> converted to a safe Rust CString or CStr (see https://doc.rust-lang.org/beta/std/ffi/struct.CStr.html#method.from_ptr)
>
> A big value add in Rust development is the automatic "Debug" implementations, allowing one to just
>        println!("{dev_info:?}");
> a struct, and that the compiler is able to identify the contents, and present them automatically.
> That will not work if we have C structures with pointers, resulting in bad debug/logging for users.
>

What about explicitly implementing Debug for types that are referenced as pointers ?
For example:

```rust
#[derive(Copy, Clone)]
struct MbufPtr(*const rte_mbuf);

impl std::fmt::Debug for MbufPtr {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         if self.0.is_null() {
             return write!(f, "MbufPtr(null)");
         }

         let mbuf = unsafe { &*self.0 };
         let data_off = unsafe { mbuf.__bindgen_anon_1.__bindgen_anon_1.data_off };
         let pkt_len = unsafe { mbuf.__bindgen_anon_2.__bindgen_anon_1.pkt_len };

         write!(f, "MbufPtr {{ address: {:p}, data_offset: {}, packet_length: {} }}",
             self.0,
             data_off,
             pkt_len
         )
     }
}
```

The activation is like this:
```rust
let mbuf_ptr: *const rte_mbuf = PTR;
println!("{:?}", MbufPtr(mbuf_ptr));
```

Output:
```
MbufPtr { address: 0x1033af140, data_offset: 128, packet_length: 62 }
```

>
>> ///
>> #[derive(Clone)]
>> pub struct DpdkPortConf {
>>     /// Information about the DPDK Ethernet device (e.g., driver, capabilities, etc.).
>>     pub dev_info: rte_eth_dev_info,
>>
>>     /// Configuration for the Ethernet device. Determines the overall behavior of the device.
>>     pub dev_conf: rte_eth_conf,
>>
>>     /// Configuration for transmitting (Tx) packets on the Ethernet device.
>>     pub tx_conf: rte_eth_txconf,
>>
>>     /// Configuration for receiving (Rx) packets on the Ethernet device.
>>     pub rx_conf: rte_eth_rxconf,
>>
>>     /// Number of receive (Rx) queues configured on this port.
>>     pub rxq_num: u16,
>>
>>     /// Number of transmit (Tx) queues configured on this port.
>>     pub txq_num: u16,
>>
>>     /// Number of descriptors for each transmit (Tx) queue.
>>     /// Descriptors represent items in a queue to handle packets.
>>     pub tx_desc_num: u16,
>>
>>     /// Number of descriptors for each receive (Rx) queue.
>>     pub rx_desc_num: u16,
>>
>>     /// NUMA socket ID associated with the memory used by receive (Rx) queues.
>>     pub rxq_socket_id: u32,
>>
>>     /// NUMA socket ID associated with the memory used by transmit (Tx) queues.
>>     pub txq_socket_id: u32,
>>
>>     /// Memory pool associated with receive (Rx) queues.
>>     /// This manages the buffers used for storing incoming packets.
>>     pub rxq_mempool: Option<Arc<DpdkMempool>>,
>> }
>
> Any reaons that rxq_mempool has an Option<>..? Is it for TX-only port configurations?
> That's the only thing I could imagine is a cause. See above API where RX and TX are
> configured independently - this avoids passing an "RX mempool" at all, if only TX is configured!
>

Right. The Option<> is for Tx only configuration.

>
>> /// A trait defining basic operations for managing and interacting with a DPDK port.
>> ///
>> /// # Overview
>> ///
>> /// The `DpdkPort` trait standardizes how to operate on DPDK ports, making it possible to:
>> /// - Configure the Ethernet device using [`configure`].
>> /// - Start the device using [`start`].
>> /// - Handle Rx and Tx bursts of packets using [`rx_burst`] and [`tx_burst`].
>> ///
>> pub trait DpdkPort: Send + Sync {
>
> This trait provides a very C API centric view. Unfortunately, that view does not include the
> Rust API semantics we need for RXQ and TXQ being Send/!Sync (see https://youtu.be/lb6xn2xQ-NQ?t=802)
>
> Example of INCORRECT usage of this RFC API (that Rust compiler cannot help with)
> (refer to https://github.com/getelson-at-mellanox/rdpdk/blob/main/port/mlx5/mlx5_port.rs#L30 for port creation)
> thread 1:
>   let port = Mlx5::from(0, &port_conf);
>   let pkts = &mut [*mut rte_mbuf; 32];
>   port.rx_burst(0, &mut pkts); // segfault simulateous access on (port/queue combo)
>
> thread 2:
>   let port = Mlx5::from(0, &port_conf);
>   let pkts = &mut [*mut rte_mbuf; 32];
>   port.rx_burst(0, &mut pkts); // segfault simulateous access on (port/queue combo)
>
> The problem here is that the user was able to create two "handles" to the same "port".
> And from that handle of a port, it was possible to call rx_burst(), (or tx_burst(), etc).
>
> The Rust compiler cannot identify that Mlx5::from(0) is not safe to call twice.
> My current preferred solution is to have a Dpdk::eth::get_ports() API, which returns
> all the port instances. This removes the user's ability to "create" ports at all, and hence
> it cannot be mis-used either.
>
> All in all, the method in which C DPDK APIs work (pass in integer ID for port/queue), is
> a very different to how safe Rust APIs will look when building in Send/Sync safety.
>
>

I see your point now.

>
>>     /// Returns the port ID of the DPDK port.
>>     ///
>>     /// # Return Value
>>     /// A `u16` that uniquely identifies the DPDK port.
>>     ///
>>     /// # Example
>>     /// ```
>>     /// let port_id = dpdk_port.port_id();
>>     /// println!("DPDK port ID: {}", port_id);
>>     /// ```
>>     fn port_id(&self) -> u16;
>>
>>     /// Returns a reference to the configuration object of the DPDK port.
>>     ///
>>     /// # Return Value
>>     /// A reference to [`DpdkPortConf`], which contains various settings like Rx/Tx queue configurations,
>>     /// memory pools, and NUMA socket IDs.
>>     ///
>>     /// # Example
>>     /// ```
>>     /// let port_config = dpdk_port.port_conf();
>>     /// println!("Rx queues: {}", port_config.rxq_num);
>>     /// ```
>>     fn port_conf(&self) -> &DpdkPortConf;
>>
>>     /// Configures the DPDK Ethernet device with the settings specified in the port configuration.
>>     ///
>>     /// This method is typically called before starting the port to ensure it is prepared for Rx and Tx operations.
>>     ///
>>     /// # Return Value
>>     /// - `Ok(())` if the configuration was applied successfully.
>>     /// - `Err(String)` with a descriptive error message if the configuration failed.
>>     ///
>>     /// # Example
>>     /// ```
>>     /// let result = dpdk_port.configure();
>>     /// if let Err(err) = result {
>>     ///     eprintln!("Failed to configure the port: {}", err);
>>     /// }
>>     /// ```
>>     fn configure(&mut self) -> Result<(), String>;
>>
>>     /// Starts the DPDK Ethernet device.
>>     ///
>>     /// This method initializes the Rx and Tx queues, making the port ready for data transmission
>>     /// and reception.
>>     ///
>>     /// # Return Value
>>     /// - `Ok(())` if the port was started successfully.
>>     /// - `Err(String)` if the startup process failed, with a descriptive error message.
>>     ///
>>     /// # Example
>>     /// ```
>>     /// let result = dpdk_port.start();
>>     /// if let Err(err) = result {
>>     ///     eprintln!("Failed to start the port: {}", err);
>>     /// }
>>     /// ```
>>     fn start(&mut self) -> Result<(), String>;
>>
>>     /// Receives a burst of packets on the specified Rx queue.
>>     ///
>>     /// # Parameters
>>     /// - `queue_id`: The ID of the Rx queue to receive packets from.
>>     /// - `pkts`: A mutable reference to an array of packet buffers (`*mut rte_mbuf`) where received packets
>>     ///   will be written.
>>     ///
>>     /// # Return Value
>>     /// - `Ok(u16)` containing the number of packets successfully received.
>>     /// - `Err(String)` if the operation failed.
>>     ///
>>     /// # Example
>>     /// ```
>>     /// let pkts: Vec<*mut rte_mbuf> = vec![std::ptr::null_mut(); 32];
>>     /// let received = dpdk_port.rx_burst(0, &pkts);
>>     /// match received {
>>     ///     Ok(count) => println!("Received {} packets", count),
>>     ///     Err(err) => eprintln!("Rx burst failed: {}", err),
>>     /// }
>>     /// ```
>>     fn rx_burst(&mut self, queue_id: u16, pkts: &[*mut rte_mbuf]) -> Result<u16, String>;
>>
>>     /// Sends a burst of packets on the specified Tx queue.
>>     ///
>>     /// # Parameters
>>     /// - `queue_id`: The ID of the Tx queue to send packets on.
>>     /// - `pkts`: A reference to an array of packet buffers (`*mut rte_mbuf`) to send.
>>     ///
>>     /// # Return Value
>>     /// - `Ok(u16)` containing the number of packets successfully sent.
>>     /// - `Err(String)` if the operation failed.
>>     ///
>>     /// # Example
>>     /// ```
>>     /// let pkts: Vec<*mut rte_mbuf> = vec![some_packet_ptr1, some_packet_ptr2];
>>     /// let sent = dpdk_port.tx_burst(0, &pkts);
>>     /// match sent {
>>     ///     Ok(count) => println!("Sent {} packets", count),
>>     ///     Err(err) => eprintln!("Tx burst failed: {}", err),
>>     /// }
>>     /// ```
>>     fn tx_burst(&mut self, queue_id: u16, pkts: &[*mut rte_mbuf]) -> Result<u16, String>;
>> }
>> ```
>
> Having a single trait with configure(), start() and rx_burst() makes it impossible to encode the
> Send/Sync attributes of the "Port" concept, and the "RxQueue" concept seperately.
> So perhaps a solution with two traits could work; I haven't investigated in detail yet.
>
>
>> The API uses raw FFI pointers in DpdkPort rx_burst() and tx_burst() to preserve DPDK performance.
>
> Cool, nice trick... but does it remove all virtual functions/function-pointer dispatch?
> - https://github.com/getelson-at-mellanox/rdpdk/blob/main/app/runpmd/runpmd.rs#L143
> This seems to accept a "&mut Vec<Box<dyn DpdkPort>>", which would mean the ".rx_burst()"
> is a virtual-function into the trait, and then a function-pointer into the PMD?
>

Mlx5 trait implementation calls PMD rx_burst() and tx_burst() functions directly.
This way I did not need to re-implement original DPDK static inlining for IO calls.
For that approach to work DPDK must be patched to expose PMD IO functions.
See 
https://github.com/getelson-at-mellanox/rdpdk/blob/main/dpdk-patches/0002-net-mlx5-refactor-Rx-Tx-functions-selection.patch


> It could be interesting to change "do_io" to a generic function, taking a generic DpdkPort instance? Then the first level
> trait-resolution would become compile-time (through Rust compiler monomorphisation of the generic do_io<Pmd: impl DpdkPort>() into a
> more specific  "do_io<Mlx5>()"  code?

For multi-hw setups that function declaration need to be `do_io<R: DpdkPort, T: DpkdPort>()`
But that will not compile for Rx-only calls.
Without generics the function works for any HW type and for all directions.

>
> I don't believe this to *actually* matter for performance. It might matter for an IO-forward, 0 packet data processing test.
> So micro-benchmark bragging, yes this has impact... but touch the packet data and it no longer matters!
>
>
>> The API implementation is here:
>>
>> API definition:
>> https://github.com/getelson-at-mellanox/rdpdk/blob/main/lib/port/port.rs
>>
>> API implementation with RTE function calls:
>> https://github.com/getelson-at-mellanox/rdpdk/blob/main/lib/port/raw_port.rs
>>
>> Implement direct calls to mlx5 Rx/Tx IO functions:
>> https://github.com/getelson-at-mellanox/rdpdk/blob/main/port/mlx5/mlx5_port.rs
>>
>>
>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>
> Thanks for sharing the links and RFC!
>
> I will share the RFC API that communicates the Send/Sync requirements to the Rust compiler for Ethdev Port Rxq as a sample,
> and we can try compare/converge the "trait DpdkPort" and the compile-time-thread-safe approaches into the best API?
>

== thumbs-up ==

Regards,
Gregory