Re: [PATCH net-next v18 07/14] memory-provider: dmabuf devmem memory provider

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/11/24 03:21, Mina Almasry wrote:
> On Fri, Aug 9, 2024 at 11:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Fri, 9 Aug 2024 16:45:50 +0100 Pavel Begunkov wrote:
>>>> I think this is good, and it doesn't seem hacky to me, because we can
>>>> check the page_pools of the netdev while we hold rtnl, so we can be
>>>> sure nothing is messing with the pp configuration in the meantime.
>>>> Like you say below it does validate the driver rather than rely on the
>>>> driver saying it's doing the right thing. I'll look into putting this
>>>> in the next version.
>>>
>>> Why not have a flag set by the driver and advertising whether it
>>> supports providers or not, which should be checked for instance in
>>> netdev_rx_queue_restart()? If set, the driver should do the right
>>> thing. That's in addition to a new pp_params flag explicitly telling
>>> if pp should use providers. It's more explicit and feels a little
>>> less hacky.
>>
>> You mean like I suggested in the previous two emails? :)
>>
>> Given how easy the check is to implement, I think it's worth
>> adding as a sanity check. But the flag should be the main API,
>> if the sanity check starts to be annoying we'll ditch it.
> 
> I think we're talking about 2 slightly different flags, AFAIU.>
> Pavel and I are suggesting the driver reports "I support memory
> providers" directly to core (via the queue-api or what not), and we
> check that flag directly in netdev_rx_queue_restart(), and fail
> immediately if the support is not there.

I might've misread Jakub, but yes, I believe it's different. It'd
communicate about support for providers to upper layers, so we can
fail even before attempting to allocate a new queue and init a
page pool.

> Jakub is suggesting a page_pool_params flag which lets the driver
> report "I support memory providers". If the driver doesn't support it
> but core is trying to configure that, then the page_pool_create will
> fail, which will cause the queue API operation
> (ndo_queue_alloc_mem_alloc) to fail, which causes
> netdev_rx_queue_restart() to fail.

And I'm not against this way either if we explicitly get an error
back instead of trying to figure it out post-factum like by
checking the references and possibly reverting the allocation.
Maybe that's where I was confused, and that refcount thing was
suggested as a WARN_ONCE?

FWIW, I think it warrants two flags. The first saying that the
driver supports providers at all:

page_pool_init() {
	if (rxq->mp_params)
		if (!(flags & PP_PROVIDERS_SUPPORTED))
			goto fail;
}

And the second telling whether the driver wants to install
providers for this particular page pool, so if there is a
separate pool for headers we can set it with plain old kernel
pages.

payload_pool = page_pool_create(rqx, PP_PROVIDERS_SUPPORTED);
header_pool = page_pool_create(rqx, PP_PROVIDERS_SUPPORTED |
                                     PP_IGNORE_PROVIDERS);

(or invert the flag). That's assuming page_pool_params::queue is
a generic thing and we don't want to draw equivalence between
it and memory providers.

-- 
Pavel Begunkov