* [ath9k-devel] [RFT] ath10k: serialize CE completions properly [not found] <87haie3mlw.fsf_-_@kamboji.qca.qualcomm.com> @ 2013-05-08 10:00 ` Michal Kazior 2013-05-09 4:07 ` Sujith Manoharan 2013-05-10 12:43 ` [ath9k-devel] [PATCH] " Michal Kazior 0 siblings, 2 replies; 4+ messages in thread From: Michal Kazior @ 2013-05-08 10:00 UTC (permalink / raw) To: ath9k-devel This fixes memory leak when doing heavy TX. ath10k_pci_check_process_ce() was checking `compl_processing` and would call ath10k_pci_process_ce() if its false. The ath10k_pci_process_ce() would then set it to true if there's at least one completion. The lock protecting the variable is not held between checking and setting the variable, meaning it is a possible race condition. If we then assume ath10k_pci_process_ce() is called simultaneusly from different contexts we have a race. And apparently this was possible: ath10k_pci_check_process_ce() was called from ath10k_ce_per_engine_service(). This in turn could be called either from multiple tasklets (MSI-X) or a tasklet (MSI, Legacy INTR) or ath10k_htc_send_complete_check(). The race itself would cause corruption of htt.refcount (ath10k_skb_cb) and we'd leak multiple skbuffs per tx. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- I'm unable to reproduce the memleak. Can someone verify this patch, please? drivers/net/wireless/ath/ath10k/pci.c | 39 +++++++++++++-------------------- 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index e348fd9..4b1d8f7 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -506,24 +506,6 @@ exit: return compl; } -static void ath10k_pci_check_process_ce(struct ath10k *ar) -{ - struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); - - /* - * Check if another tasklet is already processing - * the completion list. This could happen in multiple-MSI. - */ - spin_lock_bh(&ar_pci->compl_lock); - if (ar_pci->compl_processing) { - spin_unlock_bh(&ar_pci->compl_lock); - return; - } - spin_unlock_bh(&ar_pci->compl_lock); - - ath10k_pci_process_ce(ar); -} - /* Called by lower (CE) layer when a send to Target completes. */ static void ath10k_pci_ce_send_done(struct ce_state *ce_state, void *transfer_context, @@ -583,7 +565,7 @@ static void ath10k_pci_ce_send_done(struct ce_state *ce_state, if (!process) return; - ath10k_pci_check_process_ce(ar); + ath10k_pci_process_ce(ar); } /* Called by lower (CE) layer when data is received from the Target. */ @@ -629,7 +611,7 @@ static void ath10k_pci_ce_recv_data(struct ce_state *ce_state, &transfer_id, &flags) == 0); - ath10k_pci_check_process_ce(ar); + ath10k_pci_process_ce(ar); } /* Send the first nbytes bytes of the buffer */ @@ -888,7 +870,18 @@ static void ath10k_pci_process_ce(struct ath10k *ar) unsigned int nbytes; int ret, send_done = 0; - do { + /* Upper layers aren't ready to handle tx/rx completions in parallel so + * we must serialize all completion processing. */ + + spin_lock_bh(&ar_pci->compl_lock); + if (ar_pci->compl_processing) { + spin_unlock_bh(&ar_pci->compl_lock); + return; + } + ar_pci->compl_processing = true; + spin_unlock_bh(&ar_pci->compl_lock); + + for (;;) { spin_lock_bh(&ar_pci->compl_lock); if (list_empty(&ar_pci->compl_process)) { spin_unlock_bh(&ar_pci->compl_lock); @@ -897,7 +890,6 @@ static void ath10k_pci_process_ce(struct ath10k *ar) compl = list_first_entry(&ar_pci->compl_process, struct ath10k_pci_compl, list); list_del(&compl->list); - ar_pci->compl_processing = true; spin_unlock_bh(&ar_pci->compl_lock); if (compl->send_or_recv == HIF_CE_COMPLETE_SEND) { @@ -943,8 +935,7 @@ static void ath10k_pci_process_ce(struct ath10k *ar) list_add_tail(&compl->list, &compl->pipe_info->compl_free); compl->pipe_info->num_sends_allowed += send_done; spin_unlock_bh(&compl->pipe_info->pipe_lock); - - } while (1); + } spin_lock_bh(&ar_pci->compl_lock); ar_pci->compl_processing = false; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [ath9k-devel] [RFT] ath10k: serialize CE completions properly 2013-05-08 10:00 ` [ath9k-devel] [RFT] ath10k: serialize CE completions properly Michal Kazior @ 2013-05-09 4:07 ` Sujith Manoharan 2013-05-10 12:43 ` [ath9k-devel] [PATCH] " Michal Kazior 1 sibling, 0 replies; 4+ messages in thread From: Sujith Manoharan @ 2013-05-09 4:07 UTC (permalink / raw) To: ath9k-devel Michal Kazior wrote: > This fixes memory leak when doing heavy TX. > > ath10k_pci_check_process_ce() was checking > `compl_processing` and would call > ath10k_pci_process_ce() if its false. The > ath10k_pci_process_ce() would then set it to true > if there's at least one completion. The lock > protecting the variable is not held between > checking and setting the variable, meaning it is a > possible race condition. > > If we then assume ath10k_pci_process_ce() is > called simultaneusly from different contexts we > have a race. And apparently this was possible: > > ath10k_pci_check_process_ce() was called from > ath10k_ce_per_engine_service(). This in turn could > be called either from multiple tasklets (MSI-X) or > a tasklet (MSI, Legacy INTR) or > ath10k_htc_send_complete_check(). > > The race itself would cause corruption of > htt.refcount (ath10k_skb_cb) and we'd leak > multiple skbuffs per tx. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> Looks good to me. Sujith ^ permalink raw reply [flat|nested] 4+ messages in thread
* [ath9k-devel] [PATCH] ath10k: serialize CE completions properly 2013-05-08 10:00 ` [ath9k-devel] [RFT] ath10k: serialize CE completions properly Michal Kazior 2013-05-09 4:07 ` Sujith Manoharan @ 2013-05-10 12:43 ` Michal Kazior 2013-05-14 4:29 ` Kalle Valo 1 sibling, 1 reply; 4+ messages in thread From: Michal Kazior @ 2013-05-10 12:43 UTC (permalink / raw) To: ath9k-devel This fixes memory leak when doing heavy TX. ath10k_pci_check_process_ce() was checking `compl_processing` and would call ath10k_pci_process_ce() if its false. The ath10k_pci_process_ce() would then set it to true if there's at least one completion. The lock protecting the variable is not held between checking and setting the variable, meaning it is a possible race condition. If we then assume ath10k_pci_process_ce() is called simultaneusly from different contexts we have a race. And apparently this was possible: ath10k_pci_check_process_ce() was called from ath10k_ce_per_engine_service(). This in turn could be called either from multiple tasklets (MSI-X) or a tasklet (MSI, Legacy INTR) or ath10k_htc_send_complete_check(). The race itself would cause corruption of htt.refcount (ath10k_skb_cb) and we'd leak multiple skbuffs per tx. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- drivers/net/wireless/ath/ath10k/pci.c | 39 +++++++++++++-------------------- 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index e348fd9..4b1d8f7 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -506,24 +506,6 @@ exit: return compl; } -static void ath10k_pci_check_process_ce(struct ath10k *ar) -{ - struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); - - /* - * Check if another tasklet is already processing - * the completion list. This could happen in multiple-MSI. - */ - spin_lock_bh(&ar_pci->compl_lock); - if (ar_pci->compl_processing) { - spin_unlock_bh(&ar_pci->compl_lock); - return; - } - spin_unlock_bh(&ar_pci->compl_lock); - - ath10k_pci_process_ce(ar); -} - /* Called by lower (CE) layer when a send to Target completes. */ static void ath10k_pci_ce_send_done(struct ce_state *ce_state, void *transfer_context, @@ -583,7 +565,7 @@ static void ath10k_pci_ce_send_done(struct ce_state *ce_state, if (!process) return; - ath10k_pci_check_process_ce(ar); + ath10k_pci_process_ce(ar); } /* Called by lower (CE) layer when data is received from the Target. */ @@ -629,7 +611,7 @@ static void ath10k_pci_ce_recv_data(struct ce_state *ce_state, &transfer_id, &flags) == 0); - ath10k_pci_check_process_ce(ar); + ath10k_pci_process_ce(ar); } /* Send the first nbytes bytes of the buffer */ @@ -888,7 +870,18 @@ static void ath10k_pci_process_ce(struct ath10k *ar) unsigned int nbytes; int ret, send_done = 0; - do { + /* Upper layers aren't ready to handle tx/rx completions in parallel so + * we must serialize all completion processing. */ + + spin_lock_bh(&ar_pci->compl_lock); + if (ar_pci->compl_processing) { + spin_unlock_bh(&ar_pci->compl_lock); + return; + } + ar_pci->compl_processing = true; + spin_unlock_bh(&ar_pci->compl_lock); + + for (;;) { spin_lock_bh(&ar_pci->compl_lock); if (list_empty(&ar_pci->compl_process)) { spin_unlock_bh(&ar_pci->compl_lock); @@ -897,7 +890,6 @@ static void ath10k_pci_process_ce(struct ath10k *ar) compl = list_first_entry(&ar_pci->compl_process, struct ath10k_pci_compl, list); list_del(&compl->list); - ar_pci->compl_processing = true; spin_unlock_bh(&ar_pci->compl_lock); if (compl->send_or_recv == HIF_CE_COMPLETE_SEND) { @@ -943,8 +935,7 @@ static void ath10k_pci_process_ce(struct ath10k *ar) list_add_tail(&compl->list, &compl->pipe_info->compl_free); compl->pipe_info->num_sends_allowed += send_done; spin_unlock_bh(&compl->pipe_info->pipe_lock); - - } while (1); + } spin_lock_bh(&ar_pci->compl_lock); ar_pci->compl_processing = false; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [ath9k-devel] [PATCH] ath10k: serialize CE completions properly 2013-05-10 12:43 ` [ath9k-devel] [PATCH] " Michal Kazior @ 2013-05-14 4:29 ` Kalle Valo 0 siblings, 0 replies; 4+ messages in thread From: Kalle Valo @ 2013-05-14 4:29 UTC (permalink / raw) To: ath9k-devel Michal Kazior <michal.kazior@tieto.com> writes: > This fixes memory leak when doing heavy TX. > > ath10k_pci_check_process_ce() was checking > `compl_processing` and would call > ath10k_pci_process_ce() if its false. The > ath10k_pci_process_ce() would then set it to true > if there's at least one completion. The lock > protecting the variable is not held between > checking and setting the variable, meaning it is a > possible race condition. > > If we then assume ath10k_pci_process_ce() is > called simultaneusly from different contexts we > have a race. And apparently this was possible: > > ath10k_pci_check_process_ce() was called from > ath10k_ce_per_engine_service(). This in turn could > be called either from multiple tasklets (MSI-X) or > a tasklet (MSI, Legacy INTR) or > ath10k_htc_send_complete_check(). > > The race itself would cause corruption of > htt.refcount (ath10k_skb_cb) and we'd leak > multiple skbuffs per tx. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> Thanks, applied. -- Kalle Valo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-14 4:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <87haie3mlw.fsf_-_@kamboji.qca.qualcomm.com>
2013-05-08 10:00 ` [ath9k-devel] [RFT] ath10k: serialize CE completions properly Michal Kazior
2013-05-09 4:07 ` Sujith Manoharan
2013-05-10 12:43 ` [ath9k-devel] [PATCH] " Michal Kazior
2013-05-14 4:29 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox