| Message ID | 2c923344-04c8-cdbd-ac06-047027f7a23a@redhat.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote: > On 8/26/2016 9:35 AM, Doug Ledford wrote: > > On 8/26/2016 12:49 AM, Christophe JAILLET wrote: > >> The 2nd parameter of 'find_first_bit' is the number of bits to search. > >> In this case, we are passing 'sizeof(unsigned long)' which is likely to > >> be 4 or 8. > > > > If the size can be 4 or 8, then using 64 universally is not correct. > > Why not use sizeof() * 8 (or << 3)? > > Better yet, why not put this patch in the kernel first: > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index d96a6118d26a..a8838c87668e 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -52,6 +52,8 @@ > > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + > __must_be_array(arr)) > > +#define bitsizeof(x) (sizeof((x)) << 3) > + > #define u64_to_user_ptr(x) ( \ > { \ > typecheck(u64, x); \ > > then start going around replacing all these hard coded numbers with the > use of bitsizeof(). It can be applied not just to the find_first*bit() > routines, but to a bunch of other routines too. Just look at > include/linux/bitmap.h and any that have nbits as an argument are > candidates. There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for the similar code pieces. > > > -- > Doug Ledford <dledford@redhat.com> > GPG Key ID: 0E572FDD >
On 8/26/2016 3:29 PM, Leon Romanovsky wrote: > On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote: >> On 8/26/2016 9:35 AM, Doug Ledford wrote: >>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote: >>>> The 2nd parameter of 'find_first_bit' is the number of bits to search. >>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to >>>> be 4 or 8. >>> >>> If the size can be 4 or 8, then using 64 universally is not correct. >>> Why not use sizeof() * 8 (or << 3)? >> >> Better yet, why not put this patch in the kernel first: >> >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >> index d96a6118d26a..a8838c87668e 100644 >> --- a/include/linux/kernel.h >> +++ b/include/linux/kernel.h >> @@ -52,6 +52,8 @@ >> >> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + >> __must_be_array(arr)) >> >> +#define bitsizeof(x) (sizeof((x)) << 3) >> + >> #define u64_to_user_ptr(x) ( \ >> { \ >> typecheck(u64, x); \ >> >> then start going around replacing all these hard coded numbers with the >> use of bitsizeof(). It can be applied not just to the find_first*bit() >> routines, but to a bunch of other routines too. Just look at >> include/linux/bitmap.h and any that have nbits as an argument are >> candidates. > > There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for > the similar code pieces. BITS_PER_LONG only works if your bitfield is a single long. It doesn't work for other bitfields. What I posted above will work for anything.
On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote: > On 8/26/2016 3:29 PM, Leon Romanovsky wrote: > > On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote: > >> On 8/26/2016 9:35 AM, Doug Ledford wrote: > >>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote: > >>>> The 2nd parameter of 'find_first_bit' is the number of bits to search. > >>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to > >>>> be 4 or 8. > >>> > >>> If the size can be 4 or 8, then using 64 universally is not correct. > >>> Why not use sizeof() * 8 (or << 3)? > >> > >> Better yet, why not put this patch in the kernel first: > >> > >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h > >> index d96a6118d26a..a8838c87668e 100644 > >> --- a/include/linux/kernel.h > >> +++ b/include/linux/kernel.h > >> @@ -52,6 +52,8 @@ > >> > >> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + > >> __must_be_array(arr)) > >> > >> +#define bitsizeof(x) (sizeof((x)) << 3) > >> + > >> #define u64_to_user_ptr(x) ( \ > >> { \ > >> typecheck(u64, x); \ > >> > >> then start going around replacing all these hard coded numbers with the > >> use of bitsizeof(). It can be applied not just to the find_first*bit() > >> routines, but to a bunch of other routines too. Just look at > >> include/linux/bitmap.h and any that have nbits as an argument are > >> candidates. > > > > There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for > > the similar code pieces. > > BITS_PER_LONG only works if your bitfield is a single long. It doesn't > work for other bitfields. What I posted above will work for anything. Yes, the question to ask if it is really needed. > > > -- > Doug Ledford <dledford@redhat.com> > GPG Key ID: 0E572FDD >
On 8/28/2016 2:06 AM, Leon Romanovsky wrote: > On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote: >> On 8/26/2016 3:29 PM, Leon Romanovsky wrote: >>> On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote: >>>> On 8/26/2016 9:35 AM, Doug Ledford wrote: >>>>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote: >>>>>> The 2nd parameter of 'find_first_bit' is the number of bits to search. >>>>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to >>>>>> be 4 or 8. >>>>> >>>>> If the size can be 4 or 8, then using 64 universally is not correct. >>>>> Why not use sizeof() * 8 (or << 3)? >>>> >>>> Better yet, why not put this patch in the kernel first: >>>> >>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >>>> index d96a6118d26a..a8838c87668e 100644 >>>> --- a/include/linux/kernel.h >>>> +++ b/include/linux/kernel.h >>>> @@ -52,6 +52,8 @@ >>>> >>>> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + >>>> __must_be_array(arr)) >>>> >>>> +#define bitsizeof(x) (sizeof((x)) << 3) >>>> + >>>> #define u64_to_user_ptr(x) ( \ >>>> { \ >>>> typecheck(u64, x); \ >>>> >>>> then start going around replacing all these hard coded numbers with the >>>> use of bitsizeof(). It can be applied not just to the find_first*bit() >>>> routines, but to a bunch of other routines too. Just look at >>>> include/linux/bitmap.h and any that have nbits as an argument are >>>> candidates. >>> >>> There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for >>> the similar code pieces. >> >> BITS_PER_LONG only works if your bitfield is a single long. It doesn't >> work for other bitfields. What I posted above will work for anything. > > Yes, the question to ask if it is really needed. A quick review of the usage of find_first_bit in the kernel shows that the majority of uses that use large, complex bit arrays (things larger than a single long, where bitsizeof(complex_object) might be helpful), also tend to have limits to their bitmap sizes that do not directly equal the size of the bitmap. For example, the bitmap for an md raid array is allocated as a chunk of memory, where the chunk of memory is larger than the bitmap size as a general rule, so bitsizeof(chunk_of_memory) would run off the end of the real bitmap. Likewise for a number of other complex bitmaps (block layer tag in use bitmap for instance). So, is it useful? I think so. But it's not needed for original patch in this thread.
On Fri, Sep 02, 2016 at 10:39:11AM -0400, Doug Ledford wrote: > On 8/28/2016 2:06 AM, Leon Romanovsky wrote: > > On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote: > >> On 8/26/2016 3:29 PM, Leon Romanovsky wrote: > >>> On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote: > >>>> On 8/26/2016 9:35 AM, Doug Ledford wrote: > >>>>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote: > >>>>>> The 2nd parameter of 'find_first_bit' is the number of bits to search. > >>>>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to > >>>>>> be 4 or 8. > >>>>> > >>>>> If the size can be 4 or 8, then using 64 universally is not correct. > >>>>> Why not use sizeof() * 8 (or << 3)? > >>>> > >>>> Better yet, why not put this patch in the kernel first: > >>>> > >>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h > >>>> index d96a6118d26a..a8838c87668e 100644 > >>>> --- a/include/linux/kernel.h > >>>> +++ b/include/linux/kernel.h > >>>> @@ -52,6 +52,8 @@ > >>>> > >>>> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + > >>>> __must_be_array(arr)) > >>>> > >>>> +#define bitsizeof(x) (sizeof((x)) << 3) > >>>> + > >>>> #define u64_to_user_ptr(x) ( \ > >>>> { \ > >>>> typecheck(u64, x); \ > >>>> > >>>> then start going around replacing all these hard coded numbers with the > >>>> use of bitsizeof(). It can be applied not just to the find_first*bit() > >>>> routines, but to a bunch of other routines too. Just look at > >>>> include/linux/bitmap.h and any that have nbits as an argument are > >>>> candidates. > >>> > >>> There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for > >>> the similar code pieces. > >> > >> BITS_PER_LONG only works if your bitfield is a single long. It doesn't > >> work for other bitfields. What I posted above will work for anything. > > > > Yes, the question to ask if it is really needed. > > A quick review of the usage of find_first_bit in the kernel shows that > the majority of uses that use large, complex bit arrays (things larger > than a single long, where bitsizeof(complex_object) might be helpful), > also tend to have limits to their bitmap sizes that do not directly > equal the size of the bitmap. For example, the bitmap for an md raid > array is allocated as a chunk of memory, where the chunk of memory is > larger than the bitmap size as a general rule, so > bitsizeof(chunk_of_memory) would run off the end of the real bitmap. > Likewise for a number of other complex bitmaps (block layer tag in use > bitmap for instance). > > So, is it useful? I think so. But it's not needed for original patch > in this thread. Honestly, I wasn't convinced at all, but you are right, it is theoretical discussion. Thanks > > > -- > Doug Ledford <dledford@redhat.com> > GPG Key ID: 0E572FDD >
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index d96a6118d26a..a8838c87668e 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -52,6 +52,8 @@ #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) +#define bitsizeof(x) (sizeof((x)) << 3) + #define u64_to_user_ptr(x) ( \ { \