| Message ID | 1577968140-11528-1-git-send-email-paul.henrysd@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | ubi: Update mean Erase Counter calculation at run-time | expand |
On Thu, Jan 2, 2020 at 1:29 PM Paul HENRYS <paul.henrysd@gmail.com> 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@gmail.com> > --- > 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. > 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()? > + 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.
On Mon, Jan 13, 2020 at 12:06 AM, Richard Weinberger <richard.weinberger@gmail.com> wrote: > > On Thu, Jan 2, 2020 at 1:29 PM Paul HENRYS <paul.henrysd@gmail.com> 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@gmail.com> > > --- > > 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
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); + 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 */ 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; dbg_wl("erase PEB %d, old EC %llu", e->pnum, ec); @@ -458,6 +459,7 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e, goto out_free; ec += err; + erasecycle = err; if (ec > UBI_MAX_ERASECOUNTER) { /* * Erase counter overflow. Upgrade UBI and use 64-bit @@ -479,6 +481,7 @@ static int sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e, e->ec = ec; spin_lock(&ubi->wl_lock); + ubi->ec_sum += erasecycle; if (e->ec > ubi->max_ec) ubi->max_ec = e->ec; spin_unlock(&ubi->wl_lock); @@ -1164,6 +1167,7 @@ static int __erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk) } ubi->bad_peb_count += 1; ubi->good_peb_count -= 1; + ubi->ec_sum -= e->ec; ubi_calculate_reserved(ubi); if (available_consumed) ubi_warn(ubi, "no PEBs in the reserved pool, used an available PEB"); @@ -1738,6 +1742,7 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai) mutex_init(&ubi->move_mutex); init_rwsem(&ubi->work_sem); ubi->max_ec = ai->max_ec; + ubi->ec_sum = ai->ec_sum; INIT_LIST_HEAD(&ubi->works); sprintf(ubi->bgt_name, UBI_BGT_NAME_PATTERN, ubi->ubi_num);
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@gmail.com> --- drivers/mtd/ubi/build.c | 10 ++++++++++ drivers/mtd/ubi/ubi.h | 3 ++- drivers/mtd/ubi/wl.c | 5 +++++ 3 files changed, 17 insertions(+), 1 deletion(-)