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@xxxxxxx +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 -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
