Re: [PATCH] ubi: Update mean Erase Counter calculation at run-time

["Paul HENRYS d'AUBIGNY" <paul.henrysd@xxxxxxxxx>]

On Mon, Jan 13, 2020 at 12:06 AM, Richard Weinberger
<richard.weinberger@xxxxxxxxx> wrote:
>
> On Thu, Jan 2, 2020 at 1:29 PM Paul HENRYS <paul.henrysd@xxxxxxxxx> wrote:
> >
> > The mean EC value was only calculated when attaching a UBI device
> > but not updated afterwards. A new parameter is added to the struct
> > ubi_device to keep track of the sum of the erase counters of all
> > eraseblocks for a UBI device while they are erased and potentially
> > become bad. The mean EC is then calculated on request when reading
> > the "mean_ec" sysfs file of a UBI device.
> >
> > Signed-off-by: Paul HENRYS <paul.henrysd@xxxxxxxxx>
> > ---
> >  drivers/mtd/ubi/build.c | 10 ++++++++++
> >  drivers/mtd/ubi/ubi.h   |  3 ++-
> >  drivers/mtd/ubi/wl.c    |  5 +++++
>
> Please update Documentation/ABI/stable/sysfs-class-ubi too.
I will do that.
>
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > index d636bbe..3291fa4 100644
> > --- a/drivers/mtd/ubi/build.c
> > +++ b/drivers/mtd/ubi/build.c
> > @@ -126,6 +126,8 @@ static struct device_attribute dev_volumes_count =
> >         __ATTR(volumes_count, S_IRUGO, dev_attribute_show, NULL);
> >  static struct device_attribute dev_max_ec =
> >         __ATTR(max_ec, S_IRUGO, dev_attribute_show, NULL);
> > +static struct device_attribute dev_mean_ec =
> > +       __ATTR(mean_ec, S_IRUGO, dev_attribute_show, NULL);
> >  static struct device_attribute dev_reserved_for_bad =
> >         __ATTR(reserved_for_bad, S_IRUGO, dev_attribute_show, NULL);
> >  static struct device_attribute dev_bad_peb_count =
> > @@ -364,6 +366,13 @@ static ssize_t dev_attribute_show(struct device *dev,
> >                 ret = sprintf(buf, "%d\n", ubi->vol_count - UBI_INT_VOL_COUNT);
> >         else if (attr == &dev_max_ec)
> >                 ret = sprintf(buf, "%d\n", ubi->max_ec);
> > +       else if (attr == &dev_mean_ec) {
> > +               /* Calculate mean erase counter */
> > +               if (ubi->good_peb_count)
> > +                       ubi->mean_ec = div_u64(ubi->ec_sum,
> > +                                               ubi->good_peb_count);
>
> Is there a reason why you don't the math in sync_erase()?
I was thinking to only calculate the mean_ec, which implies a
division, when required. Hence when reading the sysfs file in the
current implementation. The aim being not to waste CPU cycles for
something not required.
Let me know what is preferred and I will adapt the patch.
>
> > +               ret = sprintf(buf, "%d\n", ubi->mean_ec);
> > +       }
> >         else if (attr == &dev_reserved_for_bad)
> >                 ret = sprintf(buf, "%d\n", ubi->beb_rsvd_pebs);
> >         else if (attr == &dev_bad_peb_count)
> > @@ -391,6 +400,7 @@ static struct attribute *ubi_dev_attrs[] = {
> >         &dev_total_eraseblocks.attr,
> >         &dev_volumes_count.attr,
> >         &dev_max_ec.attr,
> > +       &dev_mean_ec.attr,
> >         &dev_reserved_for_bad.attr,
> >         &dev_bad_peb_count.attr,
> >         &dev_max_vol_count.attr,
> > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> > index 9688b41..d796217 100644
> > --- a/drivers/mtd/ubi/ubi.h
> > +++ b/drivers/mtd/ubi/ubi.h
> > @@ -472,6 +472,7 @@ struct ubi_debug_info {
> >   *
> >   * @max_ec: current highest erase counter value
> >   * @mean_ec: current mean erase counter value
> > + * @ec_sum: a temporary variable used when calculating @mean_ec
> >   *
> >   * @global_sqnum: global sequence number
> >   * @ltree_lock: protects the lock tree and @global_sqnum
> > @@ -580,8 +581,8 @@ struct ubi_device {
> >         struct mutex device_mutex;
> >
> >         int max_ec;
> > -       /* Note, mean_ec is not updated run-time - should be fixed */
>
> Thanks for addressing this. :-)
>
> >         int mean_ec;
> > +       uint64_t ec_sum;
> >
> >         /* EBA sub-system's stuff */
> >         unsigned long long global_sqnum;
> > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> > index 5d77a38..24c47ce 100644
> > --- a/drivers/mtd/ubi/wl.c
> > +++ b/drivers/mtd/ubi/wl.c
> > @@ -442,6 +442,7 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
> >         int err;
> >         struct ubi_ec_hdr *ec_hdr;
> >         unsigned long long ec = e->ec;
> > +       unsigned long long erasecycle;
>
> Why do you need a new variable?
> We already have everything we need.
Except if I miss something, no. We loose the value assigned to err
later on when calling ubi_io_write_ec_hdr() and the value returned by
ubi_io_sync_erase() is required after the call to
ubi_io_write_ec_hdr().
>
> --
> Thanks,
> //richard
>
Cheers,
Paul

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/