Re: [PATCH v9 3/5] rust: add bitmap API.

[Boqun Feng <boqun.feng@gmail.com>]

On Thu, May 29, 2025 at 07:42:09PM +0000, Pekka Ristola wrote:
[...]
> > +    }
> > +
> > +    /// Copy `src` into this [`Bitmap`] and set any remaining bits to zero.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
> > +    /// use kernel::bitmap::Bitmap;
> > +    ///
> > +    /// let mut long_bitmap = Bitmap::new(256, GFP_KERNEL)?;
> > +    //
> > +    /// assert_eq!(None, long_bitmap.last_bit());
> > +    //
> > +    /// let mut short_bitmap = Bitmap::new(16, GFP_KERNEL)?;
> > +    //
> > +    /// short_bitmap.set_bit(7);
> > +    /// long_bitmap.copy_and_extend(&short_bitmap);
> > +    /// assert_eq!(Some(7), long_bitmap.last_bit());
> > +    ///
> > +    /// # Ok::<(), AllocError>(())
> > +    /// ```
> > +    #[inline]
> > +    pub fn copy_and_extend(&mut self, src: &Bitmap) {
> > +        let len = core::cmp::min(src.nbits, self.len());
> > +        // SAFETY: access to `self` and `src` is within bounds.
> > +        unsafe {
> > +            bindings::bitmap_copy_and_extend(
> > +                self.as_mut_ptr(),
> > +                src.as_ptr(),
> > +                len as u32,
> > +                self.len() as u32,
> > +            )
> 
> Would this cause a data race if `src` is concurrently (atomically)
> modified? The C function seems to use a plain `memcpy` which is not atomic.
> 

We need some better documentation on the effect of kernel C's
`memcpy()`-like functions regarding data races, but in general a kernel
C's `memcpy` can be treated as a volatile one (or per-byte atomic), so
it won't cause data race.

> > +        };
> > +    }
> > +
> > +    /// Finds last set bit.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
> > +    /// use kernel::bitmap::Bitmap;
> > +    ///
> > +    /// let bitmap = Bitmap::new(64, GFP_KERNEL)?;
> > +    ///
> > +    /// match bitmap.last_bit() {
> > +    ///     Some(idx) => {
> > +    ///         pr_info!("The last bit has index {idx}.\n");
> > +    ///     }
> > +    ///     None => {
> > +    ///         pr_info!("All bits in this bitmap are 0.\n");
> > +    ///     }
> > +    /// }
> > +    /// # Ok::<(), AllocError>(())
> > +    /// ```
> > +    #[inline]
> > +    pub fn last_bit(&self) -> Option<usize> {
> > +        // SAFETY: `_find_next_bit` access is within bounds due to invariant.
> > +        let index = unsafe { bindings::_find_last_bit(self.as_ptr(), self.len()) };
> 
> The C function uses non-atomic reads, so this might cause data races too.
> 

Similar here.

Regards,
Boqun

> > +        if index >= self.len() {
> > +            None
> > +        } else {
> > +            Some(index)
> > +        }
> > +    }
> 
> Pekka
>