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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191120092120.GA2538@localhost.localdomain>
Date:   Wed, 20 Nov 2019 11:21:20 +0200
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, ilias.apalodimas@...aro.org, mcroce@...hat.com
Subject: Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in
 mvneta_rx_refill

> 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
> > > 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)