| Message ID | 20231121000048.789613-9-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: page_pool: add netlink-based introspection | expand |
Jakub Kicinski wrote: > Add a Netlink spec in YAML for getting very basic information > about page pools. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > Documentation/netlink/specs/netdev.yaml | 46 +++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml > index 14511b13f305..84ca3c2ab872 100644 > --- a/Documentation/netlink/specs/netdev.yaml > +++ b/Documentation/netlink/specs/netdev.yaml > @@ -86,6 +86,34 @@ name: netdev > See Documentation/networking/xdp-rx-metadata.rst for more details. > type: u64 > enum: xdp-rx-metadata > + - > + name: page-pool > + attributes: > + - > + name: id > + doc: Unique ID of a Page Pool instance. > + type: uint > + checks: > + min: 1 > + max: u32-max > + - > + name: ifindex > + doc: | > + ifindex of the netdev to which the pool belongs. > + May be reported as 0 if the page pool was allocated for a netdev > + which got destroyed already (page pools may outlast their netdevs > + because they wait for all memory to be returned). > + type: u32 > + checks: > + min: 1 > + max: s32-max > + - > + name: napi-id > + doc: Id of NAPI using this Page Pool instance. > + type: uint > + checks: > + min: 1 > + max: u32-max Do you want to introduce a separate ID for page pools? That brings some issues regarding network namespace isolation. As a user API, it is also possible (and intuitive?) to refer to a page_pool by (namespacified) ifindex plus netdev_rx_queue index, or napi_id. In fairness, napi_id is also global, not per netns. By iterating over "for_each_netdev(net, ..", dump already limits output to pools in the same netns and get only reports pools that match the netns. So it's only a minor matter of visible numbering, and perhaps superfluous new id. No technical comments to this series. Looks solid to me.
On Tue, 21 Nov 2023 13:24:07 -0500 Willem de Bruijn wrote: > Do you want to introduce a separate ID for page pools? That brings some > issues regarding network namespace isolation. > > As a user API, it is also possible (and intuitive?) to refer to a > page_pool by (namespacified) ifindex plus netdev_rx_queue index, > or napi_id. That does not work for "destroyed" pools. In general, there is no natural key for a page pool I can think of. > In fairness, napi_id is also global, not per netns. > > By iterating over "for_each_netdev(net, ..", dump already limits > output to pools in the same netns and get only reports pools that > match the netns. > > So it's only a minor matter of visible numbering, and perhaps > superfluous new id. The IDs are not stable. Any reconfiguration of a device will create a new page pool and therefore assign a new ID. So applications can't hold onto the ID long term. That said the only use case for exposing the ID right now is to implement do/GET (since there is no other unique key). And manual debug with drgn, but that doesn't require uAPI. So if you prefer strongly I can drop the ID from the uAPI and do/GET support.
Jakub Kicinski wrote: > On Tue, 21 Nov 2023 13:24:07 -0500 Willem de Bruijn wrote: > > Do you want to introduce a separate ID for page pools? That brings some > > issues regarding network namespace isolation. > > > > As a user API, it is also possible (and intuitive?) to refer to a > > page_pool by (namespacified) ifindex plus netdev_rx_queue index, > > or napi_id. > > That does not work for "destroyed" pools. In general, there is > no natural key for a page pool I can think of. Pools for destroyed devices are attached to the loopback device. If the netns is also destroyed, would it make sense to attach them to the loopback device in the init namespace? > > In fairness, napi_id is also global, not per netns. > > > > By iterating over "for_each_netdev(net, ..", dump already limits > > output to pools in the same netns and get only reports pools that > > match the netns. > > > > So it's only a minor matter of visible numbering, and perhaps > > superfluous new id. > > The IDs are not stable. Any reconfiguration of a device will create > a new page pool and therefore assign a new ID. So applications can't > hold onto the ID long term. > > That said the only use case for exposing the ID right now is to > implement do/GET (since there is no other unique key). And manual debug > with drgn, but that doesn't require uAPI. So if you prefer strongly > I can drop the ID from the uAPI and do/GET support. No, this is fine. I just wanted to make sure that the alternative api and netns details were considered beforehand, since it's uapi.
On Tue, 21 Nov 2023 16:33:25 -0500 Willem de Bruijn wrote: > > That does not work for "destroyed" pools. In general, there is > > no natural key for a page pool I can think of. > > Pools for destroyed devices are attached to the loopback device. > If the netns is also destroyed, would it make sense to attach > them to the loopback device in the init namespace? I remember discussing this somewhere in person... netconf? I opted for only exposing the cases which are obvious now, we can extend the API later, once we get some production experience. > > The IDs are not stable. Any reconfiguration of a device will create > > a new page pool and therefore assign a new ID. So applications can't > > hold onto the ID long term. > > > > That said the only use case for exposing the ID right now is to > > implement do/GET (since there is no other unique key). And manual debug > > with drgn, but that doesn't require uAPI. So if you prefer strongly > > I can drop the ID from the uAPI and do/GET support. > > No, this is fine. I just wanted to make sure that the alternative api > and netns details were considered beforehand, since it's uapi. Okay, let me ditch it for now, in the interest of making progress.. It's easily added later.
On 11/21/23 2:00 PM, Jakub Kicinski wrote: > On Tue, 21 Nov 2023 16:33:25 -0500 Willem de Bruijn wrote: >>> That does not work for "destroyed" pools. In general, there is >>> no natural key for a page pool I can think of. >> >> Pools for destroyed devices are attached to the loopback device. >> If the netns is also destroyed, would it make sense to attach >> them to the loopback device in the init namespace? > > I remember discussing this somewhere in person... netconf? I asked why not blackhole_dev for just that problem.
On Tue, 21 Nov 2023 14:49:31 -0800 David Ahern wrote: > > I remember discussing this somewhere in person... netconf? > > I asked why not blackhole_dev for just that problem. Ack, but I don't see myself blabbering about the hidden objects on that sub-thread now.
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml index 14511b13f305..84ca3c2ab872 100644 --- a/Documentation/netlink/specs/netdev.yaml +++ b/Documentation/netlink/specs/netdev.yaml @@ -86,6 +86,34 @@ name: netdev See Documentation/networking/xdp-rx-metadata.rst for more details. type: u64 enum: xdp-rx-metadata + - + name: page-pool + attributes: + - + name: id + doc: Unique ID of a Page Pool instance. + type: uint + checks: + min: 1 + max: u32-max + - + name: ifindex + doc: | + ifindex of the netdev to which the pool belongs. + May be reported as 0 if the page pool was allocated for a netdev + which got destroyed already (page pools may outlast their netdevs + because they wait for all memory to be returned). + type: u32 + checks: + min: 1 + max: s32-max + - + name: napi-id + doc: Id of NAPI using this Page Pool instance. + type: uint + checks: + min: 1 + max: u32-max operations: list: @@ -120,6 +148,24 @@ name: netdev doc: Notification about device configuration being changed. notify: dev-get mcgrp: mgmt + - + name: page-pool-get + doc: | + Get / dump information about Page Pools. + (Only Page Pools associated with a net_device can be listed.) + attribute-set: page-pool + do: + request: + attributes: + - id + reply: &pp-reply + attributes: + - id + - ifindex + - napi-id + dump: + reply: *pp-reply + config-cond: page-pool mcast-groups: list:
Add a Netlink spec in YAML for getting very basic information about page pools. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- Documentation/netlink/specs/netdev.yaml | 46 +++++++++++++++++++++++++ 1 file changed, 46 insertions(+)