| Message ID | 0fdf4d81-58d7-c6fc-b37f-41d47675dd83@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
>> @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;