[v2,net-next,4/8] net: mvneta: sync dma buffers before refilling hw queues
diff mbox series

Message ID 744e01ea2c93200765ba8a77f0e6b0ca6baca513.1570662004.git.lorenzo@kernel.org
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • add XDP support to mvneta driver
Related show

Commit Message

Lorenzo Bianconi Oct. 9, 2019, 11:18 p.m. UTC
mvneta driver can run on not cache coherent devices so it is
necessary to sync dma buffers before sending them to the device
in order to avoid memory corruption. This patch introduce a performance
penalty and it is necessary to introduce a more sophisticated logic
in order to avoid dma sync as much as we can

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Jesper Dangaard Brouer Oct. 10, 2019, 7:08 a.m. UTC | #1
On Thu, 10 Oct 2019 01:18:34 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> mvneta driver can run on not cache coherent devices so it is
> necessary to sync dma buffers before sending them to the device
> in order to avoid memory corruption. This patch introduce a performance
> penalty and it is necessary to introduce a more sophisticated logic
> in order to avoid dma sync as much as we can

Report with benchmarks here:
 https://github.com/xdp-project/xdp-project/blob/master/areas/arm64/board_espressobin08_bench_xdp.org

We are testing this on an Espressobin board, and do see a huge
performance cost associated with this DMA-sync.   Regardless we still
want to get this patch merged, to move forward with XDP support for
this driver. 

