[PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Nick Child]

All ibmvnic resets, make a call to netdev_tx_reset_queue() when
re-opening the device. netdev_tx_reset_queue() resets the num_queued
and num_completed byte counters. These stats are used in Byte Queue
Limit (BQL) algorithms. The difference between these two stats tracks
the number of bytes currently sitting on the physical NIC. ibmvnic
increases the number of queued bytes though calls to
netdev_tx_sent_queue() in the drivers xmit function. When, VIOS reports
that it is done transmitting bytes, the ibmvnic device increases the
number of completed bytes through calls to netdev_tx_completed_queue().
It is important to note that the driver batches its transmit calls and
num_queued is increased every time that an skb is added to the next
batch, not necessarily when the batch is sent to VIOS for transmission.

Unlike other reset types, a NON FATAL reset will not flush the sub crq
tx buffers. Therefore, it is possible for the batched skb array to be
partially full. So if there is call to netdev_tx_reset_queue() when
re-opening the device, the value of num_queued (0) would not account
for the skb's that are currently batched. Eventually, when the batch
is sent to VIOS, the call to netdev_tx_completed_queue() would increase
num_completed to a value greater than the num_queued. This causes a
BUG_ON crash:

ibmvnic 30000002: Firmware reports error, cause: adapter problem.
Starting recovery...
ibmvnic 30000002: tx error 600
ibmvnic 30000002: tx error 600
ibmvnic 30000002: tx error 600
ibmvnic 30000002: tx error 600
------------[ cut here ]------------
kernel BUG at lib/dynamic_queue_limits.c:27!
Oops: Exception in kernel mode, sig: 5
[....]
NIP dql_completed+0x28/0x1c0
LR ibmvnic_complete_tx.isra.0+0x23c/0x420 [ibmvnic]
Call Trace:
ibmvnic_complete_tx.isra.0+0x3f8/0x420 [ibmvnic] (unreliable)
ibmvnic_interrupt_tx+0x40/0x70 [ibmvnic]
__handle_irq_event_percpu+0x98/0x270
---[ end trace ]---

Therefore, do not reset the dql stats when performing a NON_FATAL reset.
Simply clearing the queues off bit is sufficient.

Fixes: 0d973388185d ("ibmvnic: Introduce xmit_more support using batched subCRQ hcalls")
Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index c63d3ec9d328..5523ab52ff2b 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1816,7 +1816,18 @@ static int __ibmvnic_open(struct net_device *netdev)
 		if (prev_state == VNIC_CLOSED)
 			enable_irq(adapter->tx_scrq[i]->irq);
 		enable_scrq_irq(adapter, adapter->tx_scrq[i]);
-		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
+		/* netdev_tx_reset_queue will reset dql stats and set the stacks
+		 * flag for queue status. During NON_FATAL resets, just
+		 * clear the bit, don't reset the stats because there could
+		 * be batched skb's waiting to be sent. If we reset dql stats,
+		 * we risk num_completed being greater than num_queued.
+		 * This will cause a BUG_ON in dql_completed().
+		 */
+		if (adapter->reset_reason == VNIC_RESET_NON_FATAL)
+			clear_bit(__QUEUE_STATE_STACK_XOFF,
+				  &netdev_get_tx_queue(netdev, i)->state);
+		else
+			netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
 	}
 
 	rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_UP);
-- 
2.31.1

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Simon Horman]

+ maintainers and blamed authors

On Thu, Jun 22, 2023 at 02:03:32PM -0500, Nick Child wrote:
> All ibmvnic resets, make a call to netdev_tx_reset_queue() when
> re-opening the device. netdev_tx_reset_queue() resets the num_queued
> and num_completed byte counters. These stats are used in Byte Queue
> Limit (BQL) algorithms. The difference between these two stats tracks
> the number of bytes currently sitting on the physical NIC. ibmvnic
> increases the number of queued bytes though calls to
> netdev_tx_sent_queue() in the drivers xmit function. When, VIOS reports
> that it is done transmitting bytes, the ibmvnic device increases the
> number of completed bytes through calls to netdev_tx_completed_queue().
> It is important to note that the driver batches its transmit calls and
> num_queued is increased every time that an skb is added to the next
> batch, not necessarily when the batch is sent to VIOS for transmission.
> 
> Unlike other reset types, a NON FATAL reset will not flush the sub crq
> tx buffers. Therefore, it is possible for the batched skb array to be
> partially full. So if there is call to netdev_tx_reset_queue() when
> re-opening the device, the value of num_queued (0) would not account
> for the skb's that are currently batched. Eventually, when the batch
> is sent to VIOS, the call to netdev_tx_completed_queue() would increase
> num_completed to a value greater than the num_queued. This causes a
> BUG_ON crash:
> 
> ibmvnic 30000002: Firmware reports error, cause: adapter problem.
> Starting recovery...
> ibmvnic 30000002: tx error 600
> ibmvnic 30000002: tx error 600
> ibmvnic 30000002: tx error 600
> ibmvnic 30000002: tx error 600
> ------------[ cut here ]------------
> kernel BUG at lib/dynamic_queue_limits.c:27!
> Oops: Exception in kernel mode, sig: 5
> [....]
> NIP dql_completed+0x28/0x1c0
> LR ibmvnic_complete_tx.isra.0+0x23c/0x420 [ibmvnic]
> Call Trace:
> ibmvnic_complete_tx.isra.0+0x3f8/0x420 [ibmvnic] (unreliable)
> ibmvnic_interrupt_tx+0x40/0x70 [ibmvnic]
> __handle_irq_event_percpu+0x98/0x270
> ---[ end trace ]---
> 
> Therefore, do not reset the dql stats when performing a NON_FATAL reset.
> Simply clearing the queues off bit is sufficient.
> 
> Fixes: 0d973388185d ("ibmvnic: Introduce xmit_more support using batched subCRQ hcalls")
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index c63d3ec9d328..5523ab52ff2b 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1816,7 +1816,18 @@ static int __ibmvnic_open(struct net_device *netdev)
>  		if (prev_state == VNIC_CLOSED)
>  			enable_irq(adapter->tx_scrq[i]->irq);
>  		enable_scrq_irq(adapter, adapter->tx_scrq[i]);
> -		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
> +		/* netdev_tx_reset_queue will reset dql stats and set the stacks
> +		 * flag for queue status. During NON_FATAL resets, just
> +		 * clear the bit, don't reset the stats because there could
> +		 * be batched skb's waiting to be sent. If we reset dql stats,
> +		 * we risk num_completed being greater than num_queued.
> +		 * This will cause a BUG_ON in dql_completed().
> +		 */
> +		if (adapter->reset_reason == VNIC_RESET_NON_FATAL)
> +			clear_bit(__QUEUE_STATE_STACK_XOFF,
> +				  &netdev_get_tx_queue(netdev, i)->state);
> +		else
> +			netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
>  	}
>  
>  	rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_UP);
> -- 
> 2.31.1
> 
> 

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Rick Lindsley]

On 6/22/23 12:03, Nick Child wrote:

> Signed-off-by: Nick Child <nnac123@linux.ibm.com>

Reviewed-by: ricklind@us.ibm.com

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Simon Horman]

On Fri, Jun 23, 2023 at 09:52:26AM +0200, Simon Horman wrote:
> + maintainers and blamed authors

A second time, because something went wrong with the first attempt.

