diff mbox series

RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build

Message ID 20190522145450.25ff483d@gandalf.local.home (mailing list archive)
State Changes Requested
Headers show
Series RDMA/mlx5: Use DIV_ROUND_UP_ULL macro to allow 32 bit to build | expand

Commit Message

Steven Rostedt May 22, 2019, 6:54 p.m. UTC 
From: Steven Rostedt (VMware) <rostedt@goodmis.org>

When testing 32 bit x86, my build failed with:

  ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!

It appears that a few non-ULL roundup() calls were made, which uses a
normal division against a 64 bit number. This is fine for x86_64, but
on 32 bit x86, it causes the compiler to look for a helper function
__udivdi3, which we do not have in the kernel, and thus fails to build.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---

Comments

Jason Gunthorpe May 22, 2019, 7:28 p.m. UTC | #1 
On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
> 
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> When testing 32 bit x86, my build failed with:
> 
>   ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
> 
> It appears that a few non-ULL roundup() calls were made, which uses a
> normal division against a 64 bit number. This is fine for x86_64, but
> on 32 bit x86, it causes the compiler to look for a helper function
> __udivdi3, which we do not have in the kernel, and thus fails to build.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---

Do you like this version better?

https://patchwork.kernel.org/patch/10950913/

Jason
Steven Rostedt May 22, 2019, 7:43 p.m. UTC | #2 
On Wed, 22 May 2019 16:28:21 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
> > 
> > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > When testing 32 bit x86, my build failed with:
> > 
> >   ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
> > 
> > It appears that a few non-ULL roundup() calls were made, which uses a
> > normal division against a 64 bit number. This is fine for x86_64, but
> > on 32 bit x86, it causes the compiler to look for a helper function
> > __udivdi3, which we do not have in the kernel, and thus fails to build.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---  
> 
> Do you like this version better?
> 
> https://patchwork.kernel.org/patch/10950913/
> 

Honestly, I don't care ;-)

As long as it is correct and doesn't break my builds. I really prefer
if these kinds of things don't make it into Linus's tree to begin with.
I'm surprised the zero-day bot didn't catch this. Because this is
something that it normally does.

-- Steve
Jason Gunthorpe May 22, 2019, 8:14 p.m. UTC | #3 
On Wed, May 22, 2019 at 03:43:05PM -0400, Steven Rostedt wrote:
> On Wed, 22 May 2019 16:28:21 -0300
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
> > > 
> > > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > 
> > > When testing 32 bit x86, my build failed with:
> > > 
> > >   ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
> > > 
> > > It appears that a few non-ULL roundup() calls were made, which uses a
> > > normal division against a 64 bit number. This is fine for x86_64, but
> > > on 32 bit x86, it causes the compiler to look for a helper function
> > > __udivdi3, which we do not have in the kernel, and thus fails to build.
> > > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > Do you like this version better?
> > 
> > https://patchwork.kernel.org/patch/10950913/
> > 
> 
> Honestly, I don't care ;-)
> 
> As long as it is correct and doesn't break my builds. I really prefer
> if these kinds of things don't make it into Linus's tree to begin with.
> I'm surprised the zero-day bot didn't catch this. Because this is
> something that it normally does.

Yes, I was also surprised and I asked them.. They said they needed to
update ARM compilers to see this..

Jason
Steven Rostedt May 22, 2019, 8:19 p.m. UTC | #4 
On Wed, 22 May 2019 17:14:12 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > As long as it is correct and doesn't break my builds. I really prefer
> > if these kinds of things don't make it into Linus's tree to begin with.
> > I'm surprised the zero-day bot didn't catch this. Because this is
> > something that it normally does.  
> 
> Yes, I was also surprised and I asked them.. They said they needed to
> update ARM compilers to see this..

Really? This triggered on x86 not ARM for me.

