4 ms·

This is only partially fair. In Rust you would probably have assigned a new object into *buf here instead of overwriting the fields manually. It is good practi

This is only partially fair. In Rust you would probably have assigned a new object into *buf here instead of overwriting the fields manually. It is good practice to do this (if the code is logically an object initialization, it should actually be an object initialization, not a bunch of field assignments), but it's clunky to do so in C because you can't use initializers in assignments.

TonyTrapp 5y ago
The point is: You could have done this in Rust, but you wouldn't have been required to do so, so the exact same logic bug could have emerged. Maybe it would be more Rust-like to write the code like that, but it would have also been possible to write the code like that in C - and since we're talking about the kernel here, even if this code was written in Rust a developer might have written it in the more C-like way for performance reasons.
lanstin 5y ago
People writing C don't re-use allocated objects because it's clunky but to improve performance. The general purpose allocators are almost always much slower than something where you know the pattern of allocations. I've no idea if Rust has a similar issue. I would think that most kernel code, whether C or Rust, would need to handle "allocation fails" case and not depend on language constructs to do allocations, but that's just a guess.
- deredede 5y ago
  I'm not saying you shouldn't reuse allocated objects. I'm talking about building a local object (no dynamic allocation) and assigning it to the pointer at once. This has the same runtime behavior (assuming -O1) as assigning the fields one by one. See https://godbolt.org/z/Wh5KcTaGY https://godbolt.org/z/Wh5KcTaGY for what I'm talking about, the local allocation is easily eliminated by the compiler. The equivalent in C is to create a temporary local variable with an initializer list then write that variable to the pointer.
dzaima 5y ago
you can assign a new object into *buf in C just fine, with "*buf = (struct YourType){.prop1 = a, .prop2 = b}"; it even zero-initializes unset fields! So C and Rust give you precisely the same abilities here. edit: the "struct pipe_buffer" in question[1] has one field that even the updated code doesn't write - "private". Not sure what it's about, but it's there. Not writing unrelated fields like that is probably not much of an issue now, but it certainly can add up on low-power systems. You might also have situations where you'd want to write parts of an object in different parts of the code, for which you'd have to resort to writing fields manually. [1]: https://github.com/torvalds/linux/blob/719fce7539cd3e186598e2aed36325fe892150cf/include/linux/pipe_fs_i.h#L17-L32 https://github.com/torvalds/linux/blob/719fce7539cd3e186598e...
- deredede 5y ago
  Oh I was not aware of this syntax in C, thanks for bringing it up! I still think the pattern is more common and known in Rust but I might be wrong :) Re: your other points, "reusing a pre-allocated struct from a buffer" is basically object initialization, which is different from other times you want to write fields. In general an object initialization construct should be used in those cases, this whole thread being an argument why. Out-of-band fields such as the "private" field are a pain I agree, but they can be separated from an inner struct (in which case the inner struct is the only field that gets assigned for initialization). Taking a step back, the true solution is probably to have a proper constructor... And that can be done in any language, so I'll stand corrected.
  - dzaima 5y ago
    The question of safety is often much more about coding standards than actual language features. For a kernel, you'd be violating Rust's safety features left right and center (or have a slow kernel that just dies on OOM; choose one) and would have to come up with your own standards for preventing coding errors in those, which is what you already have with C. To be clear, some base level of trivially safe code is certainly a nice-to-have. I just don't think the amount that helps for a kernel is that much, and the added boilerplate on more unsafe things might even obscure issues.
- 5y ago

TonyTrapp 5y ago

The point is: You could have done this in Rust, but you wouldn't have been required to do so, so the exact same logic bug could have emerged. Maybe it would be more Rust-like to write the code like that, but it would have also been possible to write the code like that in C - and since we're talking about the kernel here, even if this code was written in Rust a developer might have written it in the more C-like way for performance reasons.

lanstin 5y ago

People writing C don't re-use allocated objects because it's clunky but to improve performance. The general purpose allocators are almost always much slower than something where you know the pattern of allocations. I've no idea if Rust has a similar issue. I would think that most kernel code, whether C or Rust, would need to handle "allocation fails" case and not depend on language constructs to do allocations, but that's just a guess.

deredede 5y ago
I'm not saying you shouldn't reuse allocated objects. I'm talking about building a local object (no dynamic allocation) and assigning it to the pointer at once. This has the same runtime behavior (assuming -O1) as assigning the fields one by one. See https://godbolt.org/z/Wh5KcTaGY https://godbolt.org/z/Wh5KcTaGY for what I'm talking about, the local allocation is easily eliminated by the compiler. The equivalent in C is to create a temporary local variable with an initializer list then write that variable to the pointer.

dzaima 5y ago

you can assign a new object into *buf in C just fine, with "*buf = (struct YourType){.prop1 = a, .prop2 = b}"; it even zero-initializes unset fields! So C and Rust give you precisely the same abilities here. edit: the "struct pipe_buffer" in question[1] has one field that even the updated code doesn't write - "private". Not sure what it's about, but it's there. Not writing unrelated fields like that is probably not much of an issue now, but it certainly can add up on low-power systems. You might also have situations where you'd want to write parts of an object in different parts of the code, for which you'd have to resort to writing fields manually. [1]: https://github.com/torvalds/linux/blob/719fce7539cd3e186598e2aed36325fe892150cf/include/linux/pipe_fs_i.h#L17-L32 https://github.com/torvalds/linux/blob/719fce7539cd3e186598e...

deredede 5y ago
Oh I was not aware of this syntax in C, thanks for bringing it up! I still think the pattern is more common and known in Rust but I might be wrong :) Re: your other points, "reusing a pre-allocated struct from a buffer" is basically object initialization, which is different from other times you want to write fields. In general an object initialization construct should be used in those cases, this whole thread being an argument why. Out-of-band fields such as the "private" field are a pain I agree, but they can be separated from an inner struct (in which case the inner struct is the only field that gets assigned for initialization). Taking a step back, the true solution is probably to have a proper constructor... And that can be done in any language, so I'll stand corrected.
- dzaima 5y ago
  The question of safety is often much more about coding standards than actual language features. For a kernel, you'd be violating Rust's safety features left right and center (or have a slow kernel that just dies on OOM; choose one) and would have to come up with your own standards for preventing coding errors in those, which is what you already have with C. To be clear, some base level of trivially safe code is certainly a nice-to-have. I just don't think the amount that helps for a kernel is that much, and the added boilerplate on more unsafe things might even obscure issues.
5y ago