[PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current rcar_dmac_desc_put() is using list_add_tail() in order to
push used descriptor to list of free descriptors, and next DMA transfer
try to reuse it from this list. But because it is using *_tail(),
this reuse effect can't be obtained without using all of them.
For a longer-term solution, we should allocate hardware descriptors
using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
This patch uses list_add() instead of list_add_tail() for short-term
solution.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/dma/sh/rcar-dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 02b86c6..616c63a 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -519,7 +519,7 @@ static void rcar_dmac_desc_put(struct rcar_dmac_chan *chan,
 
 	spin_lock_irqsave(&chan->lock, flags);
 	list_splice_tail_init(&desc->chunks, &chan->desc.chunks_free);
-	list_add_tail(&desc->node, &chan->desc.free);
+	list_add(&desc->node, &chan->desc.free);
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 
-- 
1.9.1

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Vinod Koul]

On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Current rcar_dmac_desc_put() is using list_add_tail() in order to
> push used descriptor to list of free descriptors, and next DMA transfer
> try to reuse it from this list. But because it is using *_tail(),
> this reuse effect can't be obtained without using all of them.
> For a longer-term solution, we should allocate hardware descriptors
> using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> This patch uses list_add() instead of list_add_tail() for short-term
> solution.

So how does reuse case help by not moving the descriptor to tail.

Also you are not reusing descriptor, you are reusing a descriptor memory,
these are two different things.

Lastly how does this help? Something doesn't seem right

> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/dma/sh/rcar-dmac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 02b86c6..616c63a 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -519,7 +519,7 @@ static void rcar_dmac_desc_put(struct rcar_dmac_chan *chan,
>  
>  	spin_lock_irqsave(&chan->lock, flags);
>  	list_splice_tail_init(&desc->chunks, &chan->desc.chunks_free);
> -	list_add_tail(&desc->node, &chan->desc.free);
> +	list_add(&desc->node, &chan->desc.free);
>  	spin_unlock_irqrestore(&chan->lock, flags);
>  }
>  

-- 
~Vinod

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Laurent Pinchart]

Hi Vinod,

On Monday 02 May 2016 15:07:12 Vinod Koul wrote:
> On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > Current rcar_dmac_desc_put() is using list_add_tail() in order to
> > push used descriptor to list of free descriptors, and next DMA transfer
> > try to reuse it from this list. But because it is using *_tail(),
> > this reuse effect can't be obtained without using all of them.
> > For a longer-term solution, we should allocate hardware descriptors
> > using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> > This patch uses list_add() instead of list_add_tail() for short-term
> > solution.
> 
> So how does reuse case help by not moving the descriptor to tail.
> 
> Also you are not reusing descriptor, you are reusing a descriptor memory,
> these are two different things.
> 
> Lastly how does this help? Something doesn't seem right

For each descriptor, in addition to the memory used by the descriptors 
structure itself, the driver allocates a list of chunks as well as a buffer 
for hardware descriptors. Descriptors themselves are preallocated, and 
allocation of the chunks and buffer is performed the first time the descriptor 
is used. The memory isn't freed when the transfer is completed, as the chunks 
and buffer will be needed again when the descriptor is reused internally, so 
the driver keeps the memory around.

If only a few descriptors are used concurrently, the current list_add_tail() 
implementation will result in all preallocated descriptors being used before 
going back to the first one, and will thus allocate chunks and a buffer for 
all preallocated descriptors. Using list_add() will put the complete 
descriptor at the head of the list of available descriptors, so the next 
transfer will be more likely to reuse a descriptor that already has associated 
memory instead of one that has never been used before.

> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> > 
> >  drivers/dma/sh/rcar-dmac.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> > index 02b86c6..616c63a 100644
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -519,7 +519,7 @@ static void rcar_dmac_desc_put(struct rcar_dmac_chan
> > *chan,> 
> >  	spin_lock_irqsave(&chan->lock, flags);
> >  	list_splice_tail_init(&desc->chunks, &chan->desc.chunks_free);
> > 
> > -	list_add_tail(&desc->node, &chan->desc.free);
> > +	list_add(&desc->node, &chan->desc.free);
> > 
> >  	spin_unlock_irqrestore(&chan->lock, flags);
> >  
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Kuninori Morimoto]

Hi Laurent, Vinod,

> > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > 
> > > Current rcar_dmac_desc_put() is using list_add_tail() in order to
> > > push used descriptor to list of free descriptors, and next DMA transfer
> > > try to reuse it from this list. But because it is using *_tail(),
> > > this reuse effect can't be obtained without using all of them.
> > > For a longer-term solution, we should allocate hardware descriptors
> > > using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> > > This patch uses list_add() instead of list_add_tail() for short-term
> > > solution.
> > 
> > So how does reuse case help by not moving the descriptor to tail.
> > 
> > Also you are not reusing descriptor, you are reusing a descriptor memory,
> > these are two different things.
> > 
> > Lastly how does this help? Something doesn't seem right
> 
> For each descriptor, in addition to the memory used by the descriptors 
> structure itself, the driver allocates a list of chunks as well as a buffer 
> for hardware descriptors. Descriptors themselves are preallocated, and 
> allocation of the chunks and buffer is performed the first time the descriptor 
> is used. The memory isn't freed when the transfer is completed, as the chunks 
> and buffer will be needed again when the descriptor is reused internally, so 
> the driver keeps the memory around.
> 
> If only a few descriptors are used concurrently, the current list_add_tail() 
> implementation will result in all preallocated descriptors being used before 
> going back to the first one, and will thus allocate chunks and a buffer for 
> all preallocated descriptors. Using list_add() will put the complete 
> descriptor at the head of the list of available descriptors, so the next 
> transfer will be more likely to reuse a descriptor that already has associated 
> memory instead of one that has never been used before.

Laurent, thank you for your help
Vinod, does above clear for you ?

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Vinod Koul]

On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Current rcar_dmac_desc_put() is using list_add_tail() in order to
> push used descriptor to list of free descriptors, and next DMA transfer
> try to reuse it from this list. But because it is using *_tail(),
> this reuse effect can't be obtained without using all of them.
> For a longer-term solution, we should allocate hardware descriptors
> using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> This patch uses list_add() instead of list_add_tail() for short-term
> solution.

Applied, thanks

-- 
~Vinod

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Vinod Koul]

On Wed, May 11, 2016 at 03:28:04AM +0000, Kuninori Morimoto wrote:
> 
> Hi Laurent, Vinod,
> 
> > > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > > 
> > > > Current rcar_dmac_desc_put() is using list_add_tail() in order to
> > > > push used descriptor to list of free descriptors, and next DMA transfer
> > > > try to reuse it from this list. But because it is using *_tail(),
> > > > this reuse effect can't be obtained without using all of them.
> > > > For a longer-term solution, we should allocate hardware descriptors
> > > > using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> > > > This patch uses list_add() instead of list_add_tail() for short-term
> > > > solution.
> > > 
> > > So how does reuse case help by not moving the descriptor to tail.
> > > 
> > > Also you are not reusing descriptor, you are reusing a descriptor memory,
> > > these are two different things.
> > > 
> > > Lastly how does this help? Something doesn't seem right
> > 
> > For each descriptor, in addition to the memory used by the descriptors 
> > structure itself, the driver allocates a list of chunks as well as a buffer 
> > for hardware descriptors. Descriptors themselves are preallocated, and 
> > allocation of the chunks and buffer is performed the first time the descriptor 
> > is used. The memory isn't freed when the transfer is completed, as the chunks 
> > and buffer will be needed again when the descriptor is reused internally, so 
> > the driver keeps the memory around.
> > 
> > If only a few descriptors are used concurrently, the current list_add_tail() 
> > implementation will result in all preallocated descriptors being used before 
> > going back to the first one, and will thus allocate chunks and a buffer for 
> > all preallocated descriptors. Using list_add() will put the complete 
> > descriptor at the head of the list of available descriptors, so the next 
> > transfer will be more likely to reuse a descriptor that already has associated 
> > memory instead of one that has never been used before.
> 
> Laurent, thank you for your help
> Vinod, does above clear for you ?

Yes makese sense now. But please add these details in the changelog. This
helps people know why a line was modified down the line

-- 
~Vinod

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Laurent Pinchart]

Hi Vinod,

On Saturday 14 May 2016 13:27:31 Vinod Koul wrote:
> On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > Current rcar_dmac_desc_put() is using list_add_tail() in order to
> > push used descriptor to list of free descriptors, and next DMA transfer
> > try to reuse it from this list. But because it is using *_tail(),
> > this reuse effect can't be obtained without using all of them.
> > For a longer-term solution, we should allocate hardware descriptors
> > using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> > This patch uses list_add() instead of list_add_tail() for short-term
> > solution.
> 
> Applied, thanks

Thanks, but where did you apply it to ? I can't find it in your tree.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Vinod Koul]

