[PATCH rdma-core] mlx4: Fix 1<<31 expressions

[PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Jason Gunthorpe]

They produce the value INT32_MIN not 0x80000000. This is sometimes OK
if it is casted appropriately but leaves a subtle trap to the user.

Signed-off-by: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx4/mlx4dv.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/providers/mlx4/mlx4dv.h b/providers/mlx4/mlx4dv.h
index d47d3cd2e1f135..5312a866b6e281 100644
--- a/providers/mlx4/mlx4dv.h
+++ b/providers/mlx4/mlx4dv.h
@@ -288,12 +288,12 @@ enum {
 };
 
 enum {
-	MLX4_WQE_BIND_TYPE_2		= (1<<31),
+	MLX4_WQE_BIND_TYPE_2		= (1UL<<31),
 	MLX4_WQE_BIND_ZERO_BASED	= (1<<30),
 };
 
 enum {
-	MLX4_INLINE_SEG		= 1 << 31,
+	MLX4_INLINE_SEG		= 1UL << 31,
 	MLX4_INLINE_ALIGN	= 64,
 };
 
@@ -304,7 +304,7 @@ enum {
 enum {
 	MLX4_WQE_MW_REMOTE_READ   = 1 << 29,
 	MLX4_WQE_MW_REMOTE_WRITE  = 1 << 30,
-	MLX4_WQE_MW_ATOMIC        = 1 << 31
+	MLX4_WQE_MW_ATOMIC        = 1UL << 31
 };
 
 struct mlx4_wqe_local_inval_seg {
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Bart Van Assche]

On Thu, 2018-02-08 at 16:38 -0700, Jason Gunthorpe wrote:
>  enum {
> -	MLX4_WQE_BIND_TYPE_2		= (1<<31),
> +	MLX4_WQE_BIND_TYPE_2		= (1UL<<31),
>  	MLX4_WQE_BIND_ZERO_BASED	= (1<<30),
>  };

Hello Jason,

A quote from the C11 standard:

"The expression that defines the value of an enumeration constant shall be an
integer constant expression that has a value representable as an int. [ ... ]
Each enumerated type shall be compatible with char, a signed integer type, or
an unsigned integer type. The choice of type is implementation-defined but
shall be capable of representing the values of all the members of the
enumeration."

Does that mean that this change relies on extensions to the C standard?

Thanks,

Bart.

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Jason Gunthorpe]

On Fri, Feb 09, 2018 at 04:01:51PM +0000, Bart Van Assche wrote:
> On Thu, 2018-02-08 at 16:38 -0700, Jason Gunthorpe wrote:
> >  enum {
> > -	MLX4_WQE_BIND_TYPE_2		= (1<<31),
> > +	MLX4_WQE_BIND_TYPE_2		= (1UL<<31),
> >  	MLX4_WQE_BIND_ZERO_BASED	= (1<<30),
> >  };
> 
> Hello Jason,
> 
> A quote from the C11 standard:
> 
> "The expression that defines the value of an enumeration constant shall be an
> integer constant expression that has a value representable as an int. [ ... ]
> Each enumerated type shall be compatible with char, a signed integer type, or
> an unsigned integer type. The choice of type is implementation-defined but
> shall be capable of representing the values of all the members of the
> enumeration."
> 
> Does that mean that this change relies on extensions to the C standard?

Yes, this relies on a common compiler extension to promote the type of
enum constants beyond int. AFAIK all compilers have done this forever,
kinda bonkers it isn't in C11, IMHO.

As I understand it, the current code is also non-conforming as (1<<31)
is apparently undefined behavior, and at least gcc throws a warning
for it in -Wpedantic mode.

I did a patch that will make our public headers mostly strictly
conforming to C11 with -Wpedantic set, which I've been wondering if it
is worth sending..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Parav Pandit]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Jason Gunthorpe
> Sent: Friday, February 09, 2018 10:11 AM
> To: Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions
> 
> On Fri, Feb 09, 2018 at 04:01:51PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-02-08 at 16:38 -0700, Jason Gunthorpe wrote:
> > >  enum {
> > > -	MLX4_WQE_BIND_TYPE_2		= (1<<31),
> > > +	MLX4_WQE_BIND_TYPE_2		= (1UL<<31),
> > >  	MLX4_WQE_BIND_ZERO_BASED	= (1<<30),
> > >  };
> >
> > Hello Jason,
> >
> > A quote from the C11 standard:
> >
> > "The expression that defines the value of an enumeration constant
> > shall be an integer constant expression that has a value representable
> > as an int. [ ... ] Each enumerated type shall be compatible with char,
> > a signed integer type, or an unsigned integer type. The choice of type
> > is implementation-defined but shall be capable of representing the
> > values of all the members of the enumeration."
> >
> > Does that mean that this change relies on extensions to the C standard?
> 
> Yes, this relies on a common compiler extension to promote the type of enum
> constants beyond int. AFAIK all compilers have done this forever, kinda bonkers
> it isn't in C11, IMHO.
> 
> As I understand it, the current code is also non-conforming as (1<<31) is
> apparently undefined behavior, and at least gcc throws a warning for it in -
> Wpedantic mode.
> 
> I did a patch that will make our public headers mostly strictly conforming to C11
> with -Wpedantic set, which I've been wondering if it is worth sending..
> 
Any reason to use open code instead of using BIT() macro from include/linux/bitops.h which does the same thing?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Jason Gunthorpe]

On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote:

> > I did a patch that will make our public headers mostly strictly conforming to C11
> > with -Wpedantic set, which I've been wondering if it is worth sending..

> Any reason to use open code instead of using BIT() macro from
> include/linux/bitops.h which does the same thing?