> On Thu, Jun 22, 2023 at 02:03:32PM -0500, Nick Child wrote:
> > All ibmvnic resets, make a call to netdev_tx_reset_queue() when
> > re-opening the device. netdev_tx_reset_queue() resets the num_queued
> > and num_completed byte counters. These stats are used in Byte Queue
> > Limit (BQL) algorithms. The difference between these two stats tracks
> > the number of bytes currently sitting on the physical NIC. ibmvnic
> > increases the number of queued bytes though calls to
> > netdev_tx_sent_queue() in the drivers xmit function. When, VIOS reports
> > that it is done transmitting bytes, the ibmvnic device increases the
> > number of completed bytes through calls to netdev_tx_completed_queue().
> > It is important to note that the driver batches its transmit calls and
> > num_queued is increased every time that an skb is added to the next
> > batch, not necessarily when the batch is sent to VIOS for transmission.
> > 
> > Unlike other reset types, a NON FATAL reset will not flush the sub crq
> > tx buffers. Therefore, it is possible for the batched skb array to be
> > partially full. So if there is call to netdev_tx_reset_queue() when
> > re-opening the device, the value of num_queued (0) would not account
> > for the skb's that are currently batched. Eventually, when the batch
> > is sent to VIOS, the call to netdev_tx_completed_queue() would increase
> > num_completed to a value greater than the num_queued. This causes a
> > BUG_ON crash:
> > 
> > ibmvnic 30000002: Firmware reports error, cause: adapter problem.
> > Starting recovery...
> > ibmvnic 30000002: tx error 600
> > ibmvnic 30000002: tx error 600
> > ibmvnic 30000002: tx error 600
> > ibmvnic 30000002: tx error 600
> > ------------[ cut here ]------------
> > kernel BUG at lib/dynamic_queue_limits.c:27!
> > Oops: Exception in kernel mode, sig: 5
> > [....]
> > NIP dql_completed+0x28/0x1c0
> > LR ibmvnic_complete_tx.isra.0+0x23c/0x420 [ibmvnic]
> > Call Trace:
> > ibmvnic_complete_tx.isra.0+0x3f8/0x420 [ibmvnic] (unreliable)
> > ibmvnic_interrupt_tx+0x40/0x70 [ibmvnic]
> > __handle_irq_event_percpu+0x98/0x270
> > ---[ end trace ]---
> > 
> > Therefore, do not reset the dql stats when performing a NON_FATAL reset.
> > Simply clearing the queues off bit is sufficient.
> > 
> > Fixes: 0d973388185d ("ibmvnic: Introduce xmit_more support using batched subCRQ hcalls")
> > Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> > ---
> >  drivers/net/ethernet/ibm/ibmvnic.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> > index c63d3ec9d328..5523ab52ff2b 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -1816,7 +1816,18 @@ static int __ibmvnic_open(struct net_device *netdev)
> >  		if (prev_state == VNIC_CLOSED)
> >  			enable_irq(adapter->tx_scrq[i]->irq);
> >  		enable_scrq_irq(adapter, adapter->tx_scrq[i]);
> > -		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
> > +		/* netdev_tx_reset_queue will reset dql stats and set the stacks
> > +		 * flag for queue status. During NON_FATAL resets, just
> > +		 * clear the bit, don't reset the stats because there could
> > +		 * be batched skb's waiting to be sent. If we reset dql stats,
> > +		 * we risk num_completed being greater than num_queued.
> > +		 * This will cause a BUG_ON in dql_completed().
> > +		 */
> > +		if (adapter->reset_reason == VNIC_RESET_NON_FATAL)
> > +			clear_bit(__QUEUE_STATE_STACK_XOFF,
> > +				  &netdev_get_tx_queue(netdev, i)->state);
> > +		else
> > +			netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));
> >  	}
> >  
> >  	rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_UP);
> > -- 
> > 2.31.1
> > 
> > 

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Jakub Kicinski]

On Thu, 22 Jun 2023 14:03:32 -0500 Nick Child wrote:
> +		if (adapter->reset_reason == VNIC_RESET_NON_FATAL)
> +			clear_bit(__QUEUE_STATE_STACK_XOFF,
> +				  &netdev_get_tx_queue(netdev, i)->state);

Why are you trying to clear this bit?

If the completions will still come the bit will be cleared (or not)
during completion handling (netdev_tx_completed_queue() et al.)

Drivers shouldn't be poking into queue state bits directly.
-- 
pw-bot: cr

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Nick Child]