On Tue, May 24, 2016 at 12:50:28PM +0300, Laurent Pinchart wrote:

Hey Laurent,

> On Saturday 14 May 2016 13:27:31 Vinod Koul wrote:
> > On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:
> > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > 
> > > Current rcar_dmac_desc_put() is using list_add_tail() in order to
> > > push used descriptor to list of free descriptors, and next DMA transfer
> > > try to reuse it from this list. But because it is using *_tail(),
> > > this reuse effect can't be obtained without using all of them.
> > > For a longer-term solution, we should allocate hardware descriptors
> > > using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> > > This patch uses list_add() instead of list_add_tail() for short-term
> > > solution.
> > 
> > Applied, thanks
> 
> Thanks, but where did you apply it to ? I can't find it in your tree.

I changed my mind after this and felt that we should document above
in the changelog as well and replied to Kuninori few moments after this [1]

Sorry if that wasn't clear that am not applying this.

Please resend it with update changelog

[1]: http://www.spinics.net/lists/dmaengine/msg09585.html

-- 
~Vinod

Re: [PATCH] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Kuninori Morimoto]

Hi Vinod

> > > > Current rcar_dmac_desc_put() is using list_add_tail() in order to
> > > > push used descriptor to list of free descriptors, and next DMA transfer
> > > > try to reuse it from this list. But because it is using *_tail(),
> > > > this reuse effect can't be obtained without using all of them.
> > > > For a longer-term solution, we should allocate hardware descriptors
> > > > using GFP_KERNEL instead of GFP_NOWAIT, but it is difficult today.
> > > > This patch uses list_add() instead of list_add_tail() for short-term
> > > > solution.
> > > 
> > > Applied, thanks
> > 
> > Thanks, but where did you apply it to ? I can't find it in your tree.
> 
> I changed my mind after this and felt that we should document above
> in the changelog as well and replied to Kuninori few moments after this [1]
> 
> Sorry if that wasn't clear that am not applying this.
> 
> Please resend it with update changelog
> 
> [1]: http://www.spinics.net/lists/dmaengine/msg09585.html

OK, will do

Best regards
---
Kuninori Morimoto

[PATCH v2] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

For each descriptor, in addition to the memory used by the descriptors
structure itself, the driver allocates a list of chunks as well as a
buffer for hardware descriptors. Descriptors themselves are preallocated,
and allocation of the chunks and buffer is performed the first time the
descriptor is used. The memory isn't freed when the transfer is completed,
as the chunks and buffer will be needed again when the descriptor is
reused internally, so the driver keeps the memory around.

If only a few descriptors are used concurrently, the current
list_add_tail() implementation will result in all preallocated descriptors
being used before going back to the first one, and will thus allocate
chunks and a buffer for all preallocated descriptors. Using list_add()
will put the complete descriptor at the head of the list of available
descriptors, so the next transfer will be more likely to reuse a
descriptor that already has associated memory instead of one that has
never been used before.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - used Laurent's explain on log

 drivers/dma/sh/rcar-dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 02b86c6..616c63a 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -519,7 +519,7 @@ static void rcar_dmac_desc_put(struct rcar_dmac_chan *chan,
 
 	spin_lock_irqsave(&chan->lock, flags);
 	list_splice_tail_init(&desc->chunks, &chan->desc.chunks_free);
-	list_add_tail(&desc->node, &chan->desc.free);
+	list_add(&desc->node, &chan->desc.free);
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 
-- 
1.9.1

Re: [PATCH v2] dma: rcar-dmac: use list_add() on rcar_dmac_desc_put()

[Vinod Koul]

On Mon, May 30, 2016 at 12:41:48AM +0000, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> For each descriptor, in addition to the memory used by the descriptors
> structure itself, the driver allocates a list of chunks as well as a
> buffer for hardware descriptors. Descriptors themselves are preallocated,
> and allocation of the chunks and buffer is performed the first time the
> descriptor is used. The memory isn't freed when the transfer is completed,
> as the chunks and buffer will be needed again when the descriptor is
> reused internally, so the driver keeps the memory around.
> 
> If only a few descriptors are used concurrently, the current
> list_add_tail() implementation will result in all preallocated descriptors
> being used before going back to the first one, and will thus allocate
> chunks and a buffer for all preallocated descriptors. Using list_add()
> will put the complete descriptor at the head of the list of available
> descriptors, so the next transfer will be more likely to reuse a
> descriptor that already has associated memory instead of one that has
> never been used before.

Applied after fixing subsystem name

-- 
~Vinod