[PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Jonathan Lemon]

netpoll may be called from IRQ context, which may access the
page pool ring.  The current _bh variants do not provide sufficient
protection, so use irqsave/restore instead.

Error observed on a modified mlx4 driver, but the code path exists
for any driver which calls page_pool_recycle from napi poll.

WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161 __local_bh_enable_ip+0x35/0x50

    __page_pool_finish_recycle+0x14f/0x180
    mlx4_en_recycle_tx_desc+0x44/0x50
    mlx4_en_process_tx_cq+0x19f/0x440
    mlx4_en_poll_rx_cq+0xd4/0xf0
    netpoll_poll_dev+0xc2/0x190
    netpoll_send_skb_on_dev+0xf5/0x230
    netpoll_send_udp+0x2b3/0x3cd
    write_ext_msg+0x1be/0x1d0
    console_unlock+0x22e/0x500
    vprintk_emit+0x23a/0x360
    printk+0x48/0x4a
    hpet_rtc_interrupt.cold.17+0xe/0x1a
    __handle_irq_event_percpu+0x43/0x180
    handle_irq_event_percpu+0x20/0x60
    handle_irq_event+0x2a/0x47
    handle_edge_irq+0x8e/0x190
    handle_irq+0xbf/0x100
    do_IRQ+0x41/0xc0
    common_interrupt+0xf/0xf
    </IRQ>

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 net/core/page_pool.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 626db912fce4..df9804e85a40 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -102,6 +102,7 @@ noinline
 static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 {
 	struct ptr_ring *r = &pool->ring;
+	unsigned long flags;
 	struct page *page;
 	int pref_nid; /* preferred NUMA node */
 
@@ -120,7 +121,7 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 #endif
 
 	/* Slower-path: Get pages from locked ring queue */
-	spin_lock(&r->consumer_lock);
+	spin_lock_irqsave(&r->consumer_lock, flags);
 
 	/* Refill alloc array, but only if NUMA match */
 	do {
@@ -146,7 +147,7 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 	if (likely(pool->alloc.count > 0))
 		page = pool->alloc.cache[--pool->alloc.count];
 
-	spin_unlock(&r->consumer_lock);
+	spin_unlock_irqrestore(&r->consumer_lock, flags);
 	return page;
 }
 
@@ -321,11 +322,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;
-	/* BH protection not needed if current is serving softirq */
-	if (in_serving_softirq())
-		ret = ptr_ring_produce(&pool->ring, page);
-	else
-		ret = ptr_ring_produce_bh(&pool->ring, page);
+
+	ret = ptr_ring_produce_any(&pool->ring, page);
 
 	return (ret == 0) ? true : false;
 }
@@ -411,7 +409,7 @@ static void page_pool_empty_ring(struct page_pool *pool)
 	struct page *page;
 
 	/* Empty recycle ring */
