RE: [PATCH v4 7/7] wifi: rtw88: sdio: add TX back-pressure and retry on page starvation

[email protected] <[email protected]> wrote:
> From: Luka Gejak <[email protected]>
> 
> Two problems show up on RTL8723BS uplink. The per-AC software FIFO is
> unbounded, so mac80211 keeps handing frames down until latency collapses
> under load. And when the chip runs out of free TX pages the queue is
> simply abandoned for that pass, which stalls the AC until something else
> kicks the worker.
> 
> Stop the mac80211 queue once a data AC fills past a high watermark and
> wake it from the drain path when it falls back to a low one. Convert the
> TX work item to a delayed work so a temporary page shortage can be
> retried shortly afterwards instead of stalling, and cancel it on
> teardown.
> 
> Measured on RTL8723BS hardware, uplink goes from 11.9 Mbit/s with 204
> TCP retransmits to 20.1 Mbit/s with 2.
> 
> Signed-off-by: Luka Gejak <[email protected]>
> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 99 ++++++++++++++++++++---
>  drivers/net/wireless/realtek/rtw88/sdio.h |  3 +-
>  2 files changed, 91 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index e9f0d23113f3..ed1bed8ea144 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -22,6 +22,16 @@
>  #define RTW_SDIO_INDIRECT_RW_RETRIES                   50
>  #define RTW_SDIO_OQT_TIMEOUT_MS                                1000
> 
> +/*
> + * 8723BS SDIO TX FIFO back-pressure watermarks: stop the mac80211 queue once
> + * the per-AC software FIFO fills past the high watermark, and wake it from the
> + * TX drain path once it falls back to the low one. Bounds the queueing latency
> + * that otherwise causes uplink bufferbloat / congestion collapse.
> + */
> +#define RTW_SDIO_TX_FIFO_HIWATER                       16
> +#define RTW_SDIO_TX_FIFO_LOWATER                       8
> +#define RTW_SDIO_TX_RETRY_DELAY                        msecs_to_jiffies(1)

If HZ is smaller than 1000, the value will be 0 I think. Is it expected?
(may be not a problem)

