DPDK patches and discussions
 help / color / mirror / Atom feed
From: Igor Gutorov <igootorov@gmail.com>
To: Gregory Etelson <getelson@nvidia.com>
Cc: dev@dpdk.org, thomas@monjalon.net, mkashani@nvidia.com,
	 Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [PATCH] rust: support DPDK API
Date: Sat, 8 Mar 2025 17:28:09 +0300	[thread overview]
Message-ID: <CAL7bPf2ddD=Hnw19uO-EzBBYYZz2vYUKxWapYoYK9Qqt7wdbfA@mail.gmail.com> (raw)
In-Reply-To: <20250306133713.393057-1-getelson@nvidia.com>

Hi Gregory!

As a DPDK as well as a Rust user, I'm quite excited about this patch.
I'm wondering though, is DPDK getting an official Rust API? I'm
subscribed to dev and user mailing lists, and haven't seen any
announcements - did I miss something?

Bellow are some stylistic suggestions

On Thu, Mar 6, 2025 at 4:38 PM Gregory Etelson <getelson@nvidia.com> wrote:
>
> 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.
>
> 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:
nit: typo (coversion -> conversion)

> +# 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"
> +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
> +  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/')"
> +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"
> 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
> 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);
> +        }
> +    }
> +}
> 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::{
> +    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 {
> +        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)
> +    }
> +}

It seems to me that it is expected to avoid iterating more than
`RTE_MAX_ETHPORTS` times.
I'd suggest to add a `.take()` call here:

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)
    }
    .take(dpdklib::rte_build_config::RTE_MAX_ETHPORTS as usize)
}

Then the users of this function don't need to know about this detail.
Also, would this make `iter_rte_eth_dev()` safe?

> +
> +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>) {
Is it safe to remove `unsafe` here? Since `Port` is already
initialized, all of the calls inside this function should be valid,
unless I'm missing something.

> +    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}");
> +    });
> +
> +}
> +unsafe fn start_port(port:&mut Port) {
Same here with `unsafe` as with `show_ports_summary`. It seems all
error conditions are checked, and the application terminates if
something goes wrong.

> +    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 {
nit: I'd suggest

if mbuf_pool.is_null() {
    ...
}

> +        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)
This is quite pedantic, but it is often recommended to use `.cast()`
in raw pointer casting:
`mbuf_pool.cast::<dpdklib::rte_ethdev::rte_mempool>()` or even simply
`mbuf_pool.cast()`. [1].

> +    };
> +    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);
> +}
> +
> +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);
> +        }
> +    }

If you prefer, this can be written like so:

let ports: Vec<Port> = unsafe {
    iter_rte_eth_dev()
        .take(dpdklib::rte_build_config::RTE_MAX_ETHPORTS as usize)
        .map(|port_id| {
            let mut port = Port::new(port_id);
            init_port_config(&mut port);
            println!("init port {port_id}");
            start_port(&mut port);
            port
        })
        .collect()
}

> +
> +    unsafe { show_ports_summary(&ports); }
> +
> +    println!("Hello, world!");
> +}
> 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
>

Some other thoughts/suggestions:
- Indentation/styling is a bit off or inconsistent. I'd suggest to run
`cargo fmt` to format the code. It is possible to configure the output
to match DPDK's needs [2]. Also, `cargo fmt --check` can check if
`rustfmt` would make any formatting changes. This could be a part of
the checks in `devtools` in the future.
- I haven't checked, but I think some of my suggestions would pop up
by `cargo clippy` analysis. It may be worth having this tool be a part
of the `devtools` checks as well in the future.
- I'd make `init_port_config` and `start_port` a method on `Port`,
rather than a free function, but that's just my preference.

[1] https://github.com/rust-lang/rust-clippy/issues/8017
[2] https://rust-lang.github.io/rustfmt/?version=master&search=

--
Regards,
Igor Gutorov

  parent reply	other threads:[~2025-03-08 14:28 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-06 13:37 Gregory Etelson
2025-03-06 19:26 ` Van Haaren, Harry
2025-03-07 16:56   ` Etelson, Gregory
2025-03-07 15:54 ` Van Haaren, Harry
2025-03-07 16:20   ` Bruce Richardson
2025-03-07 18:15     ` Etelson, Gregory
2025-03-07 18:00   ` Etelson, Gregory
2025-03-08 14:28 ` Igor Gutorov [this message]
2025-03-08 19:14   ` Etelson, Gregory
2025-03-08 18:50 ` [PATCH v2] rust: support raw " Gregory Etelson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL7bPf2ddD=Hnw19uO-EzBBYYZz2vYUKxWapYoYK9Qqt7wdbfA@mail.gmail.com' \
    --to=igootorov@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=getelson@nvidia.com \
    --cc=mkashani@nvidia.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).