[PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Jakub Kicinski]

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 :)

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: peterz@infradead.org
CC: mingo@redhat.com
CC: will@kernel.org
CC: longman@redhat.com
CC: boqun.feng@gmail.com
CC: hawk@kernel.org
CC: ilias.apalodimas@linaro.org
---
 include/linux/lockdep.h | 7 +++++++
 net/core/page_pool.c    | 4 ++++
 2 files changed, 11 insertions(+)

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)
 
+#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)
 
 # 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 a3e12a61d456..3ac760fcdc22 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
 static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
 {
 	int ret;
+
+	lockdep_assert_no_hardirq();
 	/* BH protection not needed if current is softirq */
 	if (in_softirq())
 		ret = ptr_ring_produce(&pool->ring, page);
@@ -642,6 +644,8 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
 	int i, bulk_len = 0;
 	bool in_softirq;
 
+	lockdep_assert_no_hardirq();
+
 	for (i = 0; i < count; i++) {
 		struct page *page = virt_to_head_page(data[i]);
 
-- 
2.41.0

Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Yunsheng Lin]

On 2023/7/21 1:37, Jakub Kicinski wrote:

...

> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a3e12a61d456..3ac760fcdc22 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
>  static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
>  {
>  	int ret;
> +
> +	lockdep_assert_no_hardirq();

Is there any reason not to put it in page_pool_put_defragged_page() to
catch the case with allow_direct being true when page_pool_recycle_in_ring()
may not be called?


>  	/* BH protection not needed if current is softirq */
>  	if (in_softirq())
>  		ret = ptr_ring_produce(&pool->ring, page);
> @@ -642,6 +644,8 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
>  	int i, bulk_len = 0;
>  	bool in_softirq;
>  
> +	lockdep_assert_no_hardirq();
> +
>  	for (i = 0; i < count; i++) {
>  		struct page *page = virt_to_head_page(data[i]);
>  
> 

Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Jakub Kicinski]

On Fri, 21 Jul 2023 19:53:30 +0800 Yunsheng Lin wrote:
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index a3e12a61d456..3ac760fcdc22 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
> >  static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
> >  {
> >  	int ret;
> > +
> > +	lockdep_assert_no_hardirq();  
> 
> Is there any reason not to put it in page_pool_put_defragged_page() to
> catch the case with allow_direct being true when page_pool_recycle_in_ring()
> may not be called?

I was trying to stick it into places which make an assumption about
the calling context, rather than cover the full API.
I don't have a strong preference either way, but I hope it's good
enough. The benefit I see is that it should be fairly obvious to
a seasoned kernel code reader why this warning is here.
A warning that fires from page_pool_put_defragged_page() would need
a comment to explain the reason and may go stale.

> >  	/* BH protection not needed if current is softirq */
> >  	if (in_softirq())

Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 20 Jul 2023 10:37:51 -0700

> 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 :)

+1 patch to add to my tree to base my current series on...
Time to create separate repo named "page-pool-next"? :D

> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: peterz@infradead.org
> CC: mingo@redhat.com
> CC: will@kernel.org
> CC: longman@redhat.com
> CC: boqun.feng@gmail.com
> CC: hawk@kernel.org
> CC: ilias.apalodimas@linaro.org
> ---
>  include/linux/lockdep.h | 7 +++++++
>  net/core/page_pool.c    | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> 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)
>  
> +#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)
>  
>  # 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 a3e12a61d456..3ac760fcdc22 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
>  static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)

Crap can happen earlier. Imagine that some weird code asked for direct
recycling with IRQs disabled. Then, we can hit
__page_pool_put_page:page_pool_recycle_in_cache and who knows what can
happen.
Can't we add this assertion right to the beginning of
__page_pool_put_page()? It's reasonable enough, at least for me, and
wouldn't require any commentary splats. Unlike put_defragged_page() as
Yunsheng proposes :p

Other than that (which is debatable), looks fine to me.

