[PATCH 0/4] clean up some functions in nvdimm/badrange.c and namespace_devs.c

[PATCH 0/4] clean up some functions in nvdimm/badrange.c and namespace_devs.c

[Zhen Lei]

When I learned the code of drivers/nvdimm, I found some places can be improved.

Zhen Lei (4):
  libnvdimm: remove redundant list_empty() check in badrange.c
  libnvdimm: eliminate a meaningless spinlock operation
  libnvdimm: eliminate two unnecessary zero initializations in
    badrange.c
  libnvdimm: avoid unnecessary judgments in nvdimm_namespace_disk_name()

 drivers/nvdimm/badrange.c       | 36 ++++++++++--------------------------
 drivers/nvdimm/namespace_devs.c | 12 +++++-------
 2 files changed, 15 insertions(+), 33 deletions(-)

-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH 1/4] libnvdimm: remove redundant list_empty() check in badrange.c

[Zhen Lei]

In add_badrange() or badblocks_populate(), the list_empty() branch does
the same things as the code after list_for_each_entry(). And the
list_for_each_entry() will do noting when list_empty().

So the list_empty() branch can be removed without functional change, let
the code after list_for_each_entry() to do it.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/badrange.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
index b9eeefa27e3a507..9fdba8c43e8605e 100644
--- a/drivers/nvdimm/badrange.c
+++ b/drivers/nvdimm/badrange.c
@@ -53,13 +53,6 @@ static int add_badrange(struct badrange *badrange, u64 addr, u64 length)
 	bre_new = kzalloc(sizeof(*bre_new), GFP_KERNEL);
 	spin_lock(&badrange->lock);
 
-	if (list_empty(&badrange->list)) {
-		if (!bre_new)
-			return -ENOMEM;
-		append_badrange_entry(badrange, bre_new, addr, length);
-		return 0;
-	}
-
 	/*
 	 * There is a chance this is a duplicate, check for those first.
 	 * This will be the common case as ARS_STATUS returns all known
@@ -215,9 +208,6 @@ static void badblocks_populate(struct badrange *badrange,
 {
 	struct badrange_entry *bre;
 
-	if (list_empty(&badrange->list))
-		return;
-
 	list_for_each_entry(bre, &badrange->list, list) {
 		u64 bre_end = bre->start + bre->length - 1;
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH 2/4] libnvdimm: eliminate a meaningless spinlock operation

[Zhen Lei]

badrange_add() take the lock "badrange->lock", but it's released
immediately in add_badrange(), protect nothing. Because the static
function add_badrange() is only called by badrange_add(), so spread its
content into badrange_add(), and move "kfree(bre_new)" out of the lock
protection.

Fixes: b3b454f694db ("libnvdimm: fix clear poison locking with spinlock ...")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/badrange.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
index 9fdba8c43e8605e..7f78b659057902d 100644
--- a/drivers/nvdimm/badrange.c
+++ b/drivers/nvdimm/badrange.c
@@ -45,12 +45,12 @@ static int alloc_and_append_badrange_entry(struct badrange *badrange,
 	return 0;
 }
 
-static int add_badrange(struct badrange *badrange, u64 addr, u64 length)
+int badrange_add(struct badrange *badrange, u64 addr, u64 length)
 {
 	struct badrange_entry *bre, *bre_new;
 
-	spin_unlock(&badrange->lock);
 	bre_new = kzalloc(sizeof(*bre_new), GFP_KERNEL);
+
 	spin_lock(&badrange->lock);
 
 	/*
@@ -63,6 +63,7 @@ static int add_badrange(struct badrange *badrange, u64 addr, u64 length)
 			/* If length has changed, update this list entry */
 			if (bre->length != length)
 				bre->length = length;
+			spin_unlock(&badrange->lock);
 			kfree(bre_new);
 			return 0;
 		}
@@ -72,22 +73,15 @@ static int add_badrange(struct badrange *badrange, u64 addr, u64 length)
 	 * as any overlapping ranges will get resolved when the list is consumed
 	 * and converted to badblocks
 	 */
-	if (!bre_new)
+	if (!bre_new) {
+		spin_unlock(&badrange->lock);
 		return -ENOMEM;
-	append_badrange_entry(badrange, bre_new, addr, length);
-
-	return 0;
-}
-
-int badrange_add(struct badrange *badrange, u64 addr, u64 length)
-{
-	int rc;
+	}
 
-	spin_lock(&badrange->lock);
-	rc = add_badrange(badrange, addr, length);
+	append_badrange_entry(badrange, bre_new, addr, length);
 	spin_unlock(&badrange->lock);
 
-	return rc;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(badrange_add);
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH 3/4] libnvdimm: eliminate two unnecessary zero initializations in badrange.c

[Zhen Lei]

