RE: [PATCH rtw-next v2 1/3] wifi: rtw88: usb: bound what the driver feeds the after-DTIM queue

[Ping-Ke Shih <pkshih@realtek.com>]

Mehmet Fide <mehmet.fide@gmail.com> wrote:
> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> 
> Frames routed to the high queue are transmitted right after DTIM beacons
> only, inside the ATIM window, while they wait in the shared TX page pool.
> The driver puts no limit on how many it hands over, so as long as one
> station dozes, any sustained broadcast or multicast traffic outruns the
> drain and empties the pool: measured on an RTL8822BU AP with a single
> client in power save and ~40 frames/s of mDNS chatter, the free page
> count at 0x240 goes from 1803 to 16 in about 100 seconds and stays there
> for as long as the traffic lasts. From that point every other transmit
> queues behind the backlog, authentication responses arrive too late for
> anyone to join, and the reserved page download fails ("error beacon
> valid"). The AP keeps beaconing and only a reboot recovers.
> 
> Feed the high queue through a small budget that refills below the
> measured drain rate (about 3 frames per DTIM, ~15/s at dtim_period 2 on
> the default 2 TU ATIM window); whatever exceeds the budget leaves on its
> access category queue right away. The high queue backlog is now bounded
> by the burst size under any load, so the page pool cannot run dry, at
> the price that a dozing station may miss part of a broadcast storm.
> 
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>

Some minor/nit questions. If you don't change others, just take my
acked-by to v3.

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

> 
>  drivers/net/wireless/realtek/rtw88/usb.c | 41 ++++++++++++++++++++++--
>  drivers/net/wireless/realtek/rtw88/usb.h |  5 +++
>  2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index c90802919473..5482e44f4a88 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -562,7 +562,37 @@ static int rtw_usb_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
>         return rtw_usb_write_data(rtwdev, &pkt_info, buf);
>  }
> 
> -static u8 rtw_usb_tx_queue_mapping_to_qsel(struct sk_buff *skb)
> +#define RTW_USB_HIQ_REFILL_INTERVAL    (HZ / 10)       /* jiffies per unit of budget */

Does it mean a budget per 100ms roughly?

> +#define RTW_USB_HIQ_BUDGET_MAX         16
> +
> +static bool rtw_usb_hiq_take_budget(struct rtw_usb *rtwusb)
> +{
> +       unsigned long flags, elapsed, add;
> +       bool ok;
> +
> +       spin_lock_irqsave(&rtwusb->hiq_lock, flags);
> +
> +       elapsed = jiffies - rtwusb->hiq_refill;
> +       add = elapsed / RTW_USB_HIQ_REFILL_INTERVAL;
> +       if (add) {
> +               rtwusb->hiq_budget = min_t(unsigned long, rtwusb->hiq_budget + add,
> +                                          RTW_USB_HIQ_BUDGET_MAX);
> +               /* The part of the current interval that has not completed yet
> +                * keeps counting toward the next unit
> +                */

First line of comment block should be empty.

> +               rtwusb->hiq_refill = jiffies - elapsed % RTW_USB_HIQ_REFILL_INTERVAL;
> +       }

nit: an empty line

> +       ok = rtwusb->hiq_budget > 0;
> +       if (ok)
> +               rtwusb->hiq_budget--;
> +
> +       spin_unlock_irqrestore(&rtwusb->hiq_lock, flags);
> +
> +       return ok;
> +}
> +
> +static u8 rtw_usb_tx_queue_mapping_to_qsel(struct rtw_usb *rtwusb,
> +                                          struct sk_buff *skb)
>  {
>         struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);