Re: [RFC, PATCH] net: page_pool: Don't use page->private to store dma_addr_t

From: Jesper Dangaard Brouer <brouer@redhat.com>
To: Tariq Toukan <tariqt@mellanox.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Matthew Wilcox <willy@infradead.org>,
	David Miller <davem@davemloft.net>,
	"toke@redhat.com" <toke@redhat.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"mgorman@techsingularity.net" <mgorman@techsingularity.net>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	brouer@redhat.com
Subject: Re: [RFC, PATCH] net: page_pool: Don't use page->private to store dma_addr_t
Date: Tue, 12 Feb 2019 14:49:38 +0100	[thread overview]
Message-ID: <20190212144938.36dd45b4@carbon> (raw)
In-Reply-To: <27e97aac-f25b-d46c-3e70-7d0d44f784b5@mellanox.com>

On Tue, 12 Feb 2019 12:39:59 +0000
Tariq Toukan <tariqt@mellanox.com> 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"

Yes, exactly.

> > 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.
> 
> However, what bothers me more is another issue.
> The optimization doesn't cleanly combine with the new page_pool 
> direction for maintaining a queue for "available" pages, as the put_page 
> flow would need to read pageref_bias, asynchronously, and act accordingly.
> 
> The suggested hook in put_page (to catch the 2 -> 1 "biased refcnt" 
> transition) causes a problem to the traditional pageref_bias idea, as it 
> implies a new point in which the pageref_bias field is read 
> *asynchronously*. This would risk missing the this critical 2 -> 1 
> transition! Unless pageref_bias is atomic...

I want to stop you here...

It seems to me that you are trying to shoehorn in a refcount
optimization into page_pool.  The page_pool is optimized for the XDP
case of one-frame-per-page, where we can avoid changing the refcount,
and tradeoff memory usage for speed.  It is compatible with the elevated
refcount usage, but that is not the optimization target.

If the case you are optimizing for is "packing" more frames in a page,
then the page_pool might be the wrong choice.  To me it would make more
sense to create another enum xdp_mem_type, that generalize the
pageref_bias tricks also used by some drivers.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer