Re: [EXTERNAL] Re: [PATCH V4 net] net: mana: Fix MANA VF unload when host is unresponsive

[Alexander Lobakin]

From: Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxx>
Date: Mon, 3 Jul 2023 19:55:06 +0000

> 
> 
>> -----Original Message-----
>> From: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
>> 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