BIT only works in the kernel. This is a publich header from rdma-core
userspace.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Parav Pandit]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgg-uk2M96/98Pc@public.gmane.org]
> Sent: Friday, February 09, 2018 10:13 AM
> To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions
> 
> On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote:
> 
> > > I did a patch that will make our public headers mostly strictly
> > > conforming to C11 with -Wpedantic set, which I've been wondering if it is
> worth sending..
> 
> > Any reason to use open code instead of using BIT() macro from
> > include/linux/bitops.h which does the same thing?
> 
> BIT only works in the kernel. This is a publich header from rdma-core userspace.
> 
o.k. Got it. Same duplicated definition exist in kernel too which is not yet fixed because its unused, I guess.
Likely user space headers don't have any BIT() or support macros.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On Fri, Feb 09, 2018 at 09:13:25AM -0700, Jason Gunthorpe wrote:
> On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote:
>
> > > I did a patch that will make our public headers mostly strictly conforming to C11
> > > with -Wpedantic set, which I've been wondering if it is worth sending..
>
> > Any reason to use open code instead of using BIT() macro from
> > include/linux/bitops.h which does the same thing?
>
> BIT only works in the kernel. This is a publich header from rdma-core
> userspace.

➜  rdma-core git:(master) git grep "define BIT" providers/mlx5/cq.c
providers/mlx5/cq.c:#define BIT(i) (1UL << (i))

Thanks

>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Jason Gunthorpe]

On Fri, Feb 09, 2018 at 06:42:00PM +0200, Leon Romanovsky wrote:
> On Fri, Feb 09, 2018 at 09:13:25AM -0700, Jason Gunthorpe wrote:
> > On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote:
> >
> > > > I did a patch that will make our public headers mostly strictly conforming to C11
> > > > with -Wpedantic set, which I've been wondering if it is worth sending..
> >
> > > Any reason to use open code instead of using BIT() macro from
> > > include/linux/bitops.h which does the same thing?
> >
> > BIT only works in the kernel. This is a publich header from rdma-core
> > userspace.
> 
> ➜  rdma-core git:(master) git grep "define BIT" providers/mlx5/cq.c
> providers/mlx5/cq.c:#define BIT(i) (1UL << (i))

Still can't use it in a public header.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On Fri, Feb 09, 2018 at 09:51:38AM -0700, Jason Gunthorpe wrote:
> On Fri, Feb 09, 2018 at 06:42:00PM +0200, Leon Romanovsky wrote:
> > On Fri, Feb 09, 2018 at 09:13:25AM -0700, Jason Gunthorpe wrote:
> > > On Fri, Feb 09, 2018 at 04:12:24PM +0000, Parav Pandit wrote:
> > >
> > > > > I did a patch that will make our public headers mostly strictly conforming to C11
> > > > > with -Wpedantic set, which I've been wondering if it is worth sending..
> > >
> > > > Any reason to use open code instead of using BIT() macro from
> > > > include/linux/bitops.h which does the same thing?
> > >
> > > BIT only works in the kernel. This is a publich header from rdma-core
> > > userspace.
> >
> > ➜  rdma-core git:(master) git grep "define BIT" providers/mlx5/cq.c
> > providers/mlx5/cq.c:#define BIT(i) (1UL << (i))
>
> Still can't use it in a public header.

Thanks, I missed that we are talking about mlx4dv.h

Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 604 bytes --]

On Fri, 2018-02-09 at 16:12 +0000, Parav Pandit wrote:
> Any reason to use open code instead of using BIT() macro from
> include/linux/bitops.h which does the same thing?

Personally I'm in favor of removing the BIT() macro from the kernel. That macro
is not useful and only adds confusion. Code that does not use that macro is
easier to read because one does not have to look up whether the BIT() macro
is defined as 1U << x, 1UL << x or 1ULL << x.

Bart.


N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Jason Gunthorpe]

On Fri, Feb 09, 2018 at 04:19:23PM +0000, Bart Van Assche wrote:
> On Fri, 2018-02-09 at 16:12 +0000, Parav Pandit wrote:
> > Any reason to use open code instead of using BIT() macro from
> > include/linux/bitops.h which does the same thing?
> 
> Personally I'm in favor of removing the BIT() macro from the kernel. That macro
> is not useful and only adds confusion. Code that does not use that macro is
> easier to read because one does not have to look up whether the BIT() macro
> is defined as 1U << x, 1UL << x or 1ULL << x.

Really? The rule is pretty simple:

 BIT(x) for x < 32
 BIT_ULL(x) for x >= 32 && x < 64

Why do you care about what specific type the macro produces?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx4: Fix 1<<31 expressions

[Bart Van Assche]

On 02/09/18 08:24, Jason Gunthorpe wrote:
> On Fri, Feb 09, 2018 at 04:19:23PM +0000, Bart Van Assche wrote:
>> On Fri, 2018-02-09 at 16:12 +0000, Parav Pandit wrote:
>>> Any reason to use open code instead of using BIT() macro from
>>> include/linux/bitops.h which does the same thing?
>>
>> Personally I'm in favor of removing the BIT() macro from the kernel. That macro
>> is not useful and only adds confusion. Code that does not use that macro is
>> easier to read because one does not have to look up whether the BIT() macro
>> is defined as 1U << x, 1UL << x or 1ULL << x.
> 
> Really? The rule is pretty simple:
> 
>   BIT(x) for x < 32
>   BIT_ULL(x) for x >= 32 && x < 64
> 
> Why do you care about what specific type the macro produces?

That's something additional kernel developers have to memorize. If the 
BIT() macro is not used but the shift operation is explicit then that 
saves the lookup of the BIT() definition in case one would have 
forgotten what the BIT() definition looks like.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html