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

Groups > linux.kernel > #1464699 > unrolled thread

[PATCH 0/4] KVM-S390: Fine-tuning for kvm_s390_import_bp_data()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] KVM-S390: Fine-tuning for kvm_s390_import_bp_data() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-17 20:10 +0200
    [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its  implementation SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-17 20:20 +0200
      Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating  its implementation Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-08-22 15:10 +0200
      Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating  its implementation David Hildenbrand <dahi@linux.vnet.ibm.com> - 2016-08-22 15:10 +0200
      Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating  its implementation Christian Borntraeger <borntraeger@de.ibm.com> - 2016-08-24 17:20 +0200
        [PATCH v2 0/2] KVM: s390: Fine-tuning for kvm_s390_import_bp_data() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-24 20:40 +0200
          [PATCH v2 1/2] KVM: s390: Improve determination of sizes in  kvm_s390_import_bp_data() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-24 20:50 +0200
          [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating  code SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-24 20:50 +0200
    [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-17 20:20 +0200
      Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection David Hildenbrand <dahi@linux.vnet.ibm.com> - 2016-08-22 15:00 +0200
      Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-08-22 15:10 +0200
        Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-22 19:00 +0200
          Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-08-22 21:40 +0200
            Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-22 23:20 +0200
              Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-08-22 23:30 +0200
      Re: [PATCH 3/4] KVM-S390: Less function calls in  kvm_s390_import_bp_data() after error detection Christian Borntraeger <borntraeger@de.ibm.com> - 2016-08-24 17:20 +0200
    [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a  buffer variable SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-17 20:20 +0200
      Re: [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for  a buffer variable Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-08-22 15:10 +0200
      Re: [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for  a buffer variable David Hildenbrand <dahi@linux.vnet.ibm.com> - 2016-08-22 15:10 +0200
    [PATCH 1/4] KVM-S390: Improve determination of sizes in  kvm_s390_import_bp_data() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-17 20:20 +0200
      Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data() walter harms <wharms@bfs.de> - 2016-08-18 09:30 +0200
        Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in  kvm_s390_import_bp_data() Julia Lawall <julia.lawall@lip6.fr> - 2016-08-18 11:10 +0200
          Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in  kvm_s390_import_bp_data() Paolo Bonzini <pbonzini@redhat.com> - 2016-08-18 12:10 +0200
            Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data() walter harms <wharms@bfs.de> - 2016-08-18 13:00 +0200
              Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in  kvm_s390_import_bp_data() Paolo Bonzini <pbonzini@redhat.com> - 2016-08-18 13:10 +0200
                Re: [PATCH 1/4] KVM-S390: Improve determination of sizes in  kvm_s390_import_bp_data() Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-08-22 15:00 +0200
            Re: Replacing specific kmalloc() calls by kmalloc_array()? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-24 14:20 +0200
              Re: Replacing specific kmalloc() calls by kmalloc_array()? Paolo Bonzini <pbonzini@redhat.com> - 2016-08-24 17:10 +0200

Page 1 of 2  [1] 2  Next page →

#1464699 — [PATCH 0/4] KVM-S390: Fine-tuning for kvm_s390_import_bp_data()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 19:49:29 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Improve determination of sizes
  Use memdup_user()
  Less function calls after error detection
  Delete an unnecessary initialisation

 arch/s390/kvm/guestdbg.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1464702 — [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 18:41:43 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index b68db4b..8f886ee 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -217,16 +217,9 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
-	bp_data = kmalloc(size, GFP_KERNEL);
-	if (!bp_data) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	if (copy_from_user(bp_data, dbg->arch.hw_bp, size)) {
-		ret = -EFAULT;
-		goto error;
-	}
+	bp_data = memdup_user(dbg->arch.hw_bp, size);
+	if (IS_ERR(bp_data))
+		return PTR_ERR(bp_data);
 
 	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
 		switch (bp_data[i].type) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1467597 — Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

On Wed, 17 Aug 2016 20:08:49 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:41:43 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.

Looks sane, but please take a bit of care about the subject: It's a bit
long, and the prefix should be "KVM: s390:".

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

if Christian wants to apply this (unless I beat him to it).

[toc] | [prev] | [next] | [standalone]

#1467598 — Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:41:43 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I like that. Thanks for running that analysis software against s390 KVM code!

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David

[toc] | [prev] | [next] | [standalone]

#1469535 — Re: [PATCH 2/4] KVM-S390: Use memdup_user() rather than duplicating its implementation

On 08/17/2016 02:08 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 18:41:43 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I was going to apply this patch, but it probably makes sense to respin the
first patch first as suggested to avoid me fixing up the conflicts and then
you fixing up the new conflict.

So can you respin patch 1,2 as suggested and add the acks/rb for patch 2?

> ---
>  arch/s390/kvm/guestdbg.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index b68db4b..8f886ee 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -217,16 +217,9 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
> 
>  	size = dbg->arch.nr_hw_bp * sizeof(*bp_data);
> -	bp_data = kmalloc(size, GFP_KERNEL);
> -	if (!bp_data) {
> -		ret = -ENOMEM;
> -		goto error;
> -	}
> -
> -	if (copy_from_user(bp_data, dbg->arch.hw_bp, size)) {
> -		ret = -EFAULT;
> -		goto error;
> -	}
> +	bp_data = memdup_user(dbg->arch.hw_bp, size);
> +	if (IS_ERR(bp_data))
> +		return PTR_ERR(bp_data);
> 
>  	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
>  		switch (bp_data[i].type) {
> 

[toc] | [prev] | [next] | [standalone]

#1469638 — [PATCH v2 0/2] KVM: s390: Fine-tuning for kvm_s390_import_bp_data()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Aug 2016 20:20:02 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Improve determination of sizes
  Use memdup_user() rather than duplicating code

 arch/s390/kvm/guestdbg.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1469645 — [PATCH v2 1/2] KVM: s390: Improve determination of sizes in kvm_s390_import_bp_data()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Aug 2016 19:45:23 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kmalloc_array".

  Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

  This issue was detected also by using the Coccinelle software.

* 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.

* Delete the local variable "size" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2: Rebased on source files from "Linux next-20160824".
    Advices were integrated from source code review.

 arch/s390/kvm/guestdbg.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index d1f8241..70b71ac 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -206,7 +206,7 @@ static int __import_wp_info(struct kvm_vcpu *vcpu,
 int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			    struct kvm_guest_debug *dbg)
 {
-	int ret = 0, nr_wp = 0, nr_bp = 0, i, size;
+	int ret = 0, nr_wp = 0, nr_bp = 0, i;
 	struct kvm_hw_breakpoint *bp_data = NULL;
 	struct kvm_hw_wp_info_arch *wp_info = NULL;
 	struct kvm_hw_bp_info_arch *bp_info = NULL;
@@ -216,14 +216,17 @@ 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);
-	bp_data = kmalloc(size, GFP_KERNEL);
+	bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
+				sizeof(*bp_data),
+				GFP_KERNEL);
 	if (!bp_data) {
 		ret = -ENOMEM;
 		goto error;
 	}
 
-	if (copy_from_user(bp_data, dbg->arch.hw_bp, size)) {
+	if (copy_from_user(bp_data,
+			   dbg->arch.hw_bp,
+			   sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
 		ret = -EFAULT;
 		goto error;
 	}
@@ -241,17 +244,19 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	size = nr_wp * sizeof(struct kvm_hw_wp_info_arch);
-	if (size > 0) {
-		wp_info = kmalloc(size, GFP_KERNEL);
+	if (nr_wp > 0) {
+		wp_info = kmalloc_array(nr_wp,
+					sizeof(*wp_info),
+					GFP_KERNEL);
 		if (!wp_info) {
 			ret = -ENOMEM;
 			goto error;
 		}
 	}
-	size = nr_bp * sizeof(struct kvm_hw_bp_info_arch);
-	if (size > 0) {
-		bp_info = kmalloc(size, GFP_KERNEL);
+	if (nr_bp > 0) {
+		bp_info = kmalloc_array(nr_bp,
+					sizeof(*bp_info),
+					GFP_KERNEL);
 		if (!bp_info) {
 			ret = -ENOMEM;
 			goto error;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1469649 — [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Aug 2016 20:10:09 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2: Rebased on source files from "Linux next-20160824".

 arch/s390/kvm/guestdbg.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 70b71ac..d7c6a7f 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -216,20 +216,10 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
 		return -EINVAL;
 
-	bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
-				sizeof(*bp_data),
-				GFP_KERNEL);
-	if (!bp_data) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	if (copy_from_user(bp_data,
-			   dbg->arch.hw_bp,
-			   sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
-		ret = -EFAULT;
-		goto error;
-	}
+	bp_data = memdup_user(dbg->arch.hw_bp,
+			      sizeof(*bp_data) * dbg->arch.nr_hw_bp);
+	if (IS_ERR(bp_data))
+		return PTR_ERR(bp_data);
 
 	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
 		switch (bp_data[i].type) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1464703 — [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 19:25:50 +0200

The kfree() function was called in a few cases by the
kvm_s390_import_bp_data() function during error handling
even if a passed variable contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 8f886ee..f2514af 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -239,7 +239,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		wp_info = kmalloc(size, GFP_KERNEL);
 		if (!wp_info) {
 			ret = -ENOMEM;
-			goto error;
+			goto free_bp_data;
 		}
 	}
 	size = nr_bp * sizeof(*bp_info);
@@ -247,7 +247,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 		bp_info = kmalloc(size, GFP_KERNEL);
 		if (!bp_info) {
 			ret = -ENOMEM;
-			goto error;
+			goto free_wp_info;
 		}
 	}
 
@@ -257,7 +257,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			ret = __import_wp_info(vcpu, &bp_data[i],
 					       &wp_info[nr_wp]);
 			if (ret)
-				goto error;
+				goto free_bp_info;
 			nr_wp++;
 			break;
 		case KVM_HW_BP:
@@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
 	vcpu->arch.guestdbg.hw_wp_info = wp_info;
 	return 0;
-error:
-	kfree(bp_data);
-	kfree(wp_info);
+free_bp_info:
 	kfree(bp_info);
+free_wp_info:
+	kfree(wp_info);
+free_bp_data:
+	kfree(bp_data);
 	return ret;
 }
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1467594 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:25:50 +0200
> 
> The kfree() function was called in a few cases by the
> kvm_s390_import_bp_data() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 

Nack, we don't need micro optimization for error handling code. Adding more
jump labels is never a good idea, it just increases complexity.

David

[toc] | [prev] | [next] | [standalone]

#1467605 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

On Wed, 17 Aug 2016 20:10:37 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:25:50 +0200
> 
> The kfree() function was called in a few cases by the
> kvm_s390_import_bp_data() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.

NACK.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 

> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>  	return 0;
> -error:
> -	kfree(bp_data);
> -	kfree(wp_info);
> +free_bp_info:
>  	kfree(bp_info);
> +free_wp_info:
> +	kfree(wp_info);
> +free_bp_data:
> +	kfree(bp_data);
>  	return ret;
>  }
> 

This replaces a perfectly fine fallthrough with some horrible labels.
Please don't do that.

[toc] | [prev] | [next] | [standalone]

#1467816 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>>  	return 0;
>> -error:
>> -	kfree(bp_data);
>> -	kfree(wp_info);
>> +free_bp_info:
>>  	kfree(bp_info);
>> +free_wp_info:
>> +	kfree(wp_info);
>> +free_bp_data:
>> +	kfree(bp_data);
>>  	return ret;
>>  }
>>
> 
> This replaces a perfectly fine fallthrough

The usage of a single goto label like "error" seems to be convenient.
But how do these habits fit to the current Linux coding style convention?


> with some horrible labels.

Do they explain better which processing steps should be performed
for an efficient exception handling in this function implementation?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1467995 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

On Mon, 22 Aug 2016 18:56:47 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
> >>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
> >>  	return 0;
> >> -error:
> >> -	kfree(bp_data);
> >> -	kfree(wp_info);
> >> +free_bp_info:
> >>  	kfree(bp_info);
> >> +free_wp_info:
> >> +	kfree(wp_info);
> >> +free_bp_data:
> >> +	kfree(bp_data);
> >>  	return ret;
> >>  }
> >>
> > 
> > This replaces a perfectly fine fallthrough
> 
> The usage of a single goto label like "error" seems to be convenient.
> But how do these habits fit to the current Linux coding style convention?
> 
> 
> > with some horrible labels.
> 
> Do they explain better which processing steps should be performed
> for an efficient exception handling in this function implementation?

*sigh*

It's _exception handling_. It does not need to be "efficient", it needs
to be easily parsable by humans. If in doubt, the compiler will be
_much_ better at optimizing that kind of stuff anyway.

So still NACK.

[toc] | [prev] | [next] | [standalone]

#1468060 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

>>>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>>>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>>>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>>>>  	return 0;
>>>> -error:
>>>> -	kfree(bp_data);
>>>> -	kfree(wp_info);
>>>> +free_bp_info:
>>>>  	kfree(bp_info);
>>>> +free_wp_info:
>>>> +	kfree(wp_info);
>>>> +free_bp_data:
>>>> +	kfree(bp_data);
>>>>  	return ret;
>>>>  }
>>>>
>>>
>>> This replaces a perfectly fine fallthrough
>>
>> The usage of a single goto label like "error" seems to be convenient.
>> But how do these habits fit to the current Linux coding style convention?
>>
>>
>>> with some horrible labels.
>>
>> Do they explain better which processing steps should be performed
>> for an efficient exception handling in this function implementation?
> 
> *sigh*
> 
> It's _exception handling_. It does not need to be "efficient",

I imagine that run time situations could evolve where software efficiency
will also matter for this purpose.


> it needs to be easily parsable by humans.

I guess that we have got different preferences for this detail.


> If in doubt, the compiler will be _much_ better at optimizing
> that kind of stuff anyway.

Which compiler (or optimizer) implementation is capable to restructure
the jump targets for you automatically in the way I propose here?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1468068 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

On Mon, 22 Aug 2016 23:17:26 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >>>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
> >>>>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
> >>>>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
> >>>>  	return 0;
> >>>> -error:
> >>>> -	kfree(bp_data);
> >>>> -	kfree(wp_info);
> >>>> +free_bp_info:
> >>>>  	kfree(bp_info);
> >>>> +free_wp_info:
> >>>> +	kfree(wp_info);
> >>>> +free_bp_data:
> >>>> +	kfree(bp_data);
> >>>>  	return ret;
> >>>>  }
> >>>>
> >>>
> >>> This replaces a perfectly fine fallthrough
> >>
> >> The usage of a single goto label like "error" seems to be convenient.
> >> But how do these habits fit to the current Linux coding style convention?
> >>
> >>
> >>> with some horrible labels.
> >>
> >> Do they explain better which processing steps should be performed
> >> for an efficient exception handling in this function implementation?
> > 
> > *sigh*
> > 
> > It's _exception handling_. It does not need to be "efficient",
> 
> I imagine that run time situations could evolve where software efficiency
> will also matter for this purpose.

*major sigh*

We can start to optimize error handling that should never run after we
fixed every other performance problem that we have. Not earlier.

> 
> 
> > it needs to be easily parsable by humans.
> 
> I guess that we have got different preferences for this detail.

And I'm maintainer for this code.

> 
> 
> > If in doubt, the compiler will be _much_ better at optimizing
> > that kind of stuff anyway.
> 
> Which compiler (or optimizer) implementation is capable to restructure
> the jump targets for you automatically in the way I propose here?

No, please stop right here. NACK. EOD.

[toc] | [prev] | [next] | [standalone]

#1469530 — Re: [PATCH 3/4] KVM-S390: Less function calls in kvm_s390_import_bp_data() after error detection

On 08/17/2016 02:10 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:25:50 +0200
> 
> The kfree() function was called in a few cases by the
> kvm_s390_import_bp_data() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 8f886ee..f2514af 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -239,7 +239,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		wp_info = kmalloc(size, GFP_KERNEL);
>  		if (!wp_info) {
>  			ret = -ENOMEM;
> -			goto error;
> +			goto free_bp_data;
>  		}
>  	}
>  	size = nr_bp * sizeof(*bp_info);
> @@ -247,7 +247,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  		bp_info = kmalloc(size, GFP_KERNEL);
>  		if (!bp_info) {
>  			ret = -ENOMEM;
> -			goto error;
> +			goto free_wp_info;
>  		}
>  	}
> 
> @@ -257,7 +257,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  			ret = __import_wp_info(vcpu, &bp_data[i],
>  					       &wp_info[nr_wp]);
>  			if (ret)
> -				goto error;
> +				goto free_bp_info;
>  			nr_wp++;
>  			break;
>  		case KVM_HW_BP:
> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
>  	return 0;
> -error:
> -	kfree(bp_data);
> -	kfree(wp_info);
> +free_bp_info:
>  	kfree(bp_info);
> +free_wp_info:
> +	kfree(wp_info);
> +free_bp_data:
> +	kfree(bp_data);
>  	return ret;
>  }

I agree with Cornelia, while it seems correct from a technical point of view, it will
make the code harder to maintain. For example if we ever add a new malloc and remove 
another one over time we would need to reshuffle the labels and this did went wrong
several times in the past.

Christian

[toc] | [prev] | [next] | [standalone]

#1464704 — [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a buffer variable

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 17 Aug 2016 19:28:15 +0200

The variable "bp_data" will eventually be set to an appropriate pointer
from a call of the memdup_user() function.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/guestdbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index f2514af..ad04609 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -207,7 +207,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			    struct kvm_guest_debug *dbg)
 {
 	int ret = 0, nr_wp = 0, nr_bp = 0, i, size;
-	struct kvm_hw_breakpoint *bp_data = NULL;
+	struct kvm_hw_breakpoint *bp_data;
 	struct kvm_hw_wp_info_arch *wp_info = NULL;
 	struct kvm_hw_bp_info_arch *bp_info = NULL;
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1467599 — Re: [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a buffer variable

On Wed, 17 Aug 2016 20:12:15 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:28:15 +0200
> 
> The variable "bp_data" will eventually be set to an appropriate pointer
> from a call of the memdup_user() function.
> Thus omit the explicit initialisation which became unnecessary with
> a previous update step.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kvm/guestdbg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index f2514af..ad04609 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -207,7 +207,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  			    struct kvm_guest_debug *dbg)
>  {
>  	int ret = 0, nr_wp = 0, nr_bp = 0, i, size;
> -	struct kvm_hw_breakpoint *bp_data = NULL;
> +	struct kvm_hw_breakpoint *bp_data;
>  	struct kvm_hw_wp_info_arch *wp_info = NULL;
>  	struct kvm_hw_bp_info_arch *bp_info = NULL;
> 

NACK.

[toc] | [prev] | [next] | [standalone]

#1467600 — Re: [PATCH 4/4] KVM-S390: Delete an unnecessary initialisation for a buffer variable

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 17 Aug 2016 19:28:15 +0200
> 
> The variable "bp_data" will eventually be set to an appropriate pointer
> from a call of the memdup_user() function.
> Thus omit the explicit initialisation which became unnecessary with
> a previous update step.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Looks sane to me.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

Thanks!

David

[toc] | [prev] | [next] | [standalone]

#1464705 — [PATCH 1/4] KVM-S390: Improve determination of sizes in kvm_s390_import_bp_data()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web