Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <20230930161443.58812-1-xubo3006@163.com>
In-Reply-To: <20230930161443.58812-1-xubo3006@163.com>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Sat, 30 Sep 2023 20:48:59 +0200
Message-ID: <CANiq72n+xr+3TqQMaoP3xOzM2ZqUyzKvGm2wDMd8SbsqBK7nvg@mail.gmail.com>
Subject: Re: [PATCH 1/2] rust: add Soft-RoCE driver basic structure
To:     AllenX <xubo3006@163.com>
Cc:     rust-for-linux@vger.kernel.org, ojeda@kernel.org,
        alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com,
        gary@garyguo.net, bjorn3_gh@protonmail.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Allen,

A quick few notes I noticed that you will probably want to address
before sending this to the Infiniband/rdma maintainers. Please note
that some of these apply in more than one instance, and that these are
just surface-level -- I have not actually looked at what the code is
doing or whether it is sound.

On Sat, Sep 30, 2023 at 6:16=E2=80=AFPM AllenX <xubo3006@163.com> wrote:
>
> +//! Infiniband soft-Roce devices.

Please follow the formatting/coding style of the rest of the code
already upstream. For instance, a newline should be placed here.

> +/// Soft-Roce transport registration.
> +///

Docs should not end in an empty line.

> +impl<T: RxeOperation> Registration<T> {
> +    /// Creates a new [`Registration`] but does not register it yet.
> +    ///
> +    /// It is allowed to move.
> +    pub fn new(name: &'static CStr) -> Self {
> +        // INVARIANT: `registered` is `false`

There is no `# Invariants` section in the type. Please check other
types we have to see how it is usually done.

> +    /// Registers a infiniband soft-Roce device

Docs should only have a single first line/sentence and end in a period.

> +        // SAFETY: The adapter is compatible with the rdma_link_register

Please use Markdown in code comments, just like in the documentation,
and end sentences and comments in a period for consistency with the
rest of the code; e.g.

    with the `rdma_link_register`.

> +        pr_info!("loaded");

Debugging code?

> +/// soft-Roce register net sockets

Please be consistent in the documentation, e.g. you used "Soft-Roce" above.

> +    /// Init ipv4 socket
> +    fn ipv4_init(&mut self) -> Result<()> {
> +        let mut udp_cfg =3D bindings::udp_port_cfg::default();
> +        let mut tnl_cfg =3D bindings::udp_tunnel_sock_cfg::default();
> +        let mut sock: *mut bindings::socket =3D ptr::null_mut();
> +
> +        udp_cfg.family =3D bindings::AF_INET as u8;
> +        udp_cfg.local_udp_port =3D 46866;
> +        // SAFETY: [`bindings::init_net`] and [`udp_cfg`] can be safely =
passed to [`bindings::udp_sock_create4`]
> +        // [`sock`] will be pass to [`self.sk4`] later, it will live at =
least as long as the module, which is an implicit requirement
> +        let err =3D
> +            unsafe { bindings::udp_sock_create4(&mut bindings::init_net,=
 &mut udp_cfg, &mut sock) };
> +
> +        if err < 0 {
> +            pr_err!("Failed to create IPv4 UDP tunnel\n");
> +            return Err(Error::from_kernel_errno(err));
> +        }
> +
> +        tnl_cfg.encap_type =3D 1;
> +        tnl_cfg.encap_rcv =3D RxeUdpEncapRecvFuncTable::<T>::build_func(=
);
> +
> +        // SAFETY: [`bindings::init_net`] and [`tnl_cfg`] can be safely =
passed to [`bindings::setup_udp_tunnel_sock`]
> +        // [`sock`] will be pass to [`self.sk4`] later, it will live at =
least as long as the module, which is an implicit requirement
> +        unsafe { bindings::setup_udp_tunnel_sock(&mut bindings::init_net=
, sock, &mut tnl_cfg) }
> +        self.sk4 =3D Some(sock);
> +        Ok(())
> +    }

Some networking abstractions will be needed here, instead of calling
the C APIs directly. There are some networking discussions going on in
the list and in our Zulip -- please take a look!

> +            if err < 0 {
> +                // EAFNOSUPPORT
> +                if err =3D=3D -97 {

The error code should be added to the list that we import from the C
headers, instead of hardcoding the value.

> +                    pr_err!("IPv6 is not supported, can not create a UDP=
v6 socket\n");
> +                    return Ok(());

Should this really return `Ok`?

Does the C side print errors too?

> +            if self.rxe_net_notifier.is_some() {
> +                // SAFETY: [`self.rxe_net_notifier`] is Some, it was pre=
viously created by
> +                // RxeNotifyFuncTable::<T>::build().
> +                unsafe {
> +                    bindings::unregister_netdevice_notifier(
> +                        &mut self.rxe_net_notifier.take().unwrap(),
> +                    )
> +                };
> +            }
> +            return Err(Error::from_kernel_errno(err));

This looks like it is undoing an operation before returning in an
error path -- there are usually better patterns for this, like RAII or
the `ScopeGuard` type (already in-tree).

> +config SAMPLE_RUST_RXE
> +       tristate "Soft-Roce"
> +       help
> +         This option builds the self test cases for Rust.

Nowadays we have KUnit support for running Rust code documentation
tests -- you may be interested in those.

Also, more documentation, including `# Examples` sections in the
abstractions would be very helpful, I would imagine.

Cheers,
Miguel