Re: [PATCH] rust: support DPDK API

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> From: Gregory Etelson <getelson@nvidia.com>
> Sent: Thursday, March 6, 2025 1:37 PM
> To: dev@dpdk.org <dev@dpdk.org>
> Cc: getelson@nvidia.com <getelson@nvidia.com>; thomas@monjalon.net <thomas@monjalon.net>; mkashani@nvidia.com <mkashani@nvidia.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH] rust: support DPDK API

More in-depth review in this reply..

> The patch converts include files with DPDK API to RUST and binds new
> RUST API files info dpdklib package.
> 
> The RUST dpdklib files and DPDK libraries build from C sources
> allow creation of DPDK application in RUST.
> 
> RUST DPDK application must specify the `dpdklib` package as
> dependency in Cargo.toml file.
> 
> RUST `dpdklib` package is installed into
> MESON_INSTALL_DESTDIR_PREFIX/rust directory.
> 
> Software requirements:
> - clang
> - RUST installation
> - bindgen-cli crate
> 
> RUST dpdklib installation instructions:
> 1. Configure DPDK with `-Deanble_rust=true`
> 2. Build and install DPDK. The installation procedure will create
>    MESON_INSTALL_DESTDIR_PREFIX/rust directory.
> 3. Update PKG_CONFIG_PATH to point to DPDK installation.

I've tested these instructions, using the default prefix = "/usr/local"
This puts the rust files in "/usr/local/rust", so some tidy-up required.
Debugging (or "searching for the files" was difficult - perhaps a log of
the bindgen output from "rust-env.sh" would be helpful for 1st time setup?)


> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
>  buildtools/meson.build               |   4 +
>  buildtools/rust-env.sh               |  78 +++++++++++
>  examples/rust/helloworld/Cargo.lock  |  14 ++
>  examples/rust/helloworld/Cargo.toml  |   7 +
>  examples/rust/helloworld/build.rs    |  21 +++
>  examples/rust/helloworld/src/main.rs | 197 +++++++++++++++++++++++++++
>  meson_options.txt                    |   2 +
>  7 files changed, 323 insertions(+)
>  create mode 100755 buildtools/rust-env.sh
>  create mode 100644 examples/rust/helloworld/Cargo.lock
>  create mode 100644 examples/rust/helloworld/Cargo.toml
>  create mode 100644 examples/rust/helloworld/build.rs
>  create mode 100644 examples/rust/helloworld/src/main.rs
> 
> diff --git a/buildtools/meson.build b/buildtools/meson.build
> index 4e2c1217a2..dd16567f51 100644
> --- a/buildtools/meson.build
> +++ b/buildtools/meson.build
> @@ -50,3 +50,7 @@ else
>      pmdinfo += 'ar'
>      pmdinfogen += 'elf'
>  endif
> +
> +if get_option('enable_rust')
> +    meson.add_install_script('rust-env.sh')
> +endif
> diff --git a/buildtools/rust-env.sh b/buildtools/rust-env.sh
> new file mode 100755
> index 0000000000..705bc0d95b
> --- /dev/null
> +++ b/buildtools/rust-env.sh
> @@ -0,0 +1,78 @@
> +#! /bin/sh
> +
> +# Convert DPDK API files into RUST.
> +# DPDK files selection is on demand.
> +#
> +# The coversion is done in 4 stages:
> +# 1. Preparation [Optional]
> +#    Due to the bindgen conversion utility limitations source file may need
> +#    manual adjustment.
> +# 2. Preprocessing [Mandatory]
> +#    Run preprocessor on a source file before conversion.
> +# 3. Conversion [Mandatory]
> +#    Convert preprocessed C file into RUST file
> +# 4. Post translation [Optional]
> +#    Manually fix translation.
> +
> +# DPDK files list
> +files='
> +rte_build_config.h
> +rte_eal.h
> +rte_ethdev.h
> +rte_mbuf.h
> +rte_mbuf_core.h
> +rte_mempool.h
> +'
> +
> +rust_dir="${MESON_INSTALL_DESTDIR_PREFIX}/rust"

