On 8/11/24 03:21, Mina Almasry wrote:
On Fri, Aug 9, 2024 at 11:52 PM Jakub Kicinski <kuba@xxxxxxxxxx> 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
