UBI: Fastmap fixes - round one

[Richard Weinberger]

This is the first set of fastmap fixes for v3.18.
I have more in my local queue but these depend on non-trivial
fastmap changes I made. As soon they have been fully tested I'll
submitt them too.

Thanks,
//richard

[PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL
[PATCH 2/4] UBI: Fastmap: Calc fastmap size correctly
[PATCH 3/4] UBI: Fastmap: Care about the protection queue
[PATCH 4/4] UBI: Fastmap: Ensure that only one fastmap work is

[Richard Weinberger]

Fastmap can miss a PEB if it is in the protection queue
and not jet in the used tree.
Treat every protected PEB as used.

Signed-off-by: Richard Weinberger <***@nod.at>
---
drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 2b0d8d6..2853a69 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1195,6 +1195,19 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
fm_pos += sizeof(*fec);
ubi_assert(fm_pos <= ubi->fm_size);
}
+
+ for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
+ list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
+ fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
+
+ fec->pnum = cpu_to_be32(wl_e->pnum);
+ fec->ec = cpu_to_be32(wl_e->ec);
+
+ used_peb_count++;
+ fm_pos += sizeof(*fec);
+ ubi_assert(fm_pos <= ubi->fm_size);
+ }
+ }
fmh->used_peb_count = cpu_to_be32(used_peb_count);

for (node = rb_first(&ubi->scrub); node; node = rb_next(node)) {

[Artem Bityutskiy]

Post by Richard Weinberger
+
+ for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
+ list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
+ fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);

Similarly to my other posts, is it possible to not touch the WL-internal
things like 'ubi->pq[]' from fastmap.c? Can we come up with a function
at wl.c which fastmap.c could call instead?

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger
+
+ for (i = 0; i < UBI_PROT_QUEUE_LEN; i++) {
+ list_for_each_entry(wl_e, &ubi->pq[i], u.list) {
+ fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);

Similarly to my other posts, is it possible to not touch the WL-internal
things like 'ubi->pq[]' from fastmap.c? Can we come up with a function
at wl.c which fastmap.c could call instead?

Fastmap needs basically access to all internal state of UBI, which lives mostly
within wl.c
It needs to iterate over the used, free, erase, scrub RB-trees, the protection queue, etc. to
collect the exact state of all PEBs.

In C++ or Java I would pass an iterator to fastmap.c using a function in wl.c but I'm not sure
how to do such a thing in a sane way in C.

What about having a wl-fastmap.c file which acts as glue layer between fastmap.c and wl.c?

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger
Fastmap needs basically access to all internal state of UBI, which lives mostly
within wl.c

Sounds like a very strong assertion, smells a bit fishy, need the
details.

Post by Richard Weinberger
It needs to iterate over the used, free, erase, scrub RB-trees, the
protection queue, etc. to
collect the exact state of all PEBs.

When? In 'ubi_write_fastmap()' when forming the FM pool data structures?

I think you can just add a function like this to wl.c:

int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)

Yoy will give it the PEB number you are interested in, it will return
you the information you need in 'pebinfo'. The information will contain
the EC, the state (used, free, etc).

If you need just the EC, then you do not even need the ubi_wl_peb_info
data structure.

Then you just iterate over all PEBs and fill the FM pool data
structures.

Would something like this work?

Artem.

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger
Fastmap needs basically access to all internal state of UBI, which lives mostly
within wl.c

Sounds like a very strong assertion, smells a bit fishy, need the
details.

Fastmap need to know the exact state of each PEB.
I.e. is it free, used, scheduled for erase, etc...

Post by Artem Bityutskiy

Post by Richard Weinberger
It needs to iterate over the used, free, erase, scrub RB-trees, the
protection queue, etc. to
collect the exact state of all PEBs.

When? In 'ubi_write_fastmap()' when forming the FM pool data structures?

Yes.

Post by Artem Bityutskiy
int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)
Yoy will give it the PEB number you are interested in, it will return
you the information you need in 'pebinfo'. The information will contain
the EC, the state (used, free, etc).
If you need just the EC, then you do not even need the ubi_wl_peb_info
data structure.
Then you just iterate over all PEBs and fill the FM pool data
structures.
Would something like this work?

