linux-kernel - Re: [PATCH 2/3 v4] implement ww_mutex abstraction for the Rust tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72=ER=1QqW8tbqjuPWcqR2oM2bEEm=O382x68G-whGc3MA@mail.gmail.com>
Date: Sat, 21 Jun 2025 18:15:15 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Onur Özkan <work@...rozkan.dev>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, 
	gary@...yguo.net, lossin@...nel.org, a.hindborg@...nel.org, 
	aliceryhl@...gle.com, tmgross@...ch.edu, dakr@...nel.org, 
	peterz@...radead.org, mingo@...hat.com, will@...nel.org, longman@...hat.com, 
	felipe_life@...e.com, daniel@...lak.dev, bjorn3_gh@...tonmail.com, 
	thatslyude@...il.com
Subject: Re: [PATCH 2/3 v4] implement ww_mutex abstraction for the Rust tree

Hi Onur,

A few docs-related nits, for these and future patches (some apply
several times).

On Sat, Jun 21, 2025 at 5:22 PM Onur Özkan <work@...rozkan.dev> wrote:
>
> Suggested-by: thatslyude@...il.com

Please use the usual format for names/emails, like in the other tags.
(Link also typically goes after Suggested-by to link to the
suggestion).

> +    declare_err!(EDEADLK, "Resource deadlock avoided.");

This should go after `ERANGE`.

> +//! This module provides Rust abstractions for the Linux kernel's `ww_mutex` implementation,
> +//! which provides deadlock avoidance through a wait-wound or wait-die algorithm.

Please link the C header like other modules do (with the `srctree/` syntax).

In addition, if there are good C docs, you can link those, e.g.

    https://docs.kernel.org/locking/ww-mutex-design.html

> +use crate::prelude::EBUSY;

Please import the entire prelude, rather than a subset -- that is its
goal after all.

In turn, that means you can drop other imports.

> +/// Represents a group of mutexes that can participate in deadlock avoidance together.
> +/// All mutexes that might be acquired together should use the same class.

The first paragraph is a title ("short description"), which is used in
lists of Rust items, so it is best to keep it short. So I would
recommend moving the second sentence to a new paragraph.

I would also probably suggest dropping the "Represents", i.e. you can
just start with "A group ...", since it is an item.

> +            // Ref: https://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89

These line numbers will likely go stale, and we typically don't link like this.

If you feel like you should link to particular lines, then I can
suggest git.kernel.org/linus/ instead and a tag to keep it stable,
e.g.:

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/ww_mutex.h?h=v6.16-rc2#n85

Also, please use <> to wrap links.

> +    /// Creates a `WwClass`.
> +    ///
> +    /// It's `pub` only so it can be used by the `define_ww_class!` macro.
> +    ///
> +    /// You should not use this function directly. Use the `define_ww_class!`
> +    /// macro or call `WwClass::new_wait_die` or `WwClass::new_wound_wait` instead.

Please use intra-doc links wherever they may work, e.g. [`WwClass`].

> +/// // Create mutexes

Period at the end.

> +/// let mut ctx = KBox::pin_init(WwAcquireCtx::new(&class), GFP_KERNEL).unwrap();

Please avoid `unwrap()` and other panics in examples (and in KUnit
tests too -- I will mention it there). `assert*!` is fine, though, if
you want to show that something is a particular value (that is
relevant for the example) etc.

More generally, the idea is to try to write code in examples like we
would in normal kernel code.

> /// use kernel::error::code::EDEADLK;

Some of these should be already available from the prelude, which gets
imported automatically, i.e. please check if you can reduce the number
of imports in the examples.

Thanks!

Cheers,
Miguel