| Message ID | 87r3dywh6u.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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); > } >
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); > > > > }
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 ? -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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.
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
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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }