Re: [RFC PATCH net-next v1 4/4] net: page_pool: use netmem_t instead of struct page in API

[Mina Almasry]

On Thu, Dec 21, 2023 at 10:42 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2023/12/22 5:22, Mina Almasry wrote:
> > On Thu, Dec 21, 2023 at 3:32 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> >>
> >> On 2023/12/20 11:01, Mina Almasry wrote:
> >>
> >> ...
> >>
> >>>>>> Perhaps we should aim to not export netmem_to_page(),
> >>>>>> prevent modules from accessing it directly.
> >>>>>
> >>>>> +1.
> >>>>
> >>>
> >>> I looked into this, but it turns out it's a slightly bigger change
> >>> that needs some refactoring to make it work. There are few places
> >>> where I believe I need to add netmem_to_page() that are exposed to the
> >>> drivers via inline helpers, these are:
> >>>
> >>> - skb_frag_page(), which returns NULL if the netmem is not a page, but
> >>> needs to do a netmem_to_page() to return the page otherwise.
> >>
> >> Is it possible to introduce something like skb_frag_netmem() for
> >> netmem? so that we can keep most existing users of skb_frag_page()
> >> unchanged and avoid adding additional checking overhead for existing
> >> users.
> >>
> >
> > In my experience most current skb_frag_page() users need specifically
> > the struct page*. Example is illegal_highdma() which
> > PageHighMem(skb_frag_page())
>
> For illegal_highdma() case, is it possible to use something like
> skb_readabe_frag() checking to avoid calling skb_frag_page() for netmem?
>

Not teh skb_readable_frag() check I think, because illegal_highdma()
is not trying to read the SKB per se.

But I agree with your general point, and that should be handled
correctly in this patch which adds devmem support:

https://patchwork.kernel.org/project/netdevbpf/patch/20231218024024.3516870-10-almasrymina@xxxxxxxxxx/

The idea being that skb_frag_page() can return NULL if the frag is not
paged, and the relevant callers are modified to handle that.

> >
> > But RFC v5 adds skb_frag_netmem() for callsites that want a netmem and
> > don't care about specifically a page:
> >
> > https://patchwork.kernel.org/project/netdevbpf/patch/20231218024024.3516870-10-almasrymina@xxxxxxxxxx/
> >
> >>> - The helpers inside skb_add_rx_frag(), which needs to do a
> >>> netmem_to_page() to set skb->pfmemalloc.
> >>
> >> Similar as above, perhaps introduce something like skb_add_rx_netmem_frag()?
> >>
> >
> > Yes, v3 of this series adds skb_add_rx_frag_netmem():
> >
> > https://patchwork.kernel.org/project/netdevbpf/patch/20231220214505.2303297-4-almasrymina@xxxxxxxxxx/



-- 
Thanks,
Mina