Broadcom WiFi SDIO performance regression after commit "mmc: sdhci: Remove finish_tasklet"

Broadcom WiFi SDIO performance regression after commit "mmc: sdhci: Remove finish_tasklet"

[Dmitry Osipenko]

Hello!

I was debugging WiFi performance problems on Acer A500 tablet device
that has BCM4329 WiFi chip which is connected to NVIDIA Terga20 SoC via
SDIO and found that the following commit causes a solid 5-10 Mbit/s of
WiFi throughput regression after 5.2 kernel:

commit c07a48c2651965e84d35cf193dfc0e5f7892d612
Author: Adrian Hunter <adrian.hunter@intel.com>
Date:   Fri Apr 5 15:40:20 2019 +0300

    mmc: sdhci: Remove finish_tasklet

    Remove finish_tasklet. Requests that require DMA-unmapping or
sdhci_reset
    are completed either in the IRQ thread or a workqueue if the
completion is
    not initiated by the IRQ.

Reverting the offending commit on top of recent linux-next resolves the
problem.

Ulf / Adrian, do you have any ideas what could be done in regards to
restoring the SDIO performance? Should we just revert the offending commit?

Re: Broadcom WiFi SDIO performance regression after commit "mmc: sdhci: Remove finish_tasklet"

[Adrian Hunter]

On 27/08/20 9:07 am, Dmitry Osipenko wrote:
> Hello!
> 
> I was debugging WiFi performance problems on Acer A500 tablet device
> that has BCM4329 WiFi chip which is connected to NVIDIA Terga20 SoC via
> SDIO and found that the following commit causes a solid 5-10 Mbit/s of
> WiFi throughput regression after 5.2 kernel:

What is that in percentage terms?

> 
> commit c07a48c2651965e84d35cf193dfc0e5f7892d612
> Author: Adrian Hunter <adrian.hunter@intel.com>
> Date:   Fri Apr 5 15:40:20 2019 +0300
> 
>     mmc: sdhci: Remove finish_tasklet
> 
>     Remove finish_tasklet. Requests that require DMA-unmapping or
> sdhci_reset
>     are completed either in the IRQ thread or a workqueue if the
> completion is
>     not initiated by the IRQ.
> 
> Reverting the offending commit on top of recent linux-next resolves the
> problem.
> 
> Ulf / Adrian, do you have any ideas what could be done in regards to
> restoring the SDIO performance? Should we just revert the offending commit?
> 

Unfortunately I think we are past the point of returning to the tasklet.

sdhci can complete requests in the irq handler but only if ->pre_req() and
->post_req() are used, which is not supported by SDIO at present.  pre_req
and post_req were introduced to reduce latency for the block driver, so it
seems reasonable perhaps to look at using them in SDIO as well.

Re: Broadcom WiFi SDIO performance regression after commit "mmc: sdhci: Remove finish_tasklet"

[Dmitry Osipenko]

27.08.2020 09:45, Adrian Hunter пишет:
> On 27/08/20 9:07 am, Dmitry Osipenko wrote:
>> Hello!
>>
>> I was debugging WiFi performance problems on Acer A500 tablet device
>> that has BCM4329 WiFi chip which is connected to NVIDIA Terga20 SoC via
>> SDIO and found that the following commit causes a solid 5-10 Mbit/s of
>> WiFi throughput regression after 5.2 kernel:
> 
> What is that in percentage terms?

That is about 20%.

>> commit c07a48c2651965e84d35cf193dfc0e5f7892d612
>> Author: Adrian Hunter <adrian.hunter@intel.com>
>> Date:   Fri Apr 5 15:40:20 2019 +0300
>>
>>     mmc: sdhci: Remove finish_tasklet
>>
>>     Remove finish_tasklet. Requests that require DMA-unmapping or
>> sdhci_reset
>>     are completed either in the IRQ thread or a workqueue if the
>> completion is
>>     not initiated by the IRQ.
>>
>> Reverting the offending commit on top of recent linux-next resolves the
>> problem.
>>
>> Ulf / Adrian, do you have any ideas what could be done in regards to
>> restoring the SDIO performance? Should we just revert the offending commit?
>>
> 
> Unfortunately I think we are past the point of returning to the tasklet.
> 
> sdhci can complete requests in the irq handler but only if ->pre_req() and
> ->post_req() are used, which is not supported by SDIO at present.  pre_req
> and post_req were introduced to reduce latency for the block driver, so it
> seems reasonable perhaps to look at using them in SDIO as well.
> 

I'll try to take a look at pre/post_req(), but I'm not very familiar
with the MMC code, so it may take quite some time. Will be great if you
could help with making a patch that I could test!

Re: Broadcom WiFi SDIO performance regression after commit "mmc: sdhci: Remove finish_tasklet"

[Adrian Hunter]

