[PATCH 0/2] mtip32xx: Fix arch-specific driver issues

[PATCH 0/2] mtip32xx: Fix arch-specific driver issues

[Felipe Franciosi]

This patch series fix a couple of issues with the Micron P320 driver.
The first is an issue on 32-bit architectures where unecessary bouncing of
requests causes really really bad performance. The second regards a problem
on architectures where DMA unmapping has side effects.

[PATCH 1/2] mtip32xx: Don't bounce IO requests

[Felipe Franciosi]

This device support 64-bit DMA and therefore no IO requests needs bouncing.
On 32-bit systems, blk_queue_bounce() will bounce all IO requests with high
mem pages. This makes performance really really bad.

Signed-off-by: Felipe Franciosi <felipe@paradoxo.org>
---
 drivers/block/mtip32xx/mtip32xx.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 5160269..24c87fdb 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -4082,8 +4082,6 @@ static void mtip_make_request(struct request_queue *queue, struct bio *bio)
 
 	sg = mtip_hw_get_scatterlist(dd, &tag, unaligned);
 	if (likely(sg != NULL)) {
-		blk_queue_bounce(queue, &bio);
-
 		if (unlikely((bio)->bi_vcnt > MTIP_MAX_SG)) {
 			dev_warn(&dd->pdev->dev,
 				"Maximum number of SGL entries exceeded\n");
-- 
1.7.10.4

Re: [PATCH 1/2] mtip32xx: Don't bounce IO requests

[Jens Axboe]

On 03/12/2014 10:05 AM, Felipe Franciosi wrote:
> This device support 64-bit DMA and therefore no IO requests needs bouncing.
> On 32-bit systems, blk_queue_bounce() will bounce all IO requests with high
> mem pages. This makes performance really really bad.
>
> Signed-off-by: Felipe Franciosi <felipe@paradoxo.org>
> ---
>   drivers/block/mtip32xx/mtip32xx.c |    2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
> index 5160269..24c87fdb 100644
> --- a/drivers/block/mtip32xx/mtip32xx.c
> +++ b/drivers/block/mtip32xx/mtip32xx.c
> @@ -4082,8 +4082,6 @@ static void mtip_make_request(struct request_queue *queue, struct bio *bio)
>
>   	sg = mtip_hw_get_scatterlist(dd, &tag, unaligned);
>   	if (likely(sg != NULL)) {
> -		blk_queue_bounce(queue, &bio);
> -
>   		if (unlikely((bio)->bi_vcnt > MTIP_MAX_SG)) {
>   			dev_warn(&dd->pdev->dev,
>   				"Maximum number of SGL entries exceeded\n");

That seems to be because the driver forgets to set the DMA capabilities. 
It needs a blk_queue_bounce_limit() call when it sets up the queue.

-- 
Jens Axboe

Re: [PATCH 1/2] mtip32xx: Don't bounce IO requests

[Felipe Franciosi]

On Wed, Mar 12, 2014 at 4:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 03/12/2014 10:05 AM, Felipe Franciosi wrote:
>>
>> This device support 64-bit DMA and therefore no IO requests needs bouncing.
>> On 32-bit systems, blk_queue_bounce() will bounce all IO requests with high
>> mem pages. This makes performance really really bad.
>>
>> Signed-off-by: Felipe Franciosi <felipe@paradoxo.org>
>> ---
>>   drivers/block/mtip32xx/mtip32xx.c |    2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
>> index 5160269..24c87fdb 100644
>> --- a/drivers/block/mtip32xx/mtip32xx.c
>> +++ b/drivers/block/mtip32xx/mtip32xx.c
>> @@ -4082,8 +4082,6 @@ static void mtip_make_request(struct request_queue *queue, struct bio *bio)
>>
>>         sg = mtip_hw_get_scatterlist(dd, &tag, unaligned);
>>         if (likely(sg != NULL)) {
>> -               blk_queue_bounce(queue, &bio);
>> -
>>                 if (unlikely((bio)->bi_vcnt > MTIP_MAX_SG)) {
>>                         dev_warn(&dd->pdev->dev,
>>                                 "Maximum number of SGL entries exceeded\n");
>
>
> That seems to be because the driver forgets to set the DMA capabilities. It needs a blk_queue_bounce_limit() call when it sets up the queue.
>
> --
> Jens Axboe
>

Hmm... I see what you are saying. David, would you like to comment?

Thanks,
Felipe

p.s. resending as GMail was naughty and sent my previous reply in HTML.

Re: [PATCH 1/2] mtip32xx: Don't bounce IO requests

[David Vrabel]

On 12/03/14 17:08, Felipe Franciosi wrote:
> On Wed, Mar 12, 2014 at 4:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 03/12/2014 10:05 AM, Felipe Franciosi wrote:
>>>
>>> This device support 64-bit DMA and therefore no IO requests needs bouncing.
>>> On 32-bit systems, blk_queue_bounce() will bounce all IO requests with high
>>> mem pages. This makes performance really really bad.
[...]
>>> --- a/drivers/block/mtip32xx/mtip32xx.c
>>> +++ b/drivers/block/mtip32xx/mtip32xx.c
>>> @@ -4082,8 +4082,6 @@ static void mtip_make_request(struct request_queue *queue, struct bio *bio)
>>>
>>>         sg = mtip_hw_get_scatterlist(dd, &tag, unaligned);
>>>         if (likely(sg != NULL)) {
>>> -               blk_queue_bounce(queue, &bio);
>>> -
>>>                 if (unlikely((bio)->bi_vcnt > MTIP_MAX_SG)) {
>>>                         dev_warn(&dd->pdev->dev,
>>>                                 "Maximum number of SGL entries exceeded\n");
>>
>>
>> That seems to be because the driver forgets to set the DMA
>> capabilities. It needs a blk_queue_bounce_limit() call when it sets up
>> the queue.
> 
> Hmm... I see what you are saying. David, would you like to comment?

Yes, it should also call blk_queue_bounce_limit(..., BLK_BOUNCE_ANY) in
mtip_block_initialize() as well.

But, only one other driver calls blk_queue_bounce() so it's difficult to
see why this driver would need a call here.

David

Re: [PATCH 1/2] mtip32xx: Don't bounce IO requests

[Jens Axboe]

On 03/12/2014 11:18 AM, David Vrabel wrote:
> On 12/03/14 17:08, Felipe Franciosi wrote:
>> On Wed, Mar 12, 2014 at 4:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 03/12/2014 10:05 AM, Felipe Franciosi wrote:
>>>>
>>>> This device support 64-bit DMA and therefore no IO requests needs bouncing.
>>>> On 32-bit systems, blk_queue_bounce() will bounce all IO requests with high
>>>> mem pages. This makes performance really really bad.
> [...]
>>>> --- a/drivers/block/mtip32xx/mtip32xx.c
>>>> +++ b/drivers/block/mtip32xx/mtip32xx.c
>>>> @@ -4082,8 +4082,6 @@ static void mtip_make_request(struct request_queue *queue, struct bio *bio)
>>>>
>>>>          sg = mtip_hw_get_scatterlist(dd, &tag, unaligned);
>>>>          if (likely(sg != NULL)) {
>>>> -               blk_queue_bounce(queue, &bio);
>>>> -
>>>>                  if (unlikely((bio)->bi_vcnt > MTIP_MAX_SG)) {
>>>>                          dev_warn(&dd->pdev->dev,
>>>>                                  "Maximum number of SGL entries exceeded\n");
>>>
>>>
>>> That seems to be because the driver forgets to set the DMA
>>> capabilities. It needs a blk_queue_bounce_limit() call when it sets up
>>> the queue.
>>
>> Hmm... I see what you are saying. David, would you like to comment?
>
> Yes, it should also call blk_queue_bounce_limit(..., BLK_BOUNCE_ANY) in
> mtip_block_initialize() as well.
>
> But, only one other driver calls blk_queue_bounce() so it's difficult to
> see why this driver would need a call here.

That's because most other drivers hook in the appropriate place, and the 
blk_queue_bounce() is then done by the block layer.

-- 
Jens Axboe

Re: [PATCH 1/2] mtip32xx: Don't bounce IO requests

[Felipe Franciosi]

On Wed, Mar 12, 2014 at 5:19 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 03/12/2014 11:18 AM, David Vrabel wrote:
>>
>> On 12/03/14 17:08, Felipe Franciosi wrote:
>>>
>>> On Wed, Mar 12, 2014 at 4:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>>
>>>> On 03/12/2014 10:05 AM, Felipe Franciosi wrote:
>>>>>
>>>>>
>>>>> This device support 64-bit DMA and therefore no IO requests needs
>>>>> bouncing.
>>>>> On 32-bit systems, blk_queue_bounce() will bounce all IO requests with
>>>>> high
>>>>> mem pages. This makes performance really really bad.
>>
>> [...]
>>>>>
>>>>> --- a/drivers/block/mtip32xx/mtip32xx.c
>>>>> +++ b/drivers/block/mtip32xx/mtip32xx.c
>>>>> @@ -4082,8 +4082,6 @@ static void mtip_make_request(struct
>>>>> request_queue *queue, struct bio *bio)
>>>>>
>>>>>          sg = mtip_hw_get_scatterlist(dd, &tag, unaligned);
>>>>>          if (likely(sg != NULL)) {
>>>>> -               blk_queue_bounce(queue, &bio);
>>>>> -
>>>>>                  if (unlikely((bio)->bi_vcnt > MTIP_MAX_SG)) {
>>>>>                          dev_warn(&dd->pdev->dev,
>>>>>                                  "Maximum number of SGL entries
>>>>> exceeded\n");
>>>>
>>>>
>>>>
>>>> That seems to be because the driver forgets to set the DMA
>>>> capabilities. It needs a blk_queue_bounce_limit() call when it sets up
>>>> the queue.
>>>
>>>
>>> Hmm... I see what you are saying. David, would you like to comment?
>>
>>
>> Yes, it should also call blk_queue_bounce_limit(..., BLK_BOUNCE_ANY) in
>> mtip_block_initialize() as well.
>>
>> But, only one other driver calls blk_queue_bounce() so it's difficult to
>> see why this driver would need a call here.
>
>
> That's because most other drivers hook in the appropriate place, and the
> blk_queue_bounce() is then done by the block layer.
>
> --
> Jens Axboe
>

I'll add the blk_queue_bounce_limit() on the initialisation, test it
out and submit a v2 tomorrow.

Cheers,
Felipe

[PATCH 2/2] mtip32xx: Unmap the DMA segments before completing the IO request

[Felipe Franciosi]

If the buffers are unmapped after completing a request, then stale data
might be in the request.

Signed-off-by: Felipe Franciosi <felipe@paradoxo.org>
---
 drivers/block/mtip32xx/mtip32xx.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 24c87fdb..7e8fe0d 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -266,6 +266,12 @@ static void mtip_async_complete(struct mtip_port *port,
 			"Command tag %d failed due to TFE\n", tag);
 	}
 
+	/* Unmap the DMA scatter list entries */
+	dma_unmap_sg(&dd->pdev->dev,
+		command->sg,
+		command->scatter_ents,
+		command->direction);
+
 	/* Upper layer callback */
 	if (likely(command->async_callback))
 		command->async_callback(command->async_data, cb_status);
@@ -273,12 +279,6 @@ static void mtip_async_complete(struct mtip_port *port,
 	command->async_callback = NULL;
 	command->comp_func = NULL;
 
-	/* Unmap the DMA scatter list entries */
-	dma_unmap_sg(&dd->pdev->dev,
-		command->sg,
-		command->scatter_ents,
-		command->direction);
-
 	/* Clear the allocated and active bits for the command */
 	atomic_set(&port->commands[tag].active, 0);
 	release_slot(port, tag);
@@ -709,6 +709,12 @@ static void mtip_timeout_function(unsigned long int data)
 			 */
 			writel(1 << bit, port->completed[group]);
 
+			/* Unmap the DMA scatter list entries */
+			dma_unmap_sg(&port->dd->pdev->dev,
+					command->sg,
+					command->scatter_ents,
+					command->direction);
+
 			/* Call the async completion callback. */
 			if (likely(command->async_callback))
 				command->async_callback(command->async_data,
@@ -716,12 +722,6 @@ static void mtip_timeout_function(unsigned long int data)
 			command->async_callback = NULL;
 			command->comp_func = NULL;
 
-			/* Unmap the DMA scatter list entries */
-			dma_unmap_sg(&port->dd->pdev->dev,
-					command->sg,
-					command->scatter_ents,
-					command->direction);
-
 			/*
 			 * Clear the allocated bit and active tag for the
 			 * command.
-- 
1.7.10.4

Re: [PATCH 2/2] mtip32xx: Unmap the DMA segments before completing the IO request

[Jens Axboe]

On 03/12/2014 10:05 AM, Felipe Franciosi wrote:
> If the buffers are unmapped after completing a request, then stale data
> might be in the request.

This is unfortunate, and a real bug. Good catch!

-- 
Jens Axboe

Re: [PATCH 2/2] mtip32xx: Unmap the DMA segments before completing the IO request

[Sam Bradshaw]

On 03/12/2014 09:05 AM, Felipe Franciosi wrote:
> If the buffers are unmapped after completing a request, then stale data
> might be in the request.

Good find, Felipe, thank you.  I would prefer something along the lines 
of this patch to make sure to avoid double completions / dma_unmap_sg() 
calls during surprise removal and/or timeout conditions.

Jens: note that this patch also fixes a regression in the unaligned 
workaround implementation that was introduced by the SRSI patch.

Signed-off-by: Sam Bradshaw <sbradshaw@micron.com>
diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 5160269..390ac6f 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -252,38 +252,45 @@ static void mtip_async_complete(struct mtip_port 
*port,
  				void *data,
  				int status)
  {
-	struct mtip_cmd *command;
+	struct mtip_cmd *cmd;
  	struct driver_data *dd = data;
-	int cb_status = status ? -EIO : 0;
+	int unaligned, cb_status = status ? -EIO : 0;
+	void (*func)(void *, int);

  	if (unlikely(!dd) || unlikely(!port))
  		return;

-	command = &port->commands[tag];
+	cmd = &port->commands[tag];

  	if (unlikely(status == PORT_IRQ_TF_ERR)) {
  		dev_warn(&port->dd->pdev->dev,
  			"Command tag %d failed due to TFE\n", tag);
  	}

+	/* Clear the active flag */
+	atomic_set(&port->commands[tag].active, 0);
+
  	/* Upper layer callback */
-	if (likely(command->async_callback))
-		command->async_callback(command->async_data, cb_status);
+	func = cmd->async_callback;
+	if (likely(func && cmpxchg(&cmd->async_callback, func, 0) == func)) {

-	command->async_callback = NULL;
-	command->comp_func = NULL;
+		/* Unmap the DMA scatter list entries */
+		dma_unmap_sg(&dd->pdev->dev,
+			cmd->sg,
+			cmd->scatter_ents,
+			cmd->direction);

-	/* Unmap the DMA scatter list entries */
-	dma_unmap_sg(&dd->pdev->dev,
-		command->sg,
-		command->scatter_ents,
-		command->direction);
+		func(cmd->async_data, cb_status);
+		unaligned = cmd->unaligned;

-	/* Clear the allocated and active bits for the command */
-	atomic_set(&port->commands[tag].active, 0);
-	release_slot(port, tag);
+		/* Clear the allocated bit for the command */
+		release_slot(port, tag);

-	up(&port->cmd_slot);
+		if (unlikely(unaligned))
+			up(&port->cmd_slot_unal);
+		else
+			up(&port->cmd_slot);
+	}
  }

  /*
@@ -660,11 +667,12 @@ static void mtip_timeout_function(unsigned long 
int data)
  {
  	struct mtip_port *port = (struct mtip_port *) data;
  	struct host_to_dev_fis *fis;
-	struct mtip_cmd *command;
-	int tag, cmdto_cnt = 0;
+	struct mtip_cmd *cmd;
+	int unaligned, tag, cmdto_cnt = 0;
  	unsigned int bit, group;
  	unsigned int num_command_slots;
  	unsigned long to, tagaccum[SLOTBITS_IN_LONGS];
+	void (*func)(void *, int);

  	if (unlikely(!port))
  		return;
@@ -694,8 +702,8 @@ static void mtip_timeout_function(unsigned long int 
data)
  			group = tag >> 5;
  			bit = tag & 0x1F;

-			command = &port->commands[tag];
-			fis = (struct host_to_dev_fis *) command->command;
+			cmd = &port->commands[tag];
+			fis = (struct host_to_dev_fis *) cmd->command;

  			set_bit(tag, tagaccum);
  			cmdto_cnt++;
@@ -709,27 +717,30 @@ static void mtip_timeout_function(unsigned long 
int data)
  			 */
  			writel(1 << bit, port->completed[group]);

-			/* Call the async completion callback. */
-			if (likely(command->async_callback))
-				command->async_callback(command->async_data,
-							 -EIO);
-			command->async_callback = NULL;
-			command->comp_func = NULL;
+			/* Clear the active flag for the command */
+			atomic_set(&port->commands[tag].active, 0);

-			/* Unmap the DMA scatter list entries */
-			dma_unmap_sg(&port->dd->pdev->dev,
-					command->sg,
-					command->scatter_ents,
-					command->direction);
+			func = cmd->async_callback;
+			if (func &&
+			    cmpxchg(&cmd->async_callback, func, 0) == func) {

-			/*
-			 * Clear the allocated bit and active tag for the
-			 * command.
-			 */
-			atomic_set(&port->commands[tag].active, 0);
-			release_slot(port, tag);
+				/* Unmap the DMA scatter list entries */
+				dma_unmap_sg(&port->dd->pdev->dev,
+						cmd->sg,
+						cmd->scatter_ents,
+						cmd->direction);

-			up(&port->cmd_slot);
+				func(cmd->async_data, -EIO);
+				unaligned = cmd->unaligned;
+
+				/* Clear the allocated bit for the command. */
+				release_slot(port, tag);
+
+				if (unaligned)
+					up(&port->cmd_slot_unal);
+				else
+					up(&port->cmd_slot);
+			}
  		}
  	}