diff mbox series

[1/2] mwifiex: dispatch/rotate from reorder table atomically

Message ID 20190604205323.200361-2-briannorris@chromium.org
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series mwifiex: spinlock usage improvements | expand

Commit Message

Brian Norris June 4, 2019, 8:53 p.m. UTC
mwifiex_11n_scan_and_dispatch() and
mwifiex_11n_dispatch_pkt_until_start_win() share similar patterns, where
they perform a few different actions on the same table, using the same
lock, but non-atomically. There have been other attempts to clean up
this sort of behavior, but they have had problems (incomplete;
introducing new deadlocks).

We can improve these functions' atomicity by queueing up our RX packets
in a list, to dispatch at the end of the function. This avoids problems
of another operation modifying the table in between our dispatch and
rotation operations.

This was inspired by investigations around this:

  http://lkml.kernel.org/linux-wireless/20181130175957.167031-1-briannorris@chromium.org
  Subject: [4.20 PATCH] Revert "mwifiex: restructure rx_reorder_tbl_lock usage"

While the original (now-reverted) patch had good intentions in
restructuring some of the locking patterns in this driver, it missed an
important detail: we cannot defer to softirq contexts while already in
an atomic context. We can help avoid this sort of problem by separating
the two steps of:
(1) iterating / clearing the mwifiex reordering table
(2) dispatching received packets to upper layers

This makes it much harder to make lock recursion mistakes, as these
two steps no longer need to hold the same locks.

Testing: I've played with a variety of stress tests, including download
stress tests on the same APs which caught regressions with commit
5188d5453bc9 ("mwifiex: restructure rx_reorder_tbl_lock usage"). I've
primarily tested on Marvell 8997 / PCIe, although I've given 8897 / SDIO
a quick spin as well.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 .../wireless/marvell/mwifiex/11n_rxreorder.c  | 43 +++++++++++--------
 1 file changed, 24 insertions(+), 19 deletions(-)

Comments

Ganapathi Bhat June 5, 2019, 9:15 a.m. UTC | #1
Hi Brian,

> (1) iterating / clearing the mwifiex reordering table
> (2) dispatching received packets to upper layers
> 
> This makes it much harder to make lock recursion mistakes, as these two
> steps no longer need to hold the same locks.

Yes, this is clean;

> 
> Testing: I've played with a variety of stress tests, including download stress
> tests on the same APs which caught regressions with commit
> 5188d5453bc9 ("mwifiex: restructure rx_reorder_tbl_lock usage"). I've
> primarily tested on Marvell 8997 / PCIe, although I've given 8897 / SDIO a
> quick spin as well.
> 

Thanks a lot for this; We will also run the tests locally; But, I find the change is good;

Acked-by: Ganapathi Bhat <gbhat@marvell.com>

Regards,
Ganapathi
Kalle Valo June 25, 2019, 4:45 a.m. UTC | #2
Brian Norris <briannorris@chromium.org> wrote:

> mwifiex_11n_scan_and_dispatch() and
> mwifiex_11n_dispatch_pkt_until_start_win() share similar patterns, where
> they perform a few different actions on the same table, using the same
> lock, but non-atomically. There have been other attempts to clean up
> this sort of behavior, but they have had problems (incomplete;
> introducing new deadlocks).
> 
> We can improve these functions' atomicity by queueing up our RX packets
> in a list, to dispatch at the end of the function. This avoids problems
> of another operation modifying the table in between our dispatch and
> rotation operations.
> 
> This was inspired by investigations around this:
> 
>   http://lkml.kernel.org/linux-wireless/20181130175957.167031-1-briannorris@chromium.org
>   Subject: [4.20 PATCH] Revert "mwifiex: restructure rx_reorder_tbl_lock usage"
> 
> While the original (now-reverted) patch had good intentions in
> restructuring some of the locking patterns in this driver, it missed an
> important detail: we cannot defer to softirq contexts while already in
> an atomic context. We can help avoid this sort of problem by separating
> the two steps of:
> (1) iterating / clearing the mwifiex reordering table
> (2) dispatching received packets to upper layers
> 
> This makes it much harder to make lock recursion mistakes, as these
> two steps no longer need to hold the same locks.
> 
> Testing: I've played with a variety of stress tests, including download
> stress tests on the same APs which caught regressions with commit
> 5188d5453bc9 ("mwifiex: restructure rx_reorder_tbl_lock usage"). I've
> primarily tested on Marvell 8997 / PCIe, although I've given 8897 / SDIO
> a quick spin as well.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Acked-by: Ganapathi Bhat <gbhat@marvell.com>

New warning:

drivers/net/wireless/marvell/mwifiex/wmm.c: In function 'mwifiex_wmm_process_tx':
drivers/net/wireless/marvell/mwifiex/wmm.c:1438:4: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
    mwifiex_11n_aggregate_pkt(priv, ptr, ptr_index, flags);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/marvell/mwifiex/wmm.c:1406:16: note: 'flags' was declared here
  unsigned long flags;
                ^~~~~

