Re: [PATCH v6 3/4] rust: add bitmap API.

[Yury Norov <yury.norov@gmail.com>]

On Fri, Mar 28, 2025 at 11:36:51AM +0100, Burak Emir wrote:
> On Thu, Mar 27, 2025 at 5:16 PM Burak Emir <bqe@google.com> wrote:
> >
> > +    /// Set bit with index `index`.
> 
> I missed this, will change to /// Set `index` bit,
> 
> > +    ///
> > +    /// # Panics
> > +    ///
> > +    /// Panics if `index` is greater than or equal to `self.nbits`.
> > +    #[inline]
> > +    pub fn set_bit(&mut self, index: usize) {
> > +        assert!(
> > +            index < self.nbits,
> > +            "Bit `index` must be < {}, was {}",
> > +            self.nbits,
> > +            index
> > +        );
> > +        // SAFETY: Bit `index` is within bounds.
> > +        unsafe { bindings::__set_bit(index as u32, self.as_mut_ptr()) };
> > +    }
> > +
> > +    /// Set bit with index `index`, atomically.
> 
> dto, will change to /// Set `index` bit, atomically.
> 
> > +    ///
> > +    /// WARNING: this is a relaxed atomic operation (no implied memory barriers).
> 
> Is this the kind of warning you had in mind?

The __set_bit() in C and set_bit() in rust is a non-atomic function.
Relaxed atomic API has a different meaning. Please add something like
the following on top of 'pub fn set_bit()' implementation:

    /// ATTENTION: Contrary to C, the rust set_bit() method is non-atomic.
    /// This mismatches kernel naming convention and corresponds to the C
    /// function __set_bit(). For atomicity, use the set_bit_atomic() method.
 
> > +    ///
> > +    /// # Panics
> > +    ///
> > +    /// Panics if `index` is greater than or equal to `self.nbits`.
> > +    #[inline]
> > +    pub fn set_bit_atomic(&self, index: usize) {
> > +        assert!(
> > +            index < self.nbits,
> > +            "Bit `index` must be < {}, was {}",
> > +            self.nbits,
> > +            index
> > +        );
> > +        // SAFETY: `index` is within bounds and there cannot be any data races
> > +        // because all non-atomic operations require exclusive access through
> > +        // a &mut reference.
> 
> I have considered marking set_bit_atomic as unsafe, but then come
> around to think that it is actually safe.
> 
> I'd appreciate a review of the reasoning by my fellow Rust-for-Linux folks.
> 
> What must be ensured is absence of data race, e.g. that an atomic op
> does not happen concurrently with a conflicting non-synchronized,
> non-atomic op.
> Do I need to worry about non-atomic accesses in the same thread
> (temporarily reborrowing a &mut to & in the same thread is a
> possibility)?

To me - no. Atomicity only works if everyone follow the same rules.
If someone accessed some data without grabbing a lock on it, and
ended up corrupting the kernel, it's not a problem of spinlock API.

Thanks,
Yury