The interface would work but some work in wl.c is needed.
For example if I want to find out in which state PEB 1 is wl.c would have to
look int free free, used free, protection queue, etc.. to tell me the state. This is slow.

But we could add the state information to struct ubi_wl_entry by adding a single integer attribute called "state" or "flags".
Then wl.c can set the state every time the PEB moves between internal data structures.
I.e. upon it moves from the used to free rb tree the flag UBI_STATE_FREE will be cleared and UBI_STATE_USED set.
This will also give us a very efficient way to ensure a sane state.
In fact, I have already written such a feature. I needed it to hunt down a Fastmap bug which lead to a state where a PEB existed
in both the used tree and the protection queue. Using the ->state attribute in ubi_wl_entry I was able to add various
cheap asserts to the code and found the incorrect transaction.

The cost of this bloating the ubi_wl_entry struct by sizeof(int) bytes.
But we'll get very efficient variants of self_check_in_wl_tree() and friends.

Also the implementation of ubi_wl_get_peb_info() would be easy.
It will be mostly like:
int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)
{
...
e = ubi->lookuptbl[pnum];
fill_pebinfo(pebinfo, e->state);
...
}

Would this make you happy? :)

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger

Post by Artem Bityutskiy

Post by Richard Weinberger
Fastmap needs basically access to all internal state of UBI, which lives mostly
within wl.c

Sounds like a very strong assertion, smells a bit fishy, need the
details.

Fastmap need to know the exact state of each PEB.
I.e. is it free, used, scheduled for erase, etc...

Post by Artem Bityutskiy

Post by Richard Weinberger
It needs to iterate over the used, free, erase, scrub RB-trees, the
protection queue, etc. to
collect the exact state of all PEBs.

When? In 'ubi_write_fastmap()' when forming the FM pool data structures?

Yes.

Post by Artem Bityutskiy
int ubi_wl_get_peb_info(int pnum, struct ubi_wl_peb_info *pebinfo)
Yoy will give it the PEB number you are interested in, it will return
you the information you need in 'pebinfo'. The information will contain
the EC, the state (used, free, etc).
If you need just the EC, then you do not even need the ubi_wl_peb_info
data structure.
Then you just iterate over all PEBs and fill the FM pool data
structures.
Would something like this work?

The interface would work but some work in wl.c is needed.
For example if I want to find out in which state PEB 1 is wl.c would have to
look int free free, used free, protection queue, etc.. to tell me the state. This is slow.

Well, used and free are RB-trees, looking them up is slow.

If what you need is to go through all used and free PEBs, then you can
introduce some kind of

struct ubi_wl_entry *ubi_wl_get_next_used(struct ubi_wl_entry *prev)

function, and similar functions for the free.

I would return you the next entry (or NULL would indicate the end), and
it would take the previous entry as the input, or NULL for the first
call.

We'd need to take the locks before calling this function. This is
cleaner than what we do now, right?

Post by Richard Weinberger
But we could add the state information to struct ubi_wl_entry by adding a single integer attribute called "state" or "flags".

But there is a price - memory consumption. We do not want to pay it just
for making the inter-subsystems boundaries better, there ought to be a
better reason.

Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
cost 256KiB of RAM.

Squeezing the state into the last 2 bits a memory reference would be
another possibility, BTW. Not elegant, though...

Post by Richard Weinberger
Would this make you happy? :)

Not very, I'd save this for the last resort solution.

[Bityutskiy, Artem]

Post by Artem Bityutskiy

Post by Richard Weinberger
The interface would work but some work in wl.c is needed.
For example if I want to find out in which state PEB 1 is wl.c would have to
look int free free, used free, protection queue, etc.. to tell me the state. This is slow.

Well, used and free are RB-trees, looking them up is slow.

I wanted to say "not slow" here, sorry.

--
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,

[Richard Weinberger]

Post by Artem Bityutskiy
Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

Post by Artem Bityutskiy
If what you need is to go through all used and free PEBs, then you can
introduce some kind of
struct ubi_wl_entry *ubi_wl_get_next_used(struct ubi_wl_entry *prev)
function, and similar functions for the free.
I would return you the next entry (or NULL would indicate the end), and
it would take the previous entry as the input, or NULL for the first
call.
We'd need to take the locks before calling this function. This is
cleaner than what we do now, right?

ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
to make sure that the to be taken state snapshot is consistent.