On 6/24/23 17:19, Jakub Kicinski wrote:
> On Thu, 22 Jun 2023 14:03:32 -0500 Nick Child wrote:
>> +		if (adapter->reset_reason == VNIC_RESET_NON_FATAL)
>> +			clear_bit(__QUEUE_STATE_STACK_XOFF,
>> +				  &netdev_get_tx_queue(netdev, i)->state);
> 
> Why are you trying to clear this bit?
> 
> If the completions will still come the bit will be cleared (or not)
> during completion handling (netdev_tx_completed_queue() et al.)
> 
> Drivers shouldn't be poking into queue state bits directly.

Most likely, yes there could be some bytes that will get a completion 
which would clear this bit.

That being said, it is also possible that all bytes sent to the NIC are 
already completed. In which case we would not get a completion. The 
ibmvnic driver sends its skb's to the NIC in batches, it makes a call to 
netdev_tx_sent_queue on every time an skb is added to the batch. This is 
not necessarily every-time that the batch is sent to the NIC.

I am not sure what number of bytes causes dql to set 
__QUEUE_STATE_STACK_XOFF but I do know that it is possible for up to 15 
skb's to be sitting in the queues batch. If there are no outstanding 
bytes on the NIC (ie not expecting a tx completion) and the internal 
batch has 15 references per queue, is this enough to enforce dql and set 
__QUEUE_STATE_STACK_XOFF? If so, then we must clear 
__QUEUE_STATE_STACK_XOFF when resetting.

I had a feeling this would raise some eyebrows. The main intent is to do 
everything that netdev_tx_reset_queue() does besides resetting 
statistics. Setting a "*STACK*" flag in driver code feels wrong 
(especially since a *DRV* flag exists) but I could not find an 
appropriate upper-level function. I suppose an alternative is to read 
this flag after the device finishes the reset and sending the batch if 
it is set. Is this any better?

As always, thanks for the review.
Nick

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Jakub Kicinski]

On Mon, 26 Jun 2023 10:45:38 -0500 Nick Child wrote:
> On 6/24/23 17:19, Jakub Kicinski wrote:
> > On Thu, 22 Jun 2023 14:03:32 -0500 Nick Child wrote:  
>  [...]  
> > 
> > Why are you trying to clear this bit?
> > 
> > If the completions will still come the bit will be cleared (or not)
> > during completion handling (netdev_tx_completed_queue() et al.)
> > 
> > Drivers shouldn't be poking into queue state bits directly.  
> 
> Most likely, yes there could be some bytes that will get a completion 
> which would clear this bit.
> 
> That being said, it is also possible that all bytes sent to the NIC are 
> already completed. In which case we would not get a completion. The 
> ibmvnic driver sends its skb's to the NIC in batches, it makes a call to 
> netdev_tx_sent_queue on every time an skb is added to the batch. This is 
> not necessarily every-time that the batch is sent to the NIC.

If packets can get stuck in the driver that needs a dedicated fix.

> I am not sure what number of bytes causes dql to set 
> __QUEUE_STATE_STACK_XOFF but I do know that it is possible for up to 15 
> skb's to be sitting in the queues batch. If there are no outstanding 
> bytes on the NIC (ie not expecting a tx completion) and the internal 
> batch has 15 references per queue, is this enough to enforce dql and set 
> __QUEUE_STATE_STACK_XOFF? If so, then we must clear 
> __QUEUE_STATE_STACK_XOFF when resetting.

You should only be doing the batching if xmit_more is set, really.
And xmit_more will not be set if core is about to set
__QUEUE_STATE_STACK_XOFF. 

With a correctly written driver STACK_XOFF can only be set if we're
expecting a completion.

> I had a feeling this would raise some eyebrows. The main intent is to do 
> everything that netdev_tx_reset_queue() does besides resetting 
> statistics. Setting a "*STACK*" flag in driver code feels wrong 
> (especially since a *DRV* flag exists) but I could not find an 
> appropriate upper-level function. I suppose an alternative is to read 
> this flag after the device finishes the reset and sending the batch if 
> it is set. Is this any better?

AFAIU you shouldn't have to clear this flag. We need to reset DQL when
hard resetting the queue because we may lose completions. But if no
completions are lost, STACK_XOFF should be left alone. 

Just clearing that flag is not valid either because qdiscs will not be
rescheduled, so until some socket tries to xmit next packet the data
already queued will not move.

