Re: [PATCH v5 01/11] mt76: add hash lookup for skb on TXS status_list

[Ben Greear <greearb@candelatech.com>]

On 8/13/21 10:46 AM, Felix Fietkau wrote:

>>>> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
>>>> index 6f302acb6e69..4c8504d3c904 100644
>>>> --- a/drivers/net/wireless/mediatek/mt76/tx.c
>>>> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
>>>> @@ -130,15 +154,30 @@ mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid,
>>>>    			     IEEE80211_TX_CTL_RATE_CTRL_PROBE)))
>>>>    		return MT_PACKET_ID_NO_SKB;
>>>>    
>>>> +	/* due to limited range of the pktid (7 bits), we can only
>>>> +	 * have a limited number of outstanding frames.  I think it is OK to
>>>> +	 * check the length outside of a lock since it doesn't matter too much
>>>> +	 * if we read wrong data here.
>>>> +	 * The TX-status callbacks don't always return a callback for an SKB,
>>>> +	 * so the status_list may contain some stale skbs.  Those will be cleaned
>>>> +	 * out periodically, see MT_TX_STATUS_SKB_TIMEOUT.
>>>> +	 */
>>>> +
>>>> +	qlen = skb_queue_len(&dev->status_list);
>>>> +	if (qlen > 120)
>>>> +		return MT_PACKET_ID_NO_SKB;
>>> Checking the length of the per-device status list doesn't make sense,
>>> since pktid allocation is per-wcid.
>>
>> Ok, so just remove this code, or should I set some other higher
>> limit to bound the list?
> You could just check for a duplicate skb_status_array entry.

Ok, that will happen anyway when searching for next wcid pkt-id.

The check above was a quick bail-out before locks were acquired.

I'll just remove that qlen check...

Thanks,
Ben

> 
> - Felix
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com