Re: [PATCH net-next v6 1/6] page_pool: frag API support for 32-bit arch with 64-bit DMA

[Ilias Apalodimas]

On Thu, 17 Aug 2023 at 12:06, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2023/8/17 1:01, Ilias Apalodimas wrote:
> > On Wed, 16 Aug 2023 at 15:49, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> >>
> >> On 2023/8/16 19:26, Ilias Apalodimas wrote:
> >>> Hi Yunsheng
> >>>
> >>> On Mon, 14 Aug 2023 at 15:59, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> >>>>
> >>>> Currently page_pool_alloc_frag() is not supported in 32-bit
> >>>> arch with 64-bit DMA because of the overlap issue between
> >>>> pp_frag_count and dma_addr_upper in 'struct page' for those
> >>>> arches, which seems to be quite common, see [1], which means
> >>>> driver may need to handle it when using frag API.
> >>>
> >>> That wasn't so common. IIRC it was a single TI platform that was breaking?
> >>
> >> I am not so sure about that as grepping 'ARM_LPAE' has a long
> >> list for that.
> >
> > Shouldn't we be grepping for CONFIG_ARCH_DMA_ADDR_T_64BIT and
> > PHYS_ADDR_T_64BIT to find the affected platforms?  Why LPAE?
>
>
> I used the key in the  original report:
>
> https://www.spinics.net/lists/netdev/msg779890.html
>
> >> Please see the bisection report below about a boot failure on
> >> rk3288-rock2-square which is pointing to this patch.  The issue
> >> appears to only happen with CONFIG_ARM_LPAE=y.
>
> grepping the 'CONFIG_PHYS_ADDR_T_64BIT' seems to be more common?
> https://elixir.free-electrons.com/linux/v6.4-rc6/K/ident/CONFIG_PHYS_ADDR_T_64BIT
>

Yes, grepping around a bit uncovered this arch/arm/mm/Kconfig, which
enables PHYS_ADDR_T_64BIT if ARM_LPAE is enabled.  Then
ARCH_DMA_ADDR_T_64BIT
is also enabled from kernel/dma/Kconfig.  But that doesn't mean
grepping for any of those uncovers all the problematic platforms,
there are more than Arm platforms.  The ones that will actually fail
are
- ARCH_DMA_ADDR_T_64BIT is enabled and it's a 32bit architecture
- You have a network driver for that platform that uses page pool.

The combination of these shouldn't be that common.  The only one that
comes to mind is the stmmac driver, which the report was for.

> >
> >>
> >>>
> >>>>
> >>>> In order to simplify the driver's work when using frag API
> >>>> this patch allows page_pool_alloc_frag() to call
> >>>> page_pool_alloc_pages() to return pages for those arches.
> >>>
> >>> Do we have any use cases of people needing this?  Those architectures
> >>> should be long dead and although we have to support them in the
> >>> kernel,  I don't personally see the advantage of adjusting the API to
> >>> do that.  Right now we have a very clear separation between allocating
> >>> pages or fragments.   Why should we hide a page allocation under a
> >>> frag allocation?  A driver writer can simply allocate pages for those
> >>> boards.  Am I the only one not seeing a clean win here?
> >>
> >> It is also a part of removing the per page_pool PP_FLAG_PAGE_FRAG flag
> >> in this patchset.
> >
> > Yes, that happens *because* of this patchset.  I am not against the
> > change.  In fact, I'll have a closer look tomorrow.  I am just trying
> > to figure out if we really need it.  When the recycling patches were
> > introduced into page pool we had a very specific reason.  Due to the
> > XDP verifier we *had* to allocate a packet per page.  That was
>
> Did you mean a xdp frame containing a frag page can not be passed to the
> xdp core?
> What is exact reason why the XDP verifier need a packet per page?
> Is there a code block that you can point me to?

It's been a while since I looked at this, but doesn't __xdp_return()
still sync the entire page if the mem type comes from page_pool?

>
> I wonder if it is still the case for now, as bnxt and mlx5 seems to be
> supporting frag page and xdp now.

I only looked at bnxt, but that doesnt seem to be entirely true.  That
code still allocates pages if an XDP prog is installed.  The only case
where it allocates fragments is if the kernel is compiled with 65k
pages, but the hardware doesnt support that (check for
BNXT_RX_PAGE_SHIFT)


Thanks
/Ilias
>
> > expensive so we added the recycling capabilities to compensate and get
> > some performance back. Eventually we added page fragments and had a
> > very clear separation on the API.
> >
> > Regards
> > /Ilias
> >>
> >>>
> >>> Thanks
> >>> /Ilias
> >>>
> > .
> >