-- Steve
Michal Kubecek May 23, 2019, 6:58 a.m. UTC | #5 
On Wed, May 22, 2019 at 02:54:50PM -0400, Steven Rostedt wrote:
> 
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> When testing 32 bit x86, my build failed with:
> 
>   ERROR: "__udivdi3" [drivers/infiniband/hw/mlx5/mlx5_ib.ko] undefined!
> 
> It appears that a few non-ULL roundup() calls were made, which uses a
> normal division against a 64 bit number. This is fine for x86_64, but
> on 32 bit x86, it causes the compiler to look for a helper function
> __udivdi3, which we do not have in the kernel, and thus fails to build.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
...
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index abac70ad5c7c..40d4c5f7ea43 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -2344,7 +2344,7 @@ static int handle_alloc_dm_sw_icm(struct ib_ucontext *ctx,
>  	/* Allocation size must a multiple of the basic block size
>  	 * and a power of 2.
>  	 */
> -	act_size = roundup(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
> +	act_size = DIV_ROUND_UP_ULL(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
>  	act_size = roundup_pow_of_two(act_size);
>  
>  	dm->size = act_size;

This seems wrong: roundup() rounds up to a multiple of second argument
but DIV_ROUND_UP_ULL() would divide with rounding up.

Michal Kubecek
Steven Rostedt May 23, 2019, 12:48 p.m. UTC | #6 
On Thu, 23 May 2019 08:58:03 +0200
Michal Kubecek <mkubecek@suse.cz> wrote:

> > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> > index abac70ad5c7c..40d4c5f7ea43 100644
> > --- a/drivers/infiniband/hw/mlx5/main.c
> > +++ b/drivers/infiniband/hw/mlx5/main.c
> > @@ -2344,7 +2344,7 @@ static int handle_alloc_dm_sw_icm(struct ib_ucontext *ctx,
> >  	/* Allocation size must a multiple of the basic block size
> >  	 * and a power of 2.
> >  	 */
> > -	act_size = roundup(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
> > +	act_size = DIV_ROUND_UP_ULL(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
> >  	act_size = roundup_pow_of_two(act_size);
> >  
> >  	dm->size = act_size;  
> 
> This seems wrong: roundup() rounds up to a multiple of second argument
> but DIV_ROUND_UP_ULL() would divide with rounding up.

Yeah, the macros are a bit confusing. There's unfortunately no
roundup_64() (perhaps we should make one?)

#define roundup(x, y) (					\
{							\
	typeof(y) __y = y;				\
	(((x) + (__y - 1)) / __y) * __y;		\
}							\
)


#define DIV_ROUND_DOWN_ULL(ll, d) \
	({ unsigned long long _tmp = (ll); do_div(_tmp, d); _tmp; })

#define DIV_ROUND_UP_ULL(ll, d)		DIV_ROUND_DOWN_ULL((ll) + (d) - 1, (d))


roundup(a, b) == ((a + b - 1) / b) * b

DIV_ROUND_UP_ULL(a, b) DIV_ROUND_DOWN_ULL(a + b - 1, b)
 = (a + b - 1) / b

Hmm, looks like you are right (damn, I thought I did this before
posting the patch, but I must have miscalculated something). It does
look like we are missing a "* b" in there.

I think I'll go and just add a roundup_64()!

Thanks for pointing this out.

-- Steve
Steven Rostedt May 23, 2019, 1:36 p.m. UTC | #7 
On Thu, 23 May 2019 08:48:12 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> I think I'll go and just add a roundup_64()!

Perhaps something like this?

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 34a998012bf6..cdacfe1f732c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -143,14 +143,6 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo)
 	kfree(nvbo);
 }
 
-static inline u64
-roundup_64(u64 x, u32 y)
-{
-	x += y - 1;
-	do_div(x, y);
-	return x * y;
-}
-
 static void
 nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
 		       int *align, u64 *size)
diff --git a/drivers/infiniband/hw/mlx5/cmd.c b/drivers/infiniband/hw/mlx5/cmd.c
index e3ec79b8f7f5..f080df9934e8 100644
--- a/drivers/infiniband/hw/mlx5/cmd.c
+++ b/drivers/infiniband/hw/mlx5/cmd.c
@@ -190,7 +190,7 @@ int mlx5_cmd_alloc_sw_icm(struct mlx5_dm *dm, int type, u64 length,
 			  u16 uid, phys_addr_t *addr, u32 *obj_id)
 {
 	struct mlx5_core_dev *dev = dm->dev;
-	u32 num_blocks = DIV_ROUND_UP(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
+	u32 num_blocks = DIV_ROUND_UP_ULL(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
 	u32 in[MLX5_ST_SZ_DW(create_sw_icm_in)] = {};
 	unsigned long *block_map;
@@ -266,7 +266,7 @@ int mlx5_cmd_dealloc_sw_icm(struct mlx5_dm *dm, int type, u64 length,
 			    u16 uid, phys_addr_t addr, u32 obj_id)
 {
 	struct mlx5_core_dev *dev = dm->dev;
-	u32 num_blocks = DIV_ROUND_UP(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
+	u32 num_blocks = DIV_ROUND_UP_ULL(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
 	u32 in[MLX5_ST_SZ_DW(general_obj_in_cmd_hdr)] = {};
 	unsigned long *block_map;
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index abac70ad5c7c..2d48c0e55ed2 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2344,7 +2344,7 @@ static int handle_alloc_dm_sw_icm(struct ib_ucontext *ctx,
 	/* Allocation size must a multiple of the basic block size
 	 * and a power of 2.
 	 */
-	act_size = roundup(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
+	act_size = roundup_64(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
 	act_size = roundup_pow_of_two(act_size);
 
 	dm->size = act_size;
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index edbd5a210df2..13de9d49bd52 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -207,13 +207,6 @@ static inline xfs_dev_t linux_to_xfs_dev_t(dev_t dev)
 #define xfs_sort(a,n,s,fn)	sort(a,n,s,fn,NULL)
 #define xfs_stack_trace()	dump_stack()
 
-static inline uint64_t roundup_64(uint64_t x, uint32_t y)
-{
-	x += y - 1;
-	do_div(x, y);
-	return x * y;
-}
-
 static inline uint64_t howmany_64(uint64_t x, uint32_t y)
 {
 	x += y - 1;
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 74b1ee9027f5..cd0063629357 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -115,6 +115,20 @@
 	(((x) + (__y - 1)) / __y) * __y;		\
 }							\
 )
+
+#if BITS_PER_LONG == 32
+# define roundup_64(x, y) (				\
+{							\
+	typeof(y) __y = y;				\
+	typeof(x) __x = (x) + (__y - 1);		\
+	do_div(__x, __y);				\
+	__x * __y;					\
+}							\
+)
+#else
+# define roundup_64(x, y)	roundup(x, y)
+#endif
+
 /**
  * rounddown - round down to next specified multiple
  * @x: the value to round


-- Steve
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/mlx5/cmd.c b/drivers/infiniband/hw/mlx5/cmd.c
index e3ec79b8f7f5..f080df9934e8 100644
--- a/drivers/infiniband/hw/mlx5/cmd.c
+++ b/drivers/infiniband/hw/mlx5/cmd.c
@@ -190,7 +190,7 @@  int mlx5_cmd_alloc_sw_icm(struct mlx5_dm *dm, int type, u64 length,
 			  u16 uid, phys_addr_t *addr, u32 *obj_id)
 {
 	struct mlx5_core_dev *dev = dm->dev;
-	u32 num_blocks = DIV_ROUND_UP(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
+	u32 num_blocks = DIV_ROUND_UP_ULL(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
 	u32 in[MLX5_ST_SZ_DW(create_sw_icm_in)] = {};
 	unsigned long *block_map;
@@ -266,7 +266,7 @@  int mlx5_cmd_dealloc_sw_icm(struct mlx5_dm *dm, int type, u64 length,
 			    u16 uid, phys_addr_t addr, u32 obj_id)
 {
 	struct mlx5_core_dev *dev = dm->dev;
-	u32 num_blocks = DIV_ROUND_UP(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
+	u32 num_blocks = DIV_ROUND_UP_ULL(length, MLX5_SW_ICM_BLOCK_SIZE(dev));
 	u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
 	u32 in[MLX5_ST_SZ_DW(general_obj_in_cmd_hdr)] = {};
 	unsigned long *block_map;
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index abac70ad5c7c..40d4c5f7ea43 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2344,7 +2344,7 @@  static int handle_alloc_dm_sw_icm(struct ib_ucontext *ctx,
 	/* Allocation size must a multiple of the basic block size
 	 * and a power of 2.
 	 */
-	act_size = roundup(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
+	act_size = DIV_ROUND_UP_ULL(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
 	act_size = roundup_pow_of_two(act_size);
 
 	dm->size = act_size;