* [PATCH] ubi: Update mean Erase Counter calculation at run-time
@ 2020-01-02 12:29 Paul HENRYS
2020-01-12 23:05 ` Richard Weinberger
0 siblings, 1 reply; 3+ messages in thread
From: Paul HENRYS @ 2020-01-02 12:29 UTC (permalink / raw)
To: richard, miquel.raynal, vigneshr; +Cc: linux-mtd, Paul HENRYS
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(-)
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);
--
2.7.4
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ubi: Update mean Erase Counter calculation at run-time
2020-01-02 12:29 [PATCH] ubi: Update mean Erase Counter calculation at run-time Paul HENRYS
@ 2020-01-12 23:05 ` Richard Weinberger
2020-01-13 12:50 ` Paul HENRYS d'AUBIGNY
0 siblings, 1 reply; 3+ messages in thread
From: Richard Weinberger @ 2020-01-12 23:05 UTC (permalink / raw)
To: Paul HENRYS
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal
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.
--
Thanks,
//richard
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ubi: Update mean Erase Counter calculation at run-time
2020-01-12 23:05 ` Richard Weinberger
@ 2020-01-13 12:50 ` Paul HENRYS d'AUBIGNY
0 siblings, 0 replies; 3+ messages in thread
From: Paul HENRYS d'AUBIGNY @ 2020-01-13 12:50 UTC (permalink / raw)
To: Richard Weinberger
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-13 12:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-02 12:29 [PATCH] ubi: Update mean Erase Counter calculation at run-time Paul HENRYS
2020-01-12 23:05 ` Richard Weinberger
2020-01-13 12:50 ` Paul HENRYS d'AUBIGNY
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).