Re: [PATCH v4 1/5] rust: implement `kernel::sync::Refcount`

[Gary Guo]

On Sun, 22 Jun 2025 23:05:09 +0200
"Benno Lossin" <lossin@xxxxxxxxxx> wrote:

> On Sun Jun 22, 2025 at 2:57 PM CEST, Gary Guo wrote:
> > +impl Refcount {
> > +    /// Construct a new [`Refcount`] from an initial value.
> > +    #[inline]
> > +    pub fn new(value: i32) -> Self {  
> 
> Should we really allow users to set a negative value from the get-go?
> Here a `u31` might come in real handy...
> 
> > +        // SAFETY: There are no safety requirements for this FFI call.
> > +        Self(Opaque::new(unsafe { bindings::REFCOUNT_INIT(value) }))
> > +    }
> > +
> > +    #[inline]
> > +    fn as_ptr(&self) -> *mut bindings::refcount_t {
> > +        self.0.get()
> > +    }
> > +
> > +    /// Set a refcount's value.
> > +    #[inline]
> > +    pub fn set(&self, value: i32) {  
> 
> Same here. We should of course provide a `saturate` function, but I
> don't see a reason to set it to another negative value.

Well, it's unlikely the caller would want to init/set the value to the
saturated range, but given that refcount is effectively a thin wrapper
of atomics, user can always obtain the atomics and modify the value.

Also, I did a quick grep of the C refcount API users:
* crypto/algapi.c uses -1 as a special value, and has invocation of
  refcount_set(..., -1)
* lib/stackdepot.c has a invocation to set the refcount into saturated
  range directly.
* There're a few cases where runtime values are set, so `build_assert`
  will not work for these use cases.

Ultimately I think it should be fine to expose `i32` to the user of
this API. Use of `Refcount` to manage resources typically require
reasoning on the user side, so unintended error would be caught that
way.

> 
> ---
> Cheers,
> Benno
> 
> > +        // SAFETY: `self.as_ptr()` is valid.
> > +        unsafe { bindings::refcount_set(self.as_ptr(), value) }
> > +    }  

Best,
Gary