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

Groups > linux.kernel > #1579560

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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

csiph-web