[V4,net] net: mana: Fix MANA VF unload when host is unresponsive

Commit Message

Souradeep Chakrabarti July 3, 2023, 8:49 a.m. UTC

From: Souradeep Chakrabarti <[e-mail]>

When unloading the MANA driver, mana_dealloc_queues() waits for the MANA
hardware to complete any inflight packets and set the pending send count
to zero. But if the hardware has failed, mana_dealloc_queues()
could wait forever.

Fix this by adding a timeout to the wait. Set the timeout to 120 seconds,
which is a somewhat arbitrary value that is more than long enough for
functional hardware to complete any sends.

Signed-off-by: Souradeep Chakrabarti <[e-mail]>
---
V3 -> V4:
* Fixed the commit message to describe the context.
* Removed the vf_unload_timeout, as it is not required.
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 26 ++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Comments

Haiyang Zhang July 3, 2023, 3:51 p.m. UTC | #1

> -----Original Message-----
> From: souradeep chakrabarti <[e-mail]>
> Sent: Monday, July 3, 2023 4:50 AM
> To: KY Srinivasan <[e-mail]>; Haiyang Zhang
> <[e-mail]>; [e-mail]; Dexuan Cui
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
> Sharma <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; Souradeep Chakrabarti
> <[e-mail]>; Souradeep Chakrabarti
> <[e-mail]>
> Subject: [PATCH V4 net] net: mana: Fix MANA VF unload when host is
> unresponsive
> 
> From: Souradeep Chakrabarti <[e-mail]>
> 
> When unloading the MANA driver, mana_dealloc_queues() waits for the MANA
> hardware to complete any inflight packets and set the pending send count
> to zero. But if the hardware has failed, mana_dealloc_queues()
> could wait forever.
> 
> Fix this by adding a timeout to the wait. Set the timeout to 120 seconds,
> which is a somewhat arbitrary value that is more than long enough for
> functional hardware to complete any sends.
> 
> Signed-off-by: Souradeep Chakrabarti <[e-mail]>
> ---
> V3 -> V4:
> * Fixed the commit message to describe the context.
> * Removed the vf_unload_timeout, as it is not required.
> ---
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 26 ++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index a499e460594b..d26f1da70411 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2346,7 +2346,10 @@ static int mana_dealloc_queues(struct net_device
> *ndev)
>  {
>  	struct mana_port_context *apc = netdev_priv(ndev);
>  	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	unsigned long timeout;
>  	struct mana_txq *txq;
> +	struct sk_buff *skb;
> +	struct mana_cq *cq;
>  	int i, err;
> 
>  	if (apc->port_is_up)
> @@ -2363,15 +2366,32 @@ static int mana_dealloc_queues(struct net_device
> *ndev)
>  	 * to false, but it doesn't matter since mana_start_xmit() drops any
>  	 * new packets due to apc->port_is_up being false.
>  	 *
> -	 * Drain all the in-flight TX packets
> +	 * Drain all the in-flight TX packets.
> +	 * A timeout of 120 seconds for all the queues is used.
> +	 * This will break the while loop when h/w is not responding.
> +	 * This value of 120 has been decided here considering max
> +	 * number of queues.
>  	 */
> +
> +	timeout = jiffies + 120 * HZ;
>  	for (i = 0; i < apc->num_queues; i++) {
>  		txq = &apc->tx_qp[i].txq;
> -
> -		while (atomic_read(&txq->pending_sends) > 0)
> +		while (atomic_read(&txq->pending_sends) > 0 &&
> +		       time_before(jiffies, timeout)) {
>  			usleep_range(1000, 2000);
> +		}
>  	}
> 
> +	for (i = 0; i < apc->num_queues; i++) {
> +		txq = &apc->tx_qp[i].txq;
> +		cq = &apc->tx_qp[i].tx_cq;
> +		while (atomic_read(&txq->pending_sends)) {
> +			skb = skb_dequeue(&txq->pending_skbs);
> +			mana_unmap_skb(skb, apc);
> +			napi_consume_skb(skb, cq->budget);

This is not in NAPI context, so it should be dev_consume_skb_any()

Thanks,
- Haiyang

Alexander Lobakin July 3, 2023, 4:47 p.m. UTC | #2

From: Souradeep Chakrabarti <[e-mail]>
Date: Mon,  3 Jul 2023 01:49:31 -0700

> From: Souradeep Chakrabarti <[e-mail]>

Please sync your Git name and Git mail account settings, so that your
own patches won't have "From:" when sending. From what I see, you need
to correct first letters of name and surname to capital in the Git email
settings block.

> 
> When unloading the MANA driver, mana_dealloc_queues() waits for the MANA
> hardware to complete any inflight packets and set the pending send count
> to zero. But if the hardware has failed, mana_dealloc_queues()
> could wait forever.
> 
> Fix this by adding a timeout to the wait. Set the timeout to 120 seconds,
> which is a somewhat arbitrary value that is more than long enough for
> functional hardware to complete any sends.
> 
> Signed-off-by: Souradeep Chakrabarti <[e-mail]>

Where's "Fixes:" tagging the blamed commit?

> ---
> V3 -> V4:
> * Fixed the commit message to describe the context.
> * Removed the vf_unload_timeout, as it is not required.
> ---
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 26 ++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index a499e460594b..d26f1da70411 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2346,7 +2346,10 @@ static int mana_dealloc_queues(struct net_device *ndev)
>  {
>  	struct mana_port_context *apc = netdev_priv(ndev);
>  	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	unsigned long timeout;
>  	struct mana_txq *txq;
> +	struct sk_buff *skb;
> +	struct mana_cq *cq;
>  	int i, err;
>  
>  	if (apc->port_is_up)
> @@ -2363,15 +2366,32 @@ static int mana_dealloc_queues(struct net_device *ndev)
>  	 * to false, but it doesn't matter since mana_start_xmit() drops any
>  	 * new packets due to apc->port_is_up being false.
>  	 *
> -	 * Drain all the in-flight TX packets
> +	 * Drain all the in-flight TX packets.
> +	 * A timeout of 120 seconds for all the queues is used.
> +	 * This will break the while loop when h/w is not responding.
> +	 * This value of 120 has been decided here considering max
> +	 * number of queues.
>  	 */
> +
> +	timeout = jiffies + 120 * HZ;

Why not initialize it right when declaring?

>  	for (i = 0; i < apc->num_queues; i++) {
>  		txq = &apc->tx_qp[i].txq;
> -
> -		while (atomic_read(&txq->pending_sends) > 0)
> +		while (atomic_read(&txq->pending_sends) > 0 &&
> +		       time_before(jiffies, timeout)) {
>  			usleep_range(1000, 2000);> +		}
>  	}

120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
iterations. I know usleep_range() often is much less precise, but still.
Do you really need that much time? Has this been measured during the
tests that it can take up to 120 seconds or is it just some random value
that "should be enough"?
If you really need 120 seconds, I'd suggest using a timer / delayed work
instead of wasting resources.

>  
> +	for (i = 0; i < apc->num_queues; i++) {
> +		txq = &apc->tx_qp[i].txq;
> +		cq = &apc->tx_qp[i].tx_cq;

cq can be just &txq->tx_cq.

> +		while (atomic_read(&txq->pending_sends)) {
> +			skb = skb_dequeue(&txq->pending_skbs);
> +			mana_unmap_skb(skb, apc);
> +			napi_consume_skb(skb, cq->budget);

(you already have comment about this one)

> +			atomic_sub(1, &txq->pending_sends);
> +		}
> +	}
>  	/* We're 100% sure the queues can no longer be woken up, because
>  	 * we're sure now mana_poll_tx_cq() can't be running.
>  	 */

Thanks,
Olek

Souradeep Chakrabarti July 3, 2023, 7:55 p.m. UTC | #3

>-----Original Message-----
>From: Alexander Lobakin <[e-mail]>
>Sent: Monday, July 3, 2023 10:18 PM
>To: souradeep chakrabarti <[e-mail]>
>Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
><[e-mail]>; [e-mail]; Dexuan Cui
><[e-mail]>; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
>Sharma <[e-mail]>; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail];
>[e-mail]; Souradeep Chakrabarti <[e-mail]>
>Subject: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload when
>host is unresponsive
>
>From: Souradeep Chakrabarti <[e-mail]>
>Date: Mon,  3 Jul 2023 01:49:31 -0700
>
>> From: Souradeep Chakrabarti <[e-mail]>
>
>Please sync your Git name and Git mail account settings, so that your own
>patches won't have "From:" when sending. From what I see, you need to
>correct first letters of name and surname to capital in the Git email settings
>block.
Thank you for pointing, I will fix it.
>
>>
>> When unloading the MANA driver, mana_dealloc_queues() waits for the
>> MANA hardware to complete any inflight packets and set the pending
>> send count to zero. But if the hardware has failed,
>> mana_dealloc_queues() could wait forever.
>>
>> Fix this by adding a timeout to the wait. Set the timeout to 120
>> seconds, which is a somewhat arbitrary value that is more than long
>> enough for functional hardware to complete any sends.
>>
>> Signed-off-by: Souradeep Chakrabarti
>> <[e-mail]>
>
>Where's "Fixes:" tagging the blamed commit?
This is present from the day zero of the mana driver code.
It has not been introduced in the code by any commit.
>
>> ---
>> V3 -> V4:
>> * Fixed the commit message to describe the context.
>> * Removed the vf_unload_timeout, as it is not required.
>> ---
>>  drivers/net/ethernet/microsoft/mana/mana_en.c | 26
>> ++++++++++++++++---
>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
>> b/drivers/net/ethernet/microsoft/mana/mana_en.c
>> index a499e460594b..d26f1da70411 100644
>> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
>> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
>> @@ -2346,7 +2346,10 @@ static int mana_dealloc_queues(struct
>> net_device *ndev)  {
>>  	struct mana_port_context *apc = netdev_priv(ndev);
>>  	struct gdma_dev *gd = apc->ac->gdma_dev;
>> +	unsigned long timeout;
>>  	struct mana_txq *txq;
>> +	struct sk_buff *skb;
>> +	struct mana_cq *cq;
>>  	int i, err;
>>
>>  	if (apc->port_is_up)
>> @@ -2363,15 +2366,32 @@ static int mana_dealloc_queues(struct
>net_device *ndev)
>>  	 * to false, but it doesn't matter since mana_start_xmit() drops any
>>  	 * new packets due to apc->port_is_up being false.
>>  	 *
>> -	 * Drain all the in-flight TX packets
>> +	 * Drain all the in-flight TX packets.
>> +	 * A timeout of 120 seconds for all the queues is used.
>> +	 * This will break the while loop when h/w is not responding.
>> +	 * This value of 120 has been decided here considering max
>> +	 * number of queues.
>>  	 */
>> +
>> +	timeout = jiffies + 120 * HZ;
>
>Why not initialize it right when declaring?
I will fix it in next version.
>
>>  	for (i = 0; i < apc->num_queues; i++) {
>>  		txq = &apc->tx_qp[i].txq;
>> -
>> -		while (atomic_read(&txq->pending_sends) > 0)
>> +		while (atomic_read(&txq->pending_sends) > 0 &&
>> +		       time_before(jiffies, timeout)) {
>>  			usleep_range(1000, 2000);> +		}
>>  	}
>
>120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
>iterations. I know usleep_range() often is much less precise, but still.
>Do you really need that much time? Has this been measured during the tests
>that it can take up to 120 seconds or is it just some random value that "should
>be enough"?
>If you really need 120 seconds, I'd suggest using a timer / delayed work instead
>of wasting resources.
Here the intent is not waiting for 120 seconds, rather than avoid continue checking the 
pending_sends  of each tx queues for an indefinite time, before freeing sk_buffs.
The pending_sends can only get decreased for a tx queue,  if mana_poll_tx_cq()
gets called for a completion notification and increased by xmit.

In this particular bug, apc->port_is_up is not set to false, causing
xmit to keep increasing the pending_sends for the queue and mana_poll_tx_cq()
not getting called for the queue.

If we see the comment in the function mana_dealloc_queues(), it mentions it :

2346     /* No packet can be transmitted now since apc->port_is_up is false.
2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
2348      * a txq because it may not timely see apc->port_is_up being cleared
2349      * to false, but it doesn't matter since mana_start_xmit() drops any
2350      * new packets due to apc->port_is_up being false.

The value 120 seconds has been decided here based on maximum number of queues
are allowed in this specific hardware, it is a safe assumption.
>
>>
>> +	for (i = 0; i < apc->num_queues; i++) {
>> +		txq = &apc->tx_qp[i].txq;
>> +		cq = &apc->tx_qp[i].tx_cq;
>
>cq can be just &txq->tx_cq.
mana_txq  structure does not have a pointer to mana_cq.
>
>> +		while (atomic_read(&txq->pending_sends)) {
>> +			skb = skb_dequeue(&txq->pending_skbs);
>> +			mana_unmap_skb(skb, apc);
>> +			napi_consume_skb(skb, cq->budget);
>
>(you already have comment about this one)
>
>> +			atomic_sub(1, &txq->pending_sends);
>> +		}
>> +	}
>>  	/* We're 100% sure the queues can no longer be woken up, because
>>  	 * we're sure now mana_poll_tx_cq() can't be running.
>>  	 */
>
>Thanks,
>Olek

Paolo Abeni July 4, 2023, 6:59 a.m. UTC | #4

On Mon, 2023-07-03 at 19:55 +0000, Souradeep Chakrabarti wrote:
> > -----Original Message-----
> > From: Alexander Lobakin <[e-mail]>
> > Sent: Monday, July 3, 2023 10:18 PM
> > To: souradeep chakrabarti <[e-mail]>
> > Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
> > <[e-mail]>; [e-mail]; Dexuan Cui
> > <[e-mail]>; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
> > Sharma <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Souradeep Chakrabarti <[e-mail]>
> > Subject: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload when
> > host is unresponsive
> > 
> > From: Souradeep Chakrabarti <[e-mail]>
> > Date: Mon,  3 Jul 2023 01:49:31 -0700
> > 
> > > From: Souradeep Chakrabarti <[e-mail]>
> > 
> > Please sync your Git name and Git mail account settings, so that your own
> > patches won't have "From:" when sending. From what I see, you need to
> > correct first letters of name and surname to capital in the Git email settings
> > block.
> Thank you for pointing, I will fix it.
> > 
> > > 
> > > When unloading the MANA driver, mana_dealloc_queues() waits for the
> > > MANA hardware to complete any inflight packets and set the pending
> > > send count to zero. But if the hardware has failed,
> > > mana_dealloc_queues() could wait forever.
> > > 
> > > Fix this by adding a timeout to the wait. Set the timeout to 120
> > > seconds, which is a somewhat arbitrary value that is more than long
> > > enough for functional hardware to complete any sends.
> > > 
> > > Signed-off-by: Souradeep Chakrabarti
> > > <[e-mail]>
> > 
> > Where's "Fixes:" tagging the blamed commit?
> This is present from the day zero of the mana driver code.
> It has not been introduced in the code by any commit.
> 

Then the fixes tag should be:

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

Cheers,

Paolo

Haiyang Zhang July 4, 2023, 1:42 p.m. UTC | #5

> -----Original Message-----
> From: Souradeep Chakrabarti <[e-mail]>
> Sent: Monday, July 3, 2023 3:55 PM
> To: Alexander Lobakin <[e-mail]>; souradeep chakrabarti
> <[e-mail]>
> Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
> <[e-mail]>; [e-mail]; Dexuan Cui
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
> Sharma <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: RE: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload
> when host is unresponsive
> 
> 
> 
> >-----Original Message-----
> >From: Alexander Lobakin <[e-mail]>
> >Sent: Monday, July 3, 2023 10:18 PM
> >To: souradeep chakrabarti <[e-mail]>
> >Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
> ><[e-mail]>; [e-mail]; Dexuan Cui
> ><[e-mail]>; [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
> >Sharma <[e-mail]>; [e-mail];
> >[e-mail]; [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; [e-mail];
> >[e-mail]; [e-mail];
> >[e-mail]; Souradeep Chakrabarti
> <[e-mail]>
> >Subject: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload when
> >host is unresponsive
> >
> >From: Souradeep Chakrabarti <[e-mail]>
> >Date: Mon,  3 Jul 2023 01:49:31 -0700
> >
> >> From: Souradeep Chakrabarti <[e-mail]>
> >
> >Please sync your Git name and Git mail account settings, so that your own
> >patches won't have "From:" when sending. From what I see, you need to
> >correct first letters of name and surname to capital in the Git email settings
> >block.
> Thank you for pointing, I will fix it.
> >
> >>
> >> When unloading the MANA driver, mana_dealloc_queues() waits for the
> >> MANA hardware to complete any inflight packets and set the pending
> >> send count to zero. But if the hardware has failed,
> >> mana_dealloc_queues() could wait forever.
> >>
> >> Fix this by adding a timeout to the wait. Set the timeout to 120
> >> seconds, which is a somewhat arbitrary value that is more than long
> >> enough for functional hardware to complete any sends.
> >>
> >> Signed-off-by: Souradeep Chakrabarti
> >> <[e-mail]>
> >
> >Where's "Fixes:" tagging the blamed commit?
> This is present from the day zero of the mana driver code.
> It has not been introduced in the code by any commit.
> >
> >> ---
> >> V3 -> V4:
> >> * Fixed the commit message to describe the context.
> >> * Removed the vf_unload_timeout, as it is not required.
> >> ---
> >>  drivers/net/ethernet/microsoft/mana/mana_en.c | 26
> >> ++++++++++++++++---
> >>  1 file changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> >> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> >> index a499e460594b..d26f1da70411 100644
> >> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> >> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> >> @@ -2346,7 +2346,10 @@ static int mana_dealloc_queues(struct
> >> net_device *ndev)  {
> >>  	struct mana_port_context *apc = netdev_priv(ndev);
> >>  	struct gdma_dev *gd = apc->ac->gdma_dev;
> >> +	unsigned long timeout;
> >>  	struct mana_txq *txq;
> >> +	struct sk_buff *skb;
> >> +	struct mana_cq *cq;
> >>  	int i, err;
> >>
> >>  	if (apc->port_is_up)
> >> @@ -2363,15 +2366,32 @@ static int mana_dealloc_queues(struct
> >net_device *ndev)
> >>  	 * to false, but it doesn't matter since mana_start_xmit() drops any
> >>  	 * new packets due to apc->port_is_up being false.
> >>  	 *
> >> -	 * Drain all the in-flight TX packets
> >> +	 * Drain all the in-flight TX packets.
> >> +	 * A timeout of 120 seconds for all the queues is used.
> >> +	 * This will break the while loop when h/w is not responding.
> >> +	 * This value of 120 has been decided here considering max
> >> +	 * number of queues.
> >>  	 */
> >> +
> >> +	timeout = jiffies + 120 * HZ;
> >
> >Why not initialize it right when declaring?
> I will fix it in next version.
> >
> >>  	for (i = 0; i < apc->num_queues; i++) {
> >>  		txq = &apc->tx_qp[i].txq;
> >> -
> >> -		while (atomic_read(&txq->pending_sends) > 0)
> >> +		while (atomic_read(&txq->pending_sends) > 0 &&
> >> +		       time_before(jiffies, timeout)) {
> >>  			usleep_range(1000, 2000);> +		}
> >>  	}
> >
> >120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
> >iterations. I know usleep_range() often is much less precise, but still.
> >Do you really need that much time? Has this been measured during the tests
> >that it can take up to 120 seconds or is it just some random value that "should
> >be enough"?
> >If you really need 120 seconds, I'd suggest using a timer / delayed work
> instead
> >of wasting resources.
> Here the intent is not waiting for 120 seconds, rather than avoid continue
> checking the
> pending_sends  of each tx queues for an indefinite time, before freeing
> sk_buffs.
> The pending_sends can only get decreased for a tx queue,  if mana_poll_tx_cq()
> gets called for a completion notification and increased by xmit.
> 
> In this particular bug, apc->port_is_up is not set to false, causing
> xmit to keep increasing the pending_sends for the queue and
> mana_poll_tx_cq()
> not getting called for the queue.
> 
> If we see the comment in the function mana_dealloc_queues(), it mentions it :
> 
> 2346     /* No packet can be transmitted now since apc->port_is_up is false.
> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
> 2348      * a txq because it may not timely see apc->port_is_up being cleared
> 2349      * to false, but it doesn't matter since mana_start_xmit() drops any
> 2350      * new packets due to apc->port_is_up being false.
> 
> The value 120 seconds has been decided here based on maximum number of
> queues
> are allowed in this specific hardware, it is a safe assumption.

I agree. Also, this waiting time is usually much shorter than 120 sec. The long 
wait only happens in rare and unexpected NIC HW non-responding cases. To 
further reduce the resource consumption, we can double the usleep_range() 
time in every iteration. So, the number of iterations will be greatly reduced 
before reaching 120 sec.

Thanks,
- Haiyang

Alexander Lobakin July 5, 2023, 2:35 p.m. UTC | #6

From: Souradeep Chakrabarti <[e-mail]>
Date: Mon, 3 Jul 2023 19:55:06 +0000

> 
> 
>> -----Original Message-----
>> From: Alexander Lobakin <[e-mail]>
>> Sent: Monday, July 3, 2023 10:18 PM

[...]

>>>  	for (i = 0; i < apc->num_queues; i++) {
>>>  		txq = &apc->tx_qp[i].txq;
>>> -
>>> -		while (atomic_read(&txq->pending_sends) > 0)
>>> +		while (atomic_read(&txq->pending_sends) > 0 &&
>>> +		       time_before(jiffies, timeout)) {
>>>  			usleep_range(1000, 2000);> +		}
>>>  	}
>>
>> 120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
>> iterations. I know usleep_range() often is much less precise, but still.
>> Do you really need that much time? Has this been measured during the tests
>> that it can take up to 120 seconds or is it just some random value that "should
>> be enough"?
>> If you really need 120 seconds, I'd suggest using a timer / delayed work instead
>> of wasting resources.
> Here the intent is not waiting for 120 seconds, rather than avoid continue checking the 
> pending_sends  of each tx queues for an indefinite time, before freeing sk_buffs.
> The pending_sends can only get decreased for a tx queue,  if mana_poll_tx_cq()
> gets called for a completion notification and increased by xmit.
> 
> In this particular bug, apc->port_is_up is not set to false, causing
> xmit to keep increasing the pending_sends for the queue and mana_poll_tx_cq()
> not getting called for the queue.
> 
> If we see the comment in the function mana_dealloc_queues(), it mentions it :
> 
> 2346     /* No packet can be transmitted now since apc->port_is_up is false.
> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
> 2348      * a txq because it may not timely see apc->port_is_up being cleared
> 2349      * to false, but it doesn't matter since mana_start_xmit() drops any
> 2350      * new packets due to apc->port_is_up being false.
> 
> The value 120 seconds has been decided here based on maximum number of queues

This is quite opposite to what you're saying above. How should I connect
these two:

Here the intent is not waiting for 120 seconds

+

The value 120 seconds has been decided here based on maximum number of
queues

?
Can cleaning the Tx queues really last for 120 seconds?
My understanding is that timeouts need to be sensible and not go to the
outer space. What is the medium value you got during the tests?

> are allowed in this specific hardware, it is a safe assumption.
>>
>>>
>>> +	for (i = 0; i < apc->num_queues; i++) {
>>> +		txq = &apc->tx_qp[i].txq;
>>> +		cq = &apc->tx_qp[i].tx_cq;
>>
>> cq can be just &txq->tx_cq.
> mana_txq  structure does not have a pointer to mana_cq.

Sorry, misread, my bad.

>>
>>> +		while (atomic_read(&txq->pending_sends)) {
>>> +			skb = skb_dequeue(&txq->pending_skbs);
>>> +			mana_unmap_skb(skb, apc);
>>> +			napi_consume_skb(skb, cq->budget);
>>
>> (you already have comment about this one)
>>
>>> +			atomic_sub(1, &txq->pending_sends);
>>> +		}
>>> +	}
>>>  	/* We're 100% sure the queues can no longer be woken up, because
>>>  	 * we're sure now mana_poll_tx_cq() can't be running.
>>>  	 */
>>
>> Thanks,
>> Olek
Thanks,
Olek

Souradeep Chakrabarti July 6, 2023, 10:41 a.m. UTC | #7

>-----Original Message-----
>From: Alexander Lobakin <[e-mail]>
>Sent: Wednesday, July 5, 2023 8:06 PM
>To: Souradeep Chakrabarti <[e-mail]>; souradeep
>chakrabarti <[e-mail]>
>Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
><[e-mail]>; [e-mail]; Dexuan Cui
><[e-mail]>; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
>Sharma <[e-mail]>; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail];
>[e-mail]
>Subject: Re: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload
>when host is unresponsive
>
>[You don't often get email from [e-mail]. Learn why this is
>important at https://aka.ms/LearnAboutSenderIdentification ]
>
>From: Souradeep Chakrabarti <[e-mail]>
>Date: Mon, 3 Jul 2023 19:55:06 +0000
>
>>
>>
>>> -----Original Message-----
>>> From: Alexander Lobakin <[e-mail]>
>>> Sent: Monday, July 3, 2023 10:18 PM
>
>[...]
>
>>>>     for (i = 0; i < apc->num_queues; i++) {
>>>>             txq = &apc->tx_qp[i].txq;
>>>> -
>>>> -           while (atomic_read(&txq->pending_sends) > 0)
>>>> +           while (atomic_read(&txq->pending_sends) > 0 &&
>>>> +                  time_before(jiffies, timeout)) {
>>>>                     usleep_range(1000, 2000);> +            }
>>>>     }
>>>
>>> 120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
>>> iterations. I know usleep_range() often is much less precise, but still.
>>> Do you really need that much time? Has this been measured during the
>>> tests that it can take up to 120 seconds or is it just some random
>>> value that "should be enough"?
>>> If you really need 120 seconds, I'd suggest using a timer / delayed
>>> work instead of wasting resources.
>> Here the intent is not waiting for 120 seconds, rather than avoid
>> continue checking the pending_sends  of each tx queues for an indefinite time,
>before freeing sk_buffs.
>> The pending_sends can only get decreased for a tx queue,  if
>> mana_poll_tx_cq() gets called for a completion notification and increased by
>xmit.
>>
>> In this particular bug, apc->port_is_up is not set to false, causing
>> xmit to keep increasing the pending_sends for the queue and
>> mana_poll_tx_cq() not getting called for the queue.
>>
>> If we see the comment in the function mana_dealloc_queues(), it mentions it :
>>
>> 2346     /* No packet can be transmitted now since apc->port_is_up is false.
>> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
>> 2348      * a txq because it may not timely see apc->port_is_up being cleared
>> 2349      * to false, but it doesn't matter since mana_start_xmit() drops any
>> 2350      * new packets due to apc->port_is_up being false.
>>
>> The value 120 seconds has been decided here based on maximum number of
>> queues
>
>This is quite opposite to what you're saying above. How should I connect these
>two:
>
>Here the intent is not waiting for 120 seconds
>
>+
>
>The value 120 seconds has been decided here based on maximum number of
>queues
>
>?
>Can cleaning the Tx queues really last for 120 seconds?
>My understanding is that timeouts need to be sensible and not go to the outer
>space. What is the medium value you got during the tests?
>
For each queue each takes few milli second, in a normal condition. So
based on maximum number of allowed queues for our h/w it won't
go beyond a sec. 
The 120s only happens rarely during some NIC HW issue -unexpected.
So this timeout will only trigger in a very rare scenario.
>> are allowed in this specific hardware, it is a safe assumption.
>>>
>>>>
>>>> +   for (i = 0; i < apc->num_queues; i++) {
>>>> +           txq = &apc->tx_qp[i].txq;
>>>> +           cq = &apc->tx_qp[i].tx_cq;
>>>
>>> cq can be just &txq->tx_cq.
>> mana_txq  structure does not have a pointer to mana_cq.
>
>Sorry, misread, my bad.
>
>>>
>>>> +           while (atomic_read(&txq->pending_sends)) {
>>>> +                   skb = skb_dequeue(&txq->pending_skbs);
>>>> +                   mana_unmap_skb(skb, apc);
>>>> +                   napi_consume_skb(skb, cq->budget);
>>>
>>> (you already have comment about this one)
>>>
>>>> +                   atomic_sub(1, &txq->pending_sends);
>>>> +           }
>>>> +   }
>>>>     /* We're 100% sure the queues can no longer be woken up, because
>>>>      * we're sure now mana_poll_tx_cq() can't be running.
>>>>      */
>>>
>>> Thanks,
>>> Olek
>Thanks,
>Olek

Alexander Lobakin July 6, 2023, 11:39 a.m. UTC | #8

From: Souradeep Chakrabarti <[e-mail]>
Date: Thu, 6 Jul 2023 10:41:03 +0000

> 
> 
>> -----Original Message-----
>> From: Alexander Lobakin <[e-mail]>
>> Sent: Wednesday, July 5, 2023 8:06 PM

[...]

>>>> 120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
>>>> iterations. I know usleep_range() often is much less precise, but still.
>>>> Do you really need that much time? Has this been measured during the
>>>> tests that it can take up to 120 seconds or is it just some random
>>>> value that "should be enough"?
>>>> If you really need 120 seconds, I'd suggest using a timer / delayed
>>>> work instead of wasting resources.
>>> Here the intent is not waiting for 120 seconds, rather than avoid
>>> continue checking the pending_sends  of each tx queues for an indefinite time,
>> before freeing sk_buffs.
>>> The pending_sends can only get decreased for a tx queue,  if
>>> mana_poll_tx_cq() gets called for a completion notification and increased by
>> xmit.
>>>
>>> In this particular bug, apc->port_is_up is not set to false, causing
>>> xmit to keep increasing the pending_sends for the queue and
>>> mana_poll_tx_cq() not getting called for the queue.
>>>
>>> If we see the comment in the function mana_dealloc_queues(), it mentions it :
>>>
>>> 2346     /* No packet can be transmitted now since apc->port_is_up is false.
>>> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
>>> 2348      * a txq because it may not timely see apc->port_is_up being cleared
>>> 2349      * to false, but it doesn't matter since mana_start_xmit() drops any
>>> 2350      * new packets due to apc->port_is_up being false.
>>>
>>> The value 120 seconds has been decided here based on maximum number of
>>> queues
>>
>> This is quite opposite to what you're saying above. How should I connect these
>> two:
>>
>> Here the intent is not waiting for 120 seconds
>>
>> +
>>
>> The value 120 seconds has been decided here based on maximum number of
>> queues
>>
>> ?
>> Can cleaning the Tx queues really last for 120 seconds?
>> My understanding is that timeouts need to be sensible and not go to the outer
>> space. What is the medium value you got during the tests?
>>
> For each queue each takes few milli second, in a normal condition. So
> based on maximum number of allowed queues for our h/w it won't
> go beyond a sec. 
> The 120s only happens rarely during some NIC HW issue -unexpected.
> So this timeout will only trigger in a very rare scenario.

So set the timeout to 2 seconds if it makes no difference?

>>> are allowed in this specific hardware, it is a safe assumption.
>>>>
>>>>>
>>>>> +   for (i = 0; i < apc->num_queues; i++) {
>>>>> +           txq = &apc->tx_qp[i].txq;
>>>>> +           cq = &apc->tx_qp[i].tx_cq;
[...]

Thanks,
Olek

Souradeep Chakrabarti July 6, 2023, 11:43 a.m. UTC | #9

>-----Original Message-----
>From: Alexander Lobakin <[e-mail]>
>Sent: Thursday, July 6, 2023 5:09 PM
>To: Souradeep Chakrabarti <[e-mail]>; souradeep
>chakrabarti <[e-mail]>
>Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
><[e-mail]>; [e-mail]; Dexuan Cui
><[e-mail]>; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
>Sharma <[e-mail]>; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail];
>[e-mail]
>Subject: Re: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload
>when host is unresponsive
>
>From: Souradeep Chakrabarti <[e-mail]>
>Date: Thu, 6 Jul 2023 10:41:03 +0000
>
>>
>>
>>> -----Original Message-----
>>> From: Alexander Lobakin <[e-mail]>
>>> Sent: Wednesday, July 5, 2023 8:06 PM
>
>[...]
>
>>>>> 120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
>>>>> iterations. I know usleep_range() often is much less precise, but still.
>>>>> Do you really need that much time? Has this been measured during
>>>>> the tests that it can take up to 120 seconds or is it just some
>>>>> random value that "should be enough"?
>>>>> If you really need 120 seconds, I'd suggest using a timer / delayed
>>>>> work instead of wasting resources.
>>>> Here the intent is not waiting for 120 seconds, rather than avoid
>>>> continue checking the pending_sends  of each tx queues for an
>>>> indefinite time,
>>> before freeing sk_buffs.
>>>> The pending_sends can only get decreased for a tx queue,  if
>>>> mana_poll_tx_cq() gets called for a completion notification and
>>>> increased by
>>> xmit.
>>>>
>>>> In this particular bug, apc->port_is_up is not set to false, causing
>>>> xmit to keep increasing the pending_sends for the queue and
>>>> mana_poll_tx_cq() not getting called for the queue.
>>>>
>>>> If we see the comment in the function mana_dealloc_queues(), it mentions
>it :
>>>>
>>>> 2346     /* No packet can be transmitted now since apc->port_is_up is false.
>>>> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
>>>> 2348      * a txq because it may not timely see apc->port_is_up being cleared
>>>> 2349      * to false, but it doesn't matter since mana_start_xmit() drops any
>>>> 2350      * new packets due to apc->port_is_up being false.
>>>>
>>>> The value 120 seconds has been decided here based on maximum number
>>>> of queues
>>>
>>> This is quite opposite to what you're saying above. How should I
>>> connect these
>>> two:
>>>
>>> Here the intent is not waiting for 120 seconds
>>>
>>> +
>>>
>>> The value 120 seconds has been decided here based on maximum number
>>> of queues
>>>
>>> ?
>>> Can cleaning the Tx queues really last for 120 seconds?
>>> My understanding is that timeouts need to be sensible and not go to
>>> the outer space. What is the medium value you got during the tests?
>>>
>> For each queue each takes few milli second, in a normal condition. So
>> based on maximum number of allowed queues for our h/w it won't go
>> beyond a sec.
>> The 120s only happens rarely during some NIC HW issue -unexpected.
>> So this timeout will only trigger in a very rare scenario.
>
>So set the timeout to 2 seconds if it makes no difference?
It can go near 120 seconds in a very rare MANA h/w scenario. That normally won't happen.
But during that scenario, we may need 120 seconds.
>
>>>> are allowed in this specific hardware, it is a safe assumption.
>>>>>
>>>>>>
>>>>>> +   for (i = 0; i < apc->num_queues; i++) {
>>>>>> +           txq = &apc->tx_qp[i].txq;
>>>>>> +           cq = &apc->tx_qp[i].tx_cq;
>[...]
>
>Thanks,
>Olek

Alexander Lobakin July 6, 2023, 11:48 a.m. UTC | #10

From: Souradeep Chakrabarti <[e-mail]>
Date: Thu, 6 Jul 2023 11:43:58 +0000

> 
> 
>> -----Original Message-----
>> From: Alexander Lobakin <[e-mail]>
>> Sent: Thursday, July 6, 2023 5:09 PM
>> To: Souradeep Chakrabarti <[e-mail]>; souradeep
>> chakrabarti <[e-mail]>
>> Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
>> <[e-mail]>; [e-mail]; Dexuan Cui
>> <[e-mail]>; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
>> Sharma <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload
>> when host is unresponsive
>>
>> From: Souradeep Chakrabarti <[e-mail]>
>> Date: Thu, 6 Jul 2023 10:41:03 +0000
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alexander Lobakin <[e-mail]>
>>>> Sent: Wednesday, July 5, 2023 8:06 PM
>>
>> [...]
>>
>>>>>> 120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
>>>>>> iterations. I know usleep_range() often is much less precise, but still.
>>>>>> Do you really need that much time? Has this been measured during
>>>>>> the tests that it can take up to 120 seconds or is it just some
>>>>>> random value that "should be enough"?
>>>>>> If you really need 120 seconds, I'd suggest using a timer / delayed
>>>>>> work instead of wasting resources.
>>>>> Here the intent is not waiting for 120 seconds, rather than avoid
>>>>> continue checking the pending_sends  of each tx queues for an
>>>>> indefinite time,
>>>> before freeing sk_buffs.
>>>>> The pending_sends can only get decreased for a tx queue,  if
>>>>> mana_poll_tx_cq() gets called for a completion notification and
>>>>> increased by
>>>> xmit.
>>>>>
>>>>> In this particular bug, apc->port_is_up is not set to false, causing
>>>>> xmit to keep increasing the pending_sends for the queue and
>>>>> mana_poll_tx_cq() not getting called for the queue.
>>>>>
>>>>> If we see the comment in the function mana_dealloc_queues(), it mentions
>> it :
>>>>>
>>>>> 2346     /* No packet can be transmitted now since apc->port_is_up is false.
>>>>> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-enable
>>>>> 2348      * a txq because it may not timely see apc->port_is_up being cleared
>>>>> 2349      * to false, but it doesn't matter since mana_start_xmit() drops any
>>>>> 2350      * new packets due to apc->port_is_up being false.
>>>>>
>>>>> The value 120 seconds has been decided here based on maximum number
>>>>> of queues
>>>>
>>>> This is quite opposite to what you're saying above. How should I
>>>> connect these
>>>> two:
>>>>
>>>> Here the intent is not waiting for 120 seconds
>>>>
>>>> +
>>>>
>>>> The value 120 seconds has been decided here based on maximum number
>>>> of queues
>>>>
>>>> ?
>>>> Can cleaning the Tx queues really last for 120 seconds?
>>>> My understanding is that timeouts need to be sensible and not go to
>>>> the outer space. What is the medium value you got during the tests?
>>>>
>>> For each queue each takes few milli second, in a normal condition. So
>>> based on maximum number of allowed queues for our h/w it won't go
>>> beyond a sec.
>>> The 120s only happens rarely during some NIC HW issue -unexpected.
>>> So this timeout will only trigger in a very rare scenario.
>>
>> So set the timeout to 2 seconds if it makes no difference?
> It can go near 120 seconds in a very rare MANA h/w scenario. That normally won't happen.
> But during that scenario, we may need 120 seconds.

This waiting loop is needed to let the pending Tx packets be sent. If
they weren't sent in 1 second, it most likely makes no sense already
whether they will be sent at all or not -- the destination host won't
wait for them for so long.
You say that it may happen only in case of HW issue. If so, I assume you
need to fix it some way, e.g. do a HW reset or so? If so, why bother
waiting for Tx completions if Tx is hung? You free all skbs later either
way, so there are no leaks.

>>
>>>>> are allowed in this specific hardware, it is a safe assumption.
>>>>>>
>>>>>>>
>>>>>>> +   for (i = 0; i < apc->num_queues; i++) {
>>>>>>> +           txq = &apc->tx_qp[i].txq;
>>>>>>> +           cq = &apc->tx_qp[i].tx_cq;
>> [...]
>>
>> Thanks,
>> Olek

Thanks,
Olek

Haiyang Zhang July 6, 2023, 1:54 p.m. UTC | #11

> -----Original Message-----
> From: Alexander Lobakin <[e-mail]>
> Sent: Thursday, July 6, 2023 7:49 AM
> To: Souradeep Chakrabarti <[e-mail]>; souradeep
> chakrabarti <[e-mail]>
> Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
> <[e-mail]>; [e-mail]; Dexuan Cui
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Long Li <[e-mail]>; Ajay
> Sharma <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload
> when host is unresponsive
> 
> From: Souradeep Chakrabarti <[e-mail]>
> Date: Thu, 6 Jul 2023 11:43:58 +0000
> 
> >
> >
> >> -----Original Message-----
> >> From: Alexander Lobakin <[e-mail]>
> >> Sent: Thursday, July 6, 2023 5:09 PM
> >> To: Souradeep Chakrabarti <[e-mail]>; souradeep
> >> chakrabarti <[e-mail]>
> >> Cc: KY Srinivasan <[e-mail]>; Haiyang Zhang
> >> <[e-mail]>; [e-mail]; Dexuan Cui
> >> <[e-mail]>; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Long Li <[e-mail]>;
> Ajay
> >> Sharma <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail];
> [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload
> >> when host is unresponsive
> >>
> >> From: Souradeep Chakrabarti <[e-mail]>
> >> Date: Thu, 6 Jul 2023 10:41:03 +0000
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Alexander Lobakin <[e-mail]>
> >>>> Sent: Wednesday, July 5, 2023 8:06 PM
> >>
> >> [...]
> >>
> >>>>>> 120 seconds by 2 msec step is 60000 iterations, by 1 msec is 120000
> >>>>>> iterations. I know usleep_range() often is much less precise, but still.
> >>>>>> Do you really need that much time? Has this been measured during
> >>>>>> the tests that it can take up to 120 seconds or is it just some
> >>>>>> random value that "should be enough"?
> >>>>>> If you really need 120 seconds, I'd suggest using a timer / delayed
> >>>>>> work instead of wasting resources.
> >>>>> Here the intent is not waiting for 120 seconds, rather than avoid
> >>>>> continue checking the pending_sends  of each tx queues for an
> >>>>> indefinite time,
> >>>> before freeing sk_buffs.
> >>>>> The pending_sends can only get decreased for a tx queue,  if
> >>>>> mana_poll_tx_cq() gets called for a completion notification and
> >>>>> increased by
> >>>> xmit.
> >>>>>
> >>>>> In this particular bug, apc->port_is_up is not set to false, causing
> >>>>> xmit to keep increasing the pending_sends for the queue and
> >>>>> mana_poll_tx_cq() not getting called for the queue.
> >>>>>
> >>>>> If we see the comment in the function mana_dealloc_queues(), it
> mentions
> >> it :
> >>>>>
> >>>>> 2346     /* No packet can be transmitted now since apc->port_is_up is
> false.
> >>>>> 2347      * There is still a tiny chance that mana_poll_tx_cq() can re-
> enable
> >>>>> 2348      * a txq because it may not timely see apc->port_is_up being
> cleared
> >>>>> 2349      * to false, but it doesn't matter since mana_start_xmit() drops
> any
> >>>>> 2350      * new packets due to apc->port_is_up being false.
> >>>>>
> >>>>> The value 120 seconds has been decided here based on maximum
> number
> >>>>> of queues
> >>>>
> >>>> This is quite opposite to what you're saying above. How should I
> >>>> connect these
> >>>> two:
> >>>>
> >>>> Here the intent is not waiting for 120 seconds
> >>>>
> >>>> +
> >>>>
> >>>> The value 120 seconds has been decided here based on maximum number
> >>>> of queues
> >>>>
> >>>> ?
> >>>> Can cleaning the Tx queues really last for 120 seconds?
> >>>> My understanding is that timeouts need to be sensible and not go to
> >>>> the outer space. What is the medium value you got during the tests?
> >>>>
> >>> For each queue each takes few milli second, in a normal condition. So
> >>> based on maximum number of allowed queues for our h/w it won't go
> >>> beyond a sec.
> >>> The 120s only happens rarely during some NIC HW issue -unexpected.
> >>> So this timeout will only trigger in a very rare scenario.
> >>
> >> So set the timeout to 2 seconds if it makes no difference?
> > It can go near 120 seconds in a very rare MANA h/w scenario. That normally
> won't happen.
> > But during that scenario, we may need 120 seconds.
> 
> This waiting loop is needed to let the pending Tx packets be sent. If
> they weren't sent in 1 second, it most likely makes no sense already
> whether they will be sent at all or not -- the destination host won't
> wait for them for so long.
> You say that it may happen only in case of HW issue. If so, I assume you
> need to fix it some way, e.g. do a HW reset or so? If so, why bother
> waiting for Tx completions if Tx is hung? You free all skbs later either
> way, so there are no leaks.

At that point, we don't actually care if the pending packets are sent or not. 
But if we free the queues too soon, and the HW is slow for unexpected 
reasons, a delayed completion notice will DMA into the freed memory and 
cause corruption. That's why we have a longer waiting time.

Souradeep, you may double check with hostnet team to see what's the 
best waiting time to ensure no more HW activities.

Thanks,
- Haiyang

Jason Gunthorpe July 10, 2023, 6:04 p.m. UTC | #12

On Thu, Jul 06, 2023 at 01:54:35PM +0000, Haiyang Zhang wrote:

> > This waiting loop is needed to let the pending Tx packets be sent. If
> > they weren't sent in 1 second, it most likely makes no sense already
> > whether they will be sent at all or not -- the destination host won't
> > wait for them for so long.
> > You say that it may happen only in case of HW issue. If so, I assume you
> > need to fix it some way, e.g. do a HW reset or so? If so, why bother
> > waiting for Tx completions if Tx is hung? You free all skbs later either
> > way, so there are no leaks.
> 
> At that point, we don't actually care if the pending packets are sent or not. 
> But if we free the queues too soon, and the HW is slow for unexpected 
> reasons, a delayed completion notice will DMA into the freed memory and 
> cause corruption. That's why we have a longer waiting time.

Aieiiie that is a horrible HW design to not have a strong fence of DMA.

"just wait and hope the HW doesn't UAF the kernel with DMA" is really
awful.

Jason

Patch

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index a499e460594b..d26f1da70411 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2346,7 +2346,10 @@  static int mana_dealloc_queues(struct net_device *ndev)
 {
 	struct mana_port_context *apc = netdev_priv(ndev);
 	struct gdma_dev *gd = apc->ac->gdma_dev;
+	unsigned long timeout;
 	struct mana_txq *txq;
+	struct sk_buff *skb;
+	struct mana_cq *cq;
 	int i, err;
 
 	if (apc->port_is_up)
@@ -2363,15 +2366,32 @@  static int mana_dealloc_queues(struct net_device *ndev)
 	 * to false, but it doesn't matter since mana_start_xmit() drops any
 	 * new packets due to apc->port_is_up being false.
 	 *
-	 * Drain all the in-flight TX packets
+	 * Drain all the in-flight TX packets.
+	 * A timeout of 120 seconds for all the queues is used.
+	 * This will break the while loop when h/w is not responding.
+	 * This value of 120 has been decided here considering max
+	 * number of queues.
 	 */
+
+	timeout = jiffies + 120 * HZ;
 	for (i = 0; i < apc->num_queues; i++) {
 		txq = &apc->tx_qp[i].txq;
-
-		while (atomic_read(&txq->pending_sends) > 0)
+		while (atomic_read(&txq->pending_sends) > 0 &&
+		       time_before(jiffies, timeout)) {
 			usleep_range(1000, 2000);
+		}
 	}
 
+	for (i = 0; i < apc->num_queues; i++) {
+		txq = &apc->tx_qp[i].txq;
+		cq = &apc->tx_qp[i].tx_cq;
+		while (atomic_read(&txq->pending_sends)) {
+			skb = skb_dequeue(&txq->pending_skbs);
+			mana_unmap_skb(skb, apc);
+			napi_consume_skb(skb, cq->budget);
+			atomic_sub(1, &txq->pending_sends);
+		}
+	}
 	/* We're 100% sure the queues can no longer be woken up, because
 	 * we're sure now mana_poll_tx_cq() can't be running.
 	 */