netdev - Re: [PATCH net-next v3 12/13] net: page_pool: mute the periodic warning for visible page pools

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]

Date: Wed, 22 Nov 2023 14:53:00 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, pabeni@...hat.com, 
	almasrymina@...gle.com, hawk@...nel.org, ilias.apalodimas@...aro.org, 
	dsahern@...il.com, dtatulea@...dia.com, willemb@...gle.com
Subject: Re: [PATCH net-next v3 12/13] net: page_pool: mute the periodic
 warning for visible page pools

On Wed, Nov 22, 2023 at 4:44 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Mute the periodic "stalled pool shutdown" warning if the page pool
> is visible to user space. Rolling out a driver using page pools
> to just a few hundred hosts at Meta surfaces applications which
> fail to reap their broken sockets. Obviously it's best if the
> applications are fixed, but we don't generally print warnings
> for application resource leaks. Admins can now depend on the
> netlink interface for getting page pool info to detect buggy
> apps.
>
> While at it throw in the ID of the pool into the message,
> in rare cases (pools from destroyed netns) this will make
> finding the pool with a debugger easier.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  net/core/page_pool.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 3d0938a60646..c2e7c9a6efbe 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -897,18 +897,21 @@ static void page_pool_release_retry(struct work_struct *wq)
>  {
>         struct delayed_work *dwq = to_delayed_work(wq);
>         struct page_pool *pool = container_of(dwq, typeof(*pool), release_dw);
> +       void *netdev;
>         int inflight;
>
>         inflight = page_pool_release(pool);
>         if (!inflight)
>                 return;
>
> -       /* Periodic warning */
> -       if (time_after_eq(jiffies, pool->defer_warn)) {
> +       /* Periodic warning for page pools the user can't see */
> +       netdev = READ_ONCE(pool->slow.netdev);
> +       if (time_after_eq(jiffies, pool->defer_warn) &&
> +           (!netdev || netdev == NET_PTR_POISON)) {
>                 int sec = (s32)((u32)jiffies - (u32)pool->defer_start) / HZ;

Orthogonal to your patch, but this probably could avoid all these casts.

long sec = (jiffies - pool->defer_start) / HZ;


>
> -               pr_warn("%s() stalled pool shutdown %d inflight %d sec\n",
> -                       __func__, inflight, sec);
> +               pr_warn("%s() stalled pool shutdown: id %u, %d inflight %d sec\n",
> +                       __func__, pool->user.id, inflight, sec);
>                 pool->defer_warn = jiffies + DEFER_WARN_INTERVAL;
>         }
>

Reviewed-by: Eric Dumazet <edumazet@...gle.com>