Hacker News Reader

> I don't know what type `data`

Most Rust is written with the implicit assumption that it will be read in an IDE that displays types, so explicit type annotations are usually kept to a minimum. Doesn't always translate well outside of the IDE, so I wouldn't be surprised if the consensus for kernel code shifts to more annotations.

> When I look at the kernel's spinlock type [...] that doesn't really help me determine why whatever this `gen` [variable?] is needs to be passed to `spinlock_init`

The rust version of the documentation makes an attempt at explaining it, though honestly it could be a lot clearer https://rust-for-linux.github.io/docs/kernel/sync/struct.Spi...

> And what's with the two `let data`.

That's pretty ideomatic in rust, probably borrowed from functional languages. The new definition hides the previous one, and the implication behind reusing the name is that it's fundamentally the same data, just with a different type, or different mutability (here it was converted from mutable DeviceData to an immutable Ref with the same DeviceData).

That clears some things. Thanks.

This is probably the biggest issue with Rust in the kernel; because it needs to deal with a lot of existing kernel interfaces and abstractions, you get code which is neither very idiomatic Rust, nor is terribly recognizable to someone who knows the kernel abstractions fairly well.

Now, some of that is just a matter of learning curve; once you have learned the Rust-in-Linux abstractions the above will probably be somewhat more readable.

Coming from someone who's more familiar with Rust, and less so with the kernel, here are a few notes.

* It might not be the best idea to post the example drivers with inferred types, for this familiarization reason. While there are a few types (such as those involving closures) that you can't write and can only use via type inference, or others that are too large to want to write out, these types should be fairly easy to write and it would improve readability during the familiarization process

* A lot of these things that you were having trouble with look like they are basically just dereferencing and accessing some data contained within the spinlock, and casting what is only guaranteed by the API to be an immutable (shared) reference into a mutable (unique) reference in an `unsafe` block. This is a place where trying to use simple Rust wrappers over core kernel structures which weren't designed for Rust makes things a bit more difficult to read, as you need to do a bit of an unidiomatic dance to tell the compiler "yes, the language and type don't gurantee uniqueness, but I promise this reference is unique and I won't misuse it."

* One of the small little humps to get over when learning Rust is to learn that "mut" actually means "unique", and unmarked (immutable) references means "potentially shared." But since you do need to sometimes write to objects that are potentially shared, it just means that you need to either guarantee yourself or use some kind of synchronization mechanism to ensure that at any given moment of time that data is only mutable in one place at a time.

So, I don't know the details of these APIs myself, but when I look at this here's what I see

* we're constructing some device data object protected by a spinlock

* we're providing some brief justification for why this particular usage is safe, since the API and compiler can't guarantee that it's safe

* we're extracting a mutable reference from it, for which we need to uphold the single exclusive access principle without assistance from the compiler

* we're wrapping that in a new reference to device data type which presumably provides a more idiomatic interface than what the raw primitives provide

I'm guessing that the following code which actually uses this Ref::<DeviceData> object will then be a bit more readable and require fewer uses of unsafe, as it looks like this code is what is providing that translation between some core kernel primitives which are not terribly idiomatic to use in Rust and a safer, more idiomatic wrapper.

> It might not be the best idea to post the example drivers with inferred types,

a `cargo fmt` option to add inferred types to the source code would have some advantages for that.

Yeah, I hardly understand Rust code, and I did not start learning programming yesterday.

If you want to practice, check out https://github.com/immunant/ioq3/blob/transpiled/quake3-rs/q... or whatever.

That is not idiomatic code: it is an auto-translated code straight from C.

You are judging Rust by looking at what idiomatic C code looks like in Rust.

No no no, I know that it is not idiomatic code, it says in the URL: "transpiled". I am just telling him to practice using it. I was not judging Rust at all.

That said, idiomatic Rust is difficult for me to read/understand as well. I do not expect device drivers in Rust to be readable (by me), and if they have lots of unsafe blocks then we are back to square one I would say.

how would the idiomatic code for that example look like? (seriously)

Here is the refactored code: https://github.com/immunant/ioq3/blob/refactored/quake3-rs/q...

I wouldn't call that code idiomatic either: it is cleaned up from the automatic translation, but it still a far cry from what Rust written by hand would look like

  pub static mut g_color_table: [crate::src::qcommon::q_shared::vec4_t; 8] = [
      [0f32, 0f32, 0f32, 1f32],
      [1f32, 0f32, 0f32, 1f32],
      [0f32, 1f32, 0f32, 1f32],
      [1f32, 1f32, 0f32, 1f32],
      [0f32, 0f32, 1f32, 1f32],
      [0f32, 1f32, 1f32, 1f32],
      [1f32, 0f32, 1f32, 1f32],
      [1f32, 1f32, 1f32, 1f32],
  ];

