* [PATCH 1/5] IB/hfi1: Use kcalloc() in hfi1_user_exp_rcv_init()
2017-02-10 21:00 [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring
@ 2017-02-10 21:01 ` SF Markus Elfring
2017-02-10 21:02 ` [PATCH 2/5] IB/hfi1: Use kcalloc() in hfi1_user_sdma_alloc_queues() SF Markus Elfring
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-10 21:01 UTC (permalink / raw)
To: linux-rdma, Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/5] IB/hfi1: Use kcalloc() in hfi1_user_sdma_alloc_queues()
2017-02-10 21:00 [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring
2017-02-10 21:01 ` [PATCH 1/5] IB/hfi1: Use kcalloc() in hfi1_user_exp_rcv_init() SF Markus Elfring
@ 2017-02-10 21:02 ` SF Markus Elfring
2017-02-10 21:03 ` [PATCH 3/5] IB/hfi1: Adjust another size determination " SF Markus Elfring
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-10 21:02 UTC (permalink / raw)
To: linux-rdma, Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()
2017-02-10 21:00 [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring
2017-02-10 21:01 ` [PATCH 1/5] IB/hfi1: Use kcalloc() in hfi1_user_exp_rcv_init() SF Markus Elfring
2017-02-10 21:02 ` [PATCH 2/5] IB/hfi1: Use kcalloc() in hfi1_user_sdma_alloc_queues() SF Markus Elfring
@ 2017-02-10 21:03 ` SF Markus Elfring
2017-02-13 9:10 ` Johannes Thumshirn
2017-02-10 21:04 ` [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request() SF Markus Elfring
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-10 21:03 UTC (permalink / raw)
To: linux-rdma, Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()
2017-02-10 21:03 ` [PATCH 3/5] IB/hfi1: Adjust another size determination " SF Markus Elfring
@ 2017-02-13 9:10 ` Johannes Thumshirn
2017-02-13 9:32 ` SF Markus Elfring
0 siblings, 1 reply; 17+ messages in thread
From: Johannes Thumshirn @ 2017-02-13 9:10 UTC (permalink / raw)
To: SF Markus Elfring, linux-rdma, Dennis Dalessandro, Doug Ledford,
Hal Rosenstock, Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()
2017-02-13 9:10 ` Johannes Thumshirn
@ 2017-02-13 9:32 ` SF Markus Elfring
2017-02-13 9:51 ` Johannes Thumshirn
0 siblings, 1 reply; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-13 9:32 UTC (permalink / raw)
To: Johannes Thumshirn, linux-rdma
Cc: Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
>> @@ -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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()
2017-02-13 9:32 ` SF Markus Elfring
@ 2017-02-13 9:51 ` Johannes Thumshirn
2017-02-13 10:37 ` SF Markus Elfring
0 siblings, 1 reply; 17+ messages in thread
From: Johannes Thumshirn @ 2017-02-13 9:51 UTC (permalink / raw)
To: SF Markus Elfring, linux-rdma
Cc: Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()
2017-02-13 9:51 ` Johannes Thumshirn
@ 2017-02-13 10:37 ` SF Markus Elfring
2017-02-13 10:49 ` Johannes Thumshirn
0 siblings, 1 reply; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-13 10:37 UTC (permalink / raw)
To: Johannes Thumshirn, linux-rdma
Cc: Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
>> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()
2017-02-13 10:37 ` SF Markus Elfring
@ 2017-02-13 10:49 ` Johannes Thumshirn
0 siblings, 0 replies; 17+ messages in thread
From: Johannes Thumshirn @ 2017-02-13 10:49 UTC (permalink / raw)
To: SF Markus Elfring, linux-rdma
Cc: Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
2017-02-10 21:00 [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring
` (2 preceding siblings ...)
2017-02-10 21:03 ` [PATCH 3/5] IB/hfi1: Adjust another size determination " SF Markus Elfring
@ 2017-02-10 21:04 ` SF Markus Elfring
2017-02-11 15:32 ` Dennis Dalessandro
2017-02-10 21:05 ` [PATCH 5/5] IB/hfi1: Improve another size determination " SF Markus Elfring
2017-04-20 20:29 ` [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations Doug Ledford
5 siblings, 1 reply; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-10 21:04 UTC (permalink / raw)
To: linux-rdma, Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
2017-02-10 21:04 ` [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request() SF Markus Elfring
@ 2017-02-11 15:32 ` Dennis Dalessandro
2017-02-13 9:50 ` [PATCH 27/27] IB/hfi1: Code reuse with memdup_copy SF Markus Elfring
2017-02-13 10:53 ` [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request() Dan Carpenter
0 siblings, 2 replies; 17+ messages in thread
From: Dennis Dalessandro @ 2017-02-11 15:32 UTC (permalink / raw)
To: SF Markus Elfring, linux-rdma, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 27/27] IB/hfi1: Code reuse with memdup_copy
2017-02-11 15:32 ` Dennis Dalessandro
@ 2017-02-13 9:50 ` SF Markus Elfring
2017-02-13 10:53 ` [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request() Dan Carpenter
1 sibling, 0 replies; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-13 9:50 UTC (permalink / raw)
To: Dennis Dalessandro, linux-rdma, Doug Ledford, Hal Rosenstock,
Michael J. Ruhl, Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
2017-02-11 15:32 ` Dennis Dalessandro
2017-02-13 9:50 ` [PATCH 27/27] IB/hfi1: Code reuse with memdup_copy SF Markus Elfring
@ 2017-02-13 10:53 ` Dan Carpenter
2017-02-13 11:12 ` SF Markus Elfring
1 sibling, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2017-02-13 10:53 UTC (permalink / raw)
To: Dennis Dalessandro
Cc: SF Markus Elfring, linux-rdma, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
2017-02-13 10:53 ` [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request() Dan Carpenter
@ 2017-02-13 11:12 ` SF Markus Elfring
2017-02-13 14:01 ` Dan Carpenter
0 siblings, 1 reply; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-13 11:12 UTC (permalink / raw)
To: Dan Carpenter, linux-rdma
Cc: Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
>> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
2017-02-13 11:12 ` SF Markus Elfring
@ 2017-02-13 14:01 ` Dan Carpenter
0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2017-02-13 14:01 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-rdma, Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] IB/hfi1: Improve another size determination in hfi1_user_sdma_process_request()
2017-02-10 21:00 [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring
` (3 preceding siblings ...)
2017-02-10 21:04 ` [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request() SF Markus Elfring
@ 2017-02-10 21:05 ` SF Markus Elfring
2017-04-20 20:29 ` [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations Doug Ledford
5 siblings, 0 replies; 17+ messages in thread
From: SF Markus Elfring @ 2017-02-10 21:05 UTC (permalink / raw)
To: linux-rdma, Dennis Dalessandro, Doug Ledford, Hal Rosenstock,
Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations
2017-02-10 21:00 [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations SF Markus Elfring
` (4 preceding siblings ...)
2017-02-10 21:05 ` [PATCH 5/5] IB/hfi1: Improve another size determination " SF Markus Elfring
@ 2017-04-20 20:29 ` Doug Ledford
5 siblings, 0 replies; 17+ messages in thread
From: Doug Ledford @ 2017-04-20 20:29 UTC (permalink / raw)
To: SF Markus Elfring, linux-rdma, Dennis Dalessandro,
Hal Rosenstock, Mike Marciniszyn, Sean Hefty
Cc: LKML, kernel-janitors
On Fri, 2017-02-10 at 22:00 +0100, SF Markus Elfring wrote:
> 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(-)
I took 4 of the 5, leaving out the one that was addressed separately by
an Intel patch.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
^ permalink raw reply [flat|nested] 17+ messages in thread