Re: [RFC PATCH net-next v1 4/4] net: page_pool: use netmem_t instead of struct page in API

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/1/5 2:24, Mina Almasry wrote:
> On Thu, Jan 4, 2024 at 12:48 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2024/1/4 2:38, Mina Almasry wrote:
>>> On Wed, Jan 3, 2024 at 1:47 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>>
>>>> On 2024/1/3 0:14, Mina Almasry wrote:
>>>>>
>>>>> The idea being that skb_frag_page() can return NULL if the frag is not
>>>>> paged, and the relevant callers are modified to handle that.
>>>>
>>>> There are many existing drivers which are not expecting NULL returning for
>>>> skb_frag_page() as those drivers are not supporting devmem, adding additionl
>>>> checking overhead in skb_frag_page() for those drivers does not make much
>>>> sense, IMHO, it may make more sense to introduce a new helper for the driver
>>>> supporting devmem or networking core that needing dealing with both normal
>>>> page and devmem.
>>>>
>>>> And we are also able to keep the old non-NULL returning semantic for
>>>> skb_frag_page().
>>>
>>> I think I'm seeing agreement that the direction we're heading into
>>> here is that most net stack & drivers should use the abstract netmem
>>
>> As far as I see, at least for the drivers, I don't think we have a clear
>> agreement if we should have a unified driver facing struct or API for both
>> normal page and devmem yet.
>>
> 
> To be honest I definitely read that we have agreement that we should
> have a unified driver facing struct from the responses in this thread
> like this one:
> 
> https://lore.kernel.org/netdev/20231215190126.1040fa12@kernel.org/

Which specific comment made you thinking as above?
I think it definitely need clarifying here, as I read it differently as
you did.

> 
> But I'll let folks correct me if I'm wrong.
> 
>>> type, and only specific code that needs a page or devmem (like
>>> tcp_receive_zerocopy or tcp_recvmsg_dmabuf) will be the ones that
>>> unpack the netmem and get the underlying page or devmem, using
>>> skb_frag_page() or something like skb_frag_dmabuf(), etc.
>>>
>>> As Jason says repeatedly, I'm not allowed to blindly cast a netmem to
>>> a page and assume netmem==page. Netmem can only be cast to a page
>>> after checking the low bits and verifying the netmem is actually a
>>
>> I thought it would be best to avoid casting a netmem or devmem to a
>> page in the driver, I think the main argument is that it is hard
>> to audit very single driver doing a checking before doing the casting
>> in the future? and we can do better auditting if the casting is limited
>> to a few core functions in the networking core.
>>
> 
> Correct, the drivers should never cast directly, but helpers like
> skb_frag_page() must check that the netmem is a page before doing a
> cast.
> 
>>> page. I think any suggestions that blindly cast a netmem to page
>>> without the checks will get nacked by Jason & Christian, so the
>>> checking in the specific cases where the code needs to know the
>>> underlying memory type seems necessary.
>>>
>>> IMO I'm not sure the checking is expensive. With likely/unlikely &
>>> static branches the checks should be very minimal or a straight no-op.
>>> For example in RFC v2 where we were doing a lot of checks for devmem
>>> (we don't do that anymore for RFCv5), I had run the page_pool perf
>>> tests and proved there is little to no perf regression:
>>
>> For MAX_SKB_FRAGS being 17, it means we may have 17 additional checking
>> overhead for the drivers not supporting devmem, not to mention we may
>> have bigger value for MAX_SKB_FRAGS if BIG TCP is enable.
>>
> 
> With static branch the checks should be complete no-ops unless the
> user's set up enabled devmem.

What if the user does set up enabled devmem and still want to enable
page_pool for normal page in the same system?

Is there a reason I don't know, which stops you from keeping the old
helper and introducing a new helper if it is needed for the new netmem
thing?

> 
>> Even there is no notiable performance degradation for a specific case,
>> we should avoid the overhead as much as possible for the existing use
>> case when supporting a new use case.
>>
>>>
>>> https://lore.kernel.org/netdev/CAHS8izM4w2UETAwfnV7w+ZzTMxLkz+FKO+xTgRdtYKzV8RzqXw@mail.gmail.com/
>>
>> The above test case does not even seems to be testing a code path calling
>> skb_frag_page() as my understanding.
>>
>>>
> 
> 
>