[PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Jakub Kicinski]

Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
to a few more drivers (possibly as a copy'n'paste).

DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
the rarity of these platforms is likely why we never bothered adding
the attribute in the page pool, even though it should be safe to add.

To make the page pool migration in drivers which set this flag less
of a risk (of regressing the precious sparc database workloads or
whatever needed this) let's add DMA_ATTR_WEAK_ORDERING on all
page pool DMA mappings.

We could make this a driver opt-in but frankly I don't think it's
worth complicating the API. I can't think of a reason why device
accesses to packet memory would have to be ordered.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: hawk@kernel.org
CC: ilias.apalodimas@linaro.org
---
 net/core/page_pool.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 2f6bf422ed30..97f20f7ff4fc 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -316,7 +316,8 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
 	 */
 	dma = dma_map_page_attrs(pool->p.dev, page, 0,
 				 (PAGE_SIZE << pool->p.order),
-				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
+						  DMA_ATTR_WEAK_ORDERING);
 	if (dma_mapping_error(pool->p.dev, dma))
 		return false;
 
@@ -484,7 +485,7 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
 	/* When page is unmapped, it cannot be returned to our pool */
 	dma_unmap_page_attrs(pool->p.dev, dma,
 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
-			     DMA_ATTR_SKIP_CPU_SYNC);
+			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
 	page_pool_set_dma_addr(page, 0);
 skip_dma_unmap:
 	page_pool_clear_pp_info(page);
-- 
2.39.2

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Ilias Apalodimas]

This seems sane to me, especially since we use page pool on the Rx
path for normal drivers.  If anyone has a different opinion please
shout\

On Mon, 17 Apr 2023 at 18:28, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
> DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
> to a few more drivers (possibly as a copy'n'paste).
>
> DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
> the rarity of these platforms is likely why we never bothered adding
> the attribute in the page pool, even though it should be safe to add.
>
> To make the page pool migration in drivers which set this flag less
> of a risk (of regressing the precious sparc database workloads or
> whatever needed this) let's add DMA_ATTR_WEAK_ORDERING on all
> page pool DMA mappings.
>
> We could make this a driver opt-in but frankly I don't think it's
> worth complicating the API. I can't think of a reason why device
> accesses to packet memory would have to be ordered.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: hawk@kernel.org
> CC: ilias.apalodimas@linaro.org
> ---
>  net/core/page_pool.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 2f6bf422ed30..97f20f7ff4fc 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -316,7 +316,8 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>          */
>         dma = dma_map_page_attrs(pool->p.dev, page, 0,
>                                  (PAGE_SIZE << pool->p.order),
> -                                pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> +                                pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
> +                                                 DMA_ATTR_WEAK_ORDERING);
>         if (dma_mapping_error(pool->p.dev, dma))
>                 return false;
>
> @@ -484,7 +485,7 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
>         /* When page is unmapped, it cannot be returned to our pool */
>         dma_unmap_page_attrs(pool->p.dev, dma,
>                              PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> -                            DMA_ATTR_SKIP_CPU_SYNC);
> +                            DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
>         page_pool_set_dma_addr(page, 0);
>  skip_dma_unmap:
>         page_pool_clear_pp_info(page);
> --
> 2.39.2
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Somnath Kotur]

[-- Attachment #1: Type: text/plain, Size: 2468 bytes --]

On Mon, Apr 17, 2023 at 8:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
> DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
> to a few more drivers (possibly as a copy'n'paste).
>
> DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
> the rarity of these platforms is likely why we never bothered adding
> the attribute in the page pool, even though it should be safe to add.
>
> To make the page pool migration in drivers which set this flag less
> of a risk (of regressing the precious sparc database workloads or
> whatever needed this) let's add DMA_ATTR_WEAK_ORDERING on all
> page pool DMA mappings.
>
> We could make this a driver opt-in but frankly I don't think it's
> worth complicating the API. I can't think of a reason why device
> accesses to packet memory would have to be ordered.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: hawk@kernel.org
> CC: ilias.apalodimas@linaro.org
> ---
>  net/core/page_pool.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 2f6bf422ed30..97f20f7ff4fc 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -316,7 +316,8 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>          */
>         dma = dma_map_page_attrs(pool->p.dev, page, 0,
>                                  (PAGE_SIZE << pool->p.order),
> -                                pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> +                                pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
> +                                                 DMA_ATTR_WEAK_ORDERING);
>         if (dma_mapping_error(pool->p.dev, dma))
>                 return false;
>
> @@ -484,7 +485,7 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
>         /* When page is unmapped, it cannot be returned to our pool */
>         dma_unmap_page_attrs(pool->p.dev, dma,
>                              PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> -                            DMA_ATTR_SKIP_CPU_SYNC);
> +                            DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
>         page_pool_set_dma_addr(page, 0);
>  skip_dma_unmap:
>         page_pool_clear_pp_info(page);
> --
> 2.39.2
>
Acked-by: Somnath Kotur <somnath.kotur@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Jesper Dangaard Brouer]