would maybe look like

  pub static mut g_color_table: [[f32; 4]; 8] = [
      [0, 0, 0, 1],
      [1, 0, 0, 1],
      [0, 1, 0, 1],
      [1, 1, 0, 1],
      [0, 0, 1, 1],
      [0, 1, 1, 1],
      [1, 0, 1, 1],
      [1, 1, 1, 1],
  ];

but it still looks weird to me (a static mut would be uncommon in a design, and not having a new-type to represent these would also be uncommon).

If you are interested in looking what Rust written from the ground up in the same domain looks like, you can maybe look at Bevy[1].

The biggest source of "noise" in Rust is explicit type conversions, FP constructs and longer types (`.into()`/`.as_mut_ref()`/`.map()`/`Result<Option<T>, E>`).

[1]: https://github.com/bevyengine/bevy/blob/main/crates/bevy_ren...

  BufferInfo {
    buffer_usage: BufferUsage::VERTEX,
    ..Default::default()
  },

What is "..Default" here?

`Default` is a trait[1] that can be implemented on any type that can be conceived to have a default value. For example, Option has something like:

    impl<T> Default for Option<T> {
        fn default() -> Option<T> {
            None
        }
    }

If every field of a struct impl Default, then you can you can use the `#[derive(Default)]` annotation to auto-fill that impl for that struct, otherwise you have to write it by hand.

The .. is the spread operator, and it takes every field of the value on the right and applies it to every field of the struct constructor on the left. This only works for the same type (it's not only looking at the field names).

`Default::default()` is a qualified call on the trait equivalent to `<_ as Default>::default()`, where `_` is inferred from context to be `BufferInfo`.

What that code is doing is saying "build a default BufferInfo with field buffer_usage set to VERTEX".

The spread operator has a limitation you might have intuited from the description that it can't work for types where some field(s) should always be set but not others. To get around that you can use the builder pattern

    BufferInfo::with_buffer_usage(BufferUsage::VERTEX).build()

and there's a pre-RFC in flight to become an RFC to add support for

    struct Foo {
        field: i32 = 42,
    }
    
    let foo = Foo { .. };

[1]: https://doc.rust-lang.org/std/default/trait.Default.html

Normally in Rust, to initialize a struct, you must specify a value for every field of it, since null and uninitialized values aren't allowed. To make it a little more convenient, there's a trait called Default which can be implemented for your struct to provide an instance with every field set to a value that you think should be the default. And the .. syntax allows you to copy values for the rest of the fields of a struct from another struct instance. So calling that with Default::default() gets an instance with the default values and uses those values to fill all of the rest of the fields of the struct. Thus if you have a big struct where most values are the default most of the time, you can create instances of it with one or two fields set to something different like this without an extra few dozen lines of repetitive boilerplate for every other field of the struct.

So, what we're saying here is in effect, "and all the other fields should have their default value".

Specifically, when you initialise a structure like this, the .. is "struct update syntax" and is followed by an instance of that structure from which remaining fields will be copied. Default is a Rust trait (similar to an interface if you've never seen Rust traits before) and it has the method default() which gives you a default of whatever type.

Rust has type inference, if you don't specify a type, but there is only one possibility that could work, the type inference might (and in this case does) see that and you needn't bother laboriously writing down the type. This can only happen where there was no possible ambiguity. In this case, the only type you can use for struct update syntax is the structure type you're updating of course, BufferInfo, and so Default::default() must mean the default BufferInfo.

If you found the definition of BufferInfo it probably says #[derive(Default)] which means, using a macro just implement Default for this structure in the obvious way, although it might actually have an explicit Default written out instead.

Bear in mind this is the source code of a game/game engine. They are notorious for their use of hacks, specially to squeeze out performance.

In all fairness, the Linux kernel uses various techniques here and there for optimization, too.

Where do you see hacks being used in, say, the refactored code? If you meant generally, then sure, they do.

Oh, I thought that the comment about not looking as idiomatic code was referring to the kernel::new_device_... sample in a parent/parent HN comment. I was more interested in knowing what was so unidiomatic about that.

The posted code isn't terrible, but it is a bit denser and harder to read than I'd like. Some small changes (like importing the macros to avoid a full path) makes a bit easier to read

        // SAFETY: This device doesn't support DMA.
        let base = unsafe { IoMem::try_new(res)? };

        let mut data = new_device_data!(
            RegistrationWithIrqChip::new(),
            PL061Resources { base, parent_irq: irq },
            // SAFETY: We call `spinlock_init` below.
            unsafe { SpinLock::new(PL061Data::default()) },
            "PL061::Registrations"
        )?;

        // SAFETY: General part of the data is pinned when `data` is.
        let gen = unsafe { data.as_mut().map_unchecked_mut(|d| &mut **d) };
        spinlock_init!(gen, "PL061::General");

        let data: Ref<DeviceData> = data.into();

I imagine code like this would be the body of a small function that does all this unsafe preparations and gives back a Result<Ref<DeviceData>, Error> once it's done, which would change the end to be

        Ok(data.into())

Thanks, it actually looks better and more understandable.