Post by Artem Bityutskiy

Post by Richard Weinberger
But we could add the state information to struct ubi_wl_entry by adding a single integer attribute called "state" or "flags".

But there is a price - memory consumption. We do not want to pay it just
for making the inter-subsystems boundaries better, there ought to be a
better reason.
Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
cost 256KiB of RAM.

Is 128KiB PEB size still realistic on modern NANDs?
Even if, 256KiB are not much and the kernel consumes this additionally with
every new release.
But I can understand your concerns.

Post by Artem Bityutskiy
Squeezing the state into the last 2 bits a memory reference would be
another possibility, BTW. Not elegant, though...

Post by Richard Weinberger
Would this make you happy? :)

Not very, I'd save this for the last resort solution.

Okay, I'll try harder to make you happy.

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger

Post by Artem Bityutskiy
Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

Right. 2 RB-trees, and one list. The list is empty most of the time, or
contains one element.

So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to
look at the list containing very few elements.

Not that bad, I think.

Post by Richard Weinberger
ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
to make sure that the to be taken state snapshot is consistent.

I think this is fine.

Post by Richard Weinberger

Post by Artem Bityutskiy
But there is a price - memory consumption. We do not want to pay it just
for making the inter-subsystems boundaries better, there ought to be a
better reason.
Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
cost 256KiB of RAM.

Is 128KiB PEB size still realistic on modern NANDs?
Even if, 256KiB are not much and the kernel consumes this additionally with
every new release.

Right, but the point is that bigger systems use eMMC and wouldn't bother
with raw flash. Raw flash trending down the smaller systems, where a
hundred of kilobytes matters.

Post by Richard Weinberger
But I can understand your concerns.

Thanks, yes, my point is to be careful about the RAM we consume, and try
to avoid growing this data structure, an only do it if we have to.

Post by Richard Weinberger
Okay, I'll try harder to make you happy.

Well, making me happy is not the point of course :-)

Thanks!

--
Artem

[Tanya Brokhman]

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy
Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

Right. 2 RB-trees, and one list. The list is empty most of the time, or
contains one element.
So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to
look at the list containing very few elements.
Not that bad, I think.

Post by Richard Weinberger
ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
to make sure that the to be taken state snapshot is consistent.

I think this is fine.

Post by Richard Weinberger

Post by Artem Bityutskiy
But there is a price - memory consumption. We do not want to pay it just
for making the inter-subsystems boundaries better, there ought to be a
better reason.
Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
cost 256KiB of RAM.

Is 128KiB PEB size still realistic on modern NANDs?
Even if, 256KiB are not much and the kernel consumes this additionally with
every new release.

Right, but the point is that bigger systems use eMMC and wouldn't bother
with raw flash. Raw flash trending down the smaller systems, where a
hundred of kilobytes matters.

Post by Richard Weinberger
But I can understand your concerns.

Thanks, yes, my point is to be careful about the RAM we consume, and try
to avoid growing this data structure, an only do it if we have to.

Post by Richard Weinberger
Okay, I'll try harder to make you happy.

Well, making me happy is not the point of course :-)
Thanks!
--
Artem
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Hi Artem/Richard

I think your discussion here stopped being relevant to this specific
patch but went on to the fastmap feature design in general :)
This patch fixes a real bug in the current implementation of the
feature. What you're discussing requires a re-writing and re-design of
the feature. Perhaps this one can be merged and will be "fixed" later on
when you agree on how you would like FM to access WL data structures in
general?

Thanks,
Tanya Brokhman

[Artem Bityutskiy]

Post by Tanya Brokhman
Hi Artem/Richard
I think your discussion here stopped being relevant to this specific
patch but went on to the fastmap feature design in general :)
This patch fixes a real bug in the current implementation of the
feature. What you're discussing requires a re-writing and re-design of
the feature. Perhaps this one can be merged and will be "fixed" later on
when you agree on how you would like FM to access WL data structures in
general?

First of all, "re-writing and re-design of the feature" is an
overstatement. So far this is on the "cleaning things up" side of the
spectrum, closer to the "re-factoring" area.

WRT "merge the fix now and improve later" - this is a good argument for
an "inside a company" discussion, where the primary TTM is the driving
factor.

For the community TTM is a good thing, but quality comes first.