To Hellwig,

On 17/04/2023 17.28, Jakub Kicinski wrote:
> Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
> DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
> to a few more drivers (possibly as a copy'n'paste).
> 
> DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
> the rarity of these platforms is likely why we never bothered adding
> the attribute in the page pool, even though it should be safe to add.
> 
> To make the page pool migration in drivers which set this flag less
> of a risk (of regressing the precious sparc database workloads or
> whatever needed this) let's add DMA_ATTR_WEAK_ORDERING on all
> page pool DMA mappings.
> 

This sounds reasonable to me, but I don't know the DMA APIs well enough.
Thus, I would like to hear if Hellwig thinks this is okay?

> We could make this a driver opt-in but frankly I don't think it's
> worth complicating the API. I can't think of a reason why device
> accesses to packet memory would have to be ordered.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: hawk@kernel.org
> CC: ilias.apalodimas@linaro.org
> ---
>   net/core/page_pool.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 2f6bf422ed30..97f20f7ff4fc 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -316,7 +316,8 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>   	 */
>   	dma = dma_map_page_attrs(pool->p.dev, page, 0,
>   				 (PAGE_SIZE << pool->p.order),
> -				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> +				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
> +						  DMA_ATTR_WEAK_ORDERING);
>   	if (dma_mapping_error(pool->p.dev, dma))
>   		return false;
>   
> @@ -484,7 +485,7 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
>   	/* When page is unmapped, it cannot be returned to our pool */
>   	dma_unmap_page_attrs(pool->p.dev, dma,
>   			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> -			     DMA_ATTR_SKIP_CPU_SYNC);
> +			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
>   	page_pool_set_dma_addr(page, 0);
>   skip_dma_unmap:
>   	page_pool_clear_pp_info(page);

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Mon, 17 Apr 2023 08:28:05 -0700

> Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
> DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
> to a few more drivers (possibly as a copy'n'paste).
> 
> DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
> the rarity of these platforms is likely why we never bothered adding
> the attribute in the page pool, even though it should be safe to add.
> 
> To make the page pool migration in drivers which set this flag less
> of a risk (of regressing the precious sparc database workloads or
> whatever needed this) let's add DMA_ATTR_WEAK_ORDERING on all
> page pool DMA mappings.
> 
> We could make this a driver opt-in but frankly I don't think it's
> worth complicating the API. I can't think of a reason why device
> accesses to packet memory would have to be ordered.

And you do that 2 days before I send this: [0] :D
Just jokin' :p

Unconditional weak ordering seems reasonable to me as well. BTW, I've
seen one driver converted to PP already, which manages DMA mappings on
its own *solely* because it needs to map stuff with weak ordering =\
It could be switched to PP builtin map handling thanks to this change
(-100 locs).

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: hawk@kernel.org
> CC: ilias.apalodimas@linaro.org
> ---
>  net/core/page_pool.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 2f6bf422ed30..97f20f7ff4fc 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -316,7 +316,8 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>  	 */
>  	dma = dma_map_page_attrs(pool->p.dev, page, 0,
>  				 (PAGE_SIZE << pool->p.order),
> -				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> +				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
> +						  DMA_ATTR_WEAK_ORDERING);
>  	if (dma_mapping_error(pool->p.dev, dma))
>  		return false;
>  
> @@ -484,7 +485,7 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
>  	/* When page is unmapped, it cannot be returned to our pool */
>  	dma_unmap_page_attrs(pool->p.dev, dma,
>  			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> -			     DMA_ATTR_SKIP_CPU_SYNC);
> +			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
>  	page_pool_set_dma_addr(page, 0);
>  skip_dma_unmap:
>  	page_pool_clear_pp_info(page);

