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
>
