Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753819AbbFDJme (ORCPT ); Thu, 4 Jun 2015 05:42:34 -0400 Received: from protonic.xs4all.nl ([83.163.252.89]:24088 "EHLO protonic.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753768AbbFDJm2 (ORCPT ); Thu, 4 Jun 2015 05:42:28 -0400 Date: Thu, 4 Jun 2015 11:42:25 +0200 From: David Jander To: Ulf Hansson , Adrian Hunter Cc: Pierre Ossman , Sascha Hauer , Johan Rudholm , Javier Martinez Canillas , linux-mmc , "linux-kernel@vger.kernel.org" Subject: Re: [RFC PATCH] mmc: core: Optimize case for exactly one erase-group budget TRIM Message-ID: <20150604114225.0d443e31@archvile> In-Reply-To: References: <1433320469-29453-1-git-send-email-david@protonic.nl> Organization: Protonic Holland X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.27; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5013 Lines: 129 On Thu, 4 Jun 2015 10:31:59 +0200 Ulf Hansson wrote: > On 3 June 2015 at 10:34, David Jander 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 > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/