Now, if this was about a regression, one could apply time pressure on
the maintainer. But we are talking about a problem which was there from
day 0.

It is completely normal for the maintainer to push back various
hot-fixes for the problem and request some reasonable re-factoring
first. This is what I do. This is very very usual thing in the Linux
community.

So far I did not ask anything huge and unreasonable, I think. Just
cleaner inter-subsystem APIs, less of the "fastmap uses the other
subsystems' internals" kind of things.

--
Artem.

[Tanya Brokhman]

Post by Artem Bityutskiy

Post by Tanya Brokhman
Hi Artem/Richard
I think your discussion here stopped being relevant to this specific
patch but went on to the fastmap feature design in general :)
This patch fixes a real bug in the current implementation of the
feature. What you're discussing requires a re-writing and re-design of
the feature. Perhaps this one can be merged and will be "fixed" later on
when you agree on how you would like FM to access WL data structures in
general?

First of all, "re-writing and re-design of the feature" is an
overstatement. So far this is on the "cleaning things up" side of the
spectrum, closer to the "re-factoring" area.
WRT "merge the fix now and improve later" - this is a good argument for
an "inside a company" discussion, where the primary TTM is the driving
factor.
For the community TTM is a good thing, but quality comes first.
Now, if this was about a regression, one could apply time pressure on
the maintainer. But we are talking about a problem which was there from
day 0.
It is completely normal for the maintainer to push back various
hot-fixes for the problem and request some reasonable re-factoring
first. This is what I do. This is very very usual thing in the Linux
community.
So far I did not ask anything huge and unreasonable, I think. Just
cleaner inter-subsystem APIs, less of the "fastmap uses the other
subsystems' internals" kind of things.
--
Artem.

Ok, accepted. It was just a suggestion. I'm all for quality coming
first, even if you were asking for something "huge".

Thanks,
Tanya Brokhman

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy
Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

Right. 2 RB-trees, and one list. The list is empty most of the time, or
contains one element.

Actually it is the free, used and scrub tree plus the protection queue.
Then there is another place where PEBs can hide, the erase work queue.
This brings me to an issue I've recently identified and fixed in my local queue.

ubi_wl_put_peb() does the following:
if (in_wl_tree(e, &ubi->used)) {
self_check_in_wl_tree(ubi, e, &ubi->used);
rb_erase(&e->u.rb, &ubi->used);
} else if (in_wl_tree(e, &ubi->scrub)) {
self_check_in_wl_tree(ubi, e, &ubi->scrub);
rb_erase(&e->u.rb, &ubi->scrub);
} else if (in_wl_tree(e, &ubi->erroneous)) {
self_check_in_wl_tree(ubi, e, &ubi->erroneous);
rb_erase(&e->u.rb, &ubi->erroneous);
ubi->erroneous_peb_count -= 1;
ubi_assert(ubi->erroneous_peb_count >= 0);
/* Erroneous PEBs should be tortured */
torture = 1;
} else {
err = prot_queue_del(ubi, e->pnum);
if (err) {
ubi_err("PEB %d not found", pnum);
ubi_ro_mode(ubi);
spin_unlock(&ubi->wl_lock);
return err;
}
}
}
spin_unlock(&ubi->wl_lock);

err = schedule_erase(ubi, e, vol_id, lnum, torture);

If it happens that a fastmap write is needed between spin_unlock() and schedule_erase(), fastmap
will miss this PEB. Because it go already deleted from one of the RB trees or the protection queue but not
jet queued to the work list. Yes, I hit this bug in real world.

My solution is marking the ubi_wl_entry's ->state attribute with a flag like UBI_WLE_STATE_ERASE.
This way I was also able to get rid of the ubi_is_erase_work() wart.

Post by Artem Bityutskiy
So we'd look-up 2 RB-trees most of the time. Very rarely we'd need to
look at the list containing very few elements.
Not that bad, I think.

Post by Richard Weinberger
ubi_update_fastmap() takes ubi->wl_lock anyway to block any changes in the free, used, etc. trees
to make sure that the to be taken state snapshot is consistent.

I think this is fine.

Post by Richard Weinberger

Post by Artem Bityutskiy
But there is a price - memory consumption. We do not want to pay it just
for making the inter-subsystems boundaries better, there ought to be a
better reason.
Say, for an (imaginary) 8GiB NAND chip with 128KiB PEB size this would
cost 256KiB of RAM.

