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: <188EF030-DAB4-4FD0-AFD1-107C24A10943@gmail.com>
Date:   Tue, 19 Nov 2019 14:23:05 -0800
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Lorenzo Bianconi" <lorenzo.bianconi@...hat.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'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
>>