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

Groups > linux.kernel > #1450783 > unrolled thread

[PATCH 00/12] staging-Lustre: Fine-tuning for seven function implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 00/12] staging-Lustre: Fine-tuning for seven function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:00 +0200
    [PATCH 01/12] staging/lustre/ldlm: Delete unnecessary checks before  the function call "kset_unregister" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:00 +0200
    [PATCH 04/12] staging: lustre: Split a condition check in  class_register_type() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
    [PATCH 06/12] staging: lustre: Return directly after a failed  kcalloc() in mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
    [PATCH 02/12] staging: lustre: Delete unnecessary checks before the  function call "kobject_put" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
    [PATCH 07/12] staging: lustre: Less checks after a failed  alloc_page() in mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
    [PATCH 08/12] staging: lustre: Less checks after a failed  ptlrpc_request_alloc() in mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
    [PATCH 05/12] staging: lustre: Optimize error handling in  class_register_type() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
      Re: [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type() Oleg Drokin <oleg.drokin@intel.com> - 2016-07-26 21:20 +0200
        Re: [lustre-devel] [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type() Oleg Drokin <oleg.drokin@intel.com> - 2016-07-26 21:20 +0200
        Re: staging: lustre: Optimize error handling in class_register_type() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 22:20 +0200
    Re: [PATCH 03/12] staging: lustre: One function call less in class_register_type() after error detection Oleg Drokin <oleg.drokin@intel.com> - 2016-07-26 21:10 +0200
      Re: staging: lustre: One function call less in class_register_type()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 22:00 +0200
        Re: staging: lustre: One function call less in class_register_type() after error detection Oleg Drokin <oleg.drokin@intel.com> - 2016-07-27 00:00 +0200
          Re: staging: lustre: One function call less in class_register_type()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-28 08:00 +0200
            Re: [lustre-devel] staging: lustre: One function call less in class_register_type() after error detection Oleg Drokin <oleg.drokin@intel.com> - 2016-07-29 17:30 +0200
              Re: [lustre-devel] staging: lustre: One function call less in  class_register_type() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-30 08:30 +0200
    [PATCH 03/12] staging: lustre: One function call less in  class_register_type() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:10 +0200
    [PATCH 12/12] staging: lustre: Delete an unnecessary variable  initialisation in mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:20 +0200
    [PATCH 11/12] staging: lustre: Move an assignment for the variable  "eof" in mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:20 +0200
    [PATCH 10/12] staging: lustre: Rename jump labels in  mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:20 +0200
    [PATCH 09/12] staging: lustre: Delete a check for the variable "req"  in mgc_process_recover_log() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-26 21:20 +0200

Page 1 of 2  [1] 2  Next page →

#1450783 — [PATCH 00/12] staging-Lustre: Fine-tuning for seven function implementations

From: Markus Elfring <elfring@users.sourceforge.net>

Further update suggestions were taken into account
after a patch was applied from static source code analysis.

Markus Elfring (12):
  ldlm: Delete unnecessary checks before the function call "kset_unregister"
  Delete unnecessary checks before the function call "kobject_put"
  One function call less in class_register_type() after error detection
  Split a condition check in class_register_type()
  Optimize error handling in class_register_type()
  Return directly after a failed kcalloc() in mgc_process_recover_log()
  Less checks after a failed alloc_page() in mgc_process_recover_log()
  Less checks after a failed ptlrpc_request_alloc() inmgc_process_recover_log()
  Delete a check for the variable "req" in mgc_process_recover_log()
  Rename jump labels in mgc_process_recover_log()
  Move an assignment for the variable "eof" in mgc_process_recover_log()
  Delete an unnecessary variable initialisation in mgc_process_recover_log()

 drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 10 ++---
 drivers/staging/lustre/lustre/lmv/lmv_obd.c     |  5 +--
 drivers/staging/lustre/lustre/lov/lov_obd.c     |  4 +-
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 50 ++++++++++++-------------
 drivers/staging/lustre/lustre/obdclass/genops.c | 41 ++++++++++++--------
 5 files changed, 54 insertions(+), 56 deletions(-)

-- 
2.9.2

[toc] | [next] | [standalone]

#1450789 — [PATCH 01/12] staging/lustre/ldlm: Delete unnecessary checks before the function call "kset_unregister"

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 11:33:43 +0200

The kset_unregister() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
index 821939f..2c1c2fc 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
@@ -1067,10 +1067,8 @@ static int ldlm_cleanup(void)
 	if (ldlm_state->ldlm_cb_service)
 		ptlrpc_unregister_service(ldlm_state->ldlm_cb_service);
 
-	if (ldlm_ns_kset)
-		kset_unregister(ldlm_ns_kset);
-	if (ldlm_svc_kset)
-		kset_unregister(ldlm_svc_kset);
+	kset_unregister(ldlm_ns_kset);
+	kset_unregister(ldlm_svc_kset);
 	if (ldlm_kobj)
 		kobject_put(ldlm_kobj);
 
-- 
2.9.2

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

#1450793 — [PATCH 04/12] staging: lustre: Split a condition check in class_register_type()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 14:10:55 +0200

The kfree() function was called in up to three cases by the
class_register_type() function during error handling even if
the passed data structure element contained a null pointer.

* Split a condition check for memory allocation failures.

* Improve this implementation detail by the introduction of a few
  jump labels.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/obdclass/genops.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 10dd145..fd5e61f 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -158,13 +158,22 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 		return rc;
 
 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
+	if (!type->typ_dt_ops) {
+		rc = -ENOMEM;
+		goto free_type;
+	}
+
 	type->typ_md_ops = kzalloc(sizeof(*type->typ_md_ops), GFP_NOFS);
-	type->typ_name = kzalloc(strlen(name) + 1, GFP_NOFS);
+	if (!type->typ_dt_ops) {
+		rc = -ENOMEM;
+		goto free_dt_ops;
+	}
 
-	if (!type->typ_dt_ops ||
-	    !type->typ_md_ops ||
-	    !type->typ_name)
-		goto free_name;
+	type->typ_name = kzalloc(strlen(name) + 1, GFP_NOFS);
+	if (!type->typ_name) {
+		rc = -ENOMEM;
+		goto free_md_ops;
+	}
 
 	*(type->typ_dt_ops) = *dt_ops;
 	/* md_ops is optional */
@@ -205,8 +214,11 @@ put_object:
 	kobject_put(type->typ_kobj);
 free_name:
 	kfree(type->typ_name);
+free_md_ops:
 	kfree(type->typ_md_ops);
+free_dt_ops:
 	kfree(type->typ_dt_ops);
+free_type:
 	kfree(type);
 	return rc;
 }
-- 
2.9.2

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

#1450795 — [PATCH 06/12] staging: lustre: Return directly after a failed kcalloc() in mgc_process_recover_log()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 16:32:31 +0200

Return directly after a memory allocation failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index 9d0bd47..d716bb2 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1339,10 +1339,8 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		nrpages = CONFIG_READ_NRPAGES_INIT;
 
 	pages = kcalloc(nrpages, sizeof(*pages), GFP_KERNEL);
-	if (!pages) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!pages)
+		return -ENOMEM;
 
 	for (i = 0; i < nrpages; i++) {
 		pages[i] = alloc_page(GFP_KERNEL);
-- 
2.9.2

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

#1450796 — [PATCH 02/12] staging: lustre: Delete unnecessary checks before the function call "kobject_put"

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 13:00:32 +0200

The kobject_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 4 +---
 drivers/staging/lustre/lustre/lmv/lmv_obd.c     | 5 ++---
 drivers/staging/lustre/lustre/lov/lov_obd.c     | 4 +---
 drivers/staging/lustre/lustre/obdclass/genops.c | 6 ++----
 4 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
index 2c1c2fc..52c5dd4 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
@@ -1069,9 +1069,7 @@ static int ldlm_cleanup(void)
 
 	kset_unregister(ldlm_ns_kset);
 	kset_unregister(ldlm_svc_kset);
-	if (ldlm_kobj)
-		kobject_put(ldlm_kobj);
-
+	kobject_put(ldlm_kobj);
 	ldlm_debugfs_cleanup();
 
 	kfree(ldlm_state);
diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
index 0e1588a..8c2e5b3 100644
--- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
+++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
@@ -238,7 +238,7 @@ static int lmv_connect(const struct lu_env *env,
 	if (data && data->ocd_connect_flags & OBD_CONNECT_REAL)
 		rc = lmv_check_connect(obd);
 
-	if (rc && lmv->lmv_tgts_kobj)
+	if (rc)
 		kobject_put(lmv->lmv_tgts_kobj);
 
 	return rc;
@@ -648,8 +648,7 @@ static int lmv_disconnect(struct obd_export *exp)
 		lmv_disconnect_mdc(obd, lmv->tgts[i]);
 	}
 
-	if (lmv->lmv_tgts_kobj)
-		kobject_put(lmv->lmv_tgts_kobj);
+	kobject_put(lmv->lmv_tgts_kobj);
 
 out_local:
 	/*
diff --git a/drivers/staging/lustre/lustre/lov/lov_obd.c b/drivers/staging/lustre/lustre/lov/lov_obd.c
index 9b92d55..df701f7 100644
--- a/drivers/staging/lustre/lustre/lov/lov_obd.c
+++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
@@ -106,9 +106,7 @@ static void lov_putref(struct obd_device *obd)
 			__lov_del_obd(obd, tgt);
 		}
 
-		if (lov->lov_tgts_kobj)
-			kobject_put(lov->lov_tgts_kobj);
-
+		kobject_put(lov->lov_tgts_kobj);
 	} else {
 		mutex_unlock(&lov->lov_lock);
 	}
diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 99c2da6..1b5aa9b 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -203,8 +203,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	return 0;
 
  failed:
-	if (type->typ_kobj)
-		kobject_put(type->typ_kobj);
+	kobject_put(type->typ_kobj);
 	kfree(type->typ_name);
 	kfree(type->typ_md_ops);
 	kfree(type->typ_dt_ops);
@@ -231,8 +230,7 @@ int class_unregister_type(const char *name)
 		return -EBUSY;
 	}
 
-	if (type->typ_kobj)
-		kobject_put(type->typ_kobj);
+	kobject_put(type->typ_kobj);
 
 	if (!IS_ERR_OR_NULL(type->typ_debugfs_entry))
 		ldebugfs_remove(&type->typ_debugfs_entry);
-- 
2.9.2

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

#1450797 — [PATCH 07/12] staging: lustre: Less checks after a failed alloc_page() in mgc_process_recover_log()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 17:12:51 +0200

Release memory directly after a page allocation failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index d716bb2..b064bd3 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1346,7 +1346,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		pages[i] = alloc_page(GFP_KERNEL);
 		if (!pages[i]) {
 			rc = -ENOMEM;
-			goto out;
+			goto free_pages;
 		}
 	}
 
@@ -1461,14 +1461,13 @@ out:
 	if (rc == 0 && !eof)
 		goto again;
 
-	if (pages) {
-		for (i = 0; i < nrpages; i++) {
-			if (!pages[i])
-				break;
-			__free_page(pages[i]);
-		}
-		kfree(pages);
+free_pages:
+	for (i = 0; i < nrpages; i++) {
+		if (!pages[i])
+			break;
+		__free_page(pages[i]);
 	}
+	kfree(pages);
 	return rc;
 }
 
-- 
2.9.2

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

#1450799 — [PATCH 08/12] staging: lustre: Less checks after a failed ptlrpc_request_alloc() in mgc_process_recover_log()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 17:54:24 +0200

Release memory directly after a call of the function
"ptlrpc_request_alloc" failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index b064bd3..f65bb45 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1357,7 +1357,7 @@ again:
 				   &RQF_MGS_CONFIG_READ);
 	if (!req) {
 		rc = -ENOMEM;
-		goto out;
+		goto free_pages;
 	}
 
 	rc = ptlrpc_request_pack(req, LUSTRE_MGS_VERSION, MGS_CONFIG_READ);
-- 
2.9.2

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

#1450800 — [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 14:23:23 +0200

Return a constant error code without storing it in the local variable "rc"
after a failed memory allocation at the beginning of this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/obdclass/genops.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index fd5e61f..4752091 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -152,10 +152,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 		return -EEXIST;
 	}
 
-	rc = -ENOMEM;
 	type = kzalloc(sizeof(*type), GFP_NOFS);
 	if (!type)
-		return rc;
+		return -ENOMEM;
 
 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
 	if (!type->typ_dt_ops) {
-- 
2.9.2

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

#1450812 — Re: [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type()

On Jul 26, 2016, at 3:05 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Jul 2016 14:23:23 +0200
> 
> Return a constant error code without storing it in the local variable "rc"
> after a failed memory allocation at the beginning of this function.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/lustre/lustre/obdclass/genops.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
> index fd5e61f..4752091 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -152,10 +152,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
> 		return -EEXIST;
> 	}
> 
> -	rc = -ENOMEM;

NAK.
when you do this, the next statement below breaks:

> 	type = kzalloc(sizeof(*type), GFP_NOFS);
> 	if (!type)
> -		return rc;
> +		return -ENOMEM;
> 
> 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
> 	if (!type->typ_dt_ops) {
…
                goto failed;

 failed:
…
return rc;

So we are now returning an unitialized rc, did you get a gcc warning about it when compiling?

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

#1450822 — Re: [lustre-devel] [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type()

On Jul 26, 2016, at 3:11 PM, Oleg Drokin wrote:

> 
> On Jul 26, 2016, at 3:05 PM, SF Markus Elfring wrote:
> 
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Tue, 26 Jul 2016 14:23:23 +0200
>> 
>> Return a constant error code without storing it in the local variable "rc"
>> after a failed memory allocation at the beginning of this function.
>> 
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>> drivers/staging/lustre/lustre/obdclass/genops.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
>> index fd5e61f..4752091 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
>> @@ -152,10 +152,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
>> 		return -EEXIST;
>> 	}
>> 
>> -	rc = -ENOMEM;
> 
> NAK.
> when you do this, the next statement below breaks:

Ah, I see there was patch 4 before patch 5 that actually placed rc assignments
everywhere.

So I guess it is fine after all.

Sorry for the noise.

> 
>> 	type = kzalloc(sizeof(*type), GFP_NOFS);
>> 	if (!type)
>> -		return rc;
>> +		return -ENOMEM;
>> 
>> 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
>> 	if (!type->typ_dt_ops) {
> …
>                goto failed;
> 
> failed:
> …
> return rc;
> 
> So we are now returning an unitialized rc, did you get a gcc warning about it when compiling?
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

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

#1450840 — Re: staging: lustre: Optimize error handling in class_register_type()

> NAK.
> when you do this, the next statement below breaks:

I wonder about this conclusion.


>> 	type = kzalloc(sizeof(*type), GFP_NOFS);
>> 	if (!type)
>> -		return rc;
>> +		return -ENOMEM;
>>
>> 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
>> 	if (!type->typ_dt_ops) {
> …
>                 goto failed;
> 
>  failed:
> …
> return rc;
> 
> So we are now returning an unitialized rc, did you get a gcc warning about it when compiling?

I do not get such an impression if my corresponding update suggestion
"[PATCH 04/12] staging: lustre: Split a condition check in class_register_type()"
will be considered for this use case once more.
https://lkml.org/lkml/2016/7/26/462
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1197227.html

Regards,
Markus

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

#1450801 — Re: [PATCH 03/12] staging: lustre: One function call less in class_register_type() after error detection

On Jul 26, 2016, at 3:02 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Jul 2016 13:40:47 +0200
> 
> The kobject_put() function was called in a few cases by the
> class_register_type() function during error handling even if the passed
> data structure element did not contain a pointer for a valid data item.

But kobject_put() already checks for NULL, right? you just submitted
another batch about that in other area.

> Adjust jump targets according to the Linux coding style convention.

Not that I am totally against this patch, but when we do not need the extra
checks, a single jump target is ok too in my mind (extra benefit - there's
not going to be any chance of a mistake to where to jump to).
And when we have a single jump target, there's no supersmart naming
like free_this_and_that_and_that_other_thing_too.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/lustre/lustre/obdclass/genops.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
> index 1b5aa9b..10dd145 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -164,7 +164,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
> 	if (!type->typ_dt_ops ||
> 	    !type->typ_md_ops ||
> 	    !type->typ_name)
> -		goto failed;
> +		goto free_name;
> 
> 	*(type->typ_dt_ops) = *dt_ops;
> 	/* md_ops is optional */
> @@ -180,20 +180,20 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
> 		rc = type->typ_debugfs_entry ? PTR_ERR(type->typ_debugfs_entry)
> 					     : -ENOMEM;
> 		type->typ_debugfs_entry = NULL;
> -		goto failed;
> +		goto free_name;
> 	}
> 
> 	type->typ_kobj = kobject_create_and_add(type->typ_name, lustre_kobj);
> 	if (!type->typ_kobj) {
> 		rc = -ENOMEM;
> -		goto failed;
> +		goto free_name;
> 	}
> 
> 	if (ldt) {
> 		type->typ_lu = ldt;
> 		rc = lu_device_type_init(ldt);
> 		if (rc != 0)
> -			goto failed;
> +			goto put_object;
> 	}
> 
> 	spin_lock(&obd_types_lock);
> @@ -201,9 +201,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
> 	spin_unlock(&obd_types_lock);
> 
> 	return 0;
> -
> - failed:
> +put_object:
> 	kobject_put(type->typ_kobj);
> +free_name:
> 	kfree(type->typ_name);
> 	kfree(type->typ_md_ops);
> 	kfree(type->typ_dt_ops);
> -- 
> 2.9.2

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

