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] [day] [month] [year] [list]

Date:   Wed, 20 Nov 2019 08:29:11 -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 20 Nov 2019, at 1:21, Lorenzo Bianconi wrote:

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

Fine with me - I have a naming cleanup patch pending, I can roll it into
this afterwards.
-- 
Jonathan