Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <692d71dc8068b3d27aba39d7141c811755965786.camel@gmail.com>
Subject: Re: [PATCH net-next v4 5/6] page_pool: add a lockdep check for
 recycling in hardirq
From:   Alexander H Duyck <alexander.duyck@gmail.com>
To:     Alexander Lobakin <aleksander.lobakin@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Cc:     Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Yunsheng Lin <linyunsheng@huawei.com>,
        Alexander Duyck <alexanderduyck@fb.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        Simon Horman <simon.horman@corigine.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Mon, 07 Aug 2023 07:48:54 -0700
In-Reply-To: <20230804180529.2483231-6-aleksander.lobakin@intel.com>
References: <20230804180529.2483231-1-aleksander.lobakin@intel.com>
         <20230804180529.2483231-6-aleksander.lobakin@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-08-04 at 20:05 +0200, Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@kernel.org>
>=20
> Page pool use in hardirq is prohibited, add debug checks
> to catch misuses. IIRC we previously discussed using
> DEBUG_NET_WARN_ON_ONCE() for this, but there were concerns
> that people will have DEBUG_NET enabled in perf testing.
> I don't think anyone enables lockdep in perf testing,
> so use lockdep to avoid pushback and arguing :)
>=20
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  include/linux/lockdep.h | 7 +++++++
>  net/core/page_pool.c    | 2 ++
>  2 files changed, 9 insertions(+)
>=20
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 310f85903c91..dc2844b071c2 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -625,6 +625,12 @@ do {									\
>  	WARN_ON_ONCE(__lockdep_enabled && !this_cpu_read(hardirq_context)); \
>  } while (0)
> =20
> +#define lockdep_assert_no_hardirq()					\
> +do {									\
> +	WARN_ON_ONCE(__lockdep_enabled && (this_cpu_read(hardirq_context) || \
> +					   !this_cpu_read(hardirqs_enabled))); \
> +} while (0)
> +
>  #define lockdep_assert_preemption_enabled()				\
>  do {									\
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT)	&&		\
> @@ -659,6 +665,7 @@ do {									\
>  # define lockdep_assert_irqs_enabled() do { } while (0)
>  # define lockdep_assert_irqs_disabled() do { } while (0)
>  # define lockdep_assert_in_irq() do { } while (0)
> +# define lockdep_assert_no_hardirq() do { } while (0)
> =20
>  # define lockdep_assert_preemption_enabled() do { } while (0)
>  # define lockdep_assert_preemption_disabled() do { } while (0)
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 03ad74d25959..77cb75e63aca 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -587,6 +587,8 @@ static __always_inline struct page *
>  __page_pool_put_page(struct page_pool *pool, struct page *page,
>  		     unsigned int dma_sync_size, bool allow_direct)
>  {
> +	lockdep_assert_no_hardirq();
> +
>  	/* This allocator is optimized for the XDP mode that uses
>  	 * one-frame-per-page, but have fallbacks that act like the
>  	 * regular page allocator APIs.

So two points.

First could we look at moving this inside the if statement just before
we return the page, as there isn't a risk until we get into that path
of needing a lock.

Secondly rather than returning an error is there any reason why we
couldn't just look at not returning page and instead just drop into the
release path which wouldn't take the locks in the first place? Either
that or I would even be good with some combination of the two where we
threw a warning, but still just dropped the page so we reduce our risk
further of actually locking things up.