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

[email protected] <[email protected]> wrote:

[...]

> +/*
> + * Back-pressure on the data ACs (BK/BE/VI/VO): once the software FIFO fills
> + * past the high watermark, stop the corresponding mac80211 queue so it stops
> + * handing frames down, which bounds the queueing latency. The queue is woken
> + * again from the TX drain path once the FIFO falls back to the low watermark.
> + */
> +static void rtw_sdio_8723bs_stop_tx_queue(struct rtw_dev *rtwdev,
> +                                         enum rtw_tx_queue_type queue,
> +                                         u16 q_map)
> +{
> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +
> +       if (!rtw_is_8723bs(rtwdev) || queue >= RTW_TX_QUEUE_BCN)
> +               return;
> +
> +       if (READ_ONCE(rtwsdio->tx_queue_stopped[queue]))
> +               return;
> +
> +       if (skb_queue_len(&rtwsdio->tx_queue[queue]) < RTW_SDIO_TX_FIFO_HIWATER)
> +               return;
> +
> +       WRITE_ONCE(rtwsdio->tx_queue_stopped[queue], true);
> +       ieee80211_stop_queue(rtwdev->hw, q_map);
> +
> +       /*
> +        * The worker may have drained the queue between the length check
> +        * above and the flag becoming visible; its wake check then saw the
> +        * flag still clear and this stop would never be undone. Re-check
> +        * now that the flag is set and undo the stop if so. Both sides can
> +        * wake, which is harmless; the barrier pairs with the one in
> +        * rtw_sdio_8723bs_wake_tx_queue() so at least one side does.
> +        */
> +       smp_mb();

With WRITE_ONCE() and READ_ONCE(), I think it will concurrency work
well. Did you really encounter problems with smp_mb()?

a blank line.

> +       if (skb_queue_len(&rtwsdio->tx_queue[queue]) <=
> +           RTW_SDIO_TX_FIFO_LOWATER) {
> +               WRITE_ONCE(rtwsdio->tx_queue_stopped[queue], false);
> +               ieee80211_wake_queue(rtwdev->hw, q_map);
> +       }
> +}
> +
> +static void rtw_sdio_8723bs_wake_tx_queue(struct rtw_dev *rtwdev,
> +                                         enum rtw_tx_queue_type queue,
> +                                         u16 q_map)
> +{
> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +
> +       if (!rtw_is_8723bs(rtwdev) || queue >= RTW_TX_QUEUE_BCN)
> +               return;
> +
> +       /* pairs with the barrier in rtw_sdio_8723bs_stop_tx_queue() */
> +       smp_mb();
> +
> +       if (!READ_ONCE(rtwsdio->tx_queue_stopped[queue]))
> +               return;
> +
> +       if (skb_queue_len(&rtwsdio->tx_queue[queue]) > RTW_SDIO_TX_FIFO_LOWATER)
> +               return;
> +
> +       WRITE_ONCE(rtwsdio->tx_queue_stopped[queue], false);
> +       ieee80211_wake_queue(rtwdev->hw, q_map);
> +}
> +

[...]

> 
> -static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> -                                     enum rtw_tx_queue_type queue)
> +/*
> + * Send one frame from @queue. Returns 0 when a frame was written, 1 when the
> + * 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 1;
> 
> +       q_map = skb_get_queue_mapping(skb);

Can we move below just before the user?

>         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);
> +
> +       rtw_sdio_8723bs_wake_tx_queue(rtwdev, queue, q_map);
> +
> +       return 0;
> +}
> +