Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1578801 > unrolled thread

[PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-10 22:10 +0100
    [PATCH 1/5] IB/hfi1: Use kcalloc() in hfi1_user_exp_rcv_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-10 22:10 +0100
    [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its  implementation in hfi1_user_sdma_process_request() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-10 22:10 +0100
      Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating  its implementation in hfi1_user_sdma_process_request() Dennis Dalessandro <dennis.dalessandro@intel.com> - 2017-02-11 16:40 +0100
        Re: [PATCH 27/27] IB/hfi1: Code reuse with memdup_copy SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-13 11:00 +0100
        Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating  its implementation in hfi1_user_sdma_process_request() Dan Carpenter <dan.carpenter@oracle.com> - 2017-02-13 12:00 +0100
          Re: IB/hfi1: Use memdup_user() rather than duplicating its  implementation in hfi1_user_sdma_process_request() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-13 12:20 +0100
            Re: IB/hfi1: Use memdup_user() rather than duplicating its  implementation in hfi1_user_sdma_process_request() Dan Carpenter <dan.carpenter@oracle.com> - 2017-02-13 15:10 +0100
    [PATCH 5/5] IB/hfi1: Improve another size determination in  hfi1_user_sdma_process_request() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-10 22:10 +0100
    [PATCH 3/5] IB/hfi1: Adjust another size determination in  hfi1_user_sdma_alloc_queues() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-10 22:10 +0100
      Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in  hfi1_user_sdma_alloc_queues() Johannes Thumshirn <jthumshirn@suse.de> - 2017-02-13 10:20 +0100
        Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in  hfi1_user_sdma_alloc_queues() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-13 10:40 +0100
          Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in  hfi1_user_sdma_alloc_queues() Johannes Thumshirn <jthumshirn@suse.de> - 2017-02-13 11:00 +0100
            Re: IB/hfi1: Adjust another size determination in  hfi1_user_sdma_alloc_queues() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-13 11:40 +0100
              Re: IB/hfi1: Adjust another size determination in  hfi1_user_sdma_alloc_queues() Johannes Thumshirn <jthumshirn@suse.de> - 2017-02-13 11:50 +0100
    [PATCH 2/5] IB/hfi1: Use kcalloc() in hfi1_user_sdma_alloc_queues() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-02-10 22:10 +0100

#1578801 — [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 21:53:21 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use kcalloc() in hfi1_user_exp_rcv_init()
  Use kcalloc() in hfi1_user_sdma_alloc_queues()
  Adjust another size determination in hfi1_user_sdma_alloc_queues()
  Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
  Improve another size determination in hfi1_user_sdma_process_request()

 drivers/infiniband/hw/hfi1/user_exp_rcv.c |  5 +++--
 drivers/infiniband/hw/hfi1/user_sdma.c    | 35 +++++++++++++++----------------
 2 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.11.1

[toc] | [next] | [standalone]

#1578802 — [PATCH 1/5] IB/hfi1: Use kcalloc() in hfi1_user_exp_rcv_init()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 9 Feb 2017 15:30:53 +0100

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kcalloc".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 64d26525435a..8ae0d26a34c6 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -199,8 +199,9 @@ int hfi1_user_exp_rcv_init(struct file *fp)
 
 	if (!HFI1_CAP_UGET_MASK(uctxt->flags, TID_UNMAP)) {
 		fd->invalid_tid_idx = 0;
-		fd->invalid_tids = kzalloc(uctxt->expected_count *
-					   sizeof(u32), GFP_KERNEL);
+		fd->invalid_tids = kcalloc(uctxt->expected_count,
+					   sizeof(*fd->invalid_tids),
+					   GFP_KERNEL);
 		if (!fd->invalid_tids) {
 			ret = -ENOMEM;
 			goto done;
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1578803 — [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 21:01:55 +0100

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 991e7f3d8e18..5a73d738f2ba 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -731,23 +731,18 @@ int hfi1_user_sdma_process_request(struct file *fp, struct iovec *iovec,
 			ret = -EINVAL;
 			goto free_req;
 		}
-		req->tids = kcalloc(ntids, sizeof(*req->tids), GFP_KERNEL);
-		if (!req->tids) {
-			ret = -ENOMEM;
-			goto free_req;
-		}
 		/*
 		 * We have to copy all of the tids because they may vary
 		 * in size and, therefore, the TID count might not be
 		 * equal to the pkt count. However, there is no way to
 		 * tell at this point.
 		 */
-		ret = copy_from_user(req->tids, iovec[idx].iov_base,
-				     ntids * sizeof(*req->tids));
-		if (ret) {
+		req->tids = memdup_user(iovec[idx].iov_base,
+					sizeof(*req->tids) * ntids);
+		if (IS_ERR(req->tids)) {
+			ret = PTR_ERR(req->tids);
 			SDMA_DBG(req, "Failed to copy %d TIDs (%d)",
 				 ntids, ret);
-			ret = -EFAULT;
 			goto free_req;
 		}
 		req->n_tids = ntids;
@@ -1606,7 +1601,8 @@ static void user_sdma_free_request(struct user_sdma_request *req, bool unpin)
 				atomic_dec(&node->refcount);
 		}
 	}
-	kfree(req->tids);
+	if (!IS_ERR(req->tids))
+		kfree(req->tids);
 	clear_bit(req->info.comp_idx, req->pq->req_in_use);
 }
 
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1579030 — Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()

On 02/10/2017 04:04 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Feb 2017 21:01:55 +0100
>
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks for the patch, but this one is already taken care of along with 
other similar uses of kmalloc/copy:

http://marc.info/?l=linux-rdma&m=148656088729538&w=2

Will review the rest of the patch series soon.

-Denny

[toc] | [prev] | [next] | [standalone]

#1579561 — Re: [PATCH 27/27] IB/hfi1: Code reuse with memdup_copy

> Thanks for the patch, but this one is already taken care of along with other similar uses of kmalloc/copy:
> http://marc.info/?l=linux-rdma&m=148656088729538&w=2

Thanks for your information.

The shown source code is reasonable in the update step “[PATCH 27/27] IB/hfi1:
Code reuse with memdup_copy”.
https://patchwork.kernel.org/patch/9562565/
https://lkml.kernel.org/r/<20170208132830.16442.93943.stgit@scvm10.sc.intel.com>

I find the commit subject and message partly inappropriate.
How do you think about to mention other function names there?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1579613 — Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()

On Sat, Feb 11, 2017 at 10:32:59AM -0500, Dennis Dalessandro wrote:
> On 02/10/2017 04:04 PM, SF Markus Elfring wrote:
> >From: Markus Elfring <elfring@users.sourceforge.net>
> >Date: Fri, 10 Feb 2017 21:01:55 +0100
> >
> >Reuse existing functionality from memdup_user() instead of keeping
> >duplicate source code.
> >
> >Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Thanks for the patch, but this one is already taken care of along
> with other similar uses of kmalloc/copy:
> 
> http://marc.info/?l=linux-rdma&m=148656088729538&w=2
> 

Michael's patch doesn't change user_sdma_free_request() so it introduces
a kfreeing an error pointer bug.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1579638 — Re: IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()

>> Thanks for the patch, but this one is already taken care of along
>> with other similar uses of kmalloc/copy:
>>
>> http://marc.info/?l=linux-rdma&m=148656088729538&w=2
>>
> 
> Michael's patch doesn't change user_sdma_free_request() so it introduces
> a kfreeing an error pointer bug.

Did you notice that another local variable “tmp” was introduced in the update step
“[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy” so that
the mentioned function will usually get a null pointer after a failure there?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1579803 — Re: IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()

On Mon, Feb 13, 2017 at 12:12:51PM +0100, SF Markus Elfring wrote:
> >> Thanks for the patch, but this one is already taken care of along
> >> with other similar uses of kmalloc/copy:
> >>
> >> http://marc.info/?l=linux-rdma&m=148656088729538&w=2
> >>
> > 
> > Michael's patch doesn't change user_sdma_free_request() so it introduces
> > a kfreeing an error pointer bug.
> 
> Did you notice that another local variable “tmp” was introduced in the update step
> “[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy” so that
> the mentioned function will usually get a null pointer after a failure there?
> 

Ah right.  Thanks.  I missed that.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1578804 — [PATCH 5/5] IB/hfi1: Improve another size determination in hfi1_user_sdma_process_request()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 21:45:38 +0100

Replace the specification of a data structure by a reference to
the desired member as the parameter for the operator "sizeof" to make
the corresponding size determination a bit safer according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 5a73d738f2ba..da131a530b88 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -704,7 +704,9 @@ int hfi1_user_sdma_process_request(struct file *fp, struct iovec *iovec,
 	/* Save all the IO vector structures */
 	for (i = 0; i < req->data_iovs; i++) {
 		INIT_LIST_HEAD(&req->iovs[i].list);
-		memcpy(&req->iovs[i].iov, iovec + idx++, sizeof(struct iovec));
+		memcpy(&req->iovs[i].iov,
+		       iovec + idx++,
+		       sizeof(req->iovs[i].iov));
 		ret = pin_vector_pages(req, &req->iovs[i]);
 		if (ret) {
 			req->status = ret;
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1578805 — [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 08:50:45 +0100

* Pass a product for a call of the function "vmalloc_user" without storing
  it in an intermediate variable.

* Delete the local variable "memsize" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 15194554a92b..991e7f3d8e18 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -375,7 +375,6 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
 {
 	struct hfi1_filedata *fd;
 	int ret = 0;
-	unsigned memsize;
 	char buf[64];
 	struct hfi1_devdata *dd;
 	struct hfi1_user_sdma_comp_q *cq;
@@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
 	if (!cq)
 		goto cq_nomem;
 
-	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
-	cq->comps = vmalloc_user(memsize);
+	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
+				 * hfi1_sdma_comp_ring_size));
 	if (!cq->comps)
 		goto cq_comps_nomem;
 
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1579537 — Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

On 02/10/2017 10:03 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Feb 2017 08:50:45 +0100
> 
> * Pass a product for a call of the function "vmalloc_user" without storing
>   it in an intermediate variable.
> 
> * Delete the local variable "memsize" which became unnecessary with
>   this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/hfi1/user_sdma.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
> index 15194554a92b..991e7f3d8e18 100644
> --- a/drivers/infiniband/hw/hfi1/user_sdma.c
> +++ b/drivers/infiniband/hw/hfi1/user_sdma.c
> @@ -375,7 +375,6 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>  {
>  	struct hfi1_filedata *fd;
>  	int ret = 0;
> -	unsigned memsize;
>  	char buf[64];
>  	struct hfi1_devdata *dd;
>  	struct hfi1_user_sdma_comp_q *cq;
> @@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>  	if (!cq)
>  		goto cq_nomem;
>  
> -	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
> -	cq->comps = vmalloc_user(memsize);
> +	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
> +				 * hfi1_sdma_comp_ring_size));
>  	if (!cq->comps)
>  		goto cq_comps_nomem;
>  
> 

IMHO this makes readability worse. What's the intention behind this
patch? Is there any difference in binary size or something in the likes?
I doubt so as the compiler should take care of this anyways.

I'm just a casual reader of the RDMA list not an active reviewer, but if
you did this in e.g. SCSI I'd NACK it. Code has to be readable for
humans and that means the less that's done in one line of code the
better. Let the compiler do the optimizations and get rid of local
variables.

Just my 2c.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1579553 — Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

>> @@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>>  	if (!cq)
>>  		goto cq_nomem;
>>  
>> -	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
>> -	cq->comps = vmalloc_user(memsize);
>> +	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
>> +				 * hfi1_sdma_comp_ring_size));
>>  	if (!cq->comps)
>>  		goto cq_comps_nomem;
>>  
>>
> 
> IMHO this makes readability worse.

How often does it really make sense to keep such a product in this local variable?


> What's the intention behind this patch?

I suggested just another simple omission of an extra variable.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1579560 — Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

On 02/13/2017 10:32 AM, SF Markus Elfring wrote:
>>> @@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>>>  	if (!cq)
>>>  		goto cq_nomem;
>>>  
>>> -	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
>>> -	cq->comps = vmalloc_user(memsize);
>>> +	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
>>> +				 * hfi1_sdma_comp_ring_size));
>>>  	if (!cq->comps)
>>>  		goto cq_comps_nomem;
>>>  
>>>
>>
>> IMHO this makes readability worse.
> 
> How often does it really make sense to keep such a product in this local variable?

It depends. Lets take it the other way round. If I this in a review I'd
suggest the submitter to create a local variable for the multiplication
to get rid of the line break. It's avoidable. And again, the compiler
will optimize it away.

Apart from the fact that you haven't tested your patch at all:

jthumshirn@linux-x5ow:linux (test)$ git am ~/\[PATCH\ 3_5\]\ IB_hfi1\:\
Adjust\ another\ size\ determination\ in\
hfi1_user_sdma_alloc_queues\(\).eml
Applying: IB/hfi1: Adjust another size determination in
hfi1_user_sdma_alloc_queues()
jthumshirn@linux-x5ow:linux (test)$ make
drivers/infiniband/hw/hfi1/user_sdma.o  CHK
include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      drivers/infiniband/hw/hfi1/user_sdma.o
drivers/infiniband/hw/hfi1/user_sdma.c: In function
‘hfi1_user_sdma_alloc_queues’:
drivers/infiniband/hw/hfi1/user_sdma.c:402:2: error: ‘memsize’
undeclared (first use in this function)
  memsize = sizeof(*pq->reqs) * hfi1_sdma_comp_ring_size;
  ^
drivers/infiniband/hw/hfi1/user_sdma.c:402:2: note: each undeclared
identifier is reported only once for each function it appears in
scripts/Makefile.build:294: recipe for target
'drivers/infiniband/hw/hfi1/user_sdma.o' failed
make[1]: *** [drivers/infiniband/hw/hfi1/user_sdma.o] Error 1
Makefile:1640: recipe for target
'drivers/infiniband/hw/hfi1/user_sdma.o' failed
make: *** [drivers/infiniband/hw/hfi1/user_sdma.o] Error 2

With this fixed:

jthumshirn@linux-x5ow:linux (test)$ size
drivers/infiniband/hw/hfi1/user_sdma.o.*
   text	   data	    bss	    dec	    hex	filename
  15393	    212	    292	  15897	   3e19
drivers/infiniband/hw/hfi1/user_sdma.o.old
  15393	    212	    292	  15897	   3e19
drivers/infiniband/hw/hfi1/user_sdma.o.patched


Glancing over the diff of the objdump of these two object files you'll
notice that all your patch is doing is moving the code around in the
.text section of the binary.

So to sum up: there is no evident improvement in the resulting binary
and you introduce a stylistic glitch (the new line break in a function
call).

But all of what I've provided above would have been your job as patch
submitter. You have to reason why your patch is good, it's not our job
to reason why it's bad.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1579590 — Re: IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

>> How often does it really make sense to keep such a product in this local variable?
> 
> It depends. Lets take it the other way round. If I this in a review I'd
> suggest the submitter to create a local variable for the multiplication
> to get rid of the line break. It's avoidable.

I imagine that there are further possibilities to improve the involved
programming for various arrays.


> And again, the compiler will optimize it away.
> 
> Apart from the fact that you haven't tested your patch at all:

This is true in principle as I could compile the source code adjustment at least.


> jthumshirn@linux-x5ow:linux (test)$ git am ~/\[PATCH\ 3_5\]\ IB_hfi1\:\
> Adjust\ another\ size\ determination\ in\
> hfi1_user_sdma_alloc_queues\(\).eml
> Applying: IB/hfi1: Adjust another size determination in
> hfi1_user_sdma_alloc_queues()
> jthumshirn@linux-x5ow:linux (test)$ make
> drivers/infiniband/hw/hfi1/user_sdma.o  CHK
> include/config/kernel.release
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CHK     include/generated/bounds.h
>   CHK     include/generated/timeconst.h
>   CHK     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/infiniband/hw/hfi1/user_sdma.o
> drivers/infiniband/hw/hfi1/user_sdma.c: In function
> ‘hfi1_user_sdma_alloc_queues’:
> drivers/infiniband/hw/hfi1/user_sdma.c:402:2: error: ‘memsize’
> undeclared (first use in this function)
>   memsize = sizeof(*pq->reqs) * hfi1_sdma_comp_ring_size;
>   ^

How do you think about to apply also the previous update step like “[PATCH 2/5] IB/hfi1:
Use kcalloc() in hfi1_user_sdma_alloc_queues()”?


> So to sum up: there is no evident improvement in the resulting binary

There might not be a remarkable difference with the default software build parameters.


> and you introduce a stylistic glitch (the new line break in a function call).

There are different opinions about this implementation detail, aren't there?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1579604 — Re: IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

On 02/13/2017 11:37 AM, SF Markus Elfring wrote:
>> and you introduce a stylistic glitch (the new line break in a function call).
> 
> There are different opinions about this implementation detail, aren't there?

As I said, I'm just a casual reader of the RDMA list and I expressed my
opinion that this change is counter productive. It's up to the RDMA
maintainers to decide whether they want to take the change or not.

But as someone who's dayjob is to work with this code (or better
backport patches to stable trees and fix customer issues) I prefer
keeping it as it was.

So enough bikeshedding for today.
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1578806 — [PATCH 2/5] IB/hfi1: Use kcalloc() in hfi1_user_sdma_alloc_queues()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 9 Feb 2017 16:06:12 +0100

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus reuse the corresponding function "kcalloc".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 7d22f8ee98ef..15194554a92b 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -400,13 +400,15 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
 	if (!pq)
 		goto pq_nomem;
 
-	memsize = sizeof(*pq->reqs) * hfi1_sdma_comp_ring_size;
-	pq->reqs = kzalloc(memsize, GFP_KERNEL);
+	pq->reqs = kcalloc(hfi1_sdma_comp_ring_size,
+			   sizeof(*pq->reqs),
+			   GFP_KERNEL);
 	if (!pq->reqs)
 		goto pq_reqs_nomem;
 
-	memsize = BITS_TO_LONGS(hfi1_sdma_comp_ring_size) * sizeof(long);
-	pq->req_in_use = kzalloc(memsize, GFP_KERNEL);
+	pq->req_in_use = kcalloc(BITS_TO_LONGS(hfi1_sdma_comp_ring_size),
+				 sizeof(*pq->req_in_use),
+				 GFP_KERNEL);
 	if (!pq->req_in_use)
 		goto pq_reqs_no_in_use;
 
-- 
2.11.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web