Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]

> On 19 Nov 2019, at 7:38, Lorenzo Bianconi wrote:
> 
> > [...]
> > > > > > -		page_pool_recycle_direct(rxq->page_pool,
> > > > > > -					 virt_to_head_page(xdp->data));
> > > > > > +		__page_pool_put_page(rxq->page_pool,
> > > > > > +				     virt_to_head_page(xdp->data),
> > > > > > +				     xdp->data_end - xdp->data_hard_start,
> > > > > > +				     true);
> > > > > 
> > > > > This does beg for the question: Should we create an API wrapper for
> > > > > this in the header file?
> > > > > 
> > > > > But what to name it?
> > > > > 
> > > > > I know Jonathan doesn't like the "direct" part of the
> > > > > previous function
> > > > > name page_pool_recycle_direct.  (I do considered calling
> > > > > this 'napi'
> > > > > instead, as it would be inline with networking use-cases,
> > > > > but it seemed
> > > > > limited if other subsystem end-up using this).
> > > > > 
> > > > > Does is 'page_pool_put_page_len' sound better?
> > > > > 
> > > > > But I want also want hide the bool 'allow_direct' in the API name.
> > > > > (As it makes it easier to identify users that uses this from
> > > > > softirq)
> > > > > 
> > > > > Going for 'page_pool_put_page_len_napi' starts to be come
> > > > > rather long.
> > > > 
> > > > What about removing the second 'page'? Something like:
> > > > - page_pool_put_len_napi()
> > > 
> > > Well, we (unfortunately) already have page_pool_put(), which is used
> > > for refcnt on the page_pool object itself.
> > 
> > __page_pool_put_page(pp, data, len, true) is a more generic version of
> > page_pool_recycle_direct where we can specify even the length. So what
> > about:
> > 
> > - page_pool_recycle_len_direct
> > - page_pool_recycle_len_napi
> 
> I'd suggest:
> 
> /* elevated refcounts, page may seen by networking stack */
> page_pool_drain(pool, page, count)              /* non napi, len = -1 */
> page_pool_drain_direct(pool, page, count)       /* len = -1 */
> 
> page_pool_check_put_page(page)                  /* may not belong to pool */
> 
> /* recycle variants drain/expect refcount == 1 */
> page_pool_recycle(pool, page, len)
> page_pool_recycle_direct(pool, page, len)
> 
> page_pool_put_page(pool, page, len, mode)	    /* generic, for __xdp_return

I am not against the suggestion but personally I would prefer to explicitate in
the routine name where/how it is actually used. Moreover page_pool_recycle_direct or
page_pool_put_page are currently used by multiple drivers and it seems to me
out of the scope of this series. I think we can address it in a follow-up series
and use __page_pool_put_page for the moment (it is actually just used by mvneta).
Agree?

Regards,
Lorenzo

> */
> 
> 
> I'd rather add len as a parameter, than add more wrapper variants.
> -- 
> Jonathan
> 
> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > -- 
> > > Best regards,
> > >   Jesper Dangaard Brouer
> > >   MSc.CS, Principal Kernel Engineer at Red Hat
> > >   LinkedIn: http://www.linkedin.com/in/brouer
> > > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]