On Mon, Mar 10, 2025 at 04:13:21PM +0000, Van Haaren, Harry wrote: > > From: Gregory Etelson <getel...@nvidia.com> > > Sent: Saturday, March 8, 2025 6:50 PM > > To: Van Haaren, Harry <harry.van.haa...@intel.com>; Richardson, Bruce > > <bruce.richard...@intel.com>; dev@dpdk.org <dev@dpdk.org> > > Cc: getel...@nvidia.com <getel...@nvidia.com>; mkash...@nvidia.com > > <mkash...@nvidia.com>; tho...@monjalon.net <tho...@monjalon.net> > > Subject: [PATCH v2] rust: support raw DPDK API > > > > The patch converts include files with DPDK API to RUST and binds new > > RUST API files into raw module under dpdk crate. > > > > The RUST files and DPDK libraries build from C sources > > allow creation of DPDK application in RUST. > > > > RUST DPDK application must specify the `dpdk` crate as > > dependency in Cargo.toml file. > > > > RUST `dpdk` crate is installed into > > $MESON_INSTALL_DESTDIR_PREFIX/$libdir/rust directory. > > > > Software requirements: > > - clang > > - RUST installation > > - bindgen-cli crate > > > > RUST dpdk installation instructions: > > 1. Configure DPDK with `-Deanble_rust=true` > > 2. Build and install DPDK. The installation procedure will create > > $MESON_INSTALL_DESTDIR_PREFIX/$libdir/rust crate. > > 3. Update PKG_CONFIG_PATH to point to DPDK installation. > > > > Signed-off-by: Gregory Etelson <getel...@nvidia.com> > > Please include a "revision change" summary, this helps reviewers understand > what changes you have addressed, and what changes are not present. This > usually > is formatted below a --- line, so Git doesn't include it in commit notes. > > Also add to CC any participants of the wider email discussion. > (+CC Igor and Stephen here now) > > --- > v2: > - update rust crate name to "dpdk" from "dpdklib" (Bruce) > - update binding generation to use dpdk::raw::* namespace (Bruce) > - <were other comments/notes fixed?> > --- e.g. "rte_ethdev" -> "ethdev" namespace change? > --- e.g. "cargo fmt"? > --- e.g. "build.rs" simplification & fix? > > Pulling comments from "detailed review" email thread (see v1 on patchwork) > - Gregory: "The idea was to provide generic DPDK API and let application to > resolve unsafe interfaces." > > For this patch, OK. But we (DPDK project) should take the next steps, and > provide a "Safe Rust" API. The main reasons are that: > 1) DPDK APIs have many rules around multithreading; (lcore concepts, > rx_burst() threading requirements, etc). > 2) Cross-language bindings are hard, FFI is hard (even though C/Rust is one > of the "easier" ones) > > Overall, I'd like to set the "goal" of Rust in DPDK to be "if it compiles, it > is correct". > For example, if compiler allows "rxq.rx_burst()" to be called, it must ensure > that a &mut reference is valid at that point. > That gives a Rust bindings *actual value*, as opposed to "allowing DPDK usage > from Rust (with all threading/unsafety just like in C)". > > The bindings are the required base for that: making it available. But it is > important state > that these raw bindings are NOT ready for Rust folks to go build applications > in "safe Rust". > > > - Igor "Indentation/styling is a bit off or inconsistent. I'd suggest to run > `cargo fmt` to format the code." > > Big +1. Rust has a default formatting engine, and using it means everybody > wins (no checkpatch, no manual style rework). > By reworking (and auto-saving..) my updates of the first patch, it > reformatted a bunch of code, losing my own changes. > Small things like this really hinder productivity. (Off topic: I'd like to > see a DPDK auto-formatted for C code too!) > > - Igor "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 here too. There is a logical "ownership" of functions to structs. For > value added Rust APIs, we must > not copy the existing DPDK namespacing (it does not capture "ownership" in > the way that Rust API should). > Instead, logically understanding the thread-requirements and boundaries of > concepts like an "Rxq" struct instance, > and ensuring appropriate Rust "Send" and "Sync" bounds are applied is key to > a good Rust API (as per https://youtu.be/lb6xn2xQ-NQ?t=702). > > <snip> > > > > diff --git a/examples/rust/helloworld/src/main.rs > > b/examples/rust/helloworld/src/main.rs > > new file mode 100644 > > <snip> > > > +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> { > > Looking at the above parts of "Port abstraction in Rust", I'm not sure it > really adds anything. > It feels a bit "middle of the road" (aka, adding some "abstraction", but not > going far enough). > > What if we took an even smaller step: remove the Rust struct concepts, and > just call the "dpdk::raw::*" unsafe > functions directly, as if they were C code. No "struct Port, impl Port", and > no "Iterator<DpdkPort" concepts at all. > That would show "correct" usage of the raw generated bindings, and provide > raw API unit-tests. >
Yes, definite +1 for this. Let's not try and have a half-C, half-Rust set of APIs, but instead let's have a clean low-level mapping directly to C functions, and then build a proper, safe API on top of that. /Bruce