RE: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Rick Lindsley]

On 6/24/23 15:19, Jakub Kicinski wrote:

> Why are you trying to clear this bit?
> 
> If the completions will still come the bit will be cleared (or not)
> during completion handling (netdev_tx_completed_queue() et al.)
> 
> Drivers shouldn't be poking into queue state bits directly.


+		if (adapter->reset_reason == VNIC_RESET_NON_FATAL)
+			clear_bit(__QUEUE_STATE_STACK_XOFF,
+				  &netdev_get_tx_queue(netdev, i)->state);
+		else
+			netdev_tx_reset_queue(netdev_get_tx_queue(netdev, i));

The problem is the call to dql_reset() in netdev_tx_reset_queue().  If we reset dql stats,   we risk num_completed being greater than num_queued, which will cause a BUG_ON to fire in dql_completed().

     static inline void netdev_tx_reset_queue(struct netdev_queue *q)
     {
     #ifdef CONFIG_BQL
         clear_bit(__QUEUE_STATE_STACK_XOFF, &q->state);
         dql_reset(&q->dql);
     #endif
     }

Existing code calls  netdev_tx_reset_queue() unconditionally.  When the error is non-fatal, though, we were tripping over the BUG_ON on those occasions when a few skbs were already submitted.  The patch here is more about NOT calling dql_reset() than it is about clearing __QUEUE_STATE_STACK_XOFF.  That was only included because it-was-the-other-thing-the-function-did.  So ... maybe we don't care about __QUEUE_STATE_STACK_XOFF?

Nick, do we *need* to have __QUEUE_STATE_STACK_XOFF cleared?  If not, then do we need to call or emulate netdev_tx_reset_queue() at all on a non-fatal error?

Re: [PATCH net] ibmvnic: Do not reset dql stats on NON_FATAL err

[Nick Child]

On 6/26/23 12:33, Jakub Kicinski wrote:
> 
> If packets can get stuck in the driver that needs a dedicated fix.
> 
> You should only be doing the batching if xmit_more is set, really.
> And xmit_more will not be set if core is about to set
> __QUEUE_STATE_STACK_XOFF.
> 

I am not saying that packets can get stuck in the driver, if xmit_more 
is false or if the driver is hard resetting then the batch gets sent.

During nonfatal reset, we are simply reestablishing communications with 
the NIC and it is okay to keep the batch around. It is possible that 
there are batched skb's because xmit_more and the non fatal reset work 
independently of each-other and are not mutually exclusive. The upper 
level functions have no way of knowing when an issue from the NIC will 
occur.

> With a correctly written driver STACK_XOFF can only be set if we're
> expecting a completion.
> 
> AFAIU you shouldn't have to clear this flag. We need to reset DQL when
> hard resetting the queue because we may lose completions. But if no
> completions are lost, STACK_XOFF should be left alone.
> 
> Just clearing that flag is not valid either because qdiscs will not be
> rescheduled, so until some socket tries to xmit next packet the data
> already queued will not move.

This addresses my concern. So if STACK_XOFF gets set, then xmit_more 
will be false and our batch will get sent. This will eventually lead to 
a completion which will clear STACK_XOFF. Meaning the reset should not 
have to worry about clearing STACK_XOFF.

My worry was that STACK_XOFF would get set when the batch was only 
partially full and xmit_more was true. If a non fatal reset occurred 
here then there would be no expected completions and a partially filled 
batch. So I thought we would have to clear STACK_XOFF in order to get 
the queue to be usable again. Sounds like this is not possible due to 
xmit_more being false if STACK_XOFF gets set.

On 6/26/23 14:06, Rick Lindsley wrote:
 >
 > Nick, do we *need* to have __QUEUE_STATE_STACK_XOFF cleared?  If not,
 > then do we need to call or emulate netdev_tx_reset_queue() at all on a
 > non-fatal error?

After reading Jakubs review. I believe we should remove the part where 
we clear STACK_XOFF.

If there are no objections, I will test these changes and send a v2.

Thanks