> +
>  static bool rtw_sdio_is_bus_addr(u32 addr)
>  {
>         return !!(addr & RTW_SDIO_BUS_MSK);
> @@ -1053,7 +1063,11 @@ static void rtw_sdio_tx_kick_off(struct rtw_dev *rtwdev)
>  {
>         struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> 
> -       queue_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work);
> +       /*
> +        * A retry may already be pending with a delay; re-arm it so a newly
> +        * queued frame is not held back by it.
> +        */
> +       mod_delayed_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work, 0);
>  }
> 
>  static void rtw_sdio_link_ps(struct rtw_dev *rtwdev, bool enter)
> @@ -1177,6 +1191,19 @@ static int rtw_sdio_tx_write(struct rtw_dev *rtwdev,
> 
>         skb_queue_tail(&rtwsdio->tx_queue[queue], skb);
> 
> +       /*
> +        * Back-pressure on the data ACs (BK/BE/VI/VO): once the FIFO fills past
> +        * the high watermark, stop the corresponding mac80211 queue so it stops
> +        * handing us frames, bounding the queueing latency. Resumed from the TX
> +        * drain path once the FIFO drains below the low watermark.
> +        */
> +       if (rtw_is_8723bs(rtwdev) && queue < RTW_TX_QUEUE_BCN &&
> +           !rtwsdio->queue_stopped[queue] &&
> +           skb_queue_len(&rtwsdio->tx_queue[queue]) >= RTW_SDIO_TX_FIFO_HIWATER) {
> +               rtwsdio->queue_stopped[queue] = true;
> +               ieee80211_stop_queue(rtwdev->hw, skb_get_queue_mapping(skb));
> +       }
> +

Can we move this chunk into a function? Then, you don't need to write
complicated conditions into a if-statement. 

As well as the chunk to call ieee80211_wake_queue().

>         return 0;
>  }
> 
> @@ -1479,43 +1506,92 @@ static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
>         ieee80211_tx_status_irqsafe(hw, skb);
>  }
> 
> -static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> -                                     enum rtw_tx_queue_type queue)
> +/*
> + * Send one frame from @queue. Returns 1 when a frame was written, 0 when the
> + * queue was empty 

I'd prefer 0 is successful case. 1 is special case (queue was empty).

> and a negative errno when the write failed, in which case
> + * the frame is put back at the head of the queue.
> + */
> +static int rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> +                                    enum rtw_tx_queue_type queue)
>  {
>         struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
>         struct sk_buff *skb;
> +       u16 q_map;
>         int ret;
> 
>         skb = skb_dequeue(&rtwsdio->tx_queue[queue]);
>         if (!skb)
> -               return;
> +               return 0;
> 
> +       q_map = skb_get_queue_mapping(skb);
>         ret = rtw_sdio_write_port(rtwdev, skb, queue);
>         if (ret) {
>                 skb_queue_head(&rtwsdio->tx_queue[queue], skb);
> -               return;
> +               return ret;
>         }
> 
>         rtw_sdio_indicate_tx_status(rtwdev, skb);
> +
> +       if (rtw_is_8723bs(rtwdev) && queue < RTW_TX_QUEUE_BCN &&
> +           rtwsdio->queue_stopped[queue] &&
> +           skb_queue_len(&rtwsdio->tx_queue[queue]) <=
> +           RTW_SDIO_TX_FIFO_LOWATER) {
> +               rtwsdio->queue_stopped[queue] = false;
> +               ieee80211_wake_queue(rtwdev->hw, q_map);
> +       }

Move to a function.

> +
> +       return 1;
>  }
> 
>  static void rtw_sdio_tx_handler(struct work_struct *work)
>  {
>         struct rtw_sdio_work_data *work_data =
> -               container_of(work, struct rtw_sdio_work_data, work);
> +               container_of(to_delayed_work(work), struct rtw_sdio_work_data,
> +                            work);
>         struct rtw_sdio *rtwsdio;
>         struct rtw_dev *rtwdev;
> -       int limit, queue;
> +       int limit, queue, ret;
> +       bool rtl8723bs;
> 
>         rtwdev = work_data->rtwdev;
>         rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +       rtl8723bs = rtw_is_8723bs(rtwdev);
> 
>         if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_TX_WAKE))
>                 rtw_sdio_deep_ps_leave(rtwdev);
> 
>         for (queue = RTK_MAX_TX_QUEUE_NUM - 1; queue >= 0; queue--) {
>                 for (limit = 0; limit < 1000; limit++) {
> -                       rtw_sdio_process_tx_queue(rtwdev, queue);
> +                       ret = rtw_sdio_process_tx_queue(rtwdev, queue);
> +                       if (ret == 0)
> +                               break;
> +
> +                       if (ret < 0) {
> +                               /*
> +                                * A page shortage is transient: leave the
> +                                * frame queued and come back shortly instead
> +                                * of stalling this AC until something else
> +                                * kicks the worker.
> +                                */
> +                               if (rtl8723bs && ret == -EBUSY) {
> +                                       queue_delayed_work(rtwsdio->txwq,
> +                                                          &work_data->work,
> +                                                          RTW_SDIO_TX_RETRY_DELAY);
> +                                       return;
> +                               }
> +                               break;
> +                       }
> +
> +                       /*
> +                        * Restart from the highest priority queue after every
> +                        * management frame so the join sequence is not held up
> +                        * behind a data backlog.
> +                        */
> +                       if (rtl8723bs && queue == RTW_TX_QUEUE_MGMT) {
> +                               queue_delayed_work(rtwsdio->txwq,
> +                                                  &work_data->work, 0);
> +                               return;
> +                       }

Move this chunk into a function named reschedule_tx_work() or something
like that.

> 
>                         if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
>                                 break;
> @@ -1542,14 +1618,16 @@ static int rtw_sdio_init_tx(struct rtw_dev *rtwdev)
>                 return -ENOMEM;
>         }
> 
> -       for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++)
> +       for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++) {
>                 skb_queue_head_init(&rtwsdio->tx_queue[i]);
> +               rtwsdio->queue_stopped[i] = false;
> +       }
>         rtwsdio->tx_handler_data = kmalloc_obj(*rtwsdio->tx_handler_data);
>         if (!rtwsdio->tx_handler_data)
>                 goto err_destroy_wq;
> 
>         rtwsdio->tx_handler_data->rtwdev = rtwdev;
> -       INIT_WORK(&rtwsdio->tx_handler_data->work, rtw_sdio_tx_handler);
> +       INIT_DELAYED_WORK(&rtwsdio->tx_handler_data->work, rtw_sdio_tx_handler);
> 
>         return 0;
> 
> @@ -1563,6 +1641,7 @@ static void rtw_sdio_deinit_tx(struct rtw_dev *rtwdev)
>         struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
>         int i;
> 
> +       cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
>         destroy_workqueue(rtwsdio->txwq);
>         kfree(rtwsdio->tx_handler_data);
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.h b/drivers/net/wireless/realtek/rtw88/sdio.h
> index 7474a7511811..2f1b67f4bbc7 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.h
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.h
> @@ -156,7 +156,7 @@ struct rtw_sdio_tx_data {
>  };
> 
>  struct rtw_sdio_work_data {
> -       struct work_struct work;
> +       struct delayed_work work;
>         struct rtw_dev *rtwdev;
>  };
> 
> @@ -172,6 +172,7 @@ struct rtw_sdio {
>         struct workqueue_struct *txwq;
>         struct rtw_sdio_work_data *tx_handler_data;
>         struct sk_buff_head tx_queue[RTK_MAX_TX_QUEUE_NUM];
> +       bool queue_stopped[RTK_MAX_TX_QUEUE_NUM];

add 'tx_' prefix? tx_queue_stopped[] 

> 
>         atomic_t free_pg_high;
>         atomic_t free_pg_normal;
> --
> 2.53.0