Is 128KiB PEB size still realistic on modern NANDs?
Even if, 256KiB are not much and the kernel consumes this additionally with
every new release.

Right, but the point is that bigger systems use eMMC and wouldn't bother
with raw flash. Raw flash trending down the smaller systems, where a
hundred of kilobytes matters.

Post by Richard Weinberger
But I can understand your concerns.

Thanks, yes, my point is to be careful about the RAM we consume, and try
to avoid growing this data structure, an only do it if we have to.

Currently I'm using an integer variable. But a char would also do it. I need only
a few flags.

What I'm trying to say is, state tracking would solve the "internal state accessing" problem in a clean and
sane way.

I gave the ubi_get_peb_state() idea a try. Unfortunately it won't work that well as expected.
The UBI Fastmap on-flash layout has a section which contains the state and EC counter of each PEB.
But the PEBs are not stored on flash starting with 0 to ubi->peb_count, they are stored by their state.
First free, then used, then scrub and finally to be erased PEBs. I've chosen this layout to avoid an
addition on-flash state attribute to keep the overall structure small.

So, writing a fastmap like:
for (i = 0; i < ubi->peb_count; i++)
ubi_get_peb_state(i);

...won't work. I need to iterate first over the free RB tree, then over the used tree, and so on.
This way I'd have to allocate memory and store the state somewhere or iterate multiple times over the same RB trees.

What about this:
Let's create a file like fastmap-wl.c, it will contain all Fastmap functions which deal with internal
wl.c stuff. In wl.c we can add:
#ifdef CONFIG_MTD_UBI_FASTMAP
#include "fastmap-wl.c"
#endif

This way we can get rid of almost all #ifdefs in wl.c and don't have to export a lot of new fastmap specific helper functions in wl.c
to global scope.
Including a c file is common practice in Linux.
A prominent example is USB. See drivers/usb/host/ehci-hcd.c.

What do you think?

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger
What I'm trying to say is, state tracking would solve the "internal state accessing" problem in a clean and
sane way.

Can you squeeze the stat to the lookup talbe? It contains pointers, so
the last 2 bits could be used for the state.

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger
What I'm trying to say is, state tracking would solve the "internal state accessing" problem in a clean and
sane way.

Can you squeeze the stat to the lookup talbe? It contains pointers, so
the last 2 bits could be used for the state.

IMHO this is beyond horrible. :)
The goal is the make things clean in terms of not accessing internal state
from fastmap.c. If the price for that is abusing pointers in magic ways
I'd say no.

Rather consider the fastmap-wl.c idea...

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy
Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

Right. 2 RB-trees, and one list. The list is empty most of the time, or
contains one element.

Actually it is the free, used and scrub tree plus the protection queue.

OK, still does not change my point: 3 trees and a list which is mostly
empty or short. Not that bad.

Besides, the used tree is the large one, contains most of the stuff. The
scrub tree is usually small, and mostly empty. The erroneous tree also
mostly empty.

So this in most of the cases, this will be about looking up a single
tree.

And again, all you need is:

1. all used
2. all scrub
3. all erroneous

Post by Richard Weinberger
Then there is another place where PEBs can hide, the erase work queue.

That's just fastmap code not doing the right thing. We should not touch
the work queue directly at all. What we _should_ do instead is to make
it empty by asking the subsystem which manages it to flush it.

1. Lock the work queue to prevent anyone from submitting new jobs while
we are in process of writing the fastmap.
2. Flush all works
3. do all the fastmap write stuff
4. Unlock

Post by Richard Weinberger
This brings me to an issue I've recently identified and fixed in my local queue.
if (in_wl_tree(e, &ubi->used)) {
self_check_in_wl_tree(ubi, e, &ubi->used);
rb_erase(&e->u.rb, &ubi->used);
} else if (in_wl_tree(e, &ubi->scrub)) {
self_check_in_wl_tree(ubi, e, &ubi->scrub);
rb_erase(&e->u.rb, &ubi->scrub);
} else if (in_wl_tree(e, &ubi->erroneous)) {
self_check_in_wl_tree(ubi, e, &ubi->erroneous);
rb_erase(&e->u.rb, &ubi->erroneous);
ubi->erroneous_peb_count -= 1;
ubi_assert(ubi->erroneous_peb_count >= 0);
/* Erroneous PEBs should be tortured */
torture = 1;
} else {
err = prot_queue_del(ubi, e->pnum);
if (err) {
ubi_err("PEB %d not found", pnum);
ubi_ro_mode(ubi);
spin_unlock(&ubi->wl_lock);
return err;
}
}
}
spin_unlock(&ubi->wl_lock);
err = schedule_erase(ubi, e, vol_id, lnum, torture);
If it happens that a fastmap write is needed between spin_unlock() and schedule_erase(), fastmap
will miss this PEB.

