On 2023/3/6 19:58, Alexander Lobakin wrote: > From: Yunsheng Lin <linyunsheng@xxxxxxxxxx> > Date: Mon, 6 Mar 2023 09:09:31 +0800 > >> On 2023/3/3 21:26, Alexander Lobakin wrote: >>> From: Yunsheng Lin <linyunsheng@xxxxxxxxxx> >>> Date: Fri, 3 Mar 2023 20:44:24 +0800 >>> >>>> On 2023/3/3 19:22, Alexander Lobakin wrote: >>>>> From: Yunsheng Lin <linyunsheng@xxxxxxxxxx> >>>>> Date: Thu, 2 Mar 2023 10:30:13 +0800 >>> >>> [...] >>> >>>>> And they are fixed :D >>>>> No drivers currently which use Page Pool mix PP pages with non-PP. And >>>> >>>> The wireless adapter which use Page Pool *does* mix PP pages with >>>> non-PP, see below discussion: >>>> >>>> https://lore.kernel.org/netdev/156f3e120bd0757133cb6bc11b76889637b5e0a6.camel@xxxxxxxxx/ >>> >>> Ah right, I remember that (also was fixed). >>> Not that I think it is correct to mix them -- for my PoV, a driver >>> shoule either give *all* its Rx buffers as PP-backed or not use PP at all. >>> >>> [...] >>> >>>>> As Jesper already pointed out, not having a quick way to check whether >>>>> we have to check ::pp_magic at all can decrease performance. So it's >>>>> rather a shortcut. >>>> >>>> When we are freeing a page by updating the _refcount, I think >>>> we are already touching the cache of ::pp_magic. >>> >>> But no page freeing happens before checking for skb->pp_recycle, neither >>> in skb_pp_recycle() (skb_free_head() etc.)[0] nor in skb_frag_unref()[1]. >> >> If we move to per page marker, we probably do not need checking >> skb->pp_recycle. >> >> Note both page_pool_return_skb_page() and skb_free_frag() can >> reuse the cache line triggered by per page marker checking if >> the per page marker is in the 'struct page'. > > Ah, from that perspective. Yes, you're probably right, but would need to > be tested anyway. I don't see any open problems with the PP recycling > right now on the lists, but someone may try to change it one day. > Anyway, this flag is only to do a quick test. We do have > sk_buff::pfmemalloc, but this flag doesn't mean every page from this skb > was pfmemalloced. The point seems to be that sk_buff::pfmemalloc allow false positive, which means skb->pfmemalloc can be set to true while every page from this skb is not pfmemalloced as you mentioned. While skb->pp_recycle can't allow false positive, if that happens, reference counting of the page will not be handled properly if pp and non-pp skb shares the page as the wireless adapter does. > >> >>> >>>> >>>> Anyway, I am not sure checking ::pp_magic is correct when a >>>> page will be passing between different subsystem and back to >>>> the network stack eventually, checking ::pp_magic may not be >>>> correct if this happens. >>>> >>>> Another way is to use the bottom two bits in bv_page, see: >>>> https://www.spinics.net/lists/netdev/msg874099.html >>>> >>>>> >>>>>> >>>>>>> >>>>>>> /* Allow SKB to reuse area used by xdp_frame */ >>>>>>> xdp_scrub_frame(xdpf); >>>>>>> >>>>> >>>>> Thanks, >>>>> Olek >>>>> . >>>>> >>> >>> [0] https://elixir.bootlin.com/linux/latest/source/net/core/skbuff.c#L808 >>> [1] >>> https://elixir.bootlin.com/linux/latest/source/include/linux/skbuff.h#L3385 >>> >>> Thanks, >>> Olek >>> . >>> > > Thanks, > Olek > > . >
