Re: [PATCH v2] rust: add bindings and API for bitmap.h and bitops.h.

[Yury Norov]

On Mon, Mar 03, 2025 at 02:01:44PM +0100, Alice Ryhl wrote:
> > > +void rust_helper_bitmap_copy(unsigned long *dst, const unsigned long *src, unsigned int nbits)
> > > +{
> > > +       bitmap_copy(dst, src, nbits);
> > > +}
> >
> > I was about to say that this could just be a memcpy, but ...
> >
> > > +    /// Copy up to `nbits` bits from this bitmap into another.
> > > +    ///
> > > +    /// # Panics
> > > +    ///
> > > +    /// Panics if `nbits` is too large for this bitmap or destination.
> > > +    #[inline]
> > > +    pub fn bitmap_copy(&self, dst: &mut Bitmap, nbits: usize) {
> > > +        if self.nbits < nbits {
> > > +            panic_not_in_bounds_le("nbits", self.nbits, nbits);
> > > +        }
> > > +        if dst.nbits < nbits {
> > > +            panic_not_in_bounds_le("nbits", dst.nbits, nbits);
> > > +        }
> > > +        // SAFETY: nbits == 0 is supported and access to `self` and `dst` is within bounds.
> > > +        unsafe { bindings::bitmap_copy(dst.as_mut_ptr(), self.ptr.as_ptr(), nbits as u32) };
> > > +    }
> >
> > ... then I realized that we're probably not using it correctly. I
> > would expect this to modify the first `nbits` bits in `dst`, leaving
> > any remaining bits unmodified. However, if nbits is not divisible by
> > BITS_PER_LONG it might modify some bits it shouldn't.
> >
> > That said, Binder needs this only in the case where the sizes are
> > equal. Perhaps we could rename this to `copy_from_bitmap` with this
> > signature:
> > fn copy_from_bitmap(&mut self, src: Bitmap)
> 
> Sorry I meant src: &Bitmap here.

Yes, you're right. bitmap_copy() copies the whole bitmap. So if your
Bitmap already has size, you don't need to pass it explicitly.
 
> Also, we could rewrite it to just call memcpy rather than go through
> bitmap_copy. Though that requires us to have a Rust version of
> bitmap_size, which I think it makes sense to avoid using a Rust helper
> for.

No, you couldn't. I don't want rust bindings to diverge from the main
kernel. So the rule is simple: if inline wrapper exists only for the
small_const_nbits() optimization - go ahead and use the outlined
underscored version. If the wrapper does something else - no matter
what - it should be wrapped.

> We could reimplement it by first computing the number of longs and
> then computing the number of bytes
> 
> const fn bitmap_size(nbits: usize) -> usize {
>     nbits.div_ceil(c_ulong::BITS) * size_of::<c_ulong>()
> }
> 
> Thoughts?
> 
> Alice