-	while ((page = ptr_ring_consume_bh(&pool->ring))) {
+	while ((page = ptr_ring_consume_any(&pool->ring))) {
 		/* Verify the refcnt invariant of cached pages */
 		if (!(page_ref_count(page) == 1))
 			pr_crit("%s() page_pool refcnt %d violation\n",
-- 
2.17.1

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[David Miller]

From: Jonathan Lemon <jonathan.lemon@gmail.com>
Date: Mon, 9 Mar 2020 12:49:29 -0700

> netpoll may be called from IRQ context, which may access the
> page pool ring.  The current _bh variants do not provide sufficient
> protection, so use irqsave/restore instead.
> 
> Error observed on a modified mlx4 driver, but the code path exists
> for any driver which calls page_pool_recycle from napi poll.
> 
> WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161 __local_bh_enable_ip+0x35/0x50
 ...
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

The netpoll stuff always makes the locking more complicated than it needs
to be.  I wonder if there is another way around this issue?

Because IRQ save/restore is a high cost to pay in this critical path.

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Saeed Mahameed]

On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:
> From: Jonathan Lemon <jonathan.lemon@gmail.com>
> Date: Mon, 9 Mar 2020 12:49:29 -0700
> 
> > netpoll may be called from IRQ context, which may access the
> > page pool ring.  The current _bh variants do not provide sufficient
> > protection, so use irqsave/restore instead.
> > 
> > Error observed on a modified mlx4 driver, but the code path exists
> > for any driver which calls page_pool_recycle from napi poll.
> > 
> > WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161
> __local_bh_enable_ip+0x35/0x50
>  ...
> > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> 
> The netpoll stuff always makes the locking more complicated than it
> needs
> to be.  I wonder if there is another way around this issue?
> 
> Because IRQ save/restore is a high cost to pay in this critical path.

a printk inside irq context lead to this, so maybe it can be avoided ..

or instead of checking in_serving_softirq()  change page_pool to
check in_interrupt() which is more powerful, to avoid ptr_ring locking
and the complication with netpoll altogether.

I wonder why Jesper picked in_serving_softirq() in first place, was
there a specific reason ? or he just wanted it to be as less strict as
possible ?

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Jesper Dangaard Brouer]

On Tue, 10 Mar 2020 02:30:34 +0000
Saeed Mahameed <saeedm@mellanox.com> wrote:

> On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:
> > From: Jonathan Lemon <jonathan.lemon@gmail.com>
> > Date: Mon, 9 Mar 2020 12:49:29 -0700
> >   
> > > netpoll may be called from IRQ context, which may access the
> > > page pool ring.  The current _bh variants do not provide sufficient
> > > protection, so use irqsave/restore instead.
> > > 
> > > Error observed on a modified mlx4 driver, but the code path exists
> > > for any driver which calls page_pool_recycle from napi poll.

Netpoll calls into drivers are problematic, nasty and annoying. Drivers
usually catch netpoll calls via seeing NAPI budget is zero, and handle
the situation inside the driver e.g.[1][2]. (even napi_consume_skb
catch it this way).

[1] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#L3179
[2] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#L1110

> > > 
> > > WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161  
> > __local_bh_enable_ip+0x35/0x50
> >  ...  
> > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>  
> > 
> > The netpoll stuff always makes the locking more complicated than it
> > needs to be.  I wonder if there is another way around this issue?
> > 
> > Because IRQ save/restore is a high cost to pay in this critical path.  

Yes, huge NACK from me, this would kill performance. We need to find
another way.


> a printk inside irq context lead to this, so maybe it can be avoided ..
> 
> or instead of checking in_serving_softirq()  change page_pool to
> check in_interrupt() which is more powerful, to avoid ptr_ring locking
> and the complication with netpoll altogether.
> 
> I wonder why Jesper picked in_serving_softirq() in first place, was
> there a specific reason ? or he just wanted it to be as less strict as
> possible ?

I wanted to make it specific that this is optimized for softirq.

I actually think this is a regression we have introduced recently in
combined patches:

 304db6cb7610 ("page_pool: refill page when alloc.count of pool is zero")
 44768decb7c0 ("page_pool: handle page recycle for NUMA_NO_NODE condition")

I forgot that the in_serving_softirq() check was also protecting us
when getting called from netpoll.  The general idea before was that if
the in_serving_softirq() check failed, then we falled-through to
slow-path calling normal alloc_pages_node (regular page allocator).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Jonathan Lemon]

On 9 Mar 2020, at 17:55, David Miller wrote:

> From: Jonathan Lemon <jonathan.lemon@gmail.com>
> Date: Mon, 9 Mar 2020 12:49:29 -0700
>
>> netpoll may be called from IRQ context, which may access the
>> page pool ring.  The current _bh variants do not provide sufficient
>> protection, so use irqsave/restore instead.
>>
>> Error observed on a modified mlx4 driver, but the code path exists
>> for any driver which calls page_pool_recycle from napi poll.
>>
>> WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161 
>> __local_bh_enable_ip+0x35/0x50
>  ...
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> The netpoll stuff always makes the locking more complicated than it 
> needs
> to be.  I wonder if there is another way around this issue?
>
> Because IRQ save/restore is a high cost to pay in this critical path.