Here "rust_dir"="/usr/local/rust" if the default prefix is used.
At least, a "dpdk" must be in there somewhere (we're not Rust itself after all!)
but also different systems put files in e.g: "/usr/local/lib/x86_64-linux-gnu/dpdk"

Appending a "/rust" to the end of that is probably what was intended?


> +include_dir="${MESON_INSTALL_DESTDIR_PREFIX}/include"
> +
> +if test -d "$rust_dir"; then
> +  rm -rf "$rust_dir"
> +fi
> +
> +mkdir -p "$rust_dir/src"
> +if ! test -d "$rust_dir"; then
> +  echo "failed to create Rust library $rust_dir"
> +  exit 255
> +fi
> +touch "$rust_dir/src/lib.rs"
> +
> +bindgen_opt='--no-layout-tests --no-derive-debug'
> +bindgen_clang_opt='-Wno-unused-command-line-argument'
> +
> +create_rust_lib ()
> +{
> +  base=$1
> +
> +  cp $include_dir/${base}.h /tmp/${base}.h
> +
> +# bindgen cannot process complex macro definitions
> +# manually simplify macros before conversion
> +  sed -i -e 's/RTE_BIT64(\([0-9]*\))/(1UL << \1)/g' /tmp/${base}.h
> +  sed -i -e 's/RTE_BIT32(\([0-9]*\))/(1U << \1)/g' /tmp/${base}.h
> +  sed -i -e 's/UINT64_C(\([0-9]*\))/\1/g' /tmp/${base}.h
> +
> +  # clang output has better integration with bindgen than GCC
> +  clang -E -dD -I$include_dir /tmp/${base}.h > /tmp/$base.i

# Adding this helps debug the header conversion & output file locations
echo "writing Rust converted header to ${rust_dir}/src/${base}.rs"

> +  bindgen $bindgen_opt --output $rust_dir/src/$base.rs /tmp/$base.i -- $bindgen_clang_opt
> +  rm -f /tmp/$base.i /tmp/$base.h
> +}
> +
> +for file in $files; do
> +  base=$(basename $file | cut -d. -f 1)
> +  create_rust_lib $base
> +  echo "pub mod $base;" >> "$rust_dir/src/lib.rs"
> +done
> +
> +
> +cat > "$rust_dir/Cargo.toml" <<EOF
> +[package]
> +name = "dpdklib"
> +version = "$(cat $MESON_SOURCE_ROOT/VERSION | sed 's/\.0\([1-9]\)/\.\1/')"

Missing {} brackets around MESON_SOURCE_ROOT perhaps? Got an error on "/VERSION" not found.

> +EOF
> +
> +# post conversion updates
> +# RUST does not accept aligned structures into packed structure.
> +# TODO: fix DPDK definitions.
> +sed -i 's/repr(align(2))/repr(packed(2))/g'  "$rust_dir/src/rte_ethdev.rs"

Hmm, interesting, lets keep a TODO here, but as this changes the "allowable" or valid binary layouts,
it is potentially going to cause differences between C and Rust's interpretation of the memory backing the structs.
TODO: investigate each struct individually, and determine if/what is really required/correct.


> diff --git a/examples/rust/helloworld/Cargo.lock b/examples/rust/helloworld/Cargo.lock
> new file mode 100644
> index 0000000000..d18af80c66
> --- /dev/null
> +++ b/examples/rust/helloworld/Cargo.lock
> @@ -0,0 +1,14 @@
> +# This file is automatically @generated by Cargo.
> +# It is not intended for manual editing.
> +version = 4
> +
> +[[package]]
> +name = "dpdklib"
> +version = "25.3.0-rc1"
> +
> +[[package]]
> +name = "helloworld"
> +version = "0.1.0"
> +dependencies = [
> + "dpdklib",
> +]
> diff --git a/examples/rust/helloworld/Cargo.toml b/examples/rust/helloworld/Cargo.toml
> new file mode 100644
> index 0000000000..7f9a77968a
> --- /dev/null
> +++ b/examples/rust/helloworld/Cargo.toml
> @@ -0,0 +1,7 @@
> +[package]
> +name = "helloworld"
> +version = "0.1.0"
> +edition = "2024"
> +
> +[dependencies]
> +dpdklib = {path = "MESON_INSTALL_DESTDIR_PREFIX/rust"}
> \ No newline at end of file

Above Cargo.toml looks normal. Perhaps we could suggest/provide a method where
DPDK does not need to be "ninja install"-ed, but instead process a local directory?
That would likely make it easier to locally build/test Rust bindings.


> diff --git a/examples/rust/helloworld/build.rs b/examples/rust/helloworld/build.rs
> new file mode 100644
> index 0000000000..5c76188e18
> --- /dev/null
> +++ b/examples/rust/helloworld/build.rs
> @@ -0,0 +1,21 @@
> +use std::process::Command;
> +
> +pub fn main() {
> +    let mut pkgconfig = Command::new("pkg-config");
> +
> +    match pkgconfig.args(["--libs", "libdpdk"]).output() {
> +        Ok (output) => {
> +            let stdout = String::from_utf8_lossy(&output.stdout).trim_end().to_string();
> +            for token in stdout.split_ascii_whitespace().filter(|s| !s.is_empty()) {
> +                if token.starts_with("-L") {
> +                    println!("cargo::rustc-link-search=native={}", &token[2..]);
> +                } else if token.starts_with("-l") {
> +                    println!("cargo::rustc-link-lib={}", &token[2..]);
> +                }
> +            }
> +        }
> +        Err(error) => {
> +            panic!("failed to read libdpdk package: {:?}", error);
> +        }
> +    }
> +}

Could a simple "probe()" like below replace the more complex logic? I've linked DPDK in the
past using this more boring method.. or is there a reason that the link-search-native and
link-lib parts have to be handled manually? Doing a Command::new() is hacky, nice to remove it if possible!

 if let Err(e) = pkg_config::Config::new().probe("libdpdk") {
    panic!("libdpdk probe failed: {e:?}");
 }

The build.rs file itself also needs a "rebuild if self changed" line:
  println!("cargo:rerun-if-changed=build.rs");


> diff --git a/examples/rust/helloworld/src/main.rs b/examples/rust/helloworld/src/main.rs
> new file mode 100644
> index 0000000000..cf666aece6
> --- /dev/null
> +++ b/examples/rust/helloworld/src/main.rs
> @@ -0,0 +1,197 @@
> +/// Usage: helloworld -a <port 1 params> -a <port 2 params> ...
> +
> +use std::env;
> +use std::ffi::{CString};
> +use std::os::raw::{c_int, c_char};
> +use std::ffi::CStr;
> +
> +use dpdklib::rte_eal::{
> +    // Functions
> +    rte_eal_init,
> +    rte_eal_cleanup,
> +};
> +
> +use dpdklib::rte_ethdev::{

As per first reply, I'd like to see the end-result safe-Rust namespacing be shortened.

This can be easily done by renaming the name= in Cargo.toml, and
removing the "rte_" prefix at bindgen output stage for each file.
dpdk::ethdev:: { RTE_ETH_NAME_MAX_LEN, ... };

Perhaps its a good method to put the generated (unsafe, raw-C-bindings) in this
namespace, and build a safe crate (with the dpdk::ethdev::* namespacing) over it.
Thoughts?


> +    RTE_ETH_NAME_MAX_LEN,
> +    RTE_ETH_DEV_NO_OWNER,
> +    RTE_ETH_RSS_IP,
> +    rte_eth_rx_mq_mode_RTE_ETH_MQ_RX_RSS,
> +    rte_eth_rx_mq_mode_RTE_ETH_MQ_RX_VMDQ_DCB_RSS,
> +
> +    // Structures
> +    rte_eth_dev_info,
> +    rte_eth_conf,
> +    rte_eth_txconf,
> +    rte_eth_rxconf,
> +
> +    // Functions
> +    rte_eth_dev_get_name_by_port,
> +    rte_eth_find_next_owned_by,
> +    rte_eth_dev_info_get,
> +    rte_eth_dev_configure,
> +    rte_eth_tx_queue_setup,
> +    rte_eth_rx_queue_setup,
> +    rte_eth_dev_start,
> +};
> +
> +use dpdklib::rte_build_config::{
> +    RTE_MAX_ETHPORTS,
> +};
> +
> +use dpdklib::rte_mbuf::{
> +    rte_pktmbuf_pool_create,
> +};
> +
> +use dpdklib::rte_mbuf_core::{
> +    RTE_MBUF_DEFAULT_BUF_SIZE
> +};
> +
> +pub type DpdkPort = u16;
> +pub struct Port {
> +    pub port_id:DpdkPort,
> +    pub dev_info:rte_eth_dev_info,
> +    pub dev_conf:rte_eth_conf,
> +    pub rxq_num:u16,
> +    pub txq_num:u16,
> +}
> +
> +impl Port {
> +    unsafe fn new(id:DpdkPort) -> Self {

This is where the first manual code is wrapping C concepts into Rust.
And the approach taken here is quite "C thinking", where it is possible
to misuse an API, and hence "unsafe" is used for many functions (more below)
and also for the C FFI function calls.

I believe this part can be improved, and that there is big value in those improvements:
Today the code is "C but compiled by the Rust compiler", the programmer has to use the APIs correctly.

I'll send a patch in reply to this one for review, suggesting ways to improve.
Future code goal: "Rust checks code: invalid code will not compile, or provide a clean error at runtime."



> +        Port {
> +            port_id:id,
> +            dev_info: unsafe {
> +                let uninit: ::std::mem::MaybeUninit<rte_eth_dev_info> = ::std::mem::MaybeUninit::zeroed().assume_init();
> +                *uninit.as_ptr()
> +            },
> +            dev_conf: unsafe {
> +                let uninit: ::std::mem::MaybeUninit<rte_eth_conf> = ::std::mem::MaybeUninit::zeroed().assume_init();
> +                *uninit.as_ptr()
> +            },
> +            rxq_num:1,
> +            txq_num:1,
> +        }
> +    }
> +}
> +
> +pub unsafe fn iter_rte_eth_dev_owned_by(owner_id:u64) -> impl Iterator<Item=DpdkPort> {
> +    let mut port_id:DpdkPort = 0 as DpdkPort;
> +    std::iter::from_fn(move || {
> +        let cur = port_id;
> +        port_id = unsafe {
> +            rte_eth_find_next_owned_by(cur, owner_id) as DpdkPort
> +        };
> +        if port_id == RTE_MAX_ETHPORTS as DpdkPort {
> +            return None
> +        }
> +        if cur == port_id { port_id += 1 }
> +        Some(cur)
> +    })
> +}
> +
> +pub unsafe fn iter_rte_eth_dev() -> impl Iterator<Item=DpdkPort> {
> +    unsafe {
> +      iter_rte_eth_dev_owned_by(RTE_ETH_DEV_NO_OWNER as u64)
> +    }
> +}

Cool - I like the egonomics idea of providing Iterator<DpdkPort>.
The implementation here is unsafe, which is unfortunate, we'd like to
ensure that all "end-user visible" Rust code is "safe Rust".


> +pub unsafe fn init_port_config(port: &mut Port) {
> +    let ret = unsafe {
> +        rte_eth_dev_info_get(port.port_id, &mut port.dev_info as *mut rte_eth_dev_info)
> +    };
> +    if ret != 0 {
> +        panic!("port-{}: failed to get dev info {ret}", port.port_id);
> +    }
> +
> +    port.dev_conf.rx_adv_conf.rss_conf.rss_key = std::ptr::null_mut();
> +    port.dev_conf.rx_adv_conf.rss_conf.rss_hf = if port.rxq_num > 1 {
> +        RTE_ETH_RSS_IP as u64 & port.dev_info.flow_type_rss_offloads
> +    } else {0};
> +
> +    if port.dev_conf.rx_adv_conf.rss_conf.rss_hf != 0 {
> +        port.dev_conf.rxmode.mq_mode =
> +            rte_eth_rx_mq_mode_RTE_ETH_MQ_RX_VMDQ_DCB_RSS & rte_eth_rx_mq_mode_RTE_ETH_MQ_RX_RSS;
> +    }
> +}
> +
> +pub unsafe fn show_ports_summary(ports: &Vec<Port>) {
> +    let mut name_buf:[c_char;RTE_ETH_NAME_MAX_LEN as usize]= [0 as c_char;RTE_ETH_NAME_MAX_LEN as usize];
> +    let title = format!("{:<4}    {:<32} {:<14}", "Port", "Name", "Driver");
> +    println!("{title}");
> +    ports.iter().for_each(|p| unsafe {
> +        let _rc = rte_eth_dev_get_name_by_port(p.port_id, name_buf.as_mut_ptr());
> +        let name = CStr::from_ptr(name_buf.as_ptr());
> +        let drv = CStr::from_ptr(p.dev_info.driver_name);
> +        let summary = format!("{:<4}    {:<32} {:<14}",
> +                              p.port_id, name.to_str().unwrap(), drv.to_str().unwrap());
> +        println!("{summary}");
> +    });
> +
> +}

There are some interesting changes of thoughts in Rust vs C, in how applications actually
allocate or "own" data. The above show_ports_summary() function requires a &Vec<Port>.
That demands that the backing storage for Port is a Vec<Port>, and a (immutable) reference
is passed into this function. Hence the type is:
    summary(ports: &Vec<Port>)

There is no reason to "demand" that the application stores Ports in a Vec. While common to store things
in Vec<T>, we have now demanded "more than required" from the application. In some situations
(perhaps not 100% relevant here, but..) like embedded systems, where dynamic allocation isn't available
there IS no Vec<T> available. In other application scenarios, perhaps a HashMap is better.

The best way to do this in Rust is to use the Iterator trait, which doesn't require any specific
"backing storage" (e.g. linear-data for 'slice' or Vec, or HashMap per-item allocated data, or ...).
Instead the Iterator trait allows the function itself to iterate of the "Item". Example:

pub fn show_ports_summary_iterator<'a>(ports: impl Iterator<Item = &'a Port>) {
    ports.for_each(|p| { /* p is a &Port */);
}

There should be no unsafe required for the above debug print, and no statically sized C-string allocations.
Instead of fetching the port name via C function at print-time, the Port::new() function can cache the value
and convert it to a Rust String type - avoids unsafe {}, reduces scope of unsafe variables in the program.


> +unsafe fn start_port(port:&mut Port) {
> +    let mut rc = unsafe {
> +      rte_eth_dev_configure(port.port_id, port.rxq_num, port.txq_num,
> +                            &port.dev_conf as *const rte_eth_conf)
> +    };
> +    if rc != 0 { panic!("failed to configure port-{}: {rc}", port.port_id)}
> +    println!("port-{} configured", port.port_id);
> +
> +    rc = unsafe {
> +      rte_eth_tx_queue_setup(port.port_id, 0, 64, 0, 0 as *const rte_eth_txconf)
> +    };
> +    if rc != 0 { panic!("port-{}: failed to configure TX queue 0 {rc}", port.port_id)}
> +    println!("port-{} configured TX queue 0", port.port_id);
> +
> +    let mbuf_pool_name = CString::new(format!("mbuf pool port-{}", port.port_id)).unwrap();
> +    let mbuf_pool : *mut dpdklib::rte_mbuf::rte_mempool = unsafe {
> +        rte_pktmbuf_pool_create(mbuf_pool_name.as_ptr(), 1024, 0, 0,
> +                                RTE_MBUF_DEFAULT_BUF_SIZE as u16, 0)
> +    };
> +    if mbuf_pool  == 0 as *mut dpdklib::rte_mbuf::rte_mempool {
> +        panic!("port-{}: failed to allocate mempool {rc}", port.port_id)
> +    }
> +    println!("port-{} mempool ready", port.port_id);
> +
> +    let mut rxq_conf:rte_eth_rxconf = port.dev_info.default_rxconf.clone();
> +    rxq_conf.offloads = 0;
> +    rc = unsafe {
> +        rte_eth_rx_queue_setup(port.port_id, 0, 64, 0,
> +                               &mut rxq_conf as *mut rte_eth_rxconf,
> +                               mbuf_pool as *mut dpdklib::rte_ethdev::rte_mempool)
> +    };
> +    if rc != 0 { panic!("port-{}: failed to configure RX queue 0 {rc}", port.port_id)}
> +    println!("port-{} configured RX queue 0", port.port_id);
> +    rc = unsafe {
> +        rte_eth_dev_start(port.port_id)
> +    };
> +    if rc != 0 { panic!("failed to start port-{}: {rc}", port.port_id)}
> +    println!("port-{} started", port.port_id);
> +}

Similar concepts around reworking "unsafe" away as above, future discussion.

> +fn main() {
> +
> +    let mut argv: Vec<*mut c_char> = env::args()
> +        .map(|arg| CString::new(arg).unwrap().into_raw()).collect();
> +
> +    let rc = unsafe {
> +        rte_eal_init(env::args().len() as c_int, argv.as_mut_ptr())
> +    };
> +    if rc == -1 {
> +        unsafe { rte_eal_cleanup(); }
> +    }
> +
> +    let mut ports:Vec<Port> = vec![];
> +    unsafe {
> +        for port_id in iter_rte_eth_dev()
> +            .take(dpdklib::rte_build_config::RTE_MAX_ETHPORTS as usize) {
> +            let mut port = Port::new(port_id);
> +            init_port_config(&mut port);
> +            println!("init port {port_id}");
> +            start_port(&mut port);
> +            ports.push(port);
> +        }
> +    }
> +
> +    unsafe { show_ports_summary(&ports); }
> +
> +    println!("Hello, world!");
> +}

Overall, I'm very happy to see this patch, I want to ensure folks on list hear that!
The unsafe {} around each block of "do something" is not idiomatic Rust - we can improve that.
The summary_iterator() function above can be called like a normal function:

show_ports_summary_iterator(ports.iter());


> diff --git a/meson_options.txt b/meson_options.txt
> index e49b2fc089..d37b9ba1dc 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -60,3 +60,5 @@ option('tests', type: 'boolean', value: true, description:
>         'build unit tests')
>  option('use_hpet', type: 'boolean', value: false, description:
>         'use HPET timer in EAL')
> +option('enable_rust', type: 'boolean', value: false, description:
> +       'enable RUST')
> --
> 2.45.2

Great work, and looking forward to hearing about next-steps, and discussion on the above feedback.

Regards! (& thanks for reading till the end.. this turned into a long response :) -Harry