Re: [RFC net-next 00/13] net: page_pool: add netlink-based introspection

On Wed, Aug 16, 2023 at 4:43 PM Jakub Kicinski [off-list ref] wrote:

As the page pool functionality grows we will be increasingly
constrained by the lack of any uAPI. This series is a first
step towards such a uAPI, it creates a way for users to query
information about page pools and associated statistics.

I'm purposefully exposing only the information which I'd
find useful when monitoring production workloads.

For the SET part (which to be clear isn't addressed by this
series at all) I think we'll need to turn to something more
along the lines of "netdev-level policy". Instead of configuring
page pools, which are somewhat ephemeral, and hard to change
at runtime, we should set the "expected page pool parameters"
at the netdev level, and have the driver consult those when
instantiating pools. My ramblings from yesterday about Queue API
may make this more or less clear...
https://lore.kernel.org/all/20230815171638.4c057dcd@kernel.org/ (local)

The patches themselves look good to me, and I'll provide Reviewed-by
for the ones I feel I understand well enough to review, but I'm a bit
unsure about exposing specifically page_pool stats to the user. Isn't
it better to expose rx-queue stats (slightly more generic) to the
user? In my mind, the advantages:

- we could implement better SET apis that allocate, delete, or
reconfigure rx-queues. APIs that allocate or delete page-pool make
less sense semantically maybe? The page-pool doesn't decide to
instantiate itself.

- The api can be extended for non-page-pool stats (per rx-queue
dropped packets maybe, or something like that).

- The api maybe can be extended to non-page-pool drivers. The driver
may be able to implement their own function to provide equivalent
stats (although this one may not be that important).

- rx-queue GET API fits in nicely with what you described yesterday
[1]. At the moment I'm a bit unsure because the SET api you described
yesterday sounded per-rx-queue to me. But the GET api here is
per-page-pool based. Maybe the distinction doesn't matter? Maybe
you're thinking they're unrelated APIs?

[1] https://lore.kernel.org/all/20230815171638.4c057dcd@kernel.org/ (local)

Jakub Kicinski (13):
  net: page_pool: split the page_pool_params into fast and slow
  net: page_pool: avoid touching slow on the fastpath
  net: page_pool: factor out uninit
  net: page_pool: id the page pools
  net: page_pool: record pools per netdev
  net: page_pool: stash the NAPI ID for easier access
  eth: link netdev to pp
  net: page_pool: add nlspec for basic access to page pools
  net: page_pool: implement GET in the netlink API
  net: page_pool: add netlink notifications for state changes
  net: page_pool: report when page pool was destroyed
  net: page_pool: expose page pool stats via netlink
  tools: netdev: regen after page pool changes

 Documentation/netlink/specs/netdev.yaml       | 158 +++++++
 Documentation/networking/page_pool.rst        |   5 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   1 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
 drivers/net/ethernet/microsoft/mana/mana_en.c |   1 +
 include/linux/list.h                          |  20 +
 include/linux/netdevice.h                     |   4 +
 include/net/page_pool/helpers.h               |   8 +-
 include/net/page_pool/types.h                 |  43 +-
 include/uapi/linux/netdev.h                   |  37 ++
 net/core/Makefile                             |   2 +-
 net/core/netdev-genl-gen.c                    |  41 ++
 net/core/netdev-genl-gen.h                    |  11 +
 net/core/page_pool.c                          |  56 ++-
 net/core/page_pool_priv.h                     |  12 +
 net/core/page_pool_user.c                     | 409 +++++++++++++++++
 tools/include/uapi/linux/netdev.h             |  37 ++
 tools/net/ynl/generated/netdev-user.c         | 415 ++++++++++++++++++
 tools/net/ynl/generated/netdev-user.h         | 169 +++++++
 19 files changed, 1391 insertions(+), 39 deletions(-)
 create mode 100644 net/core/page_pool_priv.h
 create mode 100644 net/core/page_pool_user.c

--
2.41.0

-- 
Thanks,
Mina