* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-17 15:28 [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings Jakub Kicinski
@ 2023-04-18 9:32 ` Ilias Apalodimas
2023-04-18 10:19 ` Somnath Kotur
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Ilias Apalodimas @ 2023-04-18 9:32 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, michael.chan, hawk
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>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-17 15:28 [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings Jakub Kicinski
2023-04-18 9:32 ` Ilias Apalodimas
@ 2023-04-18 10:19 ` Somnath Kotur
2023-04-18 11:23 ` Jesper Dangaard Brouer
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Somnath Kotur @ 2023-04-18 10:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, michael.chan, hawk,
ilias.apalodimas
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-17 15:28 [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings Jakub Kicinski
2023-04-18 9:32 ` Ilias Apalodimas
2023-04-18 10:19 ` Somnath Kotur
@ 2023-04-18 11:23 ` Jesper Dangaard Brouer
2023-04-19 15:12 ` Jesper Dangaard Brouer
2023-04-19 13:49 ` Alexander Lobakin
2023-04-19 21:20 ` patchwork-bot+netdevbpf
4 siblings, 1 reply; 8+ messages in thread
From: Jesper Dangaard Brouer @ 2023-04-18 11:23 UTC (permalink / raw)
To: Jakub Kicinski, Christoph Hellwig
Cc: brouer, netdev, edumazet, pabeni, michael.chan, hawk,
ilias.apalodimas, davem
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);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-18 11:23 ` Jesper Dangaard Brouer
@ 2023-04-19 15:12 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 8+ messages in thread
From: Jesper Dangaard Brouer @ 2023-04-19 15:12 UTC (permalink / raw)
To: Jakub Kicinski, Christoph Hellwig
Cc: brouer, netdev, edumazet, pabeni, michael.chan, hawk,
ilias.apalodimas, davem
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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-17 15:28 [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings Jakub Kicinski
` (2 preceding siblings ...)
2023-04-18 11:23 ` Jesper Dangaard Brouer
@ 2023-04-19 13:49 ` Alexander Lobakin
2023-04-19 18:24 ` Jakub Kicinski
2023-04-19 21:20 ` patchwork-bot+netdevbpf
4 siblings, 1 reply; 8+ messages in thread
From: Alexander Lobakin @ 2023-04-19 13:49 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, michael.chan, hawk,
ilias.apalodimas
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-19 13:49 ` Alexander Lobakin
@ 2023-04-19 18:24 ` Jakub Kicinski
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2023-04-19 18:24 UTC (permalink / raw)
To: Alexander Lobakin
Cc: davem, netdev, edumazet, pabeni, michael.chan, hawk,
ilias.apalodimas
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings
2023-04-17 15:28 [PATCH net-next] page_pool: add DMA_ATTR_WEAK_ORDERING on all mappings Jakub Kicinski
` (3 preceding siblings ...)
2023-04-19 13:49 ` Alexander Lobakin
@ 2023-04-19 21:20 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-04-19 21:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, michael.chan, hawk,
ilias.apalodimas
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
^ permalink raw reply [flat|nested] 8+ messages in thread