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@xxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxx>
> > ---
> > 
> >  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