#1450833 — Re: staging: lustre: One function call less in class_register_type() after error detection

> But kobject_put() already checks for NULL, right?

Yes. - Such an input parameter validation is performed by the
function implementation.


> you just submitted another batch about that in other area.

I sent update suggestions because of this function property for two
Linux software modules in the year 2015.


>> Adjust jump targets according to the Linux coding style convention.
> 
> Not that I am totally against this patch,

Thanks for your feedback.


> but when we do not need the extra checks, a single jump target is ok too in my mind

A single goto label will look convenient for a while. It will often work
for several use cases.


> (extra benefit - there's not going to be any chance of a mistake to where to jump to).

I have got an other opinion when you would like to care for a bit
more software efficiency.


> And when we have a single jump target, there's no supersmart naming
> like free_this_and_that_and_that_other_thing_too.

How often do you care for efficient exception handling in the shown
function implementations?

Regards,
Markus

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

#1450938 — Re: staging: lustre: One function call less in class_register_type() after error detection

On Jul 26, 2016, at 3:56 PM, SF Markus Elfring wrote:

>> But kobject_put() already checks for NULL, right?
> 
> Yes. - Such an input parameter validation is performed by the
> function implementation.
> 
> 
>> you just submitted another batch about that in other area.
> 
> I sent update suggestions because of this function property for two
> Linux software modules in the year 2015.
> 
> 
>>> Adjust jump targets according to the Linux coding style convention.
>> 
>> Not that I am totally against this patch,
> 
> Thanks for your feedback.
> 
> 
>> but when we do not need the extra checks, a single jump target is ok too in my mind
> 
> A single goto label will look convenient for a while. It will often work
> for several use cases.
> 
> 
>> (extra benefit - there's not going to be any chance of a mistake to where to jump to).
> 
> I have got an other opinion when you would like to care for a bit
> more software efficiency.
> 
> 
>> And when we have a single jump target, there's no supersmart naming
>> like free_this_and_that_and_that_other_thing_too.
> 
> How often do you care for efficient exception handling in the shown
> function implementations?

This function is called several times during lustre module insert.
Namely it's called 5 times for 5 types:
osc, mdc, lov, lmv, mgc.

It's not called any more than that, so it's not exactly a super hot-path function
to overoptimize it, and the failure is presumed to never happen too
(or the module would be non-functional).

I guess you have already did all the work so I don't have any principal objections
here, it's just like I said, in a non-super contended path, a single
"fail" label is probably easier on the developer when they need to add another
check there, as opposed to figuring and possibly adding a correct another
label that would do something sensible.

Thank you for your contributions.

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

#1451733 — Re: staging: lustre: One function call less in class_register_type() after error detection

> This function is called several times during lustre module insert.
> Namely it's called 5 times for 5 types:
> osc, mdc, lov, lmv, mgc.

Will any extra memory accesses matter for the successful execution
in this use case?


> It's not called any more than that, so it's not exactly a super hot-path function
> to overoptimize it, and the failure is presumed to never happen too
> (or the module would be non-functional).

Did the assignment for the local variable "rc" with a well-known error code
influence the run-time characteristics in unwanted ways?
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/lustre/lustre/obdclass/genops.c?id=6a5b99a46bedc2cfbba96dec6d255c4b90af9ff8#n140

Regards,
Markus

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

#1452447 — Re: [lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

On Jul 28, 2016, at 1:53 AM, SF Markus Elfring wrote:

>> This function is called several times during lustre module insert.
>> Namely it's called 5 times for 5 types:
>> osc, mdc, lov, lmv, mgc.
> 
> Will any extra memory accesses matter for the successful execution
> in this use case?

I doubt it.

In typical deployments outside of testing environment, this function is
called 5 times every system boot and never again.

>> It's not called any more than that, so it's not exactly a super hot-path function
>> to overoptimize it, and the failure is presumed to never happen too
>> (or the module would be non-functional).
> 
> Did the assignment for the local variable "rc" with a well-known error code
> influence the run-time characteristics in unwanted ways?
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/lustre/lustre/obdclass/genops.c?id=6a5b99a46bedc2cfbba96dec6d255c4b90af9ff8#n140

I am not sure what do you mean here.

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

#1452657 — Re: [lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

> In typical deployments outside of testing environment, this function is
> called 5 times every system boot and never again.

Does this information mean that a bit more fine-tuning is insignificant
at such a source code place?


>> Did the assignment for the local variable "rc" with a well-known error code
>> influence the run-time characteristics in unwanted ways?
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/lustre/lustre/obdclass/genops.c?id=6a5b99a46bedc2cfbba96dec6d255c4b90af9ff8#n140
> 
> I am not sure what do you mean here.

I suggest to take another look at corresponding implementation details.

An error code is assigned to the variable "rc" before four memory
allocations succeeded so far.
We hope that this function will usually return zero as a constant
for the indication of a successful execution. I find that this variable
should not be touched in the preferred case.
Will such an unnecessary assignment reduce the execution speed a bit
for the desired file system initialisation?

Regards,
Markus

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

#1450802 — [PATCH 03/12] staging: lustre: One function call less in class_register_type() after error detection

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 13:40:47 +0200

The kobject_put() function was called in a few cases by the
class_register_type() function during error handling even if the passed
data structure element did not contain a pointer for a valid data item.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/obdclass/genops.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 1b5aa9b..10dd145 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -164,7 +164,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	if (!type->typ_dt_ops ||
 	    !type->typ_md_ops ||
 	    !type->typ_name)
-		goto failed;
+		goto free_name;
 
 	*(type->typ_dt_ops) = *dt_ops;
 	/* md_ops is optional */
@@ -180,20 +180,20 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 		rc = type->typ_debugfs_entry ? PTR_ERR(type->typ_debugfs_entry)
 					     : -ENOMEM;
 		type->typ_debugfs_entry = NULL;
-		goto failed;
+		goto free_name;
 	}
 
 	type->typ_kobj = kobject_create_and_add(type->typ_name, lustre_kobj);
 	if (!type->typ_kobj) {
 		rc = -ENOMEM;
-		goto failed;
+		goto free_name;
 	}
 
 	if (ldt) {
 		type->typ_lu = ldt;
 		rc = lu_device_type_init(ldt);
 		if (rc != 0)
-			goto failed;
+			goto put_object;
 	}
 
 	spin_lock(&obd_types_lock);
@@ -201,9 +201,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	spin_unlock(&obd_types_lock);
 
 	return 0;
-
- failed:
+put_object:
 	kobject_put(type->typ_kobj);
+free_name:
 	kfree(type->typ_name);
 	kfree(type->typ_md_ops);
 	kfree(type->typ_dt_ops);
-- 
2.9.2

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

#1450809 — [PATCH 12/12] staging: lustre: Delete an unnecessary variable initialisation in mgc_process_recover_log()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 19:50:40 +0200

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index 8f5db79..6e8368e 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1315,7 +1315,7 @@ static int mgc_apply_recover_logs(struct obd_device *mgc,
 static int mgc_process_recover_log(struct obd_device *obd,
 				   struct config_llog_data *cld)
 {
-	struct ptlrpc_request *req = NULL;
+	struct ptlrpc_request *req;
 	struct config_llog_instance *cfg = &cld->cld_cfg;
 	struct mgs_config_body *body;
 	struct mgs_config_res  *res;
-- 
2.9.2

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

#1450815 — [PATCH 11/12] staging: lustre: Move an assignment for the variable "eof" in mgc_process_recover_log()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 19:40:28 +0200

Move the assignment for the local variable "eof" behind the source code
for memory allocations by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index ff60b9b..8f5db79 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1322,7 +1322,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 	struct ptlrpc_bulk_desc *desc;
 	struct page **pages;
 	int nrpages;
-	bool eof = true;
+	bool eof;
 	bool mne_swab;
 	int i;
 	int ealen;
@@ -1350,6 +1350,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		}
 	}
 
+	eof = true;
 again:
 	LASSERT(cld_is_recover(cld));
 	LASSERT(mutex_is_locked(&cld->cld_lock));
-- 
2.9.2

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web