| Message ID | 20190717012902.23958-1-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] bitmap: get last word mask from nr directly | expand |
On Wed, Jul 17, 2019 at 09:29:02AM +0800, Wei Yang wrote: > The value left in nr is the number of bits for the last word, which > could be calculate the last word mask directly. Is it true even if start does not align to BITS_PER_LONG? > > Remove the unnecessary size. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > --- > resend with wider audience > --- > util/bitmap.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/util/bitmap.c b/util/bitmap.c > index cb618c65a5..5aa60b8717 100644 > --- a/util/bitmap.c > +++ b/util/bitmap.c > @@ -160,7 +160,6 @@ int slow_bitmap_andnot(unsigned long *dst, const unsigned long *bitmap1, > void bitmap_set(unsigned long *map, long start, long nr) > { > unsigned long *p = map + BIT_WORD(start); > - const long size = start + nr; > int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); > unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); > > @@ -174,7 +173,7 @@ void bitmap_set(unsigned long *map, long start, long nr) > p++; > } > if (nr) { > - mask_to_set &= BITMAP_LAST_WORD_MASK(size); > + mask_to_set &= BITMAP_LAST_WORD_MASK(nr); > *p |= mask_to_set; > } > } > @@ -221,7 +220,6 @@ void bitmap_set_atomic(unsigned long *map, long start, long nr) > void bitmap_clear(unsigned long *map, long start, long nr) > { > unsigned long *p = map + BIT_WORD(start); > - const long size = start + nr; > int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); > unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); > > @@ -235,7 +233,7 @@ void bitmap_clear(unsigned long *map, long start, long nr) > p++; > } > if (nr) { > - mask_to_clear &= BITMAP_LAST_WORD_MASK(size); > + mask_to_clear &= BITMAP_LAST_WORD_MASK(nr); > *p &= ~mask_to_clear; > } > } > -- > 2.17.1 > Regards,
On Wed, Jul 17, 2019 at 09:59:10AM +0800, Peter Xu wrote: >On Wed, Jul 17, 2019 at 09:29:02AM +0800, Wei Yang wrote: >> The value left in nr is the number of bits for the last word, which >> could be calculate the last word mask directly. > >Is it true even if start does not align to BITS_PER_LONG? > Yes. Let me see how to explain this. When you look into the definition of BITMAP_LAST_WORD_MASK, it takes the number of total bits and give the number of bits in last word. While the value matters for the input is the number of last word. This means the following equation stands BITMAP_LAST_WORD_MASK(size) == BITMAP_FIRST_WORD_MASK(size % BITS_PER_LONG) Now let look at the calculation for nr. In each iteration, nr will be truncated to be aligned to BITS_PER_LONG. So when we exit the loop, nr keeps the number of bits in last word. So we can leverage the result, no matter the start is aligned or not. >> >> Remove the unnecessary size. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> --- >> resend with wider audience >> --- >> util/bitmap.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/util/bitmap.c b/util/bitmap.c >> index cb618c65a5..5aa60b8717 100644 >> --- a/util/bitmap.c >> +++ b/util/bitmap.c >> @@ -160,7 +160,6 @@ int slow_bitmap_andnot(unsigned long *dst, const unsigned long *bitmap1, >> void bitmap_set(unsigned long *map, long start, long nr) >> { >> unsigned long *p = map + BIT_WORD(start); >> - const long size = start + nr; >> int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >> unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >> >> @@ -174,7 +173,7 @@ void bitmap_set(unsigned long *map, long start, long nr) >> p++; >> } >> if (nr) { >> - mask_to_set &= BITMAP_LAST_WORD_MASK(size); >> + mask_to_set &= BITMAP_LAST_WORD_MASK(nr); >> *p |= mask_to_set; >> } >> } >> @@ -221,7 +220,6 @@ void bitmap_set_atomic(unsigned long *map, long start, long nr) >> void bitmap_clear(unsigned long *map, long start, long nr) >> { >> unsigned long *p = map + BIT_WORD(start); >> - const long size = start + nr; >> int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >> unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >> >> @@ -235,7 +233,7 @@ void bitmap_clear(unsigned long *map, long start, long nr) >> p++; >> } >> if (nr) { >> - mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >> + mask_to_clear &= BITMAP_LAST_WORD_MASK(nr); >> *p &= ~mask_to_clear; >> } >> } >> -- >> 2.17.1 >> > >Regards, > >-- >Peter Xu
On Wed, Jul 17, 2019 at 10:46:37AM +0800, Wei Yang wrote: > On Wed, Jul 17, 2019 at 09:59:10AM +0800, Peter Xu wrote: > >On Wed, Jul 17, 2019 at 09:29:02AM +0800, Wei Yang wrote: > >> The value left in nr is the number of bits for the last word, which > >> could be calculate the last word mask directly. > > > >Is it true even if start does not align to BITS_PER_LONG? > > > > Yes. Let me see how to explain this. > > When you look into the definition of BITMAP_LAST_WORD_MASK, it takes the > number of total bits and give the number of bits in last word. While the value > matters for the input is the number of last word. This means the following > equation stands > > BITMAP_LAST_WORD_MASK(size) == BITMAP_FIRST_WORD_MASK(size % BITS_PER_LONG) > > Now let look at the calculation for nr. In each iteration, nr will be > truncated to be aligned to BITS_PER_LONG. So when we exit the loop, nr keeps > the number of bits in last word. > > So we can leverage the result, no matter the start is aligned or not. Yes, you are right. Do you have plan to write some unit tests for these functions? :) It'll be tests/test-bitmap.c. IMHO the test cases could be even more helpful to the QEMU project as a whole comparing to this patch to guarantee changes like your patch won't break. At the meantime I think you can also do that to bitmap_set_atomic. Thanks,
On Wed, Jul 17, 2019 at 11:11:55AM +0800, Peter Xu wrote: >On Wed, Jul 17, 2019 at 10:46:37AM +0800, Wei Yang wrote: >> On Wed, Jul 17, 2019 at 09:59:10AM +0800, Peter Xu wrote: >> >On Wed, Jul 17, 2019 at 09:29:02AM +0800, Wei Yang wrote: >> >> The value left in nr is the number of bits for the last word, which >> >> could be calculate the last word mask directly. >> > >> >Is it true even if start does not align to BITS_PER_LONG? >> > >> >> Yes. Let me see how to explain this. >> >> When you look into the definition of BITMAP_LAST_WORD_MASK, it takes the >> number of total bits and give the number of bits in last word. While the value >> matters for the input is the number of last word. This means the following >> equation stands >> >> BITMAP_LAST_WORD_MASK(size) == BITMAP_FIRST_WORD_MASK(size % BITS_PER_LONG) >> >> Now let look at the calculation for nr. In each iteration, nr will be >> truncated to be aligned to BITS_PER_LONG. So when we exit the loop, nr keeps >> the number of bits in last word. >> >> So we can leverage the result, no matter the start is aligned or not. > >Yes, you are right. > >Do you have plan to write some unit tests for these functions? :) > >It'll be tests/test-bitmap.c. IMHO the test cases could be even more >helpful to the QEMU project as a whole comparing to this patch to >guarantee changes like your patch won't break. Let me have a try. :-) > >At the meantime I think you can also do that to bitmap_set_atomic. > >Thanks, > >-- >Peter Xu
diff --git a/util/bitmap.c b/util/bitmap.c index cb618c65a5..5aa60b8717 100644 --- a/util/bitmap.c +++ b/util/bitmap.c @@ -160,7 +160,6 @@ int slow_bitmap_andnot(unsigned long *dst, const unsigned long *bitmap1, void bitmap_set(unsigned long *map, long start, long nr) { unsigned long *p = map + BIT_WORD(start); - const long size = start + nr; int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); @@ -174,7 +173,7 @@ void bitmap_set(unsigned long *map, long start, long nr) p++; } if (nr) { - mask_to_set &= BITMAP_LAST_WORD_MASK(size); + mask_to_set &= BITMAP_LAST_WORD_MASK(nr); *p |= mask_to_set; } } @@ -221,7 +220,6 @@ void bitmap_set_atomic(unsigned long *map, long start, long nr) void bitmap_clear(unsigned long *map, long start, long nr) { unsigned long *p = map + BIT_WORD(start); - const long size = start + nr; int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); @@ -235,7 +233,7 @@ void bitmap_clear(unsigned long *map, long start, long nr) p++; } if (nr) { - mask_to_clear &= BITMAP_LAST_WORD_MASK(size); + mask_to_clear &= BITMAP_LAST_WORD_MASK(nr); *p &= ~mask_to_clear; } }
The value left in nr is the number of bits for the last word, which could be calculate the last word mask directly. Remove the unnecessary size. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- resend with wider audience --- util/bitmap.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)