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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izPB=x1ZYhan-sjuA8ofbHmxbHJrSbtvN3z3zfziBMmMdA@mail.gmail.com>
Date: Thu, 17 Aug 2023 14:21:26 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, hawk@...nel.org, ilias.apalodimas@...aro.org, 
	aleksander.lobakin@...el.com, linyunsheng@...wei.com, 
	Willem de Bruijn <willemb@...gle.com>, Praveen Kaligineedi <pkaligineedi@...gle.com>
Subject: Re: [RFC net-next 00/13] net: page_pool: add netlink-based introspection

On Wed, Aug 16, 2023 at 4:43 PM Jakub Kicinski <kuba@...nel.org> 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/
>

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/

> 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