You say is that LEBs change while you are writing fastmap. Of course
they do. We should have a locking mechanism in place which would prevent
this.

Mat be wl_lock needs to become a mutex, or may be there should be
separate mutex.

Or may be 'ubi_wl_put_peb()' should go through the fatmap subsystem
which should be able to block it if it is writing fastmap.

Ideally, you should be able to "freeze" the state (of course for a short
time), prepare fastmap data structures in memory, unfreeze, let users to
further IO, and wirte fastmap.

This is roughly what UBIFS does when committing. It freezes all writers,
builds the commit list, unfreezes the writers, and continues the commit
process.

Now, to be that advanced, you need a journal. Without journal, you do
freeze, prepare and write, unfreeze. The end results is that writers are
blocked for longer time, but this may be good enough.

Post by Richard Weinberger
What do you think?

I think that UBI is memory consumption grows linearly with the flash
growth. Mount time was linear too, fastmap is supposed to improve this.

I know that there are people, who also reported their issues in this
mailing list, who are very concerned about UBI memory consumption.

And I think that fastmap should try really hard to not only improve the
mount time, but also not to make the memory consumption problem worse.

So yes, I understand code niceness argument. I really do. But this
should not make UBI problem to be an even worse problem.

Please, let's try much harder to go without increasing the memory
footprint.

Thanks!

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy
Well, used and free are RB-trees, looking them up is slow.

This is true but we'd have to look it up in multiple trees and the protection queue...

Right. 2 RB-trees, and one list. The list is empty most of the time, or
contains one element.

Actually it is the free, used and scrub tree plus the protection queue.

OK, still does not change my point: 3 trees and a list which is mostly
empty or short. Not that bad.
Besides, the used tree is the large one, contains most of the stuff. The
scrub tree is usually small, and mostly empty. The erroneous tree also
mostly empty.
So this in most of the cases, this will be about looking up a single
tree.
1. all used
2. all scrub
3. all erroneous

Agreed.

Post by Artem Bityutskiy

Post by Richard Weinberger
Then there is another place where PEBs can hide, the erase work queue.

That's just fastmap code not doing the right thing. We should not touch
the work queue directly at all. What we _should_ do instead is to make
it empty by asking the subsystem which manages it to flush it.
1. Lock the work queue to prevent anyone from submitting new jobs while
we are in process of writing the fastmap.
2. Flush all works
3. do all the fastmap write stuff
4. Unlock

Are you sure? Flushing all works before every fastmap write will slow UBI down.
The fastmap write is not cheap. The goal should be to make it faster.

Post by Artem Bityutskiy

Post by Richard Weinberger
This brings me to an issue I've recently identified and fixed in my local queue.
if (in_wl_tree(e, &ubi->used)) {
self_check_in_wl_tree(ubi, e, &ubi->used);
rb_erase(&e->u.rb, &ubi->used);
} else if (in_wl_tree(e, &ubi->scrub)) {
self_check_in_wl_tree(ubi, e, &ubi->scrub);
rb_erase(&e->u.rb, &ubi->scrub);
} else if (in_wl_tree(e, &ubi->erroneous)) {
self_check_in_wl_tree(ubi, e, &ubi->erroneous);
rb_erase(&e->u.rb, &ubi->erroneous);
ubi->erroneous_peb_count -= 1;
ubi_assert(ubi->erroneous_peb_count >= 0);
/* Erroneous PEBs should be tortured */
torture = 1;
} else {
err = prot_queue_del(ubi, e->pnum);
if (err) {
ubi_err("PEB %d not found", pnum);
ubi_ro_mode(ubi);
spin_unlock(&ubi->wl_lock);
return err;
}
}
}
spin_unlock(&ubi->wl_lock);
err = schedule_erase(ubi, e, vol_id, lnum, torture);
If it happens that a fastmap write is needed between spin_unlock() and schedule_erase(), fastmap
will miss this PEB.