On 27/08/20 12:36 pm, Dmitry Osipenko wrote:
> 27.08.2020 09:45, Adrian Hunter пишет:
>> On 27/08/20 9:07 am, Dmitry Osipenko wrote:
>>> Hello!
>>>
>>> I was debugging WiFi performance problems on Acer A500 tablet device
>>> that has BCM4329 WiFi chip which is connected to NVIDIA Terga20 SoC via
>>> SDIO and found that the following commit causes a solid 5-10 Mbit/s of
>>> WiFi throughput regression after 5.2 kernel:
>>
>> What is that in percentage terms?
> 
> That is about 20%.
> 
>>> commit c07a48c2651965e84d35cf193dfc0e5f7892d612
>>> Author: Adrian Hunter <adrian.hunter@intel.com>
>>> Date:   Fri Apr 5 15:40:20 2019 +0300
>>>
>>>     mmc: sdhci: Remove finish_tasklet
>>>
>>>     Remove finish_tasklet. Requests that require DMA-unmapping or
>>> sdhci_reset
>>>     are completed either in the IRQ thread or a workqueue if the
>>> completion is
>>>     not initiated by the IRQ.
>>>
>>> Reverting the offending commit on top of recent linux-next resolves the
>>> problem.
>>>
>>> Ulf / Adrian, do you have any ideas what could be done in regards to
>>> restoring the SDIO performance? Should we just revert the offending commit?
>>>
>>
>> Unfortunately I think we are past the point of returning to the tasklet.
>>
>> sdhci can complete requests in the irq handler but only if ->pre_req() and
>> ->post_req() are used, which is not supported by SDIO at present.  pre_req
>> and post_req were introduced to reduce latency for the block driver, so it
>> seems reasonable perhaps to look at using them in SDIO as well.
>>
> 
> I'll try to take a look at pre/post_req(), but I'm not very familiar
> with the MMC code, so it may take quite some time. Will be great if you
> could help with making a patch that I could test!
> 

You could start by seeing if using pre/post_req helps at all, as below.
If that doesn't help, then it might need more analysis.


diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 93d346c01110..4c229dd2b6e5 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -121,6 +121,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	struct sg_table sgtable;
 	unsigned int nents, left_size, i;
 	unsigned int seg_size = card->host->max_seg_size;
+	int err;
 
 	WARN_ON(blksz == 0);
 
@@ -170,28 +171,32 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 
 	mmc_set_data_timeout(&data, card);
 
-	mmc_wait_for_req(card->host, &mrq);
+	mmc_pre_req(card->host, &mrq);
 
-	if (nents > 1)
-		sg_free_table(&sgtable);
+	mmc_wait_for_req(card->host, &mrq);
 
 	if (cmd.error)
-		return cmd.error;
-	if (data.error)
-		return data.error;
-
-	if (mmc_host_is_spi(card->host)) {
+		err = cmd.error;
+	else if (data.error)
+		err = data.error;
+	else if (mmc_host_is_spi(card->host))
 		/* host driver already reported errors */
-	} else {
-		if (cmd.resp[0] & R5_ERROR)
-			return -EIO;
-		if (cmd.resp[0] & R5_FUNCTION_NUMBER)
-			return -EINVAL;
-		if (cmd.resp[0] & R5_OUT_OF_RANGE)
-			return -ERANGE;
-	}
+		err = 0;
+	else if (cmd.resp[0] & R5_ERROR)
+		err = -EIO;
+	else if (cmd.resp[0] & R5_FUNCTION_NUMBER)
+		err = -EINVAL;
+	else if (cmd.resp[0] & R5_OUT_OF_RANGE)
+		err = -ERANGE;
+	else
+		err = 0;
 
-	return 0;
+	mmc_post_req(card->host, &mrq, err);
+
+	if (nents > 1)
+		sg_free_table(&sgtable);
+
+	return err;
 }
 
 int sdio_reset(struct mmc_host *host)

Re: Broadcom WiFi SDIO performance regression after commit "mmc: sdhci: Remove finish_tasklet"

[Dmitry Osipenko]

31.08.2020 18:08, Adrian Hunter пишет:
> On 27/08/20 12:36 pm, Dmitry Osipenko wrote:
>> 27.08.2020 09:45, Adrian Hunter пишет:
>>> On 27/08/20 9:07 am, Dmitry Osipenko wrote:
>>>> Hello!
>>>>
>>>> I was debugging WiFi performance problems on Acer A500 tablet device
>>>> that has BCM4329 WiFi chip which is connected to NVIDIA Terga20 SoC via
>>>> SDIO and found that the following commit causes a solid 5-10 Mbit/s of
>>>> WiFi throughput regression after 5.2 kernel:
>>>
>>> What is that in percentage terms?
>>
>> That is about 20%.
>>
>>>> commit c07a48c2651965e84d35cf193dfc0e5f7892d612
>>>> Author: Adrian Hunter <adrian.hunter@intel.com>
>>>> Date:   Fri Apr 5 15:40:20 2019 +0300
>>>>
>>>>     mmc: sdhci: Remove finish_tasklet
>>>>
>>>>     Remove finish_tasklet. Requests that require DMA-unmapping or
>>>> sdhci_reset
>>>>     are completed either in the IRQ thread or a workqueue if the
>>>> completion is
>>>>     not initiated by the IRQ.
>>>>
>>>> Reverting the offending commit on top of recent linux-next resolves the
>>>> problem.
>>>>
>>>> Ulf / Adrian, do you have any ideas what could be done in regards to
>>>> restoring the SDIO performance? Should we just revert the offending commit?
>>>>
>>>
>>> Unfortunately I think we are past the point of returning to the tasklet.
>>>
>>> sdhci can complete requests in the irq handler but only if ->pre_req() and
>>> ->post_req() are used, which is not supported by SDIO at present.  pre_req
>>> and post_req were introduced to reduce latency for the block driver, so it
>>> seems reasonable perhaps to look at using them in SDIO as well.
>>>
>>
>> I'll try to take a look at pre/post_req(), but I'm not very familiar
>> with the MMC code, so it may take quite some time. Will be great if you
>> could help with making a patch that I could test!
>>
> 
> You could start by seeing if using pre/post_req helps at all, as below.
> If that doesn't help, then it might need more analysis.

Hello, Adrian! I tested yours patch and 100% fixes the problem! Thank
you very much! Please make a proper patch and feel free to add my t-b!

Tested-by: Dmitry Osipenko <digetx@gmail.com>