| Message ID | 1433320469-29453-1-git-send-email-david@protonic.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3 June 2015 at 10:34, David Jander <david@protonic.nl> wrote: > In the (not so unlikely) case that the mmc controller timeout budget is > enough for exactly one erase-group, the simplification of allowing one > sector has an enormous performance penalty. We optimize this special case > by introducing a flag that prohibits erase-group boundary crossing, so > that we can allow trimming more than one sector at a time. > > Signed-off-by: David Jander <david@protonic.nl> Hi David, Thanks for working on this! > --- > drivers/mmc/core/core.c | 21 +++++++++++++++++---- > include/linux/mmc/card.h | 1 + > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 92e7671..6c9611b 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2089,6 +2089,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, > unsigned int arg) > { > unsigned int rem, to = from + nr; > + int err; > > if (!(card->host->caps & MMC_CAP_ERASE) || > !(card->csd.cmdclass & CCC_ERASE)) > @@ -2139,6 +2140,15 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, > /* 'from' and 'to' are inclusive */ > to -= 1; > > + if ((arg & MMC_TRIM_ARGS) && (card->eg_boundary) && > + (from % card->erase_size)) { > + rem = card->erase_size - (from % card->erase_size); > + err = mmc_do_erase(card, from, from + rem - 1, arg); > + from += rem; > + if ((err) || (to <= from)) > + return err; > + } > + Urgh, this function is becoming a bit messy. Would it be possible to split the code into some helper functions instead - and adding some additional comments of what goes on maybe? > return mmc_do_erase(card, from, to, arg); > } > EXPORT_SYMBOL(mmc_erase); > @@ -2234,16 +2244,19 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, > if (!qty) > return 0; > > + /* We can only erase one erase group special case */ Could you extend this comment a bit. I think it deserves that. > if (qty == 1) > - return 1; > + card->eg_boundary = 1; > + else > + qty--; > > /* Convert qty to sectors */ > if (card->erase_shift) > - max_discard = --qty << card->erase_shift; > + max_discard = qty << card->erase_shift; > else if (mmc_card_sd(card)) > - max_discard = qty; > + max_discard = qty + 1; > else > - max_discard = --qty * card->erase_size; > + max_discard = qty * card->erase_size; > > return max_discard; > } > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h > index 19f0175..704b60d 100644 > --- a/include/linux/mmc/card.h > +++ b/include/linux/mmc/card.h > @@ -282,6 +282,7 @@ struct mmc_card { > unsigned int erase_size; /* erase size in sectors */ > unsigned int erase_shift; /* if erase unit is power 2 */ > unsigned int pref_erase; /* in sectors */ > + unsigned int eg_boundary; /* don't cross erase-group boundaries */ > u8 erased_byte; /* value of erased bytes */ > > u32 raw_cid[4]; /* raw card CID */ > -- > 2.1.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 4 Jun 2015 10:31:59 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 3 June 2015 at 10:34, David Jander <david@protonic.nl> wrote: > > In the (not so unlikely) case that the mmc controller timeout budget is > > enough for exactly one erase-group, the simplification of allowing one > > sector has an enormous performance penalty. We optimize this special case > > by introducing a flag that prohibits erase-group boundary crossing, so > > that we can allow trimming more than one sector at a time. > > > > Signed-off-by: David Jander <david@protonic.nl> > > Hi David, > > Thanks for working on this! Thanks for reviewing! > > --- > > drivers/mmc/core/core.c | 21 +++++++++++++++++---- > > include/linux/mmc/card.h | 1 + > > 2 files changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > > index 92e7671..6c9611b 100644 > > --- a/drivers/mmc/core/core.c > > +++ b/drivers/mmc/core/core.c > > @@ -2089,6 +2089,7 @@ int mmc_erase(struct mmc_card *card, unsigned int > > from, unsigned int nr, unsigned int arg) > > { > > unsigned int rem, to = from + nr; > > + int err; > > > > if (!(card->host->caps & MMC_CAP_ERASE) || > > !(card->csd.cmdclass & CCC_ERASE)) > > @@ -2139,6 +2140,15 @@ int mmc_erase(struct mmc_card *card, unsigned int > > from, unsigned int nr, /* 'from' and 'to' are inclusive */ > > to -= 1; > > > > + if ((arg & MMC_TRIM_ARGS) && (card->eg_boundary) && > > + (from % card->erase_size)) { > > + rem = card->erase_size - (from % card->erase_size); > > + err = mmc_do_erase(card, from, from + rem - 1, arg); > > + from += rem; > > + if ((err) || (to <= from)) > > + return err; > > + } > > + > > Urgh, this function is becoming a bit messy. Would it be possible to > split the code into some helper functions instead - and adding some > additional comments of what goes on maybe? I agree on adding comments, but I don't clearly see very well how to split things out. The first 6 if-statements are relatively short error-condition checks. Those might deserve a comment for the less obvious ones. Then comes the "if (arg == MMC_ERASE_ARG)..." case, which basically makes sure that no blocks are erased that are not fully contained in the erase region. Some sort of safety check I guess. In this case a comment is definitely due, but it also makes use of all function parameters, modifies two of them and has a conditional exit that must be checked in the calling scope... not very easy nor elegant to move out to a sub-function IMHO. The last if-statement (the one I added) modifies just one parameter, but also has an exit status, so it would require at least one pointer argument for passing the "from" parameter.... is it worth the trouble to split that out? What should I do here (besides adding some comments)? > > return mmc_do_erase(card, from, to, arg); > > } > > EXPORT_SYMBOL(mmc_erase); > > @@ -2234,16 +2244,19 @@ static unsigned int mmc_do_calc_max_discard(struct > > mmc_card *card, if (!qty) > > return 0; > > > > + /* We can only erase one erase group special case */ > > Could you extend this comment a bit. I think it deserves that. Ok, will do. > > if (qty == 1) > > - return 1; > > + card->eg_boundary = 1; > > + else > > + qty--; > > > > /* Convert qty to sectors */ > > if (card->erase_shift) > > - max_discard = --qty << card->erase_shift; > > + max_discard = qty << card->erase_shift; > > else if (mmc_card_sd(card)) > > - max_discard = qty; > > + max_discard = qty + 1; > > else > > - max_discard = --qty * card->erase_size; > > + max_discard = qty * card->erase_size; > > > > return max_discard; > > } > > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h > > index 19f0175..704b60d 100644 > > --- a/include/linux/mmc/card.h > > +++ b/include/linux/mmc/card.h > > @@ -282,6 +282,7 @@ struct mmc_card { > > unsigned int erase_size; /* erase size in sectors */ > > unsigned int erase_shift; /* if erase unit is power > > 2 */ unsigned int pref_erase; /* in sectors */ > > + unsigned int eg_boundary; /* don't cross erase-group > > boundaries */ u8 erased_byte; /* value of erased > > bytes */ > > > > u32 raw_cid[4]; /* raw card CID */ > > -- > > 2.1.4 > > > > Kind regards > Uffe Best regards,
Dear Ulf, On Thu, 4 Jun 2015 10:31:59 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 3 June 2015 at 10:34, David Jander <david@protonic.nl> wrote: > > In the (not so unlikely) case that the mmc controller timeout budget is > > enough for exactly one erase-group, the simplification of allowing one > > sector has an enormous performance penalty. We optimize this special case > > by introducing a flag that prohibits erase-group boundary crossing, so > > that we can allow trimming more than one sector at a time. > > > > Signed-off-by: David Jander <david@protonic.nl> > > Hi David, > > Thanks for working on this! I have since sent an updated patch that includes more comment. It would be great if you could find the time to review it. I hope the comments are clear enough. Best regards,
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 92e7671..6c9611b 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2089,6 +2089,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, unsigned int arg) { unsigned int rem, to = from + nr; + int err; if (!(card->host->caps & MMC_CAP_ERASE) || !(card->csd.cmdclass & CCC_ERASE)) @@ -2139,6 +2140,15 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, /* 'from' and 'to' are inclusive */ to -= 1; + if ((arg & MMC_TRIM_ARGS) && (card->eg_boundary) && + (from % card->erase_size)) { + rem = card->erase_size - (from % card->erase_size); + err = mmc_do_erase(card, from, from + rem - 1, arg); + from += rem; + if ((err) || (to <= from)) + return err; + } + return mmc_do_erase(card, from, to, arg); } EXPORT_SYMBOL(mmc_erase); @@ -2234,16 +2244,19 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, if (!qty) return 0; + /* We can only erase one erase group special case */ if (qty == 1) - return 1; + card->eg_boundary = 1; + else + qty--; /* Convert qty to sectors */ if (card->erase_shift) - max_discard = --qty << card->erase_shift; + max_discard = qty << card->erase_shift; else if (mmc_card_sd(card)) - max_discard = qty; + max_discard = qty + 1; else - max_discard = --qty * card->erase_size; + max_discard = qty * card->erase_size; return max_discard; } diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index 19f0175..704b60d 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -282,6 +282,7 @@ struct mmc_card { unsigned int erase_size; /* erase size in sectors */ unsigned int erase_shift; /* if erase unit is power 2 */ unsigned int pref_erase; /* in sectors */ + unsigned int eg_boundary; /* don't cross erase-group boundaries */ u8 erased_byte; /* value of erased bytes */ u32 raw_cid[4]; /* raw card CID */
In the (not so unlikely) case that the mmc controller timeout budget is enough for exactly one erase-group, the simplification of allowing one sector has an enormous performance penalty. We optimize this special case by introducing a flag that prohibits erase-group boundary crossing, so that we can allow trimming more than one sector at a time. Signed-off-by: David Jander <david@protonic.nl> --- drivers/mmc/core/core.c | 21 +++++++++++++++++---- include/linux/mmc/card.h | 1 + 2 files changed, 18 insertions(+), 4 deletions(-)