We promised each-other (on IRC freenode #xdp) that we will follow-up
with a solution/mitigation, after this patchset is merged.  There are
several ideas, that likely should get separate upstream review.

> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

> ---
>  drivers/net/ethernet/marvell/mvneta.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 79a6bac0192b..ba4aa9bbc798 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1821,6 +1821,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>  			    struct mvneta_rx_queue *rxq,
>  			    gfp_t gfp_mask)
>  {
> +	enum dma_data_direction dma_dir;
>  	dma_addr_t phys_addr;
>  	struct page *page;
>  
> @@ -1830,6 +1831,9 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>  		return -ENOMEM;
>  
>  	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
> +	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
> +	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
> +				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
>  	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
>  
>  	return 0;
Ilias Apalodimas Oct. 10, 2019, 7:21 a.m. UTC | #2
Hi Lorenzo, Jesper,

On Thu, Oct 10, 2019 at 09:08:31AM +0200, Jesper Dangaard Brouer wrote:
> On Thu, 10 Oct 2019 01:18:34 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > mvneta driver can run on not cache coherent devices so it is
> > necessary to sync dma buffers before sending them to the device
> > in order to avoid memory corruption. This patch introduce a performance
> > penalty and it is necessary to introduce a more sophisticated logic
> > in order to avoid dma sync as much as we can
> 
> Report with benchmarks here:
>  https://github.com/xdp-project/xdp-project/blob/master/areas/arm64/board_espressobin08_bench_xdp.org
> 
> We are testing this on an Espressobin board, and do see a huge
> performance cost associated with this DMA-sync.   Regardless we still
> want to get this patch merged, to move forward with XDP support for
> this driver. 
> 
> We promised each-other (on IRC freenode #xdp) that we will follow-up
> with a solution/mitigation, after this patchset is merged.  There are
> several ideas, that likely should get separate upstream review.

I think mentioning that the patch *introduces* a performance penalty is a bit
misleading. 
The dma sync does have a performance penalty but it was always there. 
The initial driver was mapping the DMA with DMA_FROM_DEVICE, which implies
syncing as well. In page_pool we do not explicitly sync buffers on allocation
and leave it up the driver writer (and allow him some tricks to avoid that),
thus this patch is needed.

In any case what Jesper mentions is correct, we do have a plan :)

> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 79a6bac0192b..ba4aa9bbc798 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -1821,6 +1821,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> >  			    struct mvneta_rx_queue *rxq,
> >  			    gfp_t gfp_mask)
> >  {
> > +	enum dma_data_direction dma_dir;
> >  	dma_addr_t phys_addr;
> >  	struct page *page;
> >  
> > @@ -1830,6 +1831,9 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> >  		return -ENOMEM;
> >  
> >  	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
> > +	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
> > +	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
> > +				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
> >  	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
> >  
> >  	return 0;
> 
> 
> 
> -- 
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

Thanks!
/Ilias
Lorenzo Bianconi Oct. 10, 2019, 9:18 a.m. UTC | #3
> Hi Lorenzo, Jesper,
> 
> On Thu, Oct 10, 2019 at 09:08:31AM +0200, Jesper Dangaard Brouer wrote:
> > On Thu, 10 Oct 2019 01:18:34 +0200
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > 
> > > mvneta driver can run on not cache coherent devices so it is
> > > necessary to sync dma buffers before sending them to the device
> > > in order to avoid memory corruption. This patch introduce a performance
> > > penalty and it is necessary to introduce a more sophisticated logic
> > > in order to avoid dma sync as much as we can
> > 
> > Report with benchmarks here:
> >  https://github.com/xdp-project/xdp-project/blob/master/areas/arm64/board_espressobin08_bench_xdp.org
> > 
> > We are testing this on an Espressobin board, and do see a huge
> > performance cost associated with this DMA-sync.   Regardless we still
> > want to get this patch merged, to move forward with XDP support for
> > this driver. 
> > 
> > We promised each-other (on IRC freenode #xdp) that we will follow-up
> > with a solution/mitigation, after this patchset is merged.  There are
> > several ideas, that likely should get separate upstream review.
> 
> I think mentioning that the patch *introduces* a performance penalty is a bit
> misleading. 
> The dma sync does have a performance penalty but it was always there. 
> The initial driver was mapping the DMA with DMA_FROM_DEVICE, which implies
> syncing as well. In page_pool we do not explicitly sync buffers on allocation
> and leave it up the driver writer (and allow him some tricks to avoid that),
> thus this patch is needed.

Reviewing the commit log I definitely agree, I will rewrite it in v3. Thx

Regards,
Lorenzo

> 
> In any case what Jesper mentions is correct, we do have a plan :)
> 
> > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > 
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > 
> > > ---
> > >  drivers/net/ethernet/marvell/mvneta.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > > index 79a6bac0192b..ba4aa9bbc798 100644
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -1821,6 +1821,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> > >  			    struct mvneta_rx_queue *rxq,
> > >  			    gfp_t gfp_mask)
> > >  {
> > > +	enum dma_data_direction dma_dir;
> > >  	dma_addr_t phys_addr;
> > >  	struct page *page;
> > >  
> > > @@ -1830,6 +1831,9 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> > >  		return -ENOMEM;
> > >  
> > >  	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
> > > +	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
> > > +	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
> > > +				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
> > >  	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
> > >  
> > >  	return 0;
> > 
> > 
> > 
> > -- 
> > Best regards,
> >   Jesper Dangaard Brouer
> >   MSc.CS, Principal Kernel Engineer at Red Hat
> >   LinkedIn: http://www.linkedin.com/in/brouer
> 
> Thanks!
> /Ilias
Lorenzo Bianconi Oct. 10, 2019, 9:20 a.m. UTC | #4
> On Thu, 10 Oct 2019 01:18:34 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > mvneta driver can run on not cache coherent devices so it is
> > necessary to sync dma buffers before sending them to the device
> > in order to avoid memory corruption. This patch introduce a performance
> > penalty and it is necessary to introduce a more sophisticated logic
> > in order to avoid dma sync as much as we can
> 
> Report with benchmarks here:
>  https://github.com/xdp-project/xdp-project/blob/master/areas/arm64/board_espressobin08_bench_xdp.org

Thx a lot Jesper for detailed report, I will include this info in the commit log.

Regards,
Lorenzo

> 
> We are testing this on an Espressobin board, and do see a huge
> performance cost associated with this DMA-sync.   Regardless we still
> want to get this patch merged, to move forward with XDP support for
> this driver. 
> 
> We promised each-other (on IRC freenode #xdp) that we will follow-up
> with a solution/mitigation, after this patchset is merged.  There are
> several ideas, that likely should get separate upstream review.
> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 79a6bac0192b..ba4aa9bbc798 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -1821,6 +1821,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> >  			    struct mvneta_rx_queue *rxq,
> >  			    gfp_t gfp_mask)
> >  {
> > +	enum dma_data_direction dma_dir;
> >  	dma_addr_t phys_addr;
> >  	struct page *page;
> >  
> > @@ -1830,6 +1831,9 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> >  		return -ENOMEM;
> >  
> >  	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
> > +	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
> > +	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
> > +				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
> >  	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
> >  
> >  	return 0;
> 
> 
> 
> -- 
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

Patch
diff mbox series

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 79a6bac0192b..ba4aa9bbc798 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1821,6 +1821,7 @@  static int mvneta_rx_refill(struct mvneta_port *pp,
 			    struct mvneta_rx_queue *rxq,
 			    gfp_t gfp_mask)
 {
+	enum dma_data_direction dma_dir;
 	dma_addr_t phys_addr;
 	struct page *page;
 
@@ -1830,6 +1831,9 @@  static int mvneta_rx_refill(struct mvneta_port *pp,
 		return -ENOMEM;
 
 	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
+	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
+	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
+				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
 	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
 
 	return 0;