You say is that LEBs change while you are writing fastmap. Of course
they do. We should have a locking mechanism in place which would prevent
this.

Not really. As fastmap will take the work semaphore no work is processed while
writing the fastmap. In this case the PEB is in flight between RB trees and the
work queue. Fastmap can miss it. But it won't change.

Post by Artem Bityutskiy
Mat be wl_lock needs to become a mutex, or may be there should be
separate mutex.
Or may be 'ubi_wl_put_peb()' should go through the fatmap subsystem
which should be able to block it if it is writing fastmap.
Ideally, you should be able to "freeze" the state (of course for a short
time), prepare fastmap data structures in memory, unfreeze, let users to
further IO, and wirte fastmap.

Fastmap does this already. It takes various locks to freeze the state.

Post by Artem Bityutskiy
This is roughly what UBIFS does when committing. It freezes all writers,
builds the commit list, unfreezes the writers, and continues the commit
process.
Now, to be that advanced, you need a journal. Without journal, you do
freeze, prepare and write, unfreeze. The end results is that writers are
blocked for longer time, but this may be good enough.

I think we're becoming to scientific now.

Post by Artem Bityutskiy

Post by Richard Weinberger
What do you think?

I think that UBI is memory consumption grows linearly with the flash
growth. Mount time was linear too, fastmap is supposed to improve this.

With fastmap you have anyways a higher memory consumption.
To begin with the vmalloc()'ed ubi->fm_buf which holds the complete fastmap
in memory while reading/writing it.

Post by Artem Bityutskiy
I know that there are people, who also reported their issues in this
mailing list, who are very concerned about UBI memory consumption.
And I think that fastmap should try really hard to not only improve the
mount time, but also not to make the memory consumption problem worse.
So yes, I understand code niceness argument. I really do. But this
should not make UBI problem to be an even worse problem.

You can up with the niceness arguments, and now you're suddenly extremely
focusing on the memory footprint.

Post by Artem Bityutskiy
Please, let's try much harder to go without increasing the memory
footprint.

I need to go back to the drawing board. But first I have to fix/analyze
various bugs. Then I'll maybe have time again for memory/design nicefications.
Don't await any new patches for fastmap within the next weeks.

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger

Post by Artem Bityutskiy
That's just fastmap code not doing the right thing. We should not touch
the work queue directly at all. What we _should_ do instead is to make
it empty by asking the subsystem which manages it to flush it.
1. Lock the work queue to prevent anyone from submitting new jobs while
we are in process of writing the fastmap.
2. Flush all works
3. do all the fastmap write stuff
4. Unlock

Are you sure? Flushing all works before every fastmap write will slow UBI down.
The fastmap write is not cheap. The goal should be to make it faster.

Yes, you are right. So instead, we wanna "freeze" it, and save all PEBs
the jobs in fastmap too.

Why 'ubi_write_fastmap()' only cares about the erase jobs, and saves the
PEBs from the job as "must be erased".

What about the "move" jobs?

Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
along and saves it as "must be erased" in the fastmap. Fastmap finishes
its job, PEB X gets erased, and I write my data there, so PEB X is
referred to by LEB Y. Now I have power cut. Then I attach the flash
again. Surely it is not that fastmap just erases PEB X and I lose the
contents of LEB Y?

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy
That's just fastmap code not doing the right thing. We should not touch
the work queue directly at all. What we _should_ do instead is to make
it empty by asking the subsystem which manages it to flush it.
1. Lock the work queue to prevent anyone from submitting new jobs while
we are in process of writing the fastmap.
2. Flush all works
3. do all the fastmap write stuff
4. Unlock

Are you sure? Flushing all works before every fastmap write will slow UBI down.
The fastmap write is not cheap. The goal should be to make it faster.

Yes, you are right. So instead, we wanna "freeze" it, and save all PEBs
the jobs in fastmap too.
Why 'ubi_write_fastmap()' only cares about the erase jobs, and saves the
PEBs from the job as "must be erased".
What about the "move" jobs?

