As the 32-bit arch with 64-bit DMA seems to rare those days,
and page pool is carrying a lot of code and complexity for
systems that possibly don't exist.
So disable dma mapping support for such systems, if drivers
really want to work on such systems, they have to implement
their own DMA-mapping fallback tracking outside page_pool.
Signed-off-by: Yunsheng Lin <[email protected]>
---
include/linux/mm_types.h | 13 +------------
include/net/page_pool.h | 12 +-----------
net/core/page_pool.c | 10 ++++++----
3 files changed, 8 insertions(+), 27 deletions(-)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 7f8ee09c711f..436e0946d691 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -104,18 +104,7 @@ struct page {
struct page_pool *pp;
unsigned long _pp_mapping_pad;
unsigned long dma_addr;
- union {
- /**
- * dma_addr_upper: might require a 64-bit
- * value on 32-bit architectures.
- */
- unsigned long dma_addr_upper;
- /**
- * For frag page support, not supported in
- * 32-bit architectures with 64-bit DMA.
- */
- atomic_long_t pp_frag_count;
- };
+ atomic_long_t pp_frag_count;
};
struct { /* slab, slob and slub */
union {
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index a4082406a003..3855f069627f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -216,24 +216,14 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
page_pool_put_full_page(pool, page, true);
}
-#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
- (sizeof(dma_addr_t) > sizeof(unsigned long))
-
static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
{
- dma_addr_t ret = page->dma_addr;
-
- if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
- ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16;
-
- return ret;
+ return page->dma_addr;
}
static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
{
page->dma_addr = addr;
- if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
- page->dma_addr_upper = upper_32_bits(addr);
}
static inline void page_pool_set_frag_count(struct page *page, long nr)
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
* which is the XDP_TX use-case.
*/
if (pool->p.flags & PP_FLAG_DMA_MAP) {
+ /* DMA-mapping is not supported on 32-bit systems with
+ * 64-bit DMA mapping.
+ */
+ if (sizeof(dma_addr_t) > sizeof(unsigned long))
+ return -EINVAL;
+
if ((pool->p.dma_dir != DMA_FROM_DEVICE) &&
(pool->p.dma_dir != DMA_BIDIRECTIONAL))
return -EINVAL;
@@ -69,10 +75,6 @@ static int page_pool_init(struct page_pool *pool,
*/
}
- if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
- pool->p.flags & PP_FLAG_PAGE_FRAG)
- return -EINVAL;
-
if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0)
return -ENOMEM;
--
2.33.0
(+cc Matthew) but this looks safe to me.
On Wed, 22 Sept 2021 at 12:43, Yunsheng Lin <[email protected]> wrote:
>
> As the 32-bit arch with 64-bit DMA seems to rare those days,
> and page pool is carrying a lot of code and complexity for
> systems that possibly don't exist.
>
> So disable dma mapping support for such systems, if drivers
> really want to work on such systems, they have to implement
> their own DMA-mapping fallback tracking outside page_pool.
>
> Signed-off-by: Yunsheng Lin <[email protected]>
> ---
> include/linux/mm_types.h | 13 +------------
> include/net/page_pool.h | 12 +-----------
> net/core/page_pool.c | 10 ++++++----
> 3 files changed, 8 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 7f8ee09c711f..436e0946d691 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -104,18 +104,7 @@ struct page {
> struct page_pool *pp;
> unsigned long _pp_mapping_pad;
> unsigned long dma_addr;
> - union {
> - /**
> - * dma_addr_upper: might require a 64-bit
> - * value on 32-bit architectures.
> - */
> - unsigned long dma_addr_upper;
> - /**
> - * For frag page support, not supported in
> - * 32-bit architectures with 64-bit DMA.
> - */
> - atomic_long_t pp_frag_count;
> - };
> + atomic_long_t pp_frag_count;
> };
> struct { /* slab, slob and slub */
> union {
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index a4082406a003..3855f069627f 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -216,24 +216,14 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> page_pool_put_full_page(pool, page, true);
> }
>
> -#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
> - (sizeof(dma_addr_t) > sizeof(unsigned long))
> -
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - dma_addr_t ret = page->dma_addr;
> -
> - if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
> - ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16;
> -
> - return ret;
> + return page->dma_addr;
> }
>
> static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> {
> page->dma_addr = addr;
> - if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
> - page->dma_addr_upper = upper_32_bits(addr);
> }
>
> static inline void page_pool_set_frag_count(struct page *page, long nr)
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 1a6978427d6c..a65bd7972e37 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
> * which is the XDP_TX use-case.
> */
> if (pool->p.flags & PP_FLAG_DMA_MAP) {
> + /* DMA-mapping is not supported on 32-bit systems with
> + * 64-bit DMA mapping.
> + */
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + return -EINVAL;
> +
> if ((pool->p.dma_dir != DMA_FROM_DEVICE) &&
> (pool->p.dma_dir != DMA_BIDIRECTIONAL))
> return -EINVAL;
> @@ -69,10 +75,6 @@ static int page_pool_init(struct page_pool *pool,
> */
> }
>
> - if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> - pool->p.flags & PP_FLAG_PAGE_FRAG)
> - return -EINVAL;
> -
> if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0)
> return -ENOMEM;
>
> --
> 2.33.0
>
Reviewed-by: Ilias Apalodimas <[email protected]>
On 22/09/2021 11.41, Yunsheng Lin wrote:
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 1a6978427d6c..a65bd7972e37 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
> * which is the XDP_TX use-case.
> */
> if (pool->p.flags & PP_FLAG_DMA_MAP) {
> + /* DMA-mapping is not supported on 32-bit systems with
> + * 64-bit DMA mapping.
> + */
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + return -EINVAL;
As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.
I suggest using EOPNOTSUPP 95 (Operation not supported).
-Jesper
Hi Jesper,
On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
<[email protected]> wrote:
>
>
> On 22/09/2021 11.41, Yunsheng Lin wrote:
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 1a6978427d6c..a65bd7972e37 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
> > * which is the XDP_TX use-case.
> > */
> > if (pool->p.flags & PP_FLAG_DMA_MAP) {
> > + /* DMA-mapping is not supported on 32-bit systems with
> > + * 64-bit DMA mapping.
> > + */
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + return -EINVAL;
>
> As I said before, can we please use another error than EINVAL.
> We should give drivers a chance/ability to detect this error, and e.g.
> fallback to doing DMA mappings inside driver instead.
>
> I suggest using EOPNOTSUPP 95 (Operation not supported).
I am fine with both. In any case though the aforementioned driver can
just remove PP_FLAG_DMA_MAP and do it's own mappings.
Regards
/Ilias
>
> -Jesper
>
On 2021/9/23 18:02, Ilias Apalodimas wrote:
> Hi Jesper,
>
> On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
> <[email protected]> wrote:
>>
>>
>> On 22/09/2021 11.41, Yunsheng Lin wrote:
>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>> index 1a6978427d6c..a65bd7972e37 100644
>>> --- a/net/core/page_pool.c
>>> +++ b/net/core/page_pool.c
>>> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
>>> * which is the XDP_TX use-case.
>>> */
>>> if (pool->p.flags & PP_FLAG_DMA_MAP) {
>>> + /* DMA-mapping is not supported on 32-bit systems with
>>> + * 64-bit DMA mapping.
>>> + */
>>> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
>>> + return -EINVAL;
>>
>> As I said before, can we please use another error than EINVAL.
>> We should give drivers a chance/ability to detect this error, and e.g.
>> fallback to doing DMA mappings inside driver instead.
>>
>> I suggest using EOPNOTSUPP 95 (Operation not supported).
Will change it to EOPNOTSUPP, thanks.
>
> I am fine with both. In any case though the aforementioned driver can
> just remove PP_FLAG_DMA_MAP and do it's own mappings.
>
> Regards
> /Ilias
>>
>> -Jesper
>>
> .
>
On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:
> On 2021/9/23 18:02, Ilias Apalodimas wrote:
> > Hi Jesper,
> >
> > On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
> > <[email protected]> wrote:
> >>
> >>
> >> On 22/09/2021 11.41, Yunsheng Lin wrote:
> >>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> >>> index 1a6978427d6c..a65bd7972e37 100644
> >>> --- a/net/core/page_pool.c
> >>> +++ b/net/core/page_pool.c
> >>> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
> >>> * which is the XDP_TX use-case.
> >>> */
> >>> if (pool->p.flags & PP_FLAG_DMA_MAP) {
> >>> + /* DMA-mapping is not supported on 32-bit systems with
> >>> + * 64-bit DMA mapping.
> >>> + */
> >>> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> >>> + return -EINVAL;
> >>
> >> As I said before, can we please use another error than EINVAL.
> >> We should give drivers a chance/ability to detect this error, and e.g.
> >> fallback to doing DMA mappings inside driver instead.
> >>
> >> I suggest using EOPNOTSUPP 95 (Operation not supported).
>
> Will change it to EOPNOTSUPP, thanks.
Mind sending this one separately (and you can keep my reviewed-by). It
fits nicely on it's own and since I am not sure about the rest of the
changes yet, it would be nice to get this one in.
Cheers
/Ilias
>
> >
> > I am fine with both. In any case though the aforementioned driver can
> > just remove PP_FLAG_DMA_MAP and do it's own mappings.
> >
> > Regards
> > /Ilias
> >>
> >> -Jesper
> >>
> > .
> >
On 2021/9/23 21:07, Ilias Apalodimas wrote:
> On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:
>> On 2021/9/23 18:02, Ilias Apalodimas wrote:
>>> Hi Jesper,
>>>
>>> On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
>>> <[email protected]> wrote:
>>>>
>>>>
>>>> On 22/09/2021 11.41, Yunsheng Lin wrote:
>>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>>>> index 1a6978427d6c..a65bd7972e37 100644
>>>>> --- a/net/core/page_pool.c
>>>>> +++ b/net/core/page_pool.c
>>>>> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
>>>>> * which is the XDP_TX use-case.
>>>>> */
>>>>> if (pool->p.flags & PP_FLAG_DMA_MAP) {
>>>>> + /* DMA-mapping is not supported on 32-bit systems with
>>>>> + * 64-bit DMA mapping.
>>>>> + */
>>>>> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
>>>>> + return -EINVAL;
>>>>
>>>> As I said before, can we please use another error than EINVAL.
>>>> We should give drivers a chance/ability to detect this error, and e.g.
>>>> fallback to doing DMA mappings inside driver instead.
>>>>
>>>> I suggest using EOPNOTSUPP 95 (Operation not supported).
>>
>> Will change it to EOPNOTSUPP, thanks.
>
> Mind sending this one separately (and you can keep my reviewed-by). It
> fits nicely on it's own and since I am not sure about the rest of the
> changes yet, it would be nice to get this one in.
I am not sure sending this one separately really makes sense, as it is
mainly used to make supporting the "keep track of pp page when __skb_frag_ref()
is called" in patch 5 easier.
>
> Cheers
> /Ilias
>>
>>>
>>> I am fine with both. In any case though the aforementioned driver can
>>> just remove PP_FLAG_DMA_MAP and do it's own mappings.
>>>
>>> Regards
>>> /Ilias
>>>>
>>>> -Jesper
>>>>
>>> .
>>>
> .
>
On Fri, 24 Sept 2021 at 10:04, Yunsheng Lin <[email protected]> wrote:
>
> On 2021/9/23 21:07, Ilias Apalodimas wrote:
> > On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:
> >> On 2021/9/23 18:02, Ilias Apalodimas wrote:
> >>> Hi Jesper,
> >>>
> >>> On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
> >>> <[email protected]> wrote:
> >>>>
> >>>>
> >>>> On 22/09/2021 11.41, Yunsheng Lin wrote:
> >>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> >>>>> index 1a6978427d6c..a65bd7972e37 100644
> >>>>> --- a/net/core/page_pool.c
> >>>>> +++ b/net/core/page_pool.c
> >>>>> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
> >>>>> * which is the XDP_TX use-case.
> >>>>> */
> >>>>> if (pool->p.flags & PP_FLAG_DMA_MAP) {
> >>>>> + /* DMA-mapping is not supported on 32-bit systems with
> >>>>> + * 64-bit DMA mapping.
> >>>>> + */
> >>>>> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> >>>>> + return -EINVAL;
> >>>>
> >>>> As I said before, can we please use another error than EINVAL.
> >>>> We should give drivers a chance/ability to detect this error, and e.g.
> >>>> fallback to doing DMA mappings inside driver instead.
> >>>>
> >>>> I suggest using EOPNOTSUPP 95 (Operation not supported).
> >>
> >> Will change it to EOPNOTSUPP, thanks.
> >
> > Mind sending this one separately (and you can keep my reviewed-by). It
> > fits nicely on it's own and since I am not sure about the rest of the
> > changes yet, it would be nice to get this one in.
>
> I am not sure sending this one separately really makes sense, as it is
> mainly used to make supporting the "keep track of pp page when __skb_frag_ref()
> is called" in patch 5 easier.
It rips out support for devices that are 32bit and have 64bit dma and
make the whole code easier to follow. I thought we agreed on removing
the support for those devices regardless didn't we?
Regards
/Ilias
On 2021/9/24 15:25, Ilias Apalodimas wrote:
> On Fri, 24 Sept 2021 at 10:04, Yunsheng Lin <[email protected]> wrote:
>>
>> On 2021/9/23 21:07, Ilias Apalodimas wrote:
>>> On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:
>>>> On 2021/9/23 18:02, Ilias Apalodimas wrote:
>>>>> Hi Jesper,
>>>>>
>>>>> On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>> On 22/09/2021 11.41, Yunsheng Lin wrote:
>>>>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>>>>>> index 1a6978427d6c..a65bd7972e37 100644
>>>>>>> --- a/net/core/page_pool.c
>>>>>>> +++ b/net/core/page_pool.c
>>>>>>> @@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
>>>>>>> * which is the XDP_TX use-case.
>>>>>>> */
>>>>>>> if (pool->p.flags & PP_FLAG_DMA_MAP) {
>>>>>>> + /* DMA-mapping is not supported on 32-bit systems with
>>>>>>> + * 64-bit DMA mapping.
>>>>>>> + */
>>>>>>> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
>>>>>>> + return -EINVAL;
>>>>>>
>>>>>> As I said before, can we please use another error than EINVAL.
>>>>>> We should give drivers a chance/ability to detect this error, and e.g.
>>>>>> fallback to doing DMA mappings inside driver instead.
>>>>>>
>>>>>> I suggest using EOPNOTSUPP 95 (Operation not supported).
>>>>
>>>> Will change it to EOPNOTSUPP, thanks.
>>>
>>> Mind sending this one separately (and you can keep my reviewed-by). It
>>> fits nicely on it's own and since I am not sure about the rest of the
>>> changes yet, it would be nice to get this one in.
>>
>> I am not sure sending this one separately really makes sense, as it is
>> mainly used to make supporting the "keep track of pp page when __skb_frag_ref()
>> is called" in patch 5 easier.
>
> It rips out support for devices that are 32bit and have 64bit dma and
> make the whole code easier to follow. I thought we agreed on removing
> the support for those devices regardless didn't we?
I am actually not convinced that the code about PAGE_POOL_DMA_USE_PP_FRAG_COUNT
(maybe the name is somewhat confusiong) as it it now, but it is after adding patch
5, and it seems we are not handing the skb_split() case in tso_fragment() for 32bit
arch with 64bit dma too if we still keep PAGE_POOL_DMA_USE_PP_FRAG_COUNT macro.
>
> Regards
> /Ilias
> .
>