From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753890AbcHRK4C (ORCPT ); Thu, 18 Aug 2016 06:56:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45930 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752483AbcHRKz7 (ORCPT ); Thu, 18 Aug 2016 06:55:59 -0400 Subject: Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data() To: wharms@bfs.de References: <82b84c9c-38a4-4d17-910f-312668dbae01@users.sourceforge.net> <033d8595-d051-1fa8-95b1-5d2056eb5667@users.sourceforge.net> <57B562F3.1080004@bfs.de> <9db1986a-9b93-72ca-f35e-85b5b5e9f351@redhat.com> <57B5935B.3050602@bfs.de> Cc: Julia Lawall , SF Markus Elfring , kvm@vger.kernel.org, linux-s390@vger.kernel.org, =?UTF-8?Q?Christian_Borntr=c3=a4ger?= , Cornelia Huck , David Hildenbrand , Heiko Carstens , Martin Schwidefsky , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , LKML , kernel-janitors@vger.kernel.org From: Paolo Bonzini Message-ID: <38bba070-d157-dc49-b428-47768ad647ca@redhat.com> Date: Thu, 18 Aug 2016 12:55:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <57B5935B.3050602@bfs.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 18 Aug 2016 10:55:17 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18/08/2016 12:52, walter harms wrote: > > > Am 18.08.2016 11:48, schrieb Paolo Bonzini: >> >> >> On 18/08/2016 11:02, Julia Lawall wrote: >>> >>> >>> On Thu, 18 Aug 2016, walter harms wrote: >>> >>>> >>>> >>>> Am 17.08.2016 20:06, schrieb SF Markus Elfring: >>>>> From: Markus Elfring >>>>> Date: Wed, 17 Aug 2016 18:29:04 +0200 >>>>> >>>>> Replace the specification of data structures by pointer dereferences >>>>> to make the corresponding size determination a bit safer according to >>>>> the Linux coding style convention. >>>>> >>>>> Signed-off-by: Markus Elfring >>>>> --- >>>>> arch/s390/kvm/guestdbg.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c >>>>> index d1f8241..b68db4b 100644 >>>>> --- a/arch/s390/kvm/guestdbg.c >>>>> +++ b/arch/s390/kvm/guestdbg.c >>>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) >>>>> return -EINVAL; >>>>> >>>>> - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); >>>>> + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); >>>>> bp_data = kmalloc(size, GFP_KERNEL); >>>>> if (!bp_data) { >>>>> ret = -ENOMEM; >>>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> } >>>>> } >>>>> >>>>> - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); >>>>> + size = nr_wp * sizeof(*wp_info); >>>>> if (size > 0) { >>>>> wp_info = kmalloc(size, GFP_KERNEL); >>>>> if (!wp_info) { >>>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> goto error; >>>>> } >>>>> } >>>>> - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); >>>>> + size = nr_bp * sizeof(*bp_info); >>>>> if (size > 0) { >>>>> bp_info = kmalloc(size, GFP_KERNEL); >>>>> if (!bp_info) { >>>> >>>> >>>> IMHO the common pattern for kmalloc is >>>> bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); >>>> >>>> i can not remember code with a check for size < 0, i guess it is here >>>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause >>>> a malloc failure an can be ignored. >>> >>> Shoudn't it be kcalloc? >> >> Or kmalloc_array, since zeroing is not necessary. Might be an idea for >> a new Coccinelle script, like >> >> - kmalloc (N * sizeof T, GFP) >> + kmalloc_array(N, sizeof T, GFP) >> > > > my personal taste is to stay close to the libc functions. > technical there is no difference > > static inline void *kcalloc(size_t n, size_t size, gfp_t flags) > { > return kmalloc_array(n, size, flags | __GFP_ZERO); > } > > and i do not see any time critical things here, This is _not_ premature optimization. (k)calloc tells the reader that it's safe not to initialize part of the array. kmalloc_array says the opposite. Using the right function adds important hints in the code---which unlike comments cannot get stale without also introducing visible bugs. Paolo From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Date: Thu, 18 Aug 2016 10:55:11 +0000 Subject: Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data() Message-Id: <38bba070-d157-dc49-b428-47768ad647ca@redhat.com> List-Id: References: <82b84c9c-38a4-4d17-910f-312668dbae01@users.sourceforge.net> <033d8595-d051-1fa8-95b1-5d2056eb5667@users.sourceforge.net> <57B562F3.1080004@bfs.de> <9db1986a-9b93-72ca-f35e-85b5b5e9f351@redhat.com> <57B5935B.3050602@bfs.de> In-Reply-To: <57B5935B.3050602@bfs.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: wharms@bfs.de Cc: Julia Lawall , SF Markus Elfring , kvm@vger.kernel.org, linux-s390@vger.kernel.org, =?UTF-8?Q?Christian_Borntr=c3=a4ger?= , Cornelia Huck , David Hildenbrand , Heiko Carstens , Martin Schwidefsky , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , LKML , kernel-janitors@vger.kernel.org On 18/08/2016 12:52, walter harms wrote: > > > Am 18.08.2016 11:48, schrieb Paolo Bonzini: >> >> >> On 18/08/2016 11:02, Julia Lawall wrote: >>> >>> >>> On Thu, 18 Aug 2016, walter harms wrote: >>> >>>> >>>> >>>> Am 17.08.2016 20:06, schrieb SF Markus Elfring: >>>>> From: Markus Elfring >>>>> Date: Wed, 17 Aug 2016 18:29:04 +0200 >>>>> >>>>> Replace the specification of data structures by pointer dereferences >>>>> to make the corresponding size determination a bit safer according to >>>>> the Linux coding style convention. >>>>> >>>>> Signed-off-by: Markus Elfring >>>>> --- >>>>> arch/s390/kvm/guestdbg.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c >>>>> index d1f8241..b68db4b 100644 >>>>> --- a/arch/s390/kvm/guestdbg.c >>>>> +++ b/arch/s390/kvm/guestdbg.c >>>>> @@ -216,7 +216,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT) >>>>> return -EINVAL; >>>>> >>>>> - size = dbg->arch.nr_hw_bp * sizeof(struct kvm_hw_breakpoint); >>>>> + size = dbg->arch.nr_hw_bp * sizeof(*bp_data); >>>>> bp_data = kmalloc(size, GFP_KERNEL); >>>>> if (!bp_data) { >>>>> ret = -ENOMEM; >>>>> @@ -241,7 +241,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> } >>>>> } >>>>> >>>>> - size = nr_wp * sizeof(struct kvm_hw_wp_info_arch); >>>>> + size = nr_wp * sizeof(*wp_info); >>>>> if (size > 0) { >>>>> wp_info = kmalloc(size, GFP_KERNEL); >>>>> if (!wp_info) { >>>>> @@ -249,7 +249,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>>> goto error; >>>>> } >>>>> } >>>>> - size = nr_bp * sizeof(struct kvm_hw_bp_info_arch); >>>>> + size = nr_bp * sizeof(*bp_info); >>>>> if (size > 0) { >>>>> bp_info = kmalloc(size, GFP_KERNEL); >>>>> if (!bp_info) { >>>> >>>> >>>> IMHO the common pattern for kmalloc is >>>> bp_info = kmalloc( nr_bp * sizeof(*bp_info), GFP_KERNEL); >>>> >>>> i can not remember code with a check for size < 0, i guess it is here >>>> to avoid an overflow ? since kmalloc takes a size_t argument this would cause >>>> a malloc failure an can be ignored. >>> >>> Shoudn't it be kcalloc? >> >> Or kmalloc_array, since zeroing is not necessary. Might be an idea for >> a new Coccinelle script, like >> >> - kmalloc (N * sizeof T, GFP) >> + kmalloc_array(N, sizeof T, GFP) >> > > > my personal taste is to stay close to the libc functions. > technical there is no difference > > static inline void *kcalloc(size_t n, size_t size, gfp_t flags) > { > return kmalloc_array(n, size, flags | __GFP_ZERO); > } > > and i do not see any time critical things here, This is _not_ premature optimization. (k)calloc tells the reader that it's safe not to initialize part of the array. kmalloc_array says the opposite. Using the right function adds important hints in the code---which unlike comments cannot get stale without also introducing visible bugs. Paolo