From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751749AbdBMJKm (ORCPT ); Mon, 13 Feb 2017 04:10:42 -0500 Received: from mx2.suse.de ([195.135.220.15]:39822 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750975AbdBMJKl (ORCPT ); Mon, 13 Feb 2017 04:10:41 -0500 Subject: Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues() To: SF Markus Elfring , linux-rdma@vger.kernel.org, Dennis Dalessandro , Doug Ledford , Hal Rosenstock , Mike Marciniszyn , Sean Hefty References: <8a997282-09c7-0f9f-645e-d7c6e8c79e67@users.sourceforge.net> <0fdf4d81-58d7-c6fc-b37f-41d47675dd83@users.sourceforge.net> Cc: LKML , kernel-janitors@vger.kernel.org From: Johannes Thumshirn Message-ID: Date: Mon, 13 Feb 2017 10:10:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <0fdf4d81-58d7-c6fc-b37f-41d47675dd83@users.sourceforge.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/10/2017 10:03 PM, SF Markus Elfring wrote: > From: Markus Elfring > 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 > --- > 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