Re: [PATCH net-next v1 02/11] net: Add memory provider capabilities

[Björn Töpel]

Stanislav Fomichev <sdf.kernel@xxxxxxxxx> writes:

> On 11/11, Björn Töpel wrote:
>> Stanislav Fomichev <sdf.kernel@xxxxxxxxx> writes:
>> > On 11/10, Björn Töpel wrote:

>> >> +	if (static_branch_unlikely(&page_pool_mem_providers) &&
>> >> +	    WARN_ON_ONCE(!page_pool_supports_frag(pool)))
>> >> +		return 0;
>> >>  	if (WARN_ON(size > max_size))
>> >>  		return 0;
>> >>  
>> >
>> > What's the deal with that? How do we get here?
>> 
>> Hey! Thanks for taking a look at the series!
>> 
>> An explicit frag call on a whole-buffer provider is a driver bug: UMEM
>> chunks can be smaller than a page, and RX gives the whole chunk to
>> userspace.
>> 
>> But page_pool_alloc_netmem() also picks frags for small requests, so a
>> valid call can warn.
>> 
>> I think it would be better to make it return a whole buffer and its
>> actual size. The WARN stays in the frag API, before taking a new buffer?
>> Wdyt?
>
> I see.. Idk, I'd still return 0 here, but without a warn. Not sure
> we want a warn for any api misuse? Returning no netmem is gonna be enough
> to debug things? As for the calls from page_pool_alloc_netmem, I'd also
> handle it explicitly? if (size > max_size || !page_pool_supports_frag) ?
> But no strong preference, up to you, was mostly trying to understand
> what we are warning against here :-)

Yes, page_pool_alloc_netmem() gets the explicit check. Thanks!

For the frag API I'd keep a WARN_ON_ONCE, like the size check right
below it. A silent 0 looks like an empty provider: the queue just
stalls, and nothing points at the driver. With the helper fixed, only an
explicit frag call on such a pool gets there... Wdyt? I'll probably take
that route for v2.


Björn