I agree with this - the proposed patch is more of a bandaid than
anything else.  Why is the entire network poll path being called
with IRQs off?  That seems wrong - while netcons is expected to
work in all cases, it does seem like it should be smarter and for
the normal case, just queue and raise NET_TX_SOFTIRQ would be a
more logical path.
-- 
Jonathan

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Jonathan Lemon]

On 9 Mar 2020, at 19:30, Saeed Mahameed wrote:

> On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:
>> From: Jonathan Lemon <jonathan.lemon@gmail.com>
>> Date: Mon, 9 Mar 2020 12:49:29 -0700
>>
>>> netpoll may be called from IRQ context, which may access the
>>> page pool ring.  The current _bh variants do not provide sufficient
>>> protection, so use irqsave/restore instead.
>>>
>>> Error observed on a modified mlx4 driver, but the code path exists
>>> for any driver which calls page_pool_recycle from napi poll.
>>>
>>> WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161
>> __local_bh_enable_ip+0x35/0x50
>>  ...
>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>>
>> The netpoll stuff always makes the locking more complicated than it
>> needs
>> to be.  I wonder if there is another way around this issue?
>>
>> Because IRQ save/restore is a high cost to pay in this critical path.
>
> a printk inside irq context lead to this, so maybe it can be avoided ..

This was caused by a printk in hpet_rtc_timer_reinit() complaining about
RTC interrupts being lost.  I'm not sure it's practical trying to locate
all the printk cases like this.


> or instead of checking in_serving_softirq()  change page_pool to
> check in_interrupt() which is more powerful, to avoid ptr_ring locking
> and the complication with netpoll altogether.

That's another approach:

    ret = 1;
    if (!in_irq()) {
        if (in_serving_softirq())
            ret = ptr_ring_produce(....
        else
            ret = ptr_ring_produce_bh(....
    }

which would return failure and release the page from the page pool.
This doesn't address the allocation or the bulk release path.


>
> I wonder why Jesper picked in_serving_softirq() in first place, was
> there a specific reason ? or he just wanted it to be as less strict as
> possible ?

From the code, it looks like he was optimizing to avoid the _bh variant
if possible.
-- 
Jonathan

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Jonathan Lemon]

On 10 Mar 2020, at 3:04, Jesper Dangaard Brouer wrote:

> On Tue, 10 Mar 2020 02:30:34 +0000
> Saeed Mahameed <saeedm@mellanox.com> wrote:
>
>> On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:
>>> From: Jonathan Lemon <jonathan.lemon@gmail.com>
>>> Date: Mon, 9 Mar 2020 12:49:29 -0700
>>>
>>>> netpoll may be called from IRQ context, which may access the
>>>> page pool ring.  The current _bh variants do not provide sufficient
>>>> protection, so use irqsave/restore instead.
>>>>
>>>> Error observed on a modified mlx4 driver, but the code path exists
>>>> for any driver which calls page_pool_recycle from napi poll.
>
> Netpoll calls into drivers are problematic, nasty and annoying. 
> Drivers
> usually catch netpoll calls via seeing NAPI budget is zero, and handle
> the situation inside the driver e.g.[1][2]. (even napi_consume_skb
> catch it this way).
>
> [1] 
> https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#L3179
> [2] 
> https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#L1110

This still doesn't seem safe.

netpoll calls napi poll in IRQ context.
Consider the following path:

  ixgbe_poll()
   ixgbe_for_each_ring()
    ixgbe_clean_tx_irq()    /* before the L3179 check above */
     xdp_return_frame()
      page_pool_put_page( .. napi=0 )

Which bypasses the cache and tries to return the page to the ring.
Since in_serving_softirq() is false, it uses the _bh variant, which
blows up.



>>>> WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161
>>> __local_bh_enable_ip+0x35/0x50
>>>  ...
>>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>>>
>>> The netpoll stuff always makes the locking more complicated than it
>>> needs to be.  I wonder if there is another way around this issue?
>>>
>>> Because IRQ save/restore is a high cost to pay in this critical 
>>> path.
>
> Yes, huge NACK from me, this would kill performance. We need to find
> another way.

Sure - checking in_irq() in the free path is another solution.


>> a printk inside irq context lead to this, so maybe it can be avoided 
>> ..
>>
>> or instead of checking in_serving_softirq()  change page_pool to
>> check in_interrupt() which is more powerful, to avoid ptr_ring 
>> locking
>> and the complication with netpoll altogether.
>>
>> I wonder why Jesper picked in_serving_softirq() in first place, was
>> there a specific reason ? or he just wanted it to be as less strict 
>> as
>> possible ?
>
> I wanted to make it specific that this is optimized for softirq.
>
> I actually think this is a regression we have introduced recently in
> combined patches:
>
>  304db6cb7610 ("page_pool: refill page when alloc.count of pool is 
> zero")
>  44768decb7c0 ("page_pool: handle page recycle for NUMA_NO_NODE 
> condition")
>
> I forgot that the in_serving_softirq() check was also protecting us
> when getting called from netpoll.  The general idea before was that if
> the in_serving_softirq() check failed, then we falled-through to
> slow-path calling normal alloc_pages_node (regular page allocator).

I don't think it ever did that - in all cases, the in_serving_softirq()
check guards access to the cache, otherwise it falls through to the 
ring.

Here, it seems we need  a tristate:
    in_irq:  fall through to system page allocator
    napi?    use cache
    else     ring


However, looking at those two patches again, while it doesn't address
this current issue, I do agree that they are regressions:

   1) Under stress, the system allocator may return pages with
       a different node id.

   2) Since the check is now removed from page_pool_reusable(), the
      foreign page is placed in the cache and persists there until
      it is moved to the ring.

   3) on refill from ring, if this page is found, processing stops,
      regardless of the other pages in the ring.

