Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
On Tue, 2014-09-30 at 10:07 +0200, Richard Weinberger wrote: > > UBI consists of subsystems. Subsystems try to be more or less > > independent, whenever possible. They expose interface functions for > > other subsystems. Of course the split is not ideal, but we do our best. > > > > * wl.c does wear-levelling. > > * wl.c does not do fastmap. > > * fastmap.c does fastmap. > > * I am unhappy seeing yet another ifdef to wl.c > > I can warp it. What I am looking forward to is to see fastmap.c and wl.c to be separated in a better way. Would your answer be "this is impossible", or "I've got some ideas"? The end result should be no #ifdefs in wl.c, or very few of them. > > * I am unhappy seeing wl.c calling 'flush_work(>fm_work)', because > > fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a > > fastmap queue baby-sitter. fastmap.c is. > > But wl.c has to trigger the deferred fastmap work as only wl.c detects when > it is > needed. I you want I can implement a ubi_fastmap_shutdown() in fastmap.c which > calls flush_work(). > But I did it in wl.c to have it balanced. wl.c triggers and stops the fastmap > work. wl.c should be responsible for wear levelling. wl.c should not be responsible for managing the fastmap pool. Architecture-wise, is the pull something above or below the WL level? I think above. In the past, the EBA layer, which sits on top of the WL layer, called the WL functions directly to get an and put the LEB. Then you added the Fastmap pool inbetween EBA and WL. And you added it to wl.c, it seems (note, I do not know the fastmap well enough, so be patient and carefully explain me things when I am wrong, please). What I think is that the pool should live in fastmap.c instead. EBA should call the pool functions, instead of the WL functions. The pool code should call the WL functions whenever it needs to refill the pool. Can something like this be done? > Some background info: > Fastmap needs to create a snapshot of the UBI state. > This is why you cannot call it within an UBI work. As UBI works can run in > parallel. My point is, let's do it in fastmap.c. Let's keep each layer as pure a possible. Right now wl.c is populated by fastmap-specific stuff too much. It is hard to make wear-levelling improvements there because of this. I want the complexity to be partitioned. I want WL complexity be in wl.c I do want to try hard to have _no_ fastmap complexity in wl.c. I am looking for a discussion about how we could do this. This patch adds a little bit, but more fastmap complexity to the WL subsystem. I am trying to block this. -- 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
On Tue, 2014-09-30 at 10:07 +0200, Richard Weinberger wrote: UBI consists of subsystems. Subsystems try to be more or less independent, whenever possible. They expose interface functions for other subsystems. Of course the split is not ideal, but we do our best. * wl.c does wear-levelling. * wl.c does not do fastmap. * fastmap.c does fastmap. * I am unhappy seeing yet another ifdef to wl.c I can warp it. What I am looking forward to is to see fastmap.c and wl.c to be separated in a better way. Would your answer be this is impossible, or I've got some ideas? The end result should be no #ifdefs in wl.c, or very few of them. * I am unhappy seeing wl.c calling 'flush_work(ubi-fm_work)', because fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a fastmap queue baby-sitter. fastmap.c is. But wl.c has to trigger the deferred fastmap work as only wl.c detects when it is needed. I you want I can implement a ubi_fastmap_shutdown() in fastmap.c which calls flush_work(). But I did it in wl.c to have it balanced. wl.c triggers and stops the fastmap work. wl.c should be responsible for wear levelling. wl.c should not be responsible for managing the fastmap pool. Architecture-wise, is the pull something above or below the WL level? I think above. In the past, the EBA layer, which sits on top of the WL layer, called the WL functions directly to get an and put the LEB. Then you added the Fastmap pool inbetween EBA and WL. And you added it to wl.c, it seems (note, I do not know the fastmap well enough, so be patient and carefully explain me things when I am wrong, please). What I think is that the pool should live in fastmap.c instead. EBA should call the pool functions, instead of the WL functions. The pool code should call the WL functions whenever it needs to refill the pool. Can something like this be done? Some background info: Fastmap needs to create a snapshot of the UBI state. This is why you cannot call it within an UBI work. As UBI works can run in parallel. My point is, let's do it in fastmap.c. Let's keep each layer as pure a possible. Right now wl.c is populated by fastmap-specific stuff too much. It is hard to make wear-levelling improvements there because of this. I want the complexity to be partitioned. I want WL complexity be in wl.c I do want to try hard to have _no_ fastmap complexity in wl.c. I am looking for a discussion about how we could do this. This patch adds a little bit, but more fastmap complexity to the WL subsystem. I am trying to block this. -- 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Am 02.10.2014 15:05, schrieb Tanya Brokhman: >> +#ifdef CONFIG_MTD_UBI_FASTMAP >> +flush_work(>fm_work); > > flush_work returns bool. It might be useful to print that value for debugging. Why would this be useful? The sole purpose of this flush is having a barrier. If flush_work() had to wait (returns true) for a pending work, fine. If where was no work (returns false) to wait for, also fine. :) 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Hi Richard,
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f491713..dc01b1f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl("close the WL sub-system");
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(>fm_work);
flush_work returns bool. It might be useful to print that value for
debugging.
+#endif
cancel_pending(ubi);
protection_queue_destroy(ubi);
tree_destroy(>used);
Thanks
Tanya Brokhman
--
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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Hi Richard,
On 9/30/2014 1:20 AM, Richard Weinberger wrote:
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f491713..dc01b1f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl(close the WL sub-system);
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(ubi-fm_work);
flush_work returns bool. It might be useful to print that value for
debugging.
+#endif
cancel_pending(ubi);
protection_queue_destroy(ubi);
tree_destroy(ubi-used);
Thanks
Tanya Brokhman
--
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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Am 02.10.2014 15:05, schrieb Tanya Brokhman: +#ifdef CONFIG_MTD_UBI_FASTMAP +flush_work(ubi-fm_work); flush_work returns bool. It might be useful to print that value for debugging. Why would this be useful? The sole purpose of this flush is having a barrier. If flush_work() had to wait (returns true) for a pending work, fine. If where was no work (returns false) to wait for, also fine. :) 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Am 30.09.2014 09:53, schrieb Bityutskiy, Artem: > On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote: >> Am 30.09.2014 08:26, schrieb Artem Bityutskiy: >>> On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: ...otherwise the deferred work might run after datastructures got freed and corrupt memory. >>> >>> How can this happend? The background thread is stopped by this time >>> already, so what are the other possibilities? And why is this >>> fastmap-only? >> >> This has nothing do to with the background thread. >> Fastmap has a work queue. If one fastmap work has been >> scheuled we have to wait for it. > > I expected a bit more explanation. But OK, here is what I think. > > UBI consists of subsystems. Subsystems try to be more or less > independent, whenever possible. They expose interface functions for > other subsystems. Of course the split is not ideal, but we do our best. > > * wl.c does wear-levelling. > * wl.c does not do fastmap. > * fastmap.c does fastmap. > * I am unhappy seeing yet another ifdef to wl.c I can warp it. > * I am unhappy seeing wl.c calling 'flush_work(>fm_work)', because > fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a > fastmap queue baby-sitter. fastmap.c is. But wl.c has to trigger the deferred fastmap work as only wl.c detects when it is needed. I you want I can implement a ubi_fastmap_shutdown() in fastmap.c which calls flush_work(). But I did it in wl.c to have it balanced. wl.c triggers and stops the fastmap work. > Most UB subsystems have the init and close function. May be adding one > for fastmap would help? Then you could flush whatever from > 'ubi_wl_close()' ? > > Historically the work queue was implemented in wl.c because wl.c was the > only user of it. What work queue are you talking about? The fastmap work queue was added by me and has nothing to do with the UBI background worker thread. > If this layout is not good enough, we should probably extend it, may be > separate work queue management out of wl.c. One thing I can think of is getting completely rid of the UBI background thread and convert it to a work queue. But I'm not sure if this would make things easier for fastmap. > But populating wl.c with macros and little "take care of this fatmap > bit" stuff is a not going to lead to better code structure. s/fatmap/fastmap. A Freudian slip? ;) I spent already a full week on refactoring that code. My goal was making ubi_update_fastmap() callable from within the wear_leveling_worker() to get rid of the fastmap work queue completely. After one week the new code was more complicated and ugly than the current one. :-\ Some background info: Fastmap needs to create a snapshot of the UBI state. This is why you cannot call it within an UBI work. As UBI works can run in parallel. The fastmap creation does many things which can sleep. Most stuff in the wear_leveling_worker() happens in atmoic context. 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote: > Am 30.09.2014 08:26, schrieb Artem Bityutskiy: > > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: > >> ...otherwise the deferred work might run after datastructures > >> got freed and corrupt memory. > > > > How can this happend? The background thread is stopped by this time > > already, so what are the other possibilities? And why is this > > fastmap-only? > > This has nothing do to with the background thread. > Fastmap has a work queue. If one fastmap work has been > scheuled we have to wait for it. I expected a bit more explanation. But OK, here is what I think. UBI consists of subsystems. Subsystems try to be more or less independent, whenever possible. They expose interface functions for other subsystems. Of course the split is not ideal, but we do our best. * wl.c does wear-levelling. * wl.c does not do fastmap. * fastmap.c does fastmap. * I am unhappy seeing yet another ifdef to wl.c * I am unhappy seeing wl.c calling 'flush_work(>fm_work)', because fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a fastmap queue baby-sitter. fastmap.c is. Most UB subsystems have the init and close function. May be adding one for fastmap would help? Then you could flush whatever from 'ubi_wl_close()' ? Historically the work queue was implemented in wl.c because wl.c was the only user of it. If this layout is not good enough, we should probably extend it, may be separate work queue management out of wl.c. But populating wl.c with macros and little "take care of this fatmap bit" stuff is a not going to lead to better code structure. -- 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Am 30.09.2014 08:26, schrieb Artem Bityutskiy: > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: >> ...otherwise the deferred work might run after datastructures >> got freed and corrupt memory. > > How can this happend? The background thread is stopped by this time > already, so what are the other possibilities? And why is this > fastmap-only? This has nothing do to with the background thread. Fastmap has a work queue. If one fastmap work has been scheuled we have to wait for it. 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: > ...otherwise the deferred work might run after datastructures > got freed and corrupt memory. How can this happend? The background thread is stopped by this time already, so what are the other possibilities? And why is this fastmap-only? -- Best Regards, Artem Bityutskiy -- 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: ...otherwise the deferred work might run after datastructures got freed and corrupt memory. How can this happend? The background thread is stopped by this time already, so what are the other possibilities? And why is this fastmap-only? -- Best Regards, Artem Bityutskiy -- 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Am 30.09.2014 08:26, schrieb Artem Bityutskiy: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: ...otherwise the deferred work might run after datastructures got freed and corrupt memory. How can this happend? The background thread is stopped by this time already, so what are the other possibilities? And why is this fastmap-only? This has nothing do to with the background thread. Fastmap has a work queue. If one fastmap work has been scheuled we have to wait for it. 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote: Am 30.09.2014 08:26, schrieb Artem Bityutskiy: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: ...otherwise the deferred work might run after datastructures got freed and corrupt memory. How can this happend? The background thread is stopped by this time already, so what are the other possibilities? And why is this fastmap-only? This has nothing do to with the background thread. Fastmap has a work queue. If one fastmap work has been scheuled we have to wait for it. I expected a bit more explanation. But OK, here is what I think. UBI consists of subsystems. Subsystems try to be more or less independent, whenever possible. They expose interface functions for other subsystems. Of course the split is not ideal, but we do our best. * wl.c does wear-levelling. * wl.c does not do fastmap. * fastmap.c does fastmap. * I am unhappy seeing yet another ifdef to wl.c * I am unhappy seeing wl.c calling 'flush_work(ubi-fm_work)', because fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a fastmap queue baby-sitter. fastmap.c is. Most UB subsystems have the init and close function. May be adding one for fastmap would help? Then you could flush whatever from 'ubi_wl_close()' ? Historically the work queue was implemented in wl.c because wl.c was the only user of it. If this layout is not good enough, we should probably extend it, may be separate work queue management out of wl.c. But populating wl.c with macros and little take care of this fatmap bit stuff is a not going to lead to better code structure. -- 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
Am 30.09.2014 09:53, schrieb Bityutskiy, Artem: On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote: Am 30.09.2014 08:26, schrieb Artem Bityutskiy: On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote: ...otherwise the deferred work might run after datastructures got freed and corrupt memory. How can this happend? The background thread is stopped by this time already, so what are the other possibilities? And why is this fastmap-only? This has nothing do to with the background thread. Fastmap has a work queue. If one fastmap work has been scheuled we have to wait for it. I expected a bit more explanation. But OK, here is what I think. UBI consists of subsystems. Subsystems try to be more or less independent, whenever possible. They expose interface functions for other subsystems. Of course the split is not ideal, but we do our best. * wl.c does wear-levelling. * wl.c does not do fastmap. * fastmap.c does fastmap. * I am unhappy seeing yet another ifdef to wl.c I can warp it. * I am unhappy seeing wl.c calling 'flush_work(ubi-fm_work)', because fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a fastmap queue baby-sitter. fastmap.c is. But wl.c has to trigger the deferred fastmap work as only wl.c detects when it is needed. I you want I can implement a ubi_fastmap_shutdown() in fastmap.c which calls flush_work(). But I did it in wl.c to have it balanced. wl.c triggers and stops the fastmap work. Most UB subsystems have the init and close function. May be adding one for fastmap would help? Then you could flush whatever from 'ubi_wl_close()' ? Historically the work queue was implemented in wl.c because wl.c was the only user of it. What work queue are you talking about? The fastmap work queue was added by me and has nothing to do with the UBI background worker thread. If this layout is not good enough, we should probably extend it, may be separate work queue management out of wl.c. One thing I can think of is getting completely rid of the UBI background thread and convert it to a work queue. But I'm not sure if this would make things easier for fastmap. But populating wl.c with macros and little take care of this fatmap bit stuff is a not going to lead to better code structure. s/fatmap/fastmap. A Freudian slip? ;) I spent already a full week on refactoring that code. My goal was making ubi_update_fastmap() callable from within the wear_leveling_worker() to get rid of the fastmap work queue completely. After one week the new code was more complicated and ugly than the current one. :-\ Some background info: Fastmap needs to create a snapshot of the UBI state. This is why you cannot call it within an UBI work. As UBI works can run in parallel. The fastmap creation does many things which can sleep. Most stuff in the wear_leveling_worker() happens in atmoic context. 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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f491713..dc01b1f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl("close the WL sub-system");
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(>fm_work);
+#endif
cancel_pending(ubi);
protection_queue_destroy(ubi);
tree_destroy(>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 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown
...otherwise the deferred work might run after datastructures
got freed and corrupt memory.
Signed-off-by: Richard Weinberger rich...@nod.at
---
drivers/mtd/ubi/wl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f491713..dc01b1f 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -2029,6 +2029,9 @@ static void protection_queue_destroy(struct ubi_device
*ubi)
void ubi_wl_close(struct ubi_device *ubi)
{
dbg_wl(close the WL sub-system);
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ flush_work(ubi-fm_work);
+#endif
cancel_pending(ubi);
protection_queue_destroy(ubi);
tree_destroy(ubi-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/