There are no move jobs. Do you mean ubi->move_from/to used in wear_leveling_worker()?
How could it happen that a fastmap is written between these? IIRC everything is done
under wl_lock. And the PEBs used have to go through the pool.

Post by Artem Bityutskiy
Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
along and saves it as "must be erased" in the fastmap. Fastmap finishes
its job, PEB X gets erased, and I write my data there, so PEB X is
referred to by LEB Y. Now I have power cut. Then I attach the flash
again. Surely it is not that fastmap just erases PEB X and I lose the
contents of LEB Y?

This cannot happen. If X is erased you cannot write data do it. I must first go
thought the pool and the pool is scanned while attaching.

Thanks,
//richard

[Artem Bityutskiy]

Post by Richard Weinberger

Post by Artem Bityutskiy
Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
along and saves it as "must be erased" in the fastmap. Fastmap finishes
its job, PEB X gets erased, and I write my data there, so PEB X is
referred to by LEB Y. Now I have power cut. Then I attach the flash
again. Surely it is not that fastmap just erases PEB X and I lose the
contents of LEB Y?

This cannot happen. If X is erased you cannot write data do it. I must first go
thought the pool and the pool is scanned while attaching.

Just noticed from the code that we first add PEBs to the erase list, and
then we go an scan the pools.

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger

Post by Artem Bityutskiy
Also, say, PEB X is in the work queue waiting for erasure. Fastmap comes
along and saves it as "must be erased" in the fastmap. Fastmap finishes
its job, PEB X gets erased, and I write my data there, so PEB X is
referred to by LEB Y. Now I have power cut. Then I attach the flash
again. Surely it is not that fastmap just erases PEB X and I lose the
contents of LEB Y?

This cannot happen. If X is erased you cannot write data do it. I must first go
thought the pool and the pool is scanned while attaching.

Just noticed from the code that we first add PEBs to the erase list, and
then we go an scan the pools.

In the attach code, yes. But this does not matter.
It cannot happen that a fastmap is written with a PEB being in a pool
and the erase job list.

If you figure out such a state, please tell me. I'll fix it. :-)

Thanks,
//richard

[Richard Weinberger]

...otherwise the deferred work might run after datastructures
got freed and corrupt memory.

Signed-off-by: Richard Weinberger <***@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);

[Artem Bityutskiy]

Post by Richard Weinberger
...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?

[Richard Weinberger]

Post by Artem Bityutskiy

Post by Richard Weinberger
...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

[Bityutskiy, Artem]

Post by Richard Weinberger

Post by Artem Bityutskiy

Post by Richard Weinberger
...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 se

[Richard Weinberger]

Post by Bityutskiy, Artem

Post by Richard Weinberger

Post by Artem Bityutskiy

Post by Richard Weinberger
...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.

Post by Bityutskiy, Artem
* 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.

Post by Bityutskiy, Artem
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.

Post by Bityutskiy, Artem
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.

Post by Bityutskiy, Artem
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

[Artem Bityutskiy]

Post by Richard Weinberger

Post by Bityutskiy, Artem
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.

Post by Richard Weinberger

Post by Bityutskiy, Artem
* 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?

Post by Richard Weinberger
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.

[Richard Weinberger]

We need to add fm_sb too.

Signed-off-by: Richard Weinberger <***@nod.at>
---
drivers/mtd/ubi/fastmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 0431b46..2b0d8d6 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -24,7 +24,8 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
{
size_t size;

- size = sizeof(struct ubi_fm_hdr) + \
+ size = sizeof(struct ubi_fm_sb) + \
+ sizeof(struct ubi_fm_hdr) + \
sizeof(struct ubi_fm_scan_pool) + \
sizeof(struct ubi_fm_scan_pool) + \
(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \

[Artem Bityutskiy]

Post by Richard Weinberger
We need to add fm_sb too.

Pushed this one, thank you!

[Richard Weinberger]

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 <***@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++];

[Bityutskiy, Artem]

+ 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

[Richard Weinberger]

Post by Bityutskiy, Artem

+ 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

[Bityutskiy, Artem]

Post by Richard Weinberger

Post by Bityutskiy, Artem

+ 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 se

[Richard Weinberger]

Post by Bityutskiy, Artem

Post by Richard Weinberger

Post by Bityutskiy, Artem

+ 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