Messages in this thread | | | From | Yury Norov <> | Date | Mon, 13 Jun 2022 12:20:26 -0700 | Subject | Re: [PATCH] bitmap: fix a unproper remap when mpol_rebind_nodemask() |
| |
On Mon, Jun 13, 2022 at 4:31 AM Yi Wang <wang.yi59@zte.com.cn> wrote: > > Consider one situation: > > The app have two vmas which mbind() to node 1 and node3 respectively, > and its cpuset.mems is 0-3, now set its cpuset.mems to 1,3, according > to current bitmap_remap(), we got: > > 1 => 3 > 3 => 3 > > This maybe confused because node 1,3 have already in the new settiing > region but both nodes are binded to the same node 3 now. > > Actually we found the situation on a very old libvirt and qemu, but > this can be easily reproduced in the current kernel, so we try to fix > it. > > A possible fix way is to ignore the bits in @src have already existed > in @new. > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> > --- > lib/bitmap.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/bitmap.c b/lib/bitmap.c > index b18e31ea6e66..b77bf1b3852e 100644 > --- a/lib/bitmap.c > +++ b/lib/bitmap.c > @@ -1006,8 +1006,8 @@ unsigned int bitmap_ord_to_pos(const unsigned long *buf, unsigned int ord, unsig > * @dst point to the same location, then this routine copies @src > * to @dst. > * > - * The positions of unset bits in @old are mapped to themselves > - * (the identify map). > + * The positions of unset bits in @old or bits in @src have already > + * existed in @new are mapped to themselves (the identify map). > * > * Apply the above specified mapping to @src, placing the result in > * @dst, clearing any bits previously set in @dst. > @@ -1033,7 +1033,7 @@ void bitmap_remap(unsigned long *dst, const unsigned long *src, > for_each_set_bit(oldbit, src, nbits) { > int n = bitmap_pos_to_ord(old, oldbit, nbits); > > - if (n < 0 || w == 0) > + if (n < 0 || w == 0 || test_bit(oldbit, new)) > set_bit(oldbit, dst); /* identity map */ > else > set_bit(bitmap_ord_to_pos(new, n % w, nbits), dst); > -- > 2.33.0.rc0.dirty
Regarding the original problem - can you please confirm that it's reproduced on current kernels, show the execution path etc. From what I see on modern kernel, the only user of nodes_remap() is mpol_rebind_nodemask(). Is that the correct path?
Anyways, as per name, bitmap_remap() is intended to change bit positions, and it doesn't look wrong if it does so.
This is not how the function is supposed to work. For example, old: 00111000 new: 00011100
means: old: 00111 000 || \\\||| new: 000 11100
And after this patch it would be: old: 001 11000 || \||||| new: 000 11100
Which is not the same, right?
If mpol_rebind() wants to keep previous relations, then according to the comment: * The positions of unset bits in @old are mapped to themselves * (the identify map).
, you can just clear @old bits that already have good relations you'd like to preserve.
Thanks, Yury
| |