On 22 May 2018 at 18:54, Martin Hicks <mort@xxxxxxxx> wrote: > On Mon, May 21, 2018 at 10:22:11AM +0200, Ulf Hansson wrote: >> On 18 May 2018 at 17:19, Martin Hicks <mort@xxxxxxxx> wrote: >> > >> > I now have second thoughts about this change since the same path is used for >> > discarding smaller blocks, like if you have "discard" enabled on ext4. >> > These discard requests can complete very quickly and we wouldn't want to add >> > latency to these commands. It looks like the "quick" discards can complete >> > in as little as 50 loops (~1ms), but that would be dependent on the speed of >> > the MMC bus, as well as how much work the eMMC drive has to do. >> >> I fully agree. Several things comes into play, so clearly we needs >> something a bit more clever than a fixed timeout value. >> >> A simplistic approach could be to use a small timeout to start with, >> then increase it while iterating the loops. >> >> Perhaps start by sleeping 128us, then 256, 512, 1024, 2048, 4096. Then >> we probably want to stop increasing at some point. > > > Hi Uffe, > > Good suggestion. I've gone ahead an recreated the patch with a linear > backoff. I had concerns that with some faster eMMC buses (I'm only using DDR52), > that doubling the timeout each time may not have enough granularity to eliminate > unnecessary latency, so I made the backoff linear. The patch follows: In regards to latency, this boils done to what happens inside the card. An actual erase takes lots of time to complete, so no worries in regards to latency. Although, a trim/discard operations should normally be rather cheap, even if it depends on the internals of the device. > > > This drastically reduces the rate at which the MMC_SEND_STATUS cmd polls > for completion of the MMC Erase operation. The patch does this by adding > a linear backoff sleep that starts by sleeping for very short periods > (32-64us), and ramps up to sleeping for 1-2ms. So this means, we are going to iterate the loop for 32 times, before we hit the maximum sleep time. Assuming that an usleep_range(n, 2*n), puts us in the middle of that range, the above 32 iterations would mean we would be sleeping of ~25ms in total. Then each following iteration means a sleep time of ~1.5 ms. I am worried that this still means too much of polling-commands to be sent. First, I am rather sure we don't need to iterate the loop 32 times before reaching 1024 us sleep time. The steps can be bigger, possibly also increased over time. Second, capping at 1024 is way too early, especially when the total loop time ends up being in the range of seconds. > > Even on very quickly completing erase operations, the loop iterates 10+ > times, so not too much extra latency is added to these commands. A comparison between before and after the change would be nice, can you share those numbers? > > For long running discard operarations, like a full-device secure discard, > this change drops the interrupt rates on my single-core NXP I.MX6UL from > 45000/s to <1000/s, and greatly improves system responsiveness. > Still, ~1000/s is quite high, especially if it is for doing nothing. :-). > Signed-off-by: Martin Hicks <mort@xxxxxxxx> > --- > drivers/mmc/core/core.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index db1bf63..76ef580 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2159,6 +2159,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from, > unsigned int qty = 0, busy_timeout = 0; > bool use_r1b_resp = false; > unsigned long timeout; > + int loop_udelay=32, udelay_max=1024; > int err; > > mmc_retune_hold(card->host); > @@ -2283,9 +2284,15 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from, > err = -EIO; > goto out; > } > + if ((cmd.resp[0] & R1_READY_FOR_DATA) && > + R1_CURRENT_STATE(cmd.resp[0]) != R1_STATE_PRG) > + break; > + > + usleep_range(loop_udelay, loop_udelay*2); > + if (loop_udelay < udelay_max) > + loop_udelay += 32; > + } while (1); > > - } while (!(cmd.resp[0] & R1_READY_FOR_DATA) || > - (R1_CURRENT_STATE(cmd.resp[0]) == R1_STATE_PRG)); > out: > mmc_retune_release(card->host); > return err; > -- > 1.9.1 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
