Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY drivers

Trevor Gross
2023-11-19T11:06:36Z

On Sat, Nov 18, 2023 at 9:54 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> In Rust doc [1], `Send` means:
>
> Types that can be transferred across thread boundaries.
>
> but of course, we have more "thread-like" things in kernel, so I think
> "execution context" may be a better term?

The docs are pretty OS-focused here (I intend to update them at some point).

`Sync` means that if you have a >1 pointer/reference to a struct you
can access any of the fields, as allowed by the API, from any of the
references at any time (i.e. switching between any two instructions)
without causing data races. Atomic accesses or mutexes can add this
property to things that do not have it. It really doesn't matter
whether you're going between different user threads, kthreads,
interrupt/preemption contexts, or nothing at all. It's a bit more
intrinsic to the data type and it says how you _could_ use it rather
than how you do use it.

And then `Send` basically means that any pointers in your struct are
either exclusive or point to `Sync` things. Mutexes cannot add this
property to anything that does not have it.

Note that this still never lets you have more than one `&mut`
(`restrict`) reference to a piece of data at once, this mostly relates
to interior mutability (when things are allowed to be changed behind
shared `&` references - such as atomics).

The consumers of these markers are (1) the compiler knowing what can
live in statics, (2) APIs that make things potentially concurrent, and
(3) the compiler automatically marking new structs `Send`/`Sync` if
all member types follow these rules.

When trying to figure this out for C types, the question is just
whether usage of the type follows those rules. Or at least whether it
follows them whenever Rust has access to it.

---

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> +pub struct Registration {
> + drivers: Pin<&'static mut [DriverVTable]>,
> +}
>
> [...]
>
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Send for Registration {}
>
> +// SAFETY: `Registration` does not expose any of its state across threads.
> +unsafe impl Sync for Registration {}

I don't think the impl here actually makes sense. `Registration` is a
buffer of references to `DriverVTable`. That type isn't marked Sync so
by the above rules, its references should not be either.

Tomo, does this need to be Sync at all? Probably easiest to drop the
impls if not, otherwise I think it is more correct to move them to
`DriverVTable`. You may have had this before, I'm not sure if
discussion made you change it at some point...

---

> [1]: https://doc.rust-lang.org/core/marker/trait.Send.html
>
> Regards,
> Boqun

Sorry Boqun, the lengthy explanation is just for context and not aimed
at you in particular :)

- Trevor