[RFC PATCH] mmc: core: Optimize case for exactly one erase-group budget TRIM

[RFC PATCH] mmc: core: Optimize case for exactly one erase-group budget TRIM

[David Jander]

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(-)

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 */
-- 
2.1.4

Re: [RFC PATCH] mmc: core: Optimize case for exactly one erase-group budget TRIM

[Ulf Hansson]

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

Re: [RFC PATCH] mmc: core: Optimize case for exactly one erase-group budget TRIM

[David Jander]

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,

-- 
David Jander
Protonic Holland.

[RFC PATCH 2/2] mmc: core.c: Add comment to clarify special cases of ERASE/TRIM

[David Jander]

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/mmc/core/core.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6c9611b..b6aa9ad 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2109,11 +2109,20 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
 	    !(card->ext_csd.sec_feature_support & EXT_CSD_SEC_GB_CL_EN))
 		return -EOPNOTSUPP;
 
+	/*
+	 * Sanity check: If we do not erase aligned, whole erase-groups, return
+	 * an error, since we intended a "secure" erase, silently not erasing
+	 * something would be unacceptable.
+	 */
 	if (arg == MMC_SECURE_ERASE_ARG) {
 		if (from % card->erase_size || nr % card->erase_size)
 			return -EINVAL;
 	}
 