[0]
https://github.com/alobakin/linux/commit/9d3d9ab4e7765d5a4c466c65ad6cf204a3ad1a71

Thanks,
Olek

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Jesper Dangaard Brouer]

On 18/04/2023 13.23, Jesper Dangaard Brouer wrote:
> To Hellwig,
> 
> On 17/04/2023 17.28, Jakub Kicinski wrote:
>> Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
>> DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
>> to a few more drivers (possibly as a copy'n'paste).
>>
>> DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
>> the rarity of these platforms is likely why we never bothered adding
>> the attribute in the page pool, even though it should be safe to add.
>>
>> To make the page pool migration in drivers which set this flag less
>> of a risk (of regressing the precious sparc database workloads or
>> whatever needed this) let's add DMA_ATTR_WEAK_ORDERING on all
>> page pool DMA mappings.
>>
> 
> This sounds reasonable to me, but I don't know the DMA APIs well enough.
> Thus, I would like to hear if Hellwig thinks this is okay?

Acking this patch, as we can always revert if Hellwig have objections later.

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

>> We could make this a driver opt-in but frankly I don't think it's
>> worth complicating the API. I can't think of a reason why device
>> accesses to packet memory would have to be ordered.
>>
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> ---
>> CC: hawk@kernel.org
>> CC: ilias.apalodimas@linaro.org
>> ---
>>   net/core/page_pool.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>> index 2f6bf422ed30..97f20f7ff4fc 100644
>> --- a/net/core/page_pool.c
>> +++ b/net/core/page_pool.c
>> @@ -316,7 +316,8 @@ static bool page_pool_dma_map(struct page_pool 
>> *pool, struct page *page)
>>        */
>>       dma = dma_map_page_attrs(pool->p.dev, page, 0,
>>                    (PAGE_SIZE << pool->p.order),
>> -                 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
>> +                 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
>> +                          DMA_ATTR_WEAK_ORDERING);
>>       if (dma_mapping_error(pool->p.dev, dma))
>>           return false;
>> @@ -484,7 +485,7 @@ void page_pool_release_page(struct page_pool 
>> *pool, struct page *page)
>>       /* When page is unmapped, it cannot be returned to our pool */
>>       dma_unmap_page_attrs(pool->p.dev, dma,
>>                    PAGE_SIZE << pool->p.order, pool->p.dma_dir,
>> -                 DMA_ATTR_SKIP_CPU_SYNC);
>> +                 DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
>>       page_pool_set_dma_addr(page, 0);
>>   skip_dma_unmap:
>>       page_pool_clear_pp_info(page);

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[Jakub Kicinski]

On Wed, 19 Apr 2023 15:49:17 +0200 Alexander Lobakin wrote:
> And you do that 2 days before I send this: [0] :D
> Just jokin' :p

Ha! It's karma for not posting the netlink int stuff, yet :P

> Unconditional weak ordering seems reasonable to me as well. BTW, I've
> seen one driver converted to PP already, which manages DMA mappings on
> its own *solely* because it needs to map stuff with weak ordering =\
> It could be switched to PP builtin map handling thanks to this change
> (-100 locs).

Neat!  I hope it will unblock a number of conversions.

Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 17 Apr 2023 08:28:05 -0700 you wrote:
> Commit c519fe9a4f0d ("bnxt: add dma mapping attributes") added
> DMA_ATTR_WEAK_ORDERING to DMA attrs on bnxt. It has since spread
> to a few more drivers (possibly as a copy'n'paste).
> 
> DMA_ATTR_WEAK_ORDERING only seems to matter on Sparc and PowerPC/cell,
> the rarity of these platforms is likely why we never bothered adding
> the attribute in the page pool, even though it should be safe to add.
> 
> [...]

Here is the summary with links:
  - [net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
    https://git.kernel.org/netdev/net-next/c/8e4c62c7d980

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html