* [PATCH v2 1/3] mwifiex: pcie: use posted write to wake up firmware @ 2017-01-13 23:35 Brian Norris 2017-01-13 23:35 ` [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks Brian Norris ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Brian Norris @ 2017-01-13 23:35 UTC (permalink / raw) To: Amitkumar Karwar, Nishant Sarmukadam Cc: linux-kernel, Kalle Valo, linux-wireless, Cathy Luo, Dmitry Torokhov, Brian Norris Depending on system factors (e.g., the PCIe link PM state), the first read to wake up the Wifi firmware can take a long time. There is no reason to use a (blocking, non-posted) read at this point, so let's just use a write instead. Write vs. read doesn't matter functionality-wise -- it's just a dummy operation. But let's make sure to re-write with the correct "ready" signature, since we check for that in other parts of the driver. This has been shown to decrease the time spent blocking in this function on RK3399. Signed-off-by: Brian Norris <briannorris@chromium.org> --- v2: * write FIRMWARE_READY_PCIE instead of 0 --- drivers/net/wireless/marvell/mwifiex/pcie.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index 66226c615be0..3f4cda2d3b61 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -458,7 +458,6 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter, /* This function wakes up the card by reading fw_status register. */ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter) { - u32 fw_status; struct pcie_service_card *card = adapter->card; const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; @@ -468,10 +467,10 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter) if (reg->sleep_cookie) mwifiex_pcie_dev_wakeup_delay(adapter); - /* Reading fw_status register will wakeup device */ - if (mwifiex_read_reg(adapter, reg->fw_status, &fw_status)) { + /* Accessing fw_status register will wakeup device */ + if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) { mwifiex_dbg(adapter, ERROR, - "Reading fw_status register failed\n"); + "Writing fw_status register failed\n"); return -1; } -- 2.11.0.483.g087da7b7c-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks 2017-01-13 23:35 [PATCH v2 1/3] mwifiex: pcie: use posted write to wake up firmware Brian Norris @ 2017-01-13 23:35 ` Brian Norris 2017-01-16 0:54 ` Dmitry Torokhov 2017-01-13 23:35 ` [PATCH v2 3/3] mwifiex: pcie: read FROMDEVICE DMA-able memory with READ_ONCE() Brian Norris 2017-01-20 9:46 ` [v2,1/3] mwifiex: pcie: use posted write to wake up firmware Kalle Valo 2 siblings, 1 reply; 9+ messages in thread From: Brian Norris @ 2017-01-13 23:35 UTC (permalink / raw) To: Amitkumar Karwar, Nishant Sarmukadam Cc: linux-kernel, Kalle Valo, linux-wireless, Cathy Luo, Dmitry Torokhov, Brian Norris The following sequence occurs when using IEEE power-save on 8997: (a) driver sees SLEEP event (b) driver issues SLEEP CONFIRM (c) driver recevies CMD interrupt; within the interrupt processing loop, we do (d) and (e): (d) wait for FW sleep cookie (and often time out; it takes a while), FW is putting card into low power mode (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value But at (e), no one actually signaled an interrupt (i.e., we didn't check adapter->int_status). And what's more, because the card is going to sleep, this register read appears to take a very long time in some cases -- 3 milliseconds in my case! Now, I propose that (e) is completely unnecessary. If there were any additional interrupts signaled after the start of this loop, then the interrupt handler would have set adapter->int_status to non-zero and queued more work for the main loop -- and we'd catch it on the next iteration of the main loop. So this patch drops all the looping/re-reading of PCIE_HOST_INT_STATUS, which avoids the problematic (and slow) register read in step (e). Incidentally, this is a very similar issue to the one fixed in commit ec815dd2a5f1 ("mwifiex: prevent register accesses after host is sleeping"), except that the register read is just very slow instead of fatal in this case. Tested on 8997 in both MSI and (though not technically supported at the moment) MSI-X mode. Signed-off-by: Brian Norris <briannorris@chromium.org> --- v2: * new in v2, replacing an attempt to mess with step (d) above --- drivers/net/wireless/marvell/mwifiex/pcie.c | 102 +++++++++------------------- 1 file changed, 32 insertions(+), 70 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index 3f4cda2d3b61..194e0e04c3b1 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -2332,79 +2332,41 @@ static int mwifiex_process_pcie_int(struct mwifiex_adapter *adapter) } } } - while (pcie_ireg & HOST_INTR_MASK) { - if (pcie_ireg & HOST_INTR_DNLD_DONE) { - pcie_ireg &= ~HOST_INTR_DNLD_DONE; - mwifiex_dbg(adapter, INTR, - "info: TX DNLD Done\n"); - ret = mwifiex_pcie_send_data_complete(adapter); - if (ret) - return ret; - } - if (pcie_ireg & HOST_INTR_UPLD_RDY) { - pcie_ireg &= ~HOST_INTR_UPLD_RDY; - mwifiex_dbg(adapter, INTR, - "info: Rx DATA\n"); - ret = mwifiex_pcie_process_recv_data(adapter); - if (ret) - return ret; - } - if (pcie_ireg & HOST_INTR_EVENT_RDY) { - pcie_ireg &= ~HOST_INTR_EVENT_RDY; - mwifiex_dbg(adapter, INTR, - "info: Rx EVENT\n"); - ret = mwifiex_pcie_process_event_ready(adapter); - if (ret) - return ret; - } - - if (pcie_ireg & HOST_INTR_CMD_DONE) { - pcie_ireg &= ~HOST_INTR_CMD_DONE; - if (adapter->cmd_sent) { - mwifiex_dbg(adapter, INTR, - "info: CMD sent Interrupt\n"); - adapter->cmd_sent = false; - } - /* Handle command response */ - ret = mwifiex_pcie_process_cmd_complete(adapter); - if (ret) - return ret; - if (adapter->hs_activated) - return ret; - } - - if (card->msi_enable) { - spin_lock_irqsave(&adapter->int_lock, flags); - adapter->int_status = 0; - spin_unlock_irqrestore(&adapter->int_lock, flags); - } - - if (mwifiex_pcie_ok_to_access_hw(adapter)) { - if (mwifiex_read_reg(adapter, PCIE_HOST_INT_STATUS, - &pcie_ireg)) { - mwifiex_dbg(adapter, ERROR, - "Read register failed\n"); - return -1; - } - if ((pcie_ireg != 0xFFFFFFFF) && (pcie_ireg)) { - if (mwifiex_write_reg(adapter, - PCIE_HOST_INT_STATUS, - ~pcie_ireg)) { - mwifiex_dbg(adapter, ERROR, - "Write register failed\n"); - return -1; - } - } - - } - if (!card->msi_enable) { - spin_lock_irqsave(&adapter->int_lock, flags); - pcie_ireg |= adapter->int_status; - adapter->int_status = 0; - spin_unlock_irqrestore(&adapter->int_lock, flags); + if (pcie_ireg & HOST_INTR_DNLD_DONE) { + pcie_ireg &= ~HOST_INTR_DNLD_DONE; + mwifiex_dbg(adapter, INTR, "info: TX DNLD Done\n"); + ret = mwifiex_pcie_send_data_complete(adapter); + if (ret) + return ret; + } + if (pcie_ireg & HOST_INTR_UPLD_RDY) { + pcie_ireg &= ~HOST_INTR_UPLD_RDY; + mwifiex_dbg(adapter, INTR, "info: Rx DATA\n"); + ret = mwifiex_pcie_process_recv_data(adapter); + if (ret) + return ret; + } + if (pcie_ireg & HOST_INTR_EVENT_RDY) { + pcie_ireg &= ~HOST_INTR_EVENT_RDY; + mwifiex_dbg(adapter, INTR, "info: Rx EVENT\n"); + ret = mwifiex_pcie_process_event_ready(adapter); + if (ret) + return ret; + } + if (pcie_ireg & HOST_INTR_CMD_DONE) { + pcie_ireg &= ~HOST_INTR_CMD_DONE; + if (adapter->cmd_sent) { + mwifiex_dbg(adapter, INTR, + "info: CMD sent Interrupt\n"); + adapter->cmd_sent = false; } + /* Handle command response */ + ret = mwifiex_pcie_process_cmd_complete(adapter); + if (ret) + return ret; } + mwifiex_dbg(adapter, INTR, "info: cmd_sent=%d data_sent=%d\n", adapter->cmd_sent, adapter->data_sent); -- 2.11.0.483.g087da7b7c-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks 2017-01-13 23:35 ` [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks Brian Norris @ 2017-01-16 0:54 ` Dmitry Torokhov 2017-01-17 19:48 ` Brian Norris 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2017-01-16 0:54 UTC (permalink / raw) To: Brian Norris Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-kernel, Kalle Valo, linux-wireless, Cathy Luo On Fri, Jan 13, 2017 at 03:35:37PM -0800, Brian Norris wrote: > The following sequence occurs when using IEEE power-save on 8997: > (a) driver sees SLEEP event > (b) driver issues SLEEP CONFIRM > (c) driver recevies CMD interrupt; within the interrupt processing loop, > we do (d) and (e): > (d) wait for FW sleep cookie (and often time out; it takes a while), FW > is putting card into low power mode > (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value > > But at (e), no one actually signaled an interrupt (i.e., we didn't check > adapter->int_status). And what's more, because the card is going to > sleep, this register read appears to take a very long time in some cases > -- 3 milliseconds in my case! > > Now, I propose that (e) is completely unnecessary. If there were any > additional interrupts signaled after the start of this loop, then the > interrupt handler would have set adapter->int_status to non-zero and > queued more work for the main loop -- and we'd catch it on the next > iteration of the main loop. > > So this patch drops all the looping/re-reading of PCIE_HOST_INT_STATUS, > which avoids the problematic (and slow) register read in step (e). > > Incidentally, this is a very similar issue to the one fixed in commit > ec815dd2a5f1 ("mwifiex: prevent register accesses after host is > sleeping"), except that the register read is just very slow instead of > fatal in this case. > > Tested on 8997 in both MSI and (though not technically supported at the > moment) MSI-X mode. Well, that kills interrupt mitigation and with PCIE that might be somewhat important (SDIO is too slow to be important I think) and might cost you throughput. OTOH maybe Marvell should convert PICE to NAPI to make this more obvious and probably more correct. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > v2: > * new in v2, replacing an attempt to mess with step (d) above > --- > drivers/net/wireless/marvell/mwifiex/pcie.c | 102 +++++++++------------------- > 1 file changed, 32 insertions(+), 70 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c > index 3f4cda2d3b61..194e0e04c3b1 100644 > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > @@ -2332,79 +2332,41 @@ static int mwifiex_process_pcie_int(struct mwifiex_adapter *adapter) > } > } > } > - while (pcie_ireg & HOST_INTR_MASK) { > - if (pcie_ireg & HOST_INTR_DNLD_DONE) { > - pcie_ireg &= ~HOST_INTR_DNLD_DONE; > - mwifiex_dbg(adapter, INTR, > - "info: TX DNLD Done\n"); > - ret = mwifiex_pcie_send_data_complete(adapter); > - if (ret) > - return ret; > - } > - if (pcie_ireg & HOST_INTR_UPLD_RDY) { > - pcie_ireg &= ~HOST_INTR_UPLD_RDY; > - mwifiex_dbg(adapter, INTR, > - "info: Rx DATA\n"); > - ret = mwifiex_pcie_process_recv_data(adapter); > - if (ret) > - return ret; > - } > - if (pcie_ireg & HOST_INTR_EVENT_RDY) { > - pcie_ireg &= ~HOST_INTR_EVENT_RDY; > - mwifiex_dbg(adapter, INTR, > - "info: Rx EVENT\n"); > - ret = mwifiex_pcie_process_event_ready(adapter); > - if (ret) > - return ret; > - } > - > - if (pcie_ireg & HOST_INTR_CMD_DONE) { > - pcie_ireg &= ~HOST_INTR_CMD_DONE; > - if (adapter->cmd_sent) { > - mwifiex_dbg(adapter, INTR, > - "info: CMD sent Interrupt\n"); > - adapter->cmd_sent = false; > - } > - /* Handle command response */ > - ret = mwifiex_pcie_process_cmd_complete(adapter); > - if (ret) > - return ret; > - if (adapter->hs_activated) > - return ret; > - } > - > - if (card->msi_enable) { > - spin_lock_irqsave(&adapter->int_lock, flags); > - adapter->int_status = 0; > - spin_unlock_irqrestore(&adapter->int_lock, flags); > - } > - > - if (mwifiex_pcie_ok_to_access_hw(adapter)) { > - if (mwifiex_read_reg(adapter, PCIE_HOST_INT_STATUS, > - &pcie_ireg)) { > - mwifiex_dbg(adapter, ERROR, > - "Read register failed\n"); > - return -1; > - } > > - if ((pcie_ireg != 0xFFFFFFFF) && (pcie_ireg)) { > - if (mwifiex_write_reg(adapter, > - PCIE_HOST_INT_STATUS, > - ~pcie_ireg)) { > - mwifiex_dbg(adapter, ERROR, > - "Write register failed\n"); > - return -1; > - } > - } > - > - } > - if (!card->msi_enable) { > - spin_lock_irqsave(&adapter->int_lock, flags); > - pcie_ireg |= adapter->int_status; > - adapter->int_status = 0; > - spin_unlock_irqrestore(&adapter->int_lock, flags); > + if (pcie_ireg & HOST_INTR_DNLD_DONE) { > + pcie_ireg &= ~HOST_INTR_DNLD_DONE; > + mwifiex_dbg(adapter, INTR, "info: TX DNLD Done\n"); > + ret = mwifiex_pcie_send_data_complete(adapter); > + if (ret) > + return ret; > + } > + if (pcie_ireg & HOST_INTR_UPLD_RDY) { > + pcie_ireg &= ~HOST_INTR_UPLD_RDY; > + mwifiex_dbg(adapter, INTR, "info: Rx DATA\n"); > + ret = mwifiex_pcie_process_recv_data(adapter); > + if (ret) > + return ret; > + } > + if (pcie_ireg & HOST_INTR_EVENT_RDY) { > + pcie_ireg &= ~HOST_INTR_EVENT_RDY; > + mwifiex_dbg(adapter, INTR, "info: Rx EVENT\n"); > + ret = mwifiex_pcie_process_event_ready(adapter); > + if (ret) > + return ret; > + } > + if (pcie_ireg & HOST_INTR_CMD_DONE) { > + pcie_ireg &= ~HOST_INTR_CMD_DONE; > + if (adapter->cmd_sent) { > + mwifiex_dbg(adapter, INTR, > + "info: CMD sent Interrupt\n"); > + adapter->cmd_sent = false; > } > + /* Handle command response */ > + ret = mwifiex_pcie_process_cmd_complete(adapter); > + if (ret) > + return ret; > } > + > mwifiex_dbg(adapter, INTR, > "info: cmd_sent=%d data_sent=%d\n", > adapter->cmd_sent, adapter->data_sent); > -- > 2.11.0.483.g087da7b7c-goog > -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks 2017-01-16 0:54 ` Dmitry Torokhov @ 2017-01-17 19:48 ` Brian Norris 2017-01-17 20:44 ` Dmitry Torokhov 2017-01-18 13:13 ` Amitkumar Karwar 0 siblings, 2 replies; 9+ messages in thread From: Brian Norris @ 2017-01-17 19:48 UTC (permalink / raw) To: Dmitry Torokhov Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-kernel, Kalle Valo, linux-wireless, Cathy Luo On Sun, Jan 15, 2017 at 04:54:52PM -0800, Dmitry Torokhov wrote: > On Fri, Jan 13, 2017 at 03:35:37PM -0800, Brian Norris wrote: > > The following sequence occurs when using IEEE power-save on 8997: > > (a) driver sees SLEEP event > > (b) driver issues SLEEP CONFIRM > > (c) driver recevies CMD interrupt; within the interrupt processing loop, > > we do (d) and (e): > > (d) wait for FW sleep cookie (and often time out; it takes a while), FW > > is putting card into low power mode > > (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value > > > > But at (e), no one actually signaled an interrupt (i.e., we didn't check > > adapter->int_status). And what's more, because the card is going to > > sleep, this register read appears to take a very long time in some cases > > -- 3 milliseconds in my case! > > > > Now, I propose that (e) is completely unnecessary. If there were any > > additional interrupts signaled after the start of this loop, then the > > interrupt handler would have set adapter->int_status to non-zero and > > queued more work for the main loop -- and we'd catch it on the next > > iteration of the main loop. > > > > So this patch drops all the looping/re-reading of PCIE_HOST_INT_STATUS, > > which avoids the problematic (and slow) register read in step (e). > > > > Incidentally, this is a very similar issue to the one fixed in commit > > ec815dd2a5f1 ("mwifiex: prevent register accesses after host is > > sleeping"), except that the register read is just very slow instead of > > fatal in this case. > > > > Tested on 8997 in both MSI and (though not technically supported at the > > moment) MSI-X mode. > > Well, that kills interrupt mitigation and with PCIE that might be > somewhat important (SDIO is too slow to be important I think) and might > cost you throughput. Hmm, well I don't see us disabling interrupts in here, at least for MSI mode, so it doesn't actually look like a mitigation mechanism. More like a redundancy. But I'm not an expert on MSI, and definitely not on network performance. Also, FWIW, I did some fairly non-scientific tests of this on my systems, and I didn't see much difference. I can run better tests, and even collect data on how often we loop here vs. see new interrupts. > OTOH maybe Marvell should convert PICE to NAPI to make this more > obvious and probably more correct. >From my brief reading, that sounds like a better way to make this configurable. So I'm not sure which way you'd suggest then; take a patch like this, which makes the driver more clear and less buggy? Or write some different patch that isolates just the power-save related condition, so we break out of this look [1]? I'm also interested in any opinions from the Marvell side -- potentially testing results, rationale behind this code structure, or even a better alternative patch. Brian [1] i.e., along the lines of commit ec815dd2a5f1 ("mwifiex: prevent register accesses after host is sleeping"). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks 2017-01-17 19:48 ` Brian Norris @ 2017-01-17 20:44 ` Dmitry Torokhov 2017-01-18 2:09 ` Brian Norris 2017-01-18 13:13 ` Amitkumar Karwar 1 sibling, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2017-01-17 20:44 UTC (permalink / raw) To: Brian Norris Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-kernel, Kalle Valo, linux-wireless, Cathy Luo On Tue, Jan 17, 2017 at 11:48:22AM -0800, Brian Norris wrote: > On Sun, Jan 15, 2017 at 04:54:52PM -0800, Dmitry Torokhov wrote: > > On Fri, Jan 13, 2017 at 03:35:37PM -0800, Brian Norris wrote: > > > The following sequence occurs when using IEEE power-save on 8997: > > > (a) driver sees SLEEP event > > > (b) driver issues SLEEP CONFIRM > > > (c) driver recevies CMD interrupt; within the interrupt processing loop, > > > we do (d) and (e): > > > (d) wait for FW sleep cookie (and often time out; it takes a while), FW > > > is putting card into low power mode > > > (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value > > > > > > But at (e), no one actually signaled an interrupt (i.e., we didn't check > > > adapter->int_status). And what's more, because the card is going to > > > sleep, this register read appears to take a very long time in some cases > > > -- 3 milliseconds in my case! > > > > > > Now, I propose that (e) is completely unnecessary. If there were any > > > additional interrupts signaled after the start of this loop, then the > > > interrupt handler would have set adapter->int_status to non-zero and > > > queued more work for the main loop -- and we'd catch it on the next > > > iteration of the main loop. > > > > > > So this patch drops all the looping/re-reading of PCIE_HOST_INT_STATUS, > > > which avoids the problematic (and slow) register read in step (e). > > > > > > Incidentally, this is a very similar issue to the one fixed in commit > > > ec815dd2a5f1 ("mwifiex: prevent register accesses after host is > > > sleeping"), except that the register read is just very slow instead of > > > fatal in this case. > > > > > > Tested on 8997 in both MSI and (though not technically supported at the > > > moment) MSI-X mode. > > > > Well, that kills interrupt mitigation and with PCIE that might be > > somewhat important (SDIO is too slow to be important I think) and might > > cost you throughput. > > Hmm, well I don't see us disabling interrupts in here, at least for MSI > mode, so it doesn't actually look like a mitigation mechanism. More like > a redundancy. But I'm not an expert on MSI, and definitely not on > network performance. Well, right, maybe not mitigation, but at least you have a chance to avoid scheduling latency at times. > > Also, FWIW, I did some fairly non-scientific tests of this on my > systems, and I didn't see much difference. I can run better tests, and > even collect data on how often we loop here vs. see new interrupts. That would be great. Maybe packet aggregation takes care of interrupts arriving "too closely" together most of the time, I dunno. > > > OTOH maybe Marvell should convert PICE to NAPI to make this more > > obvious and probably more correct. > > From my brief reading, that sounds like a better way to make this > configurable. > > So I'm not sure which way you'd suggest then; take a patch like this, > which makes the driver more clear and less buggy? Or write some > different patch that isolates just the power-save related condition, so > we break out of this look [1]? I think it really depends on the test results. If we do not see degradation in both throughput and latency then I think we can take this patch and then see if switching to NAPI would be the ultimate solution. > > I'm also interested in any opinions from the Marvell side -- potentially > testing results, rationale behind this code structure, or even a better > alternative patch. > > Brian > > [1] i.e., along the lines of commit ec815dd2a5f1 ("mwifiex: prevent > register accesses after host is sleeping"). Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks 2017-01-17 20:44 ` Dmitry Torokhov @ 2017-01-18 2:09 ` Brian Norris 0 siblings, 0 replies; 9+ messages in thread From: Brian Norris @ 2017-01-18 2:09 UTC (permalink / raw) To: Dmitry Torokhov Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-kernel, Kalle Valo, linux-wireless, Cathy Luo [-- Attachment #1: Type: text/plain, Size: 2653 bytes --] On Tue, Jan 17, 2017 at 12:44:55PM -0800, Dmitry Torokhov wrote: > On Tue, Jan 17, 2017 at 11:48:22AM -0800, Brian Norris wrote: > > Also, FWIW, I did some fairly non-scientific tests of this on my > > systems, and I didn't see much difference. I can run better tests, and > > even collect data on how often we loop here vs. see new interrupts. > > That would be great. Maybe packet aggregation takes care of interrupts > arriving "too closely" together most of the time, I dunno. OK, so I did some basic accounting of how many times this while loop runs in a row. I don't know if they're highly illuminating, but here goes. They're listed as a histogram, where the first column is number of samples that exhibited the behavior and second column is number of times going through the loop before exiting (i.e., seeing no more INT_STATUS): Idle (just scanning for networks occasionally, and loading a web page or two) for a minute or two: 1 265 1 2 2 Downloading a Chrome .deb package via wget, in a loop: 857 0 36406 1 32386 2 2230 3 153 4 11 5 Running a perf client test (i.e., TX traffic) in a loop: 1694 0 247897 1 25530 2 441 3 18 4 So it seems like in some cases, it's at least *possible* to have a little bit of potential savings on 10-50% of interrupts when under load. (i.e., see that ~50% of interrupt checks take 2, 3, 4, or 5 loops in the second example.) Now, I also did some perf numbers with iperf between a Raspberry Pi iperf server and an ARM64 system running mwifiex. On the whole, the TX side was probably bottlenecked by the RPi, but the RX side was pretty good. I'll attach full numbers, but the percentage delta is as follows: Mean Median ------ ------ % change, bi-direction (RX): -0.3 -4.5 % change, bi-direction (TX): 1.034 4.45 % change, TX only: 12.96 13.35 % change, RX only: -6.5 -3 I'm not sure I have a good explanation for the gain in TX performance. Perhaps partly the reduction in complexity (e.g., unnecessary register reads). Perhaps also because I had IEEE power-save enabled, so without this patch, performance could (theoretically) be harmed by the issue mentioned in the commit description (i.e., occasional slow PCIe reads) -- though I guess we probably don't enter power-save often during iperf tests. So, there could definitely be some methodology mistakes or other variables involved, but these don't seem to show any particularly bad performance loss, and if we did, we might consider other approaches like NAPI for tuning them. Brian [-- Attachment #2: summary.csv --] [-- Type: text/csv, Size: 1140 bytes --] ,Mbit/s,,,,,,,,,,Min,Max,Mean,Median,Stddev Before: Bidirectional RX,193,163,198,147,194,195,129,167,198,174,129,198,175.8,183.5,24.142171494 Before: Bidirectional TX,15.2,21.4,15.8,26.2,13.8,12.6,22.9,14.3,14.7,35.1,12.6,35.1,19.2,15.5,7.1870253466 Before: TX only,60.6,52.4,70,66,60.9,64.9,58.3,59.8,53.3,58.3,52.4,70,60.45,60.2,5.4530827163 Before: RX only,151,186,209,201,180,208,210,189,176,196,151,210,190.6,192.5,18.476411388 After: Bidirectional RX,171,194,187,165,167,197,163,199,192,120,120,199,175.5,179,24.0381641007 After: Bidirectional TX,30.4,19,19.8,20.2,20.7,20.1,27.3,18.8,19.3,6.74,6.74,30.4,20.234,19.95,6.1485322548 After: TX only,73.9,78.1,73.3,73.2,74.5,82.1,73.8,69.6,68.7,66.9,66.9,82.1,73.41,73.55,4.4500811478 After: RX only,160,163,179,195,203,187,192,202,207,153,153,207,184.1,189.5,19.4676369621 ,,,,,,,,,,,,,,, Delta: Bidirectional RX,,,,,,,,,,,,,-0.3,-4.5, Delta: Bidirectional TX,,,,,,,,,,,,,1.034,4.45, Delta: TX only,,,,,,,,,,,,,12.96,13.35, Delta: RX only,,,,,,,,,,,,,-6.5,-3, ,,,,,,,,,,,,,,, ,,,,,,,,,,,,,-0.17%,-2.45%, ,,,,,,,,,,,,,5.39%,28.71%, ,,,,,,,,,,,,,21.44%,22.18%, ,,,,,,,,,,,,,-3.41%,-1.56%, ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks 2017-01-17 19:48 ` Brian Norris 2017-01-17 20:44 ` Dmitry Torokhov @ 2017-01-18 13:13 ` Amitkumar Karwar 1 sibling, 0 replies; 9+ messages in thread From: Amitkumar Karwar @ 2017-01-18 13:13 UTC (permalink / raw) To: Brian Norris, Dmitry Torokhov Cc: Nishant Sarmukadam, linux-kernel, Kalle Valo, linux-wireless, Cathy Luo Hi Brian, > From: Brian Norris [mailto:briannorris@chromium.org] > Sent: Wednesday, January 18, 2017 1:18 AM > To: Dmitry Torokhov > Cc: Amitkumar Karwar; Nishant Sarmukadam; linux-kernel@vger.kernel.org; > Kalle Valo; linux-wireless@vger.kernel.org; Cathy Luo > Subject: Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry > interrupt status checks > > On Sun, Jan 15, 2017 at 04:54:52PM -0800, Dmitry Torokhov wrote: > > On Fri, Jan 13, 2017 at 03:35:37PM -0800, Brian Norris wrote: > > > The following sequence occurs when using IEEE power-save on 8997: > > > (a) driver sees SLEEP event > > > (b) driver issues SLEEP CONFIRM > > > (c) driver recevies CMD interrupt; within the interrupt processing > loop, > > > we do (d) and (e): > > > (d) wait for FW sleep cookie (and often time out; it takes a > while), FW > > > is putting card into low power mode > > > (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value > > > > > > But at (e), no one actually signaled an interrupt (i.e., we didn't > > > check > > > adapter->int_status). And what's more, because the card is going to > > > sleep, this register read appears to take a very long time in some > > > cases > > > -- 3 milliseconds in my case! > > > > > > Now, I propose that (e) is completely unnecessary. If there were > any > > > additional interrupts signaled after the start of this loop, then > > > the interrupt handler would have set adapter->int_status to non- > zero > > > and queued more work for the main loop -- and we'd catch it on the > > > next iteration of the main loop. > > > > > > So this patch drops all the looping/re-reading of > > > PCIE_HOST_INT_STATUS, which avoids the problematic (and slow) > register read in step (e). > > > > > > Incidentally, this is a very similar issue to the one fixed in > > > commit > > > ec815dd2a5f1 ("mwifiex: prevent register accesses after host is > > > sleeping"), except that the register read is just very slow instead > > > of fatal in this case. > > > > > > Tested on 8997 in both MSI and (though not technically supported at > > > the > > > moment) MSI-X mode. > > > > Well, that kills interrupt mitigation and with PCIE that might be > > somewhat important (SDIO is too slow to be important I think) and > > might cost you throughput. > > Hmm, well I don't see us disabling interrupts in here, at least for MSI > mode, so it doesn't actually look like a mitigation mechanism. More > like a redundancy. But I'm not an expert on MSI, and definitely not on > network performance. > > Also, FWIW, I did some fairly non-scientific tests of this on my > systems, and I didn't see much difference. I can run better tests, and > even collect data on how often we loop here vs. see new interrupts. > > > OTOH maybe Marvell should convert PICE to NAPI to make this more > > obvious and probably more correct. > > From my brief reading, that sounds like a better way to make this > configurable. > > So I'm not sure which way you'd suggest then; take a patch like this, > which makes the driver more clear and less buggy? Or write some > different patch that isolates just the power-save related condition, so > we break out of this look [1]? > > I'm also interested in any opinions from the Marvell side -- > potentially testing results, rationale behind this code structure, or > even a better alternative patch. > Thanks for the fix. It looks fine to me. Alternate fix would be below change. We ran throughput tests with your patch vs below change. Results are almost same. -------- @@ -2370,7 +2370,7 @@ static int mwifiex_process_pcie_int(struct mwifiex_adapter *adapter) ret = mwifiex_pcie_process_cmd_complete(adapter); if (ret) return ret; - if (adapter->hs_activated) + if (adapter->hs_activated || adapter->ps_state == PS_STATE_SLEEP) return ret; -------- The logic of having while loop here was to avoid scheduling latency. As Dmitry pointed out in other email packet aggregation takes care of interrupts arriving too closely together. We have Rx buffer ring of size 32. So we may get a single interrupt to deliver 32 Rx packets. Regards, Amitkumar ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] mwifiex: pcie: read FROMDEVICE DMA-able memory with READ_ONCE() 2017-01-13 23:35 [PATCH v2 1/3] mwifiex: pcie: use posted write to wake up firmware Brian Norris 2017-01-13 23:35 ` [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks Brian Norris @ 2017-01-13 23:35 ` Brian Norris 2017-01-20 9:46 ` [v2,1/3] mwifiex: pcie: use posted write to wake up firmware Kalle Valo 2 siblings, 0 replies; 9+ messages in thread From: Brian Norris @ 2017-01-13 23:35 UTC (permalink / raw) To: Amitkumar Karwar, Nishant Sarmukadam Cc: linux-kernel, Kalle Valo, linux-wireless, Cathy Luo, Dmitry Torokhov, Brian Norris In mwifiex_delay_for_sleep_cookie(), we're looping and waiting for the PCIe endpoint to write a magic value back to memory, to signal that it has finished going to sleep. We're not letting the compiler know that this might change underneath our feet though. Let's do that, for good hygiene. I'm not aware of this fixing any concrete problems. I also give no guarantee that this loop is actually correct in any other way, but at least this looks like an improvement to me. Signed-off-by: Brian Norris <briannorris@chromium.org> --- v2: new in v2 --- drivers/net/wireless/marvell/mwifiex/pcie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index 194e0e04c3b1..c2511f212502 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -440,7 +440,7 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter, for (count = 0; count < max_delay_loop_cnt; count++) { buffer = card->cmdrsp_buf->data - INTF_HEADER_LEN; - sleep_cookie = *(u32 *)buffer; + sleep_cookie = READ_ONCE(*(u32 *)buffer); if (sleep_cookie == MWIFIEX_DEF_SLEEP_COOKIE) { mwifiex_dbg(adapter, INFO, -- 2.11.0.483.g087da7b7c-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [v2,1/3] mwifiex: pcie: use posted write to wake up firmware 2017-01-13 23:35 [PATCH v2 1/3] mwifiex: pcie: use posted write to wake up firmware Brian Norris 2017-01-13 23:35 ` [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks Brian Norris 2017-01-13 23:35 ` [PATCH v2 3/3] mwifiex: pcie: read FROMDEVICE DMA-able memory with READ_ONCE() Brian Norris @ 2017-01-20 9:46 ` Kalle Valo 2 siblings, 0 replies; 9+ messages in thread From: Kalle Valo @ 2017-01-20 9:46 UTC (permalink / raw) To: Brian Norris Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-kernel, linux-wireless, Cathy Luo, Dmitry Torokhov, Brian Norris Brian Norris <briannorris@chromium.org> wrote: > Depending on system factors (e.g., the PCIe link PM state), the first > read to wake up the Wifi firmware can take a long time. There is no > reason to use a (blocking, non-posted) read at this point, so let's just > use a write instead. Write vs. read doesn't matter functionality-wise -- > it's just a dummy operation. But let's make sure to re-write with the > correct "ready" signature, since we check for that in other parts of the > driver. > > This has been shown to decrease the time spent blocking in this function > on RK3399. > > Signed-off-by: Brian Norris <briannorris@chromium.org> 3 patches applied to wireless-drivers-next.git, thanks. 062e008a6e83 mwifiex: pcie: use posted write to wake up firmware 5d5ddb5e0d9b mwifiex: pcie: don't loop/retry interrupt status checks fe1167883939 mwifiex: pcie: read FROMDEVICE DMA-able memory with READ_ONCE() -- https://patchwork.kernel.org/patch/9516615/ Documentation about submitting wireless patches and checking status from patchwork: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-01-20 9:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-13 23:35 [PATCH v2 1/3] mwifiex: pcie: use posted write to wake up firmware Brian Norris 2017-01-13 23:35 ` [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks Brian Norris 2017-01-16 0:54 ` Dmitry Torokhov 2017-01-17 19:48 ` Brian Norris 2017-01-17 20:44 ` Dmitry Torokhov 2017-01-18 2:09 ` Brian Norris 2017-01-18 13:13 ` Amitkumar Karwar 2017-01-13 23:35 ` [PATCH v2 3/3] mwifiex: pcie: read FROMDEVICE DMA-able memory with READ_ONCE() Brian Norris 2017-01-20 9:46 ` [v2,1/3] mwifiex: pcie: use posted write to wake up firmware Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).