+	/*
+	 * Make sure only erase-groups that are fully contained in the erase
+	 * region are erased. Silently ignore the rest.
+	 */
 	if (arg == MMC_ERASE_ARG) {
 		rem = from % card->erase_size;
 		if (rem) {
@@ -2140,6 +2149,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
 	/* 'from' and 'to' are inclusive */
 	to -= 1;
 
+	/*
+	 * Special case where only one erase-group fits in the timout budget:
+	 * If the region crosses an erase-group boundary on this particular
+	 * case, we will be trimming more than one erase-group which, does not
+	 * fit in the timeout budget of the controller, so we need to split it
+	 * and call mmc_do_erase() twice if necessary. This special case is
+	 * identified by the card->eg_boundary flag.
+	 */
 	if ((arg & MMC_TRIM_ARGS) && (card->eg_boundary) &&
 	    (from % card->erase_size)) {
 		rem = card->erase_size - (from % card->erase_size);
@@ -2244,7 +2261,16 @@ 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 */
+	/*
+	 * When specifying a sector range to trim, chances are we might cross
+	 * an erase-group boundary even if the amount of sectors is less than
+	 * one erase-group.
+	 * If we can only fit one erase-group in the controller timeout budget,
+	 * we have to care that erase-group boundaries are not crossed by a
+	 * single trim operation. We flag that special case with "eg_boundary".
+	 * In all other cases we can just decrement qty and pretend that we
+	 * always touch (qty + 1) erase-groups as a simple optimization.
+	 */
 	if (qty == 1)
 		card->eg_boundary = 1;
 	else
-- 
2.1.4

Re: [RFC PATCH 2/2] mmc: core.c: Add comment to clarify special cases of ERASE/TRIM

[Adrian Hunter]

On 04/06/15 13:20, David Jander wrote:
> Signed-off-by: David Jander <david@protonic.nl>

Please never send delta patches.  Always send a new version of the whole patch.

> ---
>  drivers/mmc/core/core.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6c9611b..b6aa9ad 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2109,11 +2109,20 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
>  	    !(card->ext_csd.sec_feature_support & EXT_CSD_SEC_GB_CL_EN))
>  		return -EOPNOTSUPP;
>  
> +	/*
> +	 * Sanity check: If we do not erase aligned, whole erase-groups, return
> +	 * an error, since we intended a "secure" erase, silently not erasing
> +	 * something would be unacceptable.
> +	 */

I am not sure the value of a comment that can anyway be inferred from the code.

>  	if (arg == MMC_SECURE_ERASE_ARG) {
>  		if (from % card->erase_size || nr % card->erase_size)
>  			return -EINVAL;
>  	}
>  
> +	/*
> +	 * Make sure only erase-groups that are fully contained in the erase
> +	 * region are erased. Silently ignore the rest.
> +	 */

Ditto

>  	if (arg == MMC_ERASE_ARG) {
>  		rem = from % card->erase_size;
>  		if (rem) {
> @@ -2140,6 +2149,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
>  	/* 'from' and 'to' are inclusive */
>  	to -= 1;
>  
> +	/*
> +	 * Special case where only one erase-group fits in the timout budget:

timout -> timeout

> +	 * If the region crosses an erase-group boundary on this particular
> +	 * case, we will be trimming more than one erase-group which, does not
> +	 * fit in the timeout budget of the controller, so we need to split it
> +	 * and call mmc_do_erase() twice if necessary. This special case is
> +	 * identified by the card->eg_boundary flag.
> +	 */
>  	if ((arg & MMC_TRIM_ARGS) && (card->eg_boundary) &&
>  	    (from % card->erase_size)) {
>  		rem = card->erase_size - (from % card->erase_size);
> @@ -2244,7 +2261,16 @@ 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 */
> +	/*
> +	 * When specifying a sector range to trim, chances are we might cross
> +	 * an erase-group boundary even if the amount of sectors is less than
> +	 * one erase-group.
> +	 * If we can only fit one erase-group in the controller timeout budget,
> +	 * we have to care that erase-group boundaries are not crossed by a
> +	 * single trim operation. We flag that special case with "eg_boundary".
> +	 * In all other cases we can just decrement qty and pretend that we
> +	 * always touch (qty + 1) erase-groups as a simple optimization.

The language seems a little odd here. We are setting the max_discard limit
which does not involve "pretending" or "optimization", it is just a
calculation.  The important point is that the calculation has to count the
maximum number of erase blocks affected not the size in erase blocks.  You
could give an example e.g. if a 2 sector trim crosses an erase block
boundary then that counts as 2 erase blocks affected.

> +	 */
>  	if (qty == 1)
>  		card->eg_boundary = 1;
>  	else
> 

Re: [RFC PATCH 2/2] mmc: core.c: Add comment to clarify special cases of ERASE/TRIM

[David Jander]

Dear Adrian,

Thanks for reacting.

On Thu, 04 Jun 2015 14:16:23 +0300
Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 04/06/15 13:20, David Jander wrote:
> > Signed-off-by: David Jander <david@protonic.nl>
> 
> Please never send delta patches.  Always send a new version of the whole
> patch.

Sorry for that. This was meant as a separate patch though... the original
would be part 1/2 and this is part 2/2 (as noted in the subject), and I did it
only to get an idea if I understood Ulf correctly (RFC).

> > ---
> >  drivers/mmc/core/core.c | 28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 6c9611b..b6aa9ad 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2109,11 +2109,20 @@ int mmc_erase(struct mmc_card *card, unsigned int
> > from, unsigned int nr, !(card->ext_csd.sec_feature_support &
> > EXT_CSD_SEC_GB_CL_EN)) return -EOPNOTSUPP;
> >  
> > +	/*
> > +	 * Sanity check: If we do not erase aligned, whole erase-groups,
> > return
> > +	 * an error, since we intended a "secure" erase, silently not
> > erasing
> > +	 * something would be unacceptable.
> > +	 */
> 
> I am not sure the value of a comment that can anyway be inferred from the
> code.

Neither am I, but Ulf suggested to put some more comments to the code in this
function. At least that's what I understood... I figured it was not so obvious
why we take different approaches to ERASE and SECURE_ERASE, so some explaining
might have been desirable. I even took the time to go through the eMMC
specification to see if there was some recommendation about this...

> >  	if (arg == MMC_SECURE_ERASE_ARG) {
> >  		if (from % card->erase_size || nr % card->erase_size)
> >  			return -EINVAL;
> >  	}
> >  
> > +	/*
> > +	 * Make sure only erase-groups that are fully contained in the
> > erase
> > +	 * region are erased. Silently ignore the rest.
> > +	 */
> 
> Ditto
> 
> >  	if (arg == MMC_ERASE_ARG) {
> >  		rem = from % card->erase_size;
> >  		if (rem) {
> > @@ -2140,6 +2149,14 @@ int mmc_erase(struct mmc_card *card, unsigned int
> > from, unsigned int nr, /* 'from' and 'to' are inclusive */
> >  	to -= 1;
> >  
> > +	/*
> > +	 * Special case where only one erase-group fits in the timout
> > budget:
> 
> timout -> timeout

Oops. Thanks.

> > +	 * If the region crosses an erase-group boundary on this
> > particular
> > +	 * case, we will be trimming more than one erase-group which,
> > does not
> > +	 * fit in the timeout budget of the controller, so we need to
> > split it
> > +	 * and call mmc_do_erase() twice if necessary. This special case
> > is
> > +	 * identified by the card->eg_boundary flag.
> > +	 */
> >  	if ((arg & MMC_TRIM_ARGS) && (card->eg_boundary) &&
> >  	    (from % card->erase_size)) {
> >  		rem = card->erase_size - (from % card->erase_size);
> > @@ -2244,7 +2261,16 @@ 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 */
> > +	/*
> > +	 * When specifying a sector range to trim, chances are we might
> > cross
> > +	 * an erase-group boundary even if the amount of sectors is less
> > than
> > +	 * one erase-group.
> > +	 * If we can only fit one erase-group in the controller timeout
> > budget,
> > +	 * we have to care that erase-group boundaries are not crossed by
> > a
> > +	 * single trim operation. We flag that special case with
> > "eg_boundary".
> > +	 * In all other cases we can just decrement qty and pretend that
> > we
> > +	 * always touch (qty + 1) erase-groups as a simple optimization.
> 
> The language seems a little odd here. We are setting the max_discard limit
> which does not involve "pretending" or "optimization", it is just a
> calculation.  The important point is that the calculation has to count the
> maximum number of erase blocks affected not the size in erase blocks.  You
> could give an example e.g. if a 2 sector trim crosses an erase block
> boundary then that counts as 2 erase blocks affected.

Sorry to disagree here. Strictly speaking we are not only calculating
max_discard, because max_discard is useless for a function that takes sectors
as arguments, when this value depends on erase-groups and not sectors. There
is no valid function converting from one to the other, so we _need_ to pretend
something. That's what the somewhat obscure "if (qty==1) return 1" trickery
does, together with the magical "--qty" afterwards. The original code pretends
that we always cross an erase-group boundary, hence the --qty. This needs
explaining, because strictly speaking it is not correct because max_discard
can be higher. It just doesn't produce wrong results because we "are on the
safe side". And doing something different for the case qty==1 is definitely an
optimization... which is what the first patch intends to do.
Maybe the name of the function is misleading...?

> > +	 */
> >  	if (qty == 1)
> >  		card->eg_boundary = 1;
> >  	else
> > 
> 

Best regards,

-- 
David Jander
Protonic Holland.

Re: [RFC PATCH] mmc: core: Optimize case for exactly one erase-group budget TRIM

[David Jander]

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,

-- 
David Jander
Protonic Holland.