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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kuninori Morimoto-2

From: Kuninori Morimoto <[hidden email]>

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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vinod Koul-3
On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:

>
> From: Kuninori Morimoto <[hidden email]>
>
> 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 <[hidden email]>
> ---
>  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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]>
> >
> > 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 <[hidden email]>
> > ---
> >
> >  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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kuninori Morimoto-2

Hi Laurent, Vinod,

> > > From: Kuninori Morimoto <[hidden email]>
> > >
> > > 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 ?


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vinod Koul-3
In reply to this post by Kuninori Morimoto-2
On Fri, Apr 22, 2016 at 01:50:04AM +0000, Kuninori Morimoto wrote:

>
> From: Kuninori Morimoto <[hidden email]>
>
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vinod Koul-3
In reply to this post by Kuninori Morimoto-2
On Wed, May 11, 2016 at 03:28:04AM +0000, Kuninori Morimoto wrote:

>
> Hi Laurent, Vinod,
>
> > > > From: Kuninori Morimoto <[hidden email]>
> > > >
> > > > 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Laurent Pinchart
In reply to this post by Vinod Koul-3
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 <[hidden email]>
> >
> > 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vinod Koul-3
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 <[hidden email]>
> > >
> > > 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
Loading...