On 2021/7/6 12:54, Ilias Apalodimas wrote: > Hi Yunsheng, > > Thanks for having a look! Hi, Thanks for reviewing. > > On Fri, Jul 02, 2021 at 06:15:13PM +0800, Yunsheng Lin wrote: >> On 2021/7/2 17:42, Jesper Dangaard Brouer wrote: >>> >>> On 30/06/2021 11.17, Yunsheng Lin wrote: >>>> Currently page pool only support page recycling only when >>>> refcnt of page is one, which means it can not support the >>>> split page recycling implemented in the most ethernet driver. >>> >>> Cc. Alex Duyck as I consider him an expert in this area. >> >> Thanks. >> >>> >>> >>>> So add elevated refcnt support in page pool, and support >>>> allocating page frag to enable multi-frames-per-page based >>>> on the elevated refcnt support. >>>> >>>> As the elevated refcnt is per page, and there is no space >>>> for that in "struct page" now, so add a dynamically allocated >>>> "struct page_pool_info" to record page pool ptr and refcnt >>>> corrsponding to a page for now. Later, we can recycle the >>>> "struct page_pool_info" too, or use part of page memory to >>>> record pp_info. >>> >>> I'm not happy with allocating a memory (slab) object "struct page_pool_info" per page. >>> >>> This also gives us an extra level of indirection. >> >> I'm not happy with that either, if there is better way to >> avoid that, I will be happy to change it:) > > I think what we have to answer here is, do we want and does it make sense > for page_pool to do the housekeeping of the buffer splitting or are we > better of having each driver do that. IIRC your previous patch on top of > the original recycling patchset was just 'atomic' refcnts on top of page pool. You are right that driver was doing the the buffer splitting in previous patch. The reason why I abandoned that is: 1. Currently the meta-data of page in the driver is per desc, which means it might not be able to use first half of a page for a desc, and the second half of the same page for another desc, this ping-pong way of reusing the whole page for only one desc in the driver seems unnecessary and waste a lot of memory when there is already reusing in the page pool. 2. Easy use of API for the driver too, which means the driver uses page_pool_dev_alloc_frag() and page_pool_put_full_page() for elevated refcnt case, corresponding to page_pool_dev_alloc_pages() and page_pool_put_full_page() for non-elevated refcnt case, the driver does not need to worry about the meta-data of a page. > > I think I'd prefer each driver having it's own meta-data of how he splits > the page, mostly due to hardware diversity, but tbh I don't have any > strong preference atm. Usually how the driver split the page is fixed for a given rx configuration( like MTU), so the driver is able to pass that info to page pool. > >> >>> >>> >>> You are also adding a page "frag" API inside page pool, which I'm not 100% convinced belongs inside page_pool APIs. >>> >>> Please notice the APIs that Alex Duyck added in mm/page_alloc.c: >> >> Actually, that is where the idea of using "page frag" come from. >> >> Aside from the performance improvement, there is memory usage >> decrease for 64K page size kernel, which means a 64K page can >> be used by 32 description with 2k buffer size, and that is a >> lot of memory saving for 64 page size kernel comparing to the >> current split page reusing implemented in the driver. >> > > Whether the driver or page_pool itself keeps the meta-data, the outcome > here won't change. We'll still be able to use page frags. As above, it is the ping-pong way of reusing when the driver keeps the meta-data, and it is page-frag way of reusing when the page pool keeps the meta-data. I am not sure if the page-frag way of reusing is possible when we still keep the meta-data in the driver, which seems very complex at the initial thinking. > > > Cheers > /Ilias >> >>> >>> __page_frag_cache_refill() + __page_frag_cache_drain() + page_frag_alloc_align() >>> >>> >> >> [...] > . >
