On Fri, 27 Sept 2024 at 12:50, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > > On 2024/9/27 17:21, Ilias Apalodimas wrote: > > Hi Yunsheng > > > > On Fri, 27 Sept 2024 at 06:58, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > >> > >> On 2024/9/27 2:15, Mina Almasry wrote: > >>> > >>>> In order not to do the dma unmmapping after driver has already > >>>> unbound and stall the unloading of the networking driver, add > >>>> the pool->items array to record all the pages including the ones > >>>> which are handed over to network stack, so the page_pool can > >>>> do the dma unmmapping for those pages when page_pool_destroy() > >>>> is called. > >>> > >>> One thing I could not understand from looking at the code: if the > >>> items array is in the struct page_pool, why do you need to modify the > >>> page_pool entry in the struct page and in the struct net_iov? I think > >>> the code could be made much simpler if you can remove these changes, > >>> and you wouldn't need to modify the public api of the page_pool. > >> > >> As mentioned in [1]: > >> "There is no space in 'struct page' to track the inflight pages, so > >> 'pp' in 'struct page' is renamed to 'pp_item' to enable the tracking > >> of inflight page" > > > > I have the same feeling as Mina here. First of all, we do have an > > unsigned long in struct page we use for padding IIRC. More > > I am assuming you are referring to '_pp_mapping_pad' in 'struct page', > unfortunately the field might be used when a page is mmap'ed to user > space as my understanding. > Ah good point, I just grepped for it and didn't look at the surrounding unions. > https://elixir.bootlin.com/linux/v6.7-rc8/source/include/linux/mm_types.h#L126 > > > importantly, though, why does struct page need to know about this? > > Can't we have the same information in page pool? > > When the driver allocates pages it does via page_pool_dev_alloc_XXXXX > > or something similar. Cant we do what you suggest here ? IOW when we > > allocate a page we put it in a list, and when that page returns to > > page_pool (and it's mapped) we remove it. > > Yes, that is the basic idea, but the important part is how to do that > with less performance impact. Yes, but do you think that keeping that list of allocated pages in struct page_pool will end up being more costly somehow compared to struct page? Thanks /Ilias
