On 2/12/2019 8:20 PM, Ilias Apalodimas wrote: > Hi Alexander, > > On Tue, Feb 12, 2019 at 10:13:30AM -0800, Alexander Duyck wrote: >> On Tue, Feb 12, 2019 at 7:16 AM Eric Dumazet <eric.duma...@gmail.com> wrote: >>> >>> >>> >>> On 02/12/2019 04:39 AM, Tariq Toukan wrote: >>>> >>>> >>>> On 2/11/2019 7:14 PM, Eric Dumazet wrote: >>>>> >>>>> >>>>> On 02/11/2019 12:53 AM, Tariq Toukan wrote: >>>>>> >>>>> >>>>>> Hi, >>>>>> >>>>>> It's great to use the struct page to store its dma mapping, but I am >>>>>> worried about extensibility. >>>>>> page_pool is evolving, and it would need several more per-page fields. >>>>>> One of them would be pageref_bias, a planned optimization to reduce the >>>>>> number of the costly atomic pageref operations (and replace existing >>>>>> code in several drivers). >>>>>> >>>>> >>>>> But the point about pageref_bias is to place it in a different cache line >>>>> than "struct page" >>>>> >>>>> The major cost is having a cache line bouncing between producer and >>>>> consumer. >>>>> >>>> >>>> pageref_bias is meant to be dirtied only by the page requester, i.e. the >>>> NIC driver / page_pool. >>>> All other components (basically, SKB release flow / put_page) should >>>> continue working with the atomic page_refcnt, and not dirty the >>>> pageref_bias. >>> >>> This is exactly my point. >>> >>> You suggested to put pageref_bias in struct page, which breaks this >>> completely. >>> >>> pageref_bias is better kept in a driver structure, with appropriate >>> prefetching >>> since most NIC use a ring buffer for their queues. >>> >>> The dma address _can_ be put in the struct page, since the driver does not >>> dirty it >>> and does not even read it when page can be recycled. >> >> Instead of maintaining the pageref_bias in the page itself it could be >> maintained in some sort of separate structure. You could just maintain >> a pointer to a slot in an array somewhere. Then you can still access >> it if needed, the pointer would be static for as long as it is in the >> page pool, and you could invalidate the pointer prior to removing the >> bias from the page. > > I think that's what Tariq was suggesting in the first place. > > /Ilias >
Correct. But not relevant anymore, as it won't work for other reasons. Tariq