2 patches set to Changes Requested.

10976083 [1/2] mwifiex: dispatch/rotate from reorder table atomically
10976087 [2/2] mwifiex: don't disable hardirqs; just softirqs
Brian Norris June 25, 2019, 5:26 p.m. UTC | #3
On Mon, Jun 24, 2019 at 9:45 PM Kalle Valo <kvalo@codeaurora.org> wrote:
> New warning:
>
> drivers/net/wireless/marvell/mwifiex/wmm.c: In function 'mwifiex_wmm_process_tx':
> drivers/net/wireless/marvell/mwifiex/wmm.c:1438:4: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     mwifiex_11n_aggregate_pkt(priv, ptr, ptr_index, flags);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/marvell/mwifiex/wmm.c:1406:16: note: 'flags' was declared here
>   unsigned long flags;
>                 ^~~~~

Yikes! Not sure how I missed that, as I *thought* I had -Werror
enabled. Maybe things got lost a bit in the shuffles from GCC to Clang
for building our kernels internally.

Anyway, "used" is a bit generous here, since these are just useless
function arguments (never *actually* used now). I should remove these
args entirely.

> 2 patches set to Changes Requested.

Will send a v2.

Thanks,
Brian
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index 5380fba652cc..77bdf16d6573 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -76,7 +76,8 @@  static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
 /* This function will process the rx packet and forward it to kernel/upper
  * layer.
  */
-static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
+static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv,
+				    struct sk_buff *payload)
 {
 
 	int ret;
@@ -109,27 +110,26 @@  mwifiex_11n_dispatch_pkt_until_start_win(struct mwifiex_private *priv,
 					 struct mwifiex_rx_reorder_tbl *tbl,
 					 int start_win)
 {
+	struct sk_buff_head list;
+	struct sk_buff *skb;
 	int pkt_to_send, i;
-	void *rx_tmp_ptr;
 	unsigned long flags;
 
+	__skb_queue_head_init(&list);
+	spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
+
 	pkt_to_send = (start_win > tbl->start_win) ?
 		      min((start_win - tbl->start_win), tbl->win_size) :
 		      tbl->win_size;
 
 	for (i = 0; i < pkt_to_send; ++i) {
-		spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
-		rx_tmp_ptr = NULL;
 		if (tbl->rx_reorder_ptr[i]) {
-			rx_tmp_ptr = tbl->rx_reorder_ptr[i];
+			skb = tbl->rx_reorder_ptr[i];
+			__skb_queue_tail(&list, skb);
 			tbl->rx_reorder_ptr[i] = NULL;
 		}
-		spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
-		if (rx_tmp_ptr)
-			mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
 	}
 
-	spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
 	/*
 	 * We don't have a circular buffer, hence use rotation to simulate
 	 * circular buffer
@@ -141,6 +141,9 @@  mwifiex_11n_dispatch_pkt_until_start_win(struct mwifiex_private *priv,
 
 	tbl->start_win = start_win;
 	spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
+
+	while ((skb = __skb_dequeue(&list)))
+		mwifiex_11n_dispatch_pkt(priv, skb);
 }
 
 /*
@@ -155,24 +158,22 @@  static void
 mwifiex_11n_scan_and_dispatch(struct mwifiex_private *priv,
 			      struct mwifiex_rx_reorder_tbl *tbl)
 {
+	struct sk_buff_head list;
+	struct sk_buff *skb;
 	int i, j, xchg;
-	void *rx_tmp_ptr;
 	unsigned long flags;
 
+	__skb_queue_head_init(&list);
+	spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
+
 	for (i = 0; i < tbl->win_size; ++i) {
-		spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
-		if (!tbl->rx_reorder_ptr[i]) {
-			spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
-					       flags);
+		if (!tbl->rx_reorder_ptr[i])
 			break;
-		}
-		rx_tmp_ptr = tbl->rx_reorder_ptr[i];
+		skb = tbl->rx_reorder_ptr[i];
+		__skb_queue_tail(&list, skb);
 		tbl->rx_reorder_ptr[i] = NULL;
-		spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
-		mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
 	}
 
-	spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
 	/*
 	 * We don't have a circular buffer, hence use rotation to simulate
 	 * circular buffer
@@ -185,7 +186,11 @@  mwifiex_11n_scan_and_dispatch(struct mwifiex_private *priv,
 		}
 	}
 	tbl->start_win = (tbl->start_win + i) & (MAX_TID_VALUE - 1);
+
 	spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
+
+	while ((skb = __skb_dequeue(&list)))
+		mwifiex_11n_dispatch_pkt(priv, skb);
 }
 
 /*