Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On 9/30/2014 10:44 AM, Richard Weinberger wrote: Am 30.09.2014 09:39, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(>wl_lock); + ubi->fm_work_scheduled = 0; + spin_unlock(>wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. I've added the spinlock to have a barrier in any case. Examples of any? You mean a case where the compiler would reorder code and the barrier is needed? I don't have one, but I'm not that creative as a modern C compiler. If you say that no barrier is needed I'll trust you. :-) we just implemented the same thing :) It's being tested Why not use atomic_t fm_work_scheduled and save the spin_lock? Thanks, //richard __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On 9/30/2014 10:44 AM, Richard Weinberger wrote: Am 30.09.2014 09:39, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(ubi-wl_lock); + ubi-fm_work_scheduled = 0; + spin_unlock(ubi-wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. I've added the spinlock to have a barrier in any case. Examples of any? You mean a case where the compiler would reorder code and the barrier is needed? I don't have one, but I'm not that creative as a modern C compiler. If you say that no barrier is needed I'll trust you. :-) we just implemented the same thing :) It's being tested Why not use atomic_t fm_work_scheduled and save the spin_lock? Thanks, //richard __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
Am 30.09.2014 09:39, schrieb Bityutskiy, Artem: > On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: >> Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: >>> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(>wl_lock); + ubi->fm_work_scheduled = 0; + spin_unlock(>wl_lock); >>> >>> Andrew Morton once said me that if I am protecting an integer change >>> like this with a spinlock, I have a problem in my locking design. He was >>> right for my particular case. >>> >>> Integer is changes atomic. The only other thing spinlock adds are the >>> barriers. >> >> I've added the spinlock to have a barrier in any case. > > Examples of any? You mean a case where the compiler would reorder code and the barrier is needed? I don't have one, but I'm not that creative as a modern C compiler. If you say that no barrier is needed I'll trust you. :-) Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: > Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: > > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: > >> + spin_lock(>wl_lock); > >> + ubi->fm_work_scheduled = 0; > >> + spin_unlock(>wl_lock); > > > > Andrew Morton once said me that if I am protecting an integer change > > like this with a spinlock, I have a problem in my locking design. He was > > right for my particular case. > > > > Integer is changes atomic. The only other thing spinlock adds are the > > barriers. > > I've added the spinlock to have a barrier in any case. Examples of any? -- Best Regards, Artem Bityutskiy - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: >> + spin_lock(>wl_lock); >> + ubi->fm_work_scheduled = 0; >> + spin_unlock(>wl_lock); > > Andrew Morton once said me that if I am protecting an integer change > like this with a spinlock, I have a problem in my locking design. He was > right for my particular case. > > Integer is changes atomic. The only other thing spinlock adds are the > barriers. I've added the spinlock to have a barrier in any case. Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: > + spin_lock(>wl_lock); > + ubi->fm_work_scheduled = 0; > + spin_unlock(>wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. -- Best Regards, Artem Bityutskiy - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(ubi-wl_lock); + ubi-fm_work_scheduled = 0; + spin_unlock(ubi-wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. -- Best Regards, Artem Bityutskiy - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(ubi-wl_lock); + ubi-fm_work_scheduled = 0; + spin_unlock(ubi-wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. I've added the spinlock to have a barrier in any case. Thanks, //richard -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(ubi-wl_lock); + ubi-fm_work_scheduled = 0; + spin_unlock(ubi-wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. I've added the spinlock to have a barrier in any case. Examples of any? -- Best Regards, Artem Bityutskiy - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
Am 30.09.2014 09:39, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 08:59 +0200, Richard Weinberger wrote: Am 30.09.2014 08:45, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: + spin_lock(ubi-wl_lock); + ubi-fm_work_scheduled = 0; + spin_unlock(ubi-wl_lock); Andrew Morton once said me that if I am protecting an integer change like this with a spinlock, I have a problem in my locking design. He was right for my particular case. Integer is changes atomic. The only other thing spinlock adds are the barriers. I've added the spinlock to have a barrier in any case. Examples of any? You mean a case where the compiler would reorder code and the barrier is needed? I don't have one, but I'm not that creative as a modern C compiler. If you say that no barrier is needed I'll trust you. :-) Thanks, //richard -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
If the WL pool runs out of PEBs we schedule a fastmap write
to refill it was soon as possible.
Ensure that only one at a time is scheduled otherwise we might end in
a fastmap write storm was writing the fastmap can schedule another
write if bitflips were detected.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/ubi.h | 2 ++
drivers/mtd/ubi/wl.c | 8 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 7bf4163..529dfb0 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -426,6 +426,7 @@ struct ubi_debug_info {
* @fm_size: fastmap size in bytes
* @fm_sem: allows ubi_update_fastmap() to block EBA table changes
* @fm_work: fastmap work queue
+ * @fm_work_scheduled: non-zero if fastmap work was scheduled
*
* @used: RB-tree of used physical eraseblocks
* @erroneous: RB-tree of erroneous used physical eraseblocks
@@ -531,6 +532,7 @@ struct ubi_device {
void *fm_buf;
size_t fm_size;
struct work_struct fm_work;
+ int fm_work_scheduled;
/* Wear-leveling sub-system's stuff */
struct rb_root used;
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index dc01b1f..4c02a6e 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -149,6 +149,9 @@ static void update_fastmap_work_fn(struct work_struct *wrk)
{
struct ubi_device *ubi = container_of(wrk, struct ubi_device, fm_work);
ubi_update_fastmap(ubi);
+ spin_lock(>wl_lock);
+ ubi->fm_work_scheduled = 0;
+ spin_unlock(>wl_lock);
}
/**
@@ -657,7 +660,10 @@ static struct ubi_wl_entry *get_peb_for_wl(struct
ubi_device *ubi)
/* We cannot update the fastmap here because this
* function is called in atomic context.
* Let's fail here and refill/update it as soon as possible. */
- schedule_work(>fm_work);
+ if (!ubi->fm_work_scheduled) {
+ ubi->fm_work_scheduled = 1;
+ schedule_work(>fm_work);
+ }
return NULL;
} else {
pnum = pool->pebs[pool->used++];
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is scheduled
If the WL pool runs out of PEBs we schedule a fastmap write
to refill it was soon as possible.
Ensure that only one at a time is scheduled otherwise we might end in
a fastmap write storm was writing the fastmap can schedule another
write if bitflips were detected.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/ubi.h | 2 ++
drivers/mtd/ubi/wl.c | 8 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 7bf4163..529dfb0 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -426,6 +426,7 @@ struct ubi_debug_info {
* @fm_size: fastmap size in bytes
* @fm_sem: allows ubi_update_fastmap() to block EBA table changes
* @fm_work: fastmap work queue
+ * @fm_work_scheduled: non-zero if fastmap work was scheduled
*
* @used: RB-tree of used physical eraseblocks
* @erroneous: RB-tree of erroneous used physical eraseblocks
@@ -531,6 +532,7 @@ struct ubi_device {
void *fm_buf;
size_t fm_size;
struct work_struct fm_work;
+ int fm_work_scheduled;
/* Wear-leveling sub-system's stuff */
struct rb_root used;
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index dc01b1f..4c02a6e 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -149,6 +149,9 @@ static void update_fastmap_work_fn(struct work_struct *wrk)
{
struct ubi_device *ubi = container_of(wrk, struct ubi_device, fm_work);
ubi_update_fastmap(ubi);
+ spin_lock(ubi-wl_lock);
+ ubi-fm_work_scheduled = 0;
+ spin_unlock(ubi-wl_lock);
}
/**
@@ -657,7 +660,10 @@ static struct ubi_wl_entry *get_peb_for_wl(struct
ubi_device *ubi)
/* We cannot update the fastmap here because this
* function is called in atomic context.
* Let's fail here and refill/update it as soon as possible. */
- schedule_work(ubi-fm_work);
+ if (!ubi-fm_work_scheduled) {
+ ubi-fm_work_scheduled = 1;
+ schedule_work(ubi-fm_work);
+ }
return NULL;
} else {
pnum = pool-pebs[pool-used++];
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
