| Message ID | 20220222163144.1782447-5-kbusch@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 64-bit data integrity field support | expand |
On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > Recent data integrity field enhancements allow 48-bit reference tags. > Introduce a helper macro since this will be a repeated operation. [] > diff --git a/include/linux/kernel.h b/include/linux/kernel.h [] > @@ -63,6 +63,12 @@ > } \ > ) > > +/** > + * lower_48_bits - return bits 0-47 of a number > + * @n: the number we're accessing > + */ > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) why not make this a static inline function? And visually, it's difficult to quickly count a repeated character to 12. Perhaps: static inline u64 lower_48_bits(u64 val) { return val & GENMASK_ULL(47, 0); }
On 2/22/22 08:31, Keith Busch wrote: > Recent data integrity field enhancements allow 48-bit reference tags. > Introduce a helper macro since this will be a repeated operation. > > Suggested-by: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > Recent data integrity field enhancements allow 48-bit reference tags. > > Introduce a helper macro since this will be a repeated operation. > [] > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > [] > > @@ -63,6 +63,12 @@ > > } \ > > ) > > > > +/** > > + * lower_48_bits - return bits 0-47 of a number > > + * @n: the number we're accessing > > + */ > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > why not make this a static inline function? Agreed. > And visually, it's difficult to quickly count a repeated character to 12. > > Perhaps: > > static inline u64 lower_48_bits(u64 val) > { > return val & GENMASK_ULL(47, 0); > } For anyone who has a minimum knowledge of C and hardware your version is an obsfucated clusterfuck, while the version Keith wrote is trivial to read.
On Tue, Feb 22, 2022 at 05:50:45PM +0100, Christoph Hellwig wrote: > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > > +/** > > > + * lower_48_bits - return bits 0-47 of a number > > > + * @n: the number we're accessing > > > + */ > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > > > why not make this a static inline function? > > Agreed. Sure, that sounds good to me. I only did it this way to match the existing local convention, but I personally prefer the inline function too.
On Tue, 2022-02-22 at 17:50 +0100, Christoph Hellwig wrote: > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > > Recent data integrity field enhancements allow 48-bit reference tags. > > > Introduce a helper macro since this will be a repeated operation. > > [] > > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > > [] > > > @@ -63,6 +63,12 @@ > > > } \ > > > ) > > > > > > +/** > > > + * lower_48_bits - return bits 0-47 of a number > > > + * @n: the number we're accessing > > > + */ > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > > > why not make this a static inline function? > > Agreed. > > > And visually, it's difficult to quickly count a repeated character to 12. > > > > Perhaps: > > > > static inline u64 lower_48_bits(u64 val) > > { > > return val & GENMASK_ULL(47, 0); > > } > > For anyone who has a minimum knowledge of C and hardware your version > is an obsfucated clusterfuck, while the version Keith wrote is > trivial to read. Don't think so. I've dealt with hardware and have more than once seen defects introduced by firmware developers that can't count. be quick, which one is it: 0xfffffffffffULL or 0xffffffffffffULL or 0xfffffffffffffULL or 0xffffffffffffffULL
From: Christoph Hellwig > Sent: 22 February 2022 16:51 > > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > > Recent data integrity field enhancements allow 48-bit reference tags. > > > Introduce a helper macro since this will be a repeated operation. > > [] > > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > > [] > > > @@ -63,6 +63,12 @@ > > > } \ > > > ) > > > > > > +/** > > > + * lower_48_bits - return bits 0-47 of a number > > > + * @n: the number we're accessing > > > + */ > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > > > why not make this a static inline function? > > Agreed. > > > And visually, it's difficult to quickly count a repeated character to 12. > > > > Perhaps: > > > > static inline u64 lower_48_bits(u64 val) > > { > > return val & GENMASK_ULL(47, 0); > > } > > For anyone who has a minimum knowledge of C and hardware your version > is an obsfucated clusterfuck, while the version Keith wrote is trivial > to read. I'd use the explicit: val & ((1ull << 48) - 1) I think it is even fewer characters. David. - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 2/22/22 08:50, Christoph Hellwig wrote: > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: >> On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: >>> Recent data integrity field enhancements allow 48-bit reference tags. >>> Introduce a helper macro since this will be a repeated operation. >> [] >>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >> [] >>> @@ -63,6 +63,12 @@ >>> } \ >>> ) >>> >>> +/** >>> + * lower_48_bits - return bits 0-47 of a number >>> + * @n: the number we're accessing >>> + */ >>> +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) >> >> why not make this a static inline function? > > Agreed. > All the bit maskd macros in the same file needs to be converted into static inline to have the right data type, however that needs to be done once this series is in, since clearly objective of this series is different than cleanup of include/linux/kernel.h bit macros. -ck
On Tue, 2022-02-22 at 08:56 -0800, Keith Busch wrote: > On Tue, Feb 22, 2022 at 05:50:45PM +0100, Christoph Hellwig wrote: > > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > > > +/ * > > > > + * lower_48_bits - return bits 0-47 of a number > > > > + * @n: the number we're accessing > > > > + */ > > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > > > > > why not make this a static inline function? > > > > Agreed. > > Sure, that sounds good to me. I only did it this way to match the > existing local convention, but I personally prefer the inline function > too. The existing convention is used there to allow the compiler to avoid warnings and unnecessary conversions of a u32 to a u64 when shifting by 32 or more bits. If it's possible to be used with an architecture dependent typedef like dma_addr_t, then perhaps it's reasonable to do something like: #define lower_48_bits(val) \ ({ \ typeof(val) high = lower_16_bits(upper_32_bits(val)); \ typeof(val) low = lower_32_bits(val); \ \ (high << 16 << 16) | low; \ }) and have the compiler have the return value be an appropriate type.
From: Joe Perches > Sent: 22 February 2022 18:43 > > On Tue, 2022-02-22 at 08:56 -0800, Keith Busch wrote: > > On Tue, Feb 22, 2022 at 05:50:45PM +0100, Christoph Hellwig wrote: > > > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > > > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > > > > +/ * > > > > > + * lower_48_bits - return bits 0-47 of a number > > > > > + * @n: the number we're accessing > > > > > + */ > > > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > > > > > > > why not make this a static inline function? > > > > > > Agreed. > > > > Sure, that sounds good to me. I only did it this way to match the > > existing local convention, but I personally prefer the inline function > > too. > > The existing convention is used there to allow the compiler to > avoid warnings and unnecessary conversions of a u32 to a u64 when > shifting by 32 or more bits. > > If it's possible to be used with an architecture dependent typedef > like dma_addr_t, then perhaps it's reasonable to do something like: > > #define lower_48_bits(val) \ > ({ \ > typeof(val) high = lower_16_bits(upper_32_bits(val)); \ > typeof(val) low = lower_32_bits(val); \ > \ > (high << 16 << 16) | low; \ > }) > > and have the compiler have the return value be an appropriate type. The compiler could make a real pigs breakfast of that. For lower_46_bits() an integer promotion to u64 does no harm. But for some other cases you get in a right mess when values that should be unsigned get sign extended. Although I think: (val) & (((typeof(val))1 << 48) - 1) avoids any promotion if anyone tries lower_48_bits(int_var). (It is even likely to be a compile error.) Oh, did you look for GENMASK([^,]*,[ 0]*) ? I'd only use something GENMASK() for bit ranges. Even then it is often easier to just write the value in hex. I think the only time I've written anything like that recently (last 30 years) was for some hardware registers when the documentation user 'bit 1' for the most significant bit. It's rather like I just know that (x & (x - 1)) checks for 1 bit being set. I have to lookup is_power_of_2() to see what it does. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, 2022-02-22 at 20:09 +0000, David Laight wrote: > From: Joe Perches > > Sent: 22 February 2022 18:43 > > > > On Tue, 2022-02-22 at 08:56 -0800, Keith Busch wrote: > > > On Tue, Feb 22, 2022 at 05:50:45PM +0100, Christoph Hellwig wrote: > > > > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote: > > > > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote: > > > > > > +/ * > > > > > > + * lower_48_bits - return bits 0-47 of a number > > > > > > + * @n: the number we're accessing > > > > > > + */ > > > > > > +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) > > > > > > > > > > why not make this a static inline function? > > > > > > > > Agreed. > > > > > > Sure, that sounds good to me. I only did it this way to match the > > > existing local convention, but I personally prefer the inline function > > > too. > > > > The existing convention is used there to allow the compiler to > > avoid warnings and unnecessary conversions of a u32 to a u64 when > > shifting by 32 or more bits. > > > > If it's possible to be used with an architecture dependent typedef > > like dma_addr_t, then perhaps it's reasonable to do something like: > > > > #define lower_48_bits(val) \ > > ({ \ > > typeof(val) high = lower_16_bits(upper_32_bits(val)); \ > > typeof(val) low = lower_32_bits(val); \ > > \ > > (high << 16 << 16) | low; \ > > }) > > > > and have the compiler have the return value be an appropriate type. > > The compiler could make a real pigs breakfast of that. Both gcc and clang optimize it just fine. btw: to return the same type the last line should be: (typeof(val))((high << 16 << 16) | low); otherwise the return is sizeof(int) if typeof(val) is not u64 > Oh, did you look for GENMASK([^,]*,[ 0]*) ? No, why? I did look for 47, 0 though. But it's pretty common really. $ git grep -P 'GENMASK(?:_ULL)?\s*\(\s*\d+\s*,\s*0\s*\)' | wc -l 6233 > I'd only use something GENMASK() for bit ranges. > Even then it is often easier to just write the value in hex. Mostly it's the count of the repeated f that's difficult to quickly verify visually. > I think the only time I've written anything like that recently > (last 30 years) was for some hardware registers when the documentation > user 'bit 1' for the most significant bit. Luckily the hardware I've had to deal with never did that. Not even the least significant bit too.
On Tue, Feb 22, 2022 at 12:31:30PM -0800, Joe Perches wrote: > > I'd only use something GENMASK() for bit ranges. > > Even then it is often easier to just write the value in hex. > > Mostly it's the count of the repeated f that's difficult to > quickly verify visually. I admit I made this counting mistake in earlier versions of this series. I think the earlier suggested "(1ull << 48) - 1" style in an inline function seems good, and it doesn't appear to cause compiler concerns.
On Tue, 2022-02-22 at 13:12 -0800, Keith Busch wrote: > On Tue, Feb 22, 2022 at 12:31:30PM -0800, Joe Perches wrote: > > > I'd only use something GENMASK() for bit ranges. > > > Even then it is often easier to just write the value in hex. > > > > Mostly it's the count of the repeated f that's difficult to > > quickly verify visually. > > I admit I made this counting mistake in earlier versions of this series. It's simply hard for humans to do... https://en.wikipedia.org/wiki/Subitizing
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 33f47a996513..c1fa9fc2b5cd 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -63,6 +63,12 @@ } \ ) +/** + * lower_48_bits - return bits 0-47 of a number + * @n: the number we're accessing + */ +#define lower_48_bits(n) ((u64)((n) & 0xffffffffffffull)) + /** * upper_32_bits - return bits 32-63 of a number * @n: the number we're accessing
Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation. Suggested-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Keith Busch <kbusch@kernel.org> --- include/linux/kernel.h | 6 ++++++ 1 file changed, 6 insertions(+)