The above regression(s) are introduced by trying to avoid checking
the page node_id at free time.  Are there any metrics showing that
this is a performance issue?  Drivers which do page biasing still
have to perform this check in the driver (e.g.: mlx4, i40e) regardless.
-- 
Jonathan

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Jakub Kicinski]

On Tue, 10 Mar 2020 11:04:12 +0100 Jesper Dangaard Brouer wrote:
> On Tue, 10 Mar 2020 02:30:34 +0000
> Saeed Mahameed <saeedm@mellanox.com> wrote:
> > On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:  
> > > From: Jonathan Lemon <jonathan.lemon@gmail.com>
> > > Date: Mon, 9 Mar 2020 12:49:29 -0700
> > >     
> > > > netpoll may be called from IRQ context, which may access the
> > > > page pool ring.  The current _bh variants do not provide sufficient
> > > > protection, so use irqsave/restore instead.
> > > > 
> > > > Error observed on a modified mlx4 driver, but the code path exists
> > > > for any driver which calls page_pool_recycle from napi poll.  
> 
> Netpoll calls into drivers are problematic, nasty and annoying. Drivers
> usually catch netpoll calls via seeing NAPI budget is zero, and handle
> the situation inside the driver e.g.[1][2]. (even napi_consume_skb
> catch it this way).

I'm probably just repeating what you said, but would it be reasonable
to expect page_pool users to special-case XDP rings to not be cleaned?
netpoll has no use for them.

Perhaps that would not solve the issue for those funky drivers which
use the same rings for both XDP and the stack. Sigh. Do we care about
them?

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Saeed Mahameed]