>  {
>  	int ret;
> +
> +	lockdep_assert_no_hardirq();
>  	/* BH protection not needed if current is softirq */
>  	if (in_softirq())
>  		ret = ptr_ring_produce(&pool->ring, page);
> @@ -642,6 +644,8 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
>  	int i, bulk_len = 0;
>  	bool in_softirq;
>  
> +	lockdep_assert_no_hardirq();
> +
>  	for (i = 0; i < count; i++) {
>  		struct page *page = virt_to_head_page(data[i]);
>  

Thanks,
Olek

Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Jakub Kicinski]

On Fri, 21 Jul 2023 17:48:25 +0200 Alexander Lobakin wrote:
> > 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 :)  
> 
> +1 patch to add to my tree to base my current series on...
> Time to create separate repo named "page-pool-next"? :D

You joke but I've been scheming how to expose the page pool stats
via the netdev netlink family, which would be another conflict to
be added to the pile :D When it rains it pours.

You should probably start sending uncontroversial stuff out even
if it doesn't have in-tree users yet.

> >  # 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 a3e12a61d456..3ac760fcdc22 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
> >  static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)  
> 
> Crap can happen earlier. Imagine that some weird code asked for direct
> recycling with IRQs disabled. Then, we can hit
> __page_pool_put_page:page_pool_recycle_in_cache and who knows what can
> happen.
> Can't we add this assertion right to the beginning of
> __page_pool_put_page()? It's reasonable enough, at least for me, and
> wouldn't require any commentary splats. Unlike put_defragged_page() as
> Yunsheng proposes :p
> 
> Other than that (which is debatable), looks fine to me.

No strong preference. Would you mind taking over this one? 
It'd also benefit from testing that the lockdep warning actually 
fires as expected, I just tested that it doesn't false positive TBH :)

Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 21 Jul 2023 09:05:38 -0700

> On Fri, 21 Jul 2023 17:48:25 +0200 Alexander Lobakin wrote:
>>> 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 :)  
>>
>> +1 patch to add to my tree to base my current series on...
>> Time to create separate repo named "page-pool-next"? :D
> 
> You joke but I've been scheming how to expose the page pool stats
> via the netdev netlink family, which would be another conflict to
> be added to the pile :D When it rains it pours.
> 
> You should probably start sending uncontroversial stuff out even
> if it doesn't have in-tree users yet.
> 
>>>  # 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 a3e12a61d456..3ac760fcdc22 100644
>>> --- a/net/core/page_pool.c
>>> +++ b/net/core/page_pool.c
>>> @@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
>>>  static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)  
>>
>> Crap can happen earlier. Imagine that some weird code asked for direct
>> recycling with IRQs disabled. Then, we can hit
>> __page_pool_put_page:page_pool_recycle_in_cache and who knows what can
>> happen.
>> Can't we add this assertion right to the beginning of
>> __page_pool_put_page()? It's reasonable enough, at least for me, and
>> wouldn't require any commentary splats. Unlike put_defragged_page() as
>> Yunsheng proposes :p
>>
>> Other than that (which is debatable), looks fine to me.
> 
> No strong preference. Would you mind taking over this one? 
> It'd also benefit from testing that the lockdep warning actually 
> fires as expected, I just tested that it doesn't false positive TBH :)

Sure! I'll add it to the optimization series as a pre-req to more
aggressive direct recycling, would that be fine?

Other than that, it's mostly Yunsheng's 2 submissions (PP header file
split and hybrid allocation) I'm basing both of my series on. The
optimization series can go without the allocation just fine, but as for
the header split, I'd like it to go first, it simplifies things :D

Thanks,
Olek

Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

[Jakub Kicinski]

On Fri, 21 Jul 2023 18:33:39 +0200 Alexander Lobakin wrote:
> > No strong preference. Would you mind taking over this one? 
> > It'd also benefit from testing that the lockdep warning actually 
> > fires as expected, I just tested that it doesn't false positive TBH :)  
> 
> Sure! I'll add it to the optimization series as a pre-req to more
> aggressive direct recycling, would that be fine?

SG!
-- 
pw-bot: cr