[v4,2/9] dmapool: remove checks for dev == NULL

Message ID	df529b6e-6744-b1af-01ce-a1b691fbcf0d@cybernetics.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <owner-linux-mm@kvack.org> Received-SPF: pass (google.com: domain of btv1==854ac0a7dab==tonyb@cybernetics.com designates 173.71.130.66 as permitted sender) client-ip=173.71.130.66; From: Tony Battersby <tonyb@cybernetics.com> Subject: [PATCH v4 2/9] dmapool: remove checks for dev == NULL To: Matthew Wilcox <willy@infradead.org>, Christoph Hellwig <hch@lst.de>, Marek Szyprowski <m.szyprowski@samsung.com>, iommu@lists.linux-foundation.org, linux-mm@kvack.org dmapool: remove checks for dev == NULL Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org> Message-ID: <df529b6e-6744-b1af-01ce-a1b691fbcf0d@cybernetics.com> Date: Mon, 12 Nov 2018 10:42:12 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: owner-linux-mm@kvack.org Precedence: bulk
Series	mpt3sas and dmapool scalability \| expand [v4,0/9] mpt3sas and dmapool scalability [v4,1/9] dmapool: fix boundary comparison [v4,2/9] dmapool: remove checks for dev == NULL [v4,3/9] dmapool: cleanup dma_pool_destroy [v4,4/9] dmapool: improve scalability of dma_pool_alloc [v4,5/9] dmapool: rename fields in dma_page [v4,6/9] dmapool: improve scalability of dma_pool_free [v4,7/9] dmapool: cleanup integer types [v4,8/9] dmapool: improve accuracy of debug statistics [v4,9/9] dmapool: debug: prevent endless loop in case of corruption

Message ID

df529b6e-6744-b1af-01ce-a1b691fbcf0d@cybernetics.com (mailing list archive)

State

New, archived

Headers

Received-SPF: pass (google.com: domain of
 btv1==854ac0a7dab==tonyb@cybernetics.com designates 173.71.130.66 as
 permitted sender) client-ip=173.71.130.66;
From: Tony Battersby <tonyb@cybernetics.com>
Subject: [PATCH v4 2/9] dmapool: remove checks for dev == NULL
To: Matthew Wilcox <willy@infradead.org>, Christoph Hellwig <hch@lst.de>,
 Marek Szyprowski <m.szyprowski@samsung.com>,
 iommu@lists.linux-foundation.org, linux-mm@kvack.org
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Message-ID: <df529b6e-6744-b1af-01ce-a1b691fbcf0d@cybernetics.com>
Date: Mon, 12 Nov 2018 10:42:12 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: owner-linux-mm@kvack.org
Precedence: bulk

Series

mpt3sas and dmapool scalability | expand

Commit Message

Tony Battersby Nov. 12, 2018, 3:42 p.m. UTC

dmapool originally tried to support pools without a device because
dma_alloc_coherent() supports allocations without a device.  But nobody
ended up using dma pools without a device, so the current checks in
dmapool.c for pool->dev == NULL are both insufficient and causing bloat.
Remove them.

Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
---

Comments

John Garry Nov. 12, 2018, 4:32 p.m. UTC | #1

On 12/11/2018 15:42, Tony Battersby wrote:
> dmapool originally tried to support pools without a device because
> dma_alloc_coherent() supports allocations without a device.  But nobody
> ended up using dma pools without a device, so the current checks in
> dmapool.c for pool->dev == NULL are both insufficient and causing bloat.
> Remove them.
>

As an aside, is it right that dma_pool_create() does not actually reject 
dev==NULL and would crash from a NULL-pointer dereference?

Thanks,
John

> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
> ---
> --- linux/mm/dmapool.c.orig	2018-08-03 16:12:23.000000000 -0400
> +++ linux/mm/dmapool.c	2018-08-03 16:13:44.000000000 -0400
> @@ -277,7 +277,7 @@ void dma_pool_destroy(struct dma_pool *p
>  	mutex_lock(&pools_reg_lock);
>  	mutex_lock(&pools_lock);
>  	list_del(&pool->pools);
> -	if (pool->dev && list_empty(&pool->dev->dma_pools))
> +	if (list_empty(&pool->dev->dma_pools))
>  		empty = true;
>  	mutex_unlock(&pools_lock);
>  	if (empty)
> @@ -289,13 +289,9 @@ void dma_pool_destroy(struct dma_pool *p
>  		page = list_entry(pool->page_list.next,
>  				  struct dma_page, page_list);
>  		if (is_page_busy(page)) {
> -			if (pool->dev)
> -				dev_err(pool->dev,
> -					"dma_pool_destroy %s, %p busy\n",
> -					pool->name, page->vaddr);
> -			else
> -				pr_err("dma_pool_destroy %s, %p busy\n",
> -				       pool->name, page->vaddr);
> +			dev_err(pool->dev,
> +				"dma_pool_destroy %s, %p busy\n",
> +				pool->name, page->vaddr);
>  			/* leak the still-in-use consistent memory */
>  			list_del(&page->page_list);
>  			kfree(page);
> @@ -357,13 +353,9 @@ void *dma_pool_alloc(struct dma_pool *po
>  		for (i = sizeof(page->offset); i < pool->size; i++) {
>  			if (data[i] == POOL_POISON_FREED)
>  				continue;
> -			if (pool->dev)
> -				dev_err(pool->dev,
> -					"dma_pool_alloc %s, %p (corrupted)\n",
> -					pool->name, retval);
> -			else
> -				pr_err("dma_pool_alloc %s, %p (corrupted)\n",
> -					pool->name, retval);
> +			dev_err(pool->dev,
> +				"dma_pool_alloc %s, %p (corrupted)\n",
> +				pool->name, retval);
>
>  			/*
>  			 * Dump the first 4 bytes even if they are not
> @@ -418,13 +410,9 @@ void dma_pool_free(struct dma_pool *pool
>  	page = pool_find_page(pool, dma);
>  	if (!page) {
>  		spin_unlock_irqrestore(&pool->lock, flags);
> -		if (pool->dev)
> -			dev_err(pool->dev,
> -				"dma_pool_free %s, %p/%lx (bad dma)\n",
> -				pool->name, vaddr, (unsigned long)dma);
> -		else
> -			pr_err("dma_pool_free %s, %p/%lx (bad dma)\n",
> -			       pool->name, vaddr, (unsigned long)dma);
> +		dev_err(pool->dev,
> +			"dma_pool_free %s, %p/%lx (bad dma)\n",
> +			pool->name, vaddr, (unsigned long)dma);
>  		return;
>  	}
>
> @@ -432,13 +420,9 @@ void dma_pool_free(struct dma_pool *pool
>  #ifdef	DMAPOOL_DEBUG
>  	if ((dma - page->dma) != offset) {
>  		spin_unlock_irqrestore(&pool->lock, flags);
> -		if (pool->dev)
> -			dev_err(pool->dev,
> -				"dma_pool_free %s, %p (bad vaddr)/%pad\n",
> -				pool->name, vaddr, &dma);
> -		else
> -			pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n",
> -			       pool->name, vaddr, &dma);
> +		dev_err(pool->dev,
> +			"dma_pool_free %s, %p (bad vaddr)/%pad\n",
> +			pool->name, vaddr, &dma);
>  		return;
>  	}
>  	{
> @@ -449,12 +433,9 @@ void dma_pool_free(struct dma_pool *pool
>  				continue;
>  			}
>  			spin_unlock_irqrestore(&pool->lock, flags);
> -			if (pool->dev)
> -				dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n",
> -					pool->name, &dma);
> -			else
> -				pr_err("dma_pool_free %s, dma %pad already free\n",
> -				       pool->name, &dma);
> +			dev_err(pool->dev,
> +				"dma_pool_free %s, dma %pad already free\n",
> +				pool->name, &dma);
>  			return;
>  		}
>  	}
>
>
> .
>

Tony Battersby Nov. 12, 2018, 5:06 p.m. UTC | #2

On 11/12/18 11:32 AM, John Garry wrote:
> On 12/11/2018 15:42, Tony Battersby wrote:
>> dmapool originally tried to support pools without a device because
>> dma_alloc_coherent() supports allocations without a device.  But nobody
>> ended up using dma pools without a device, so the current checks in
>> dmapool.c for pool->dev == NULL are both insufficient and causing bloat.
>> Remove them.
>>
> As an aside, is it right that dma_pool_create() does not actually reject 
> dev==NULL and would crash from a NULL-pointer dereference?
>
> Thanks,
> John
>
When passed a NULL dev, dma_pool_create() will already crash with a
NULL-pointer dereference before this patch series, because it checks for
dev == NULL in some places but not others.  Specifically, it will crash
in one of these two places in dma_pool_create():

	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
-or-
	if (list_empty(&dev->dma_pools))

So removing the checks for dev == NULL will not make previously-working
code to start crashing suddenly.  And since passing dev == NULL would be
an API misuse error and not a runtime error, I would rather not add a
new check to reject it.

Tony

Matthew Wilcox Nov. 13, 2018, 6:14 a.m. UTC | #3

On Mon, Nov 12, 2018 at 10:42:12AM -0500, Tony Battersby wrote:
> dmapool originally tried to support pools without a device because
> dma_alloc_coherent() supports allocations without a device.  But nobody
> ended up using dma pools without a device, so the current checks in
> dmapool.c for pool->dev == NULL are both insufficient and causing bloat.
> Remove them.
> 
> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>

Acked-by: Matthew Wilcox <willy@infradead.org>

--- linux/mm/dmapool.c.orig	2018-08-03 16:12:23.000000000 -0400
+++ linux/mm/dmapool.c	2018-08-03 16:13:44.000000000 -0400
@@ -277,7 +277,7 @@  void dma_pool_destroy(struct dma_pool *p
 	mutex_lock(&pools_reg_lock);
 	mutex_lock(&pools_lock);
 	list_del(&pool->pools);
-	if (pool->dev && list_empty(&pool->dev->dma_pools))
+	if (list_empty(&pool->dev->dma_pools))
 		empty = true;
 	mutex_unlock(&pools_lock);
 	if (empty)
@@ -289,13 +289,9 @@  void dma_pool_destroy(struct dma_pool *p
 		page = list_entry(pool->page_list.next,
 				  struct dma_page, page_list);
 		if (is_page_busy(page)) {
-			if (pool->dev)
-				dev_err(pool->dev,
-					"dma_pool_destroy %s, %p busy\n",
-					pool->name, page->vaddr);
-			else
-				pr_err("dma_pool_destroy %s, %p busy\n",
-				       pool->name, page->vaddr);
+			dev_err(pool->dev,
+				"dma_pool_destroy %s, %p busy\n",
+				pool->name, page->vaddr);
 			/* leak the still-in-use consistent memory */
 			list_del(&page->page_list);
 			kfree(page);
@@ -357,13 +353,9 @@  void *dma_pool_alloc(struct dma_pool *po
 		for (i = sizeof(page->offset); i < pool->size; i++) {
 			if (data[i] == POOL_POISON_FREED)
 				continue;
-			if (pool->dev)
-				dev_err(pool->dev,
-					"dma_pool_alloc %s, %p (corrupted)\n",
-					pool->name, retval);
-			else
-				pr_err("dma_pool_alloc %s, %p (corrupted)\n",
-					pool->name, retval);
+			dev_err(pool->dev,
+				"dma_pool_alloc %s, %p (corrupted)\n",
+				pool->name, retval);
 
 			/*
 			 * Dump the first 4 bytes even if they are not
@@ -418,13 +410,9 @@  void dma_pool_free(struct dma_pool *pool
 	page = pool_find_page(pool, dma);
 	if (!page) {
 		spin_unlock_irqrestore(&pool->lock, flags);
-		if (pool->dev)
-			dev_err(pool->dev,
-				"dma_pool_free %s, %p/%lx (bad dma)\n",
-				pool->name, vaddr, (unsigned long)dma);
-		else
-			pr_err("dma_pool_free %s, %p/%lx (bad dma)\n",
-			       pool->name, vaddr, (unsigned long)dma);
+		dev_err(pool->dev,
+			"dma_pool_free %s, %p/%lx (bad dma)\n",
+			pool->name, vaddr, (unsigned long)dma);
 		return;
 	}
 
@@ -432,13 +420,9 @@  void dma_pool_free(struct dma_pool *pool
 #ifdef	DMAPOOL_DEBUG
 	if ((dma - page->dma) != offset) {
 		spin_unlock_irqrestore(&pool->lock, flags);
-		if (pool->dev)
-			dev_err(pool->dev,
-				"dma_pool_free %s, %p (bad vaddr)/%pad\n",
-				pool->name, vaddr, &dma);
-		else
-			pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n",
-			       pool->name, vaddr, &dma);
+		dev_err(pool->dev,
+			"dma_pool_free %s, %p (bad vaddr)/%pad\n",
+			pool->name, vaddr, &dma);
 		return;
 	}
 	{
@@ -449,12 +433,9 @@  void dma_pool_free(struct dma_pool *pool
 				continue;
 			}
 			spin_unlock_irqrestore(&pool->lock, flags);
-			if (pool->dev)
-				dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n",
-					pool->name, &dma);
-			else
-				pr_err("dma_pool_free %s, dma %pad already free\n",
-				       pool->name, &dma);
+			dev_err(pool->dev,
+				"dma_pool_free %s, dma %pad already free\n",
+				pool->name, &dma);
 			return;
 		}
 	}

[v4,2/9] dmapool: remove checks for dev == NULL

Commit Message

Comments

Patch