Currently, the "struct badrange_entry" has three members: start, length,
list. In append_badrange_entry(), "start" and "length" will be assigned
later, and "list" does not need to be initialized before calling
list_add_tail(). That means, the kzalloc() in badrange_add() or
alloc_and_append_badrange_entry() can be replaced with kmalloc(), because
the zero initialization is not required.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/badrange.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
index 7f78b659057902d..13145001c52ff39 100644
--- a/drivers/nvdimm/badrange.c
+++ b/drivers/nvdimm/badrange.c
@@ -37,7 +37,7 @@ static int alloc_and_append_badrange_entry(struct badrange *badrange,
 {
 	struct badrange_entry *bre;
 
-	bre = kzalloc(sizeof(*bre), flags);
+	bre = kmalloc(sizeof(*bre), flags);
 	if (!bre)
 		return -ENOMEM;
 
@@ -49,7 +49,7 @@ int badrange_add(struct badrange *badrange, u64 addr, u64 length)
 {
 	struct badrange_entry *bre, *bre_new;
 
-	bre_new = kzalloc(sizeof(*bre_new), GFP_KERNEL);
+	bre_new = kmalloc(sizeof(*bre_new), GFP_KERNEL);
 
 	spin_lock(&badrange->lock);
 
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH 3/4] libnvdimm: eliminate two unnecessary zero initializations in badrange.c

[Leizhen (ThunderTown)]

I will drop this patch, because badrange_add() is unlikely to be called.
There's no need to care about trivial performance improvements.

On 2020/8/20 22:30, Zhen Lei wrote:
> Currently, the "struct badrange_entry" has three members: start, length,
> list. In append_badrange_entry(), "start" and "length" will be assigned
> later, and "list" does not need to be initialized before calling
> list_add_tail(). That means, the kzalloc() in badrange_add() or
> alloc_and_append_badrange_entry() can be replaced with kmalloc(), because
> the zero initialization is not required.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/nvdimm/badrange.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
> index 7f78b659057902d..13145001c52ff39 100644
> --- a/drivers/nvdimm/badrange.c
> +++ b/drivers/nvdimm/badrange.c
> @@ -37,7 +37,7 @@ static int alloc_and_append_badrange_entry(struct badrange *badrange,
>  {
>  	struct badrange_entry *bre;
>  
> -	bre = kzalloc(sizeof(*bre), flags);
> +	bre = kmalloc(sizeof(*bre), flags);
>  	if (!bre)
>  		return -ENOMEM;
>  
> @@ -49,7 +49,7 @@ int badrange_add(struct badrange *badrange, u64 addr, u64 length)
>  {
>  	struct badrange_entry *bre, *bre_new;
>  
> -	bre_new = kzalloc(sizeof(*bre_new), GFP_KERNEL);
> +	bre_new = kmalloc(sizeof(*bre_new), GFP_KERNEL);
>  
>  	spin_lock(&badrange->lock);
>  
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH 4/4] libnvdimm: avoid unnecessary judgments in nvdimm_namespace_disk_name()

[Zhen Lei]

suffix ? suffix : "" appears three times, it's easy to get rid of it
by initialize the local variable "suffix" to empty string.

To avoid having rows that exceed 80 columns, add a new local variable
"region_id".

No functional change, but it can reduce the code size.
Before:
   text    data     bss     dec     hex filename
  41749    3697      16   45462    b196 drivers/nvdimm/namespace_devs.o

After:
   text    data     bss     dec     hex filename
  41653    3697      16   45366    b136 drivers/nvdimm/namespace_devs.o

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/nvdimm/namespace_devs.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 6da67f4d641a27c..ef2800c5da4c99c 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -157,7 +157,8 @@ const char *nvdimm_namespace_disk_name(struct nd_namespace_common *ndns,
 		char *name)
 {
 	struct nd_region *nd_region = to_nd_region(ndns->dev.parent);
-	const char *suffix = NULL;
+	const char *suffix = "";
+	int region_id = nd_region->id;
 
 	if (ndns->claim && is_nd_btt(ndns->claim))
 		suffix = "s";
@@ -173,17 +174,14 @@ const char *nvdimm_namespace_disk_name(struct nd_namespace_common *ndns,
 		}
 
 		if (nsidx)
-			sprintf(name, "pmem%d.%d%s", nd_region->id, nsidx,
-					suffix ? suffix : "");
+			sprintf(name, "pmem%d.%d%s", region_id, nsidx, suffix);
 		else
-			sprintf(name, "pmem%d%s", nd_region->id,
-					suffix ? suffix : "");
+			sprintf(name, "pmem%d%s", region_id, suffix);
 	} else if (is_namespace_blk(&ndns->dev)) {
 		struct nd_namespace_blk *nsblk;
 
 		nsblk = to_nd_namespace_blk(&ndns->dev);
-		sprintf(name, "ndblk%d.%d%s", nd_region->id, nsblk->id,
-				suffix ? suffix : "");
+		sprintf(name, "ndblk%d.%d%s", region_id, nsblk->id, suffix);
 	} else {
 		return NULL;
 	}
-- 
1.8.3

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org