On Mon, Feb 17, 2025 at 6:35 AM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>
> On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> > `XArray` is an efficient sparse array of pointers. Add a Rust
> > abstraction for this type.
> >
> > This implementation bounds the element type on `ForeignOwnable` and
> > requires explicit locking for all operations. Future work may leverage
> > RCU to enable lockless operation.
> >
> > Inspired-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > Inspired-by: Asahi Lina <lina@xxxxxxxxxxxxx>
> > Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
> > Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx>
> > ---
> > rust/bindings/bindings_helper.h | 6 +
> > rust/helpers/helpers.c | 1 +
> > rust/helpers/xarray.c | 28 ++++
> > rust/kernel/alloc.rs | 5 +
> > rust/kernel/lib.rs | 1 +
> > rust/kernel/xarray.rs | 276 ++++++++++++++++++++++++++++++++++++++++
> > 6 files changed, 317 insertions(+)
> >
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index fc9c9c41cd79..77840413598d 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -39,6 +39,11 @@
> > pub struct Flags(u32);
> >
> > impl Flags {
> > + /// Get a flags value with all bits unset.
> > + pub fn empty() -> Self {
> > + Self(0)
> > + }
>
> No! Zero is not a reasonable default for GFP flags.
This is not a default.
> In fact, I don't know any
> place in the kernel where we would want no reclaim + no IO + no FS without any
> other flags (such as high-priority or kswapd can wake). Especially, because for
> NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
> guards should be used instead.
>
> You also don't seem to use this anywhere anyways.
This was used in an earlier iteration that included support for
reservations. I used this value when fulfilling a reservation because
it was an invariant of the API that no allocation would take place.
> Please also make sure to not bury such changes in unrelated other patches.
Thank you for spotting this errant change. Please consider whether it
serves anyone's purpose to accuse someone of underhanded behavior.
> > +/// The error returned by [`store`](Guard::store).
> > +///
> > +/// Contains the underlying error and the value that was not stored.
> > +pub struct StoreError<T> {
> > + /// The error that occurred.
> > + pub error: Error,
> > + /// The value that was not stored.
> > + pub value: T,
> > +}
> > +
> > +impl<T> From<StoreError<T>> for Error {
> > + fn from(value: StoreError<T>) -> Self {
> > + let StoreError { error, value: _ } = value;
> > + error
>
> Why not just `value.error`?
I prefer the clarity that this results in the value being dropped. Is
there written guidance on this matter?