On Tue, 2020-03-10 at 10:07 -0700, Jonathan Lemon wrote:
> 
> On 9 Mar 2020, at 19:30, Saeed Mahameed wrote:
> 
> > On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:
> > > From: Jonathan Lemon <jonathan.lemon@gmail.com>
> > > Date: Mon, 9 Mar 2020 12:49:29 -0700
> > > 
> > > > netpoll may be called from IRQ context, which may access the
> > > > page pool ring.  The current _bh variants do not provide
> > > > sufficient
> > > > protection, so use irqsave/restore instead.
> > > > 
> > > > Error observed on a modified mlx4 driver, but the code path
> > > > exists
> > > > for any driver which calls page_pool_recycle from napi poll.
> > > > 
> > > > WARNING: CPU: 34 PID: 550248 at /ro/source/kernel/softirq.c:161
> > > __local_bh_enable_ip+0x35/0x50
> > >  ...
> > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> > > 
> > > The netpoll stuff always makes the locking more complicated than
> > > it
> > > needs
> > > to be.  I wonder if there is another way around this issue?
> > > 
> > > Because IRQ save/restore is a high cost to pay in this critical
> > > path.
> > 
> > a printk inside irq context lead to this, so maybe it can be
> > avoided ..
> 
> This was caused by a printk in hpet_rtc_timer_reinit() complaining
> about
> RTC interrupts being lost.  I'm not sure it's practical trying to
> locate
> all the printk cases like this.
> 
> 
> > or instead of checking in_serving_softirq()  change page_pool to
> > check in_interrupt() which is more powerful, to avoid ptr_ring
> > locking
> > and the complication with netpoll altogether.
> 
> That's another approach:
> 
>     ret = 1;
>     if (!in_irq()) {
>         if (in_serving_softirq())
>             ret = ptr_ring_produce(....
>         else
>             ret = ptr_ring_produce_bh(....
>     }
> 
> which would return failure and release the page from the page pool.
> This doesn't address the allocation or the bulk release path.
> 
> 
> > I wonder why Jesper picked in_serving_softirq() in first place, was
> > there a specific reason ? or he just wanted it to be as less strict
> > as
> > possible ?
> 
> From the code, it looks like he was optimizing to avoid the _bh
> variant
> if possible.

I think that if you use another variant this it intorduce a possiblerace bestrewn sofitq and hardirq .. 

Re: [PATCH] page_pool: use irqsave/irqrestore to protect ring access.

[Saeed Mahameed]

On Tue, 2020-03-10 at 14:09 -0700, Jakub Kicinski wrote:
> On Tue, 10 Mar 2020 11:04:12 +0100 Jesper Dangaard Brouer wrote:
> > On Tue, 10 Mar 2020 02:30:34 +0000
> > Saeed Mahameed <saeedm@mellanox.com> wrote:
> > > On Mon, 2020-03-09 at 17:55 -0700, David Miller wrote:  
> > > > From: Jonathan Lemon <jonathan.lemon@gmail.com>
> > > > Date: Mon, 9 Mar 2020 12:49:29 -0700
> > > >     
> > > > > netpoll may be called from IRQ context, which may access the
> > > > > page pool ring.  The current _bh variants do not provide
> > > > > sufficient
> > > > > protection, so use irqsave/restore instead.
> > > > > 
> > > > > Error observed on a modified mlx4 driver, but the code path
> > > > > exists
> > > > > for any driver which calls page_pool_recycle from napi
> > > > > poll.  
> > 
> > Netpoll calls into drivers are problematic, nasty and annoying.
> > Drivers
> > usually catch netpoll calls via seeing NAPI budget is zero, and
> > handle
> > the situation inside the driver e.g.[1][2]. (even napi_consume_skb
> > catch it this way).
> 
> I'm probably just repeating what you said, but would it be reasonable
> to expect page_pool users to special-case XDP rings to not be
> cleaned?
> netpoll has no use for them.
> 

sounds like a better band-aid

> Perhaps that would not solve the issue for those funky drivers which
> use the same rings for both XDP and the stack. Sigh. Do we care about
> them?

They can special case per-page, if it is an XDP tx page, don't recycle
to ptr ring ..