This mirrors test_runtime in test.py, which has been useful for finding
test cases that are slowing down our tests.
Though note bench.py's output is per-probe, so summing bench_runtime
would be longer than the total runtime of the bench if multiple probes
are involved. Probes can be nested, so I'm not sure this is avoidable. I
guess it's the worst-case runtime if all probes were run independently?
Also note, confusingly, bench_runtime is cumulative while bench_simtime
remains per-sample. Maybe this will help prevent interchanging the two?
This is the inverse of accumulate, returning the difference between
subsequent results. In theory accumulate(delta(x)) and
delta(accumulate(x)) are noops.
This is particularly useful for normalizing our bench n value in
scripts. It's the only value still returned as a cumulative measurement,
which is a bit inconsistent, but necessary for uniquely identifying
probe steps.
Note this matches the behavior of mods, e.g. I would expect this to not
break if ORDER is missing:
./scripts/csv.py \
-bcase='%(case)s+%(probe)s+%(ORDER)s' \
-ft=accumulate(bench_simtime, case, probe, ORDER)
Normally the expr compiler would force typechecking of ORDER, giving it
a default value of int(0) if missing, but we intentionally bypass
typechecking in enumerate/accumulate's by fields since they may be
strings.
These were copied from external benchmarks, and tweaked/simplified a
bit based on gained experience.
I mostly just wanted something to test the bench runner/scripts, with
bench_rbyd showcasing a low-level litmus benchmark, and bench_wt
showcasing a high-level throughput benchmark.
Though bench_wt has proven to be a _very_ versatile benchmark, and will
likely be the first stop for getting an understanding of high-level
performance implications.
---
Also added bench_helpers.h/c, which includes a couple helper functions:
- bench_helpers_warmup - Warm up the filesystem by writing a 1 block
file 2*block_count times. This is meant to exhaust any preerased
state, post-format lookahead buffers, etc.
- bench_helpers_usage - Find a tight bound on disk usage. This allocates
a bitmap to find the tight bound, unlike lfs3_fs_usage, which is
best-effort. However the bitmap is hidden behind BENCH_HEAP_PAUSE to
prevent messing with parallel heap measurements.
I think this is currently only possible with overlapping by/field
fields, but hiding results with conflicting by fields is not ideal.
Especially since this function is central to so many scripts:
cat test.csv
a,b,c
x,2,1
x,1,2
x,1,3
Before:
./scripts/csv.py test.csv -ba -bb -fb -fc
warning: by fields are unstable
a,b b c
x,2 2 1
TOTAL 4 6
After:
./scripts/csv.py test.csv -ba -bb -fb -fc
a,b b c
x,2 2 5
x,2 2 1
TOTAL 4 6
This solves the main issue with unstable by fields, so no more warning.
Note that some features rely on by being unique to work (added/removed
numbers, compare fields, etc). They shouldn't error, but may be
incorrect/unintuitive with conflicting by fields, so avoiding
conflicting by fields is still a good idea.
So it turns out this _can_ happen, without an in-script coding error.
Consider the behavior of a script with overlapping by/field fields:
$ cat test.csv
a,b
x,2
x,1
x,1
$ ./scripts/csv.py test.csv -ba -bb -fb
During the first fold, rows 2 and 3 will contain b=1, but during the
second fold they will have been merged, resulting in b=2.
So, relaxing to a warning for now. Maybe the table renderer should be
rewritten to avoid folding? (note diffing results may be tricky)
Having BENCH/TEST_NAND ifdefs that enable the relevant timings, but
_not_ the relevant geometry, is certainly a choice.
Defaulting to NAND geometry when BENCH/TEST_NAND is defined is more
useful, if only for minimizing confusion.
No idea how this ended up with the wrong url! I only noticed when tSE
didn't match what was in the datasheet (expected 45ms, found 50ms).
Ugh. I've been copying this url around for a while now without noticing,
so this is not the only repo that needs fixing...
Initial results with the new timing calculations looked weird. Turns
out different block sizes perform surprisingly when they all cost the
same!
Fortunately, erases are the one operation where per-byte vs per-op
timing doesn't really matter, so reverting to only per-byte timing
solves this problem. Now, erasing 2 4KiB blocks should take the same
time as 1 8KiB block, instead of twice as long.
---
Arguably, erase timing shouldn't be _strictly_ linear w.r.t. block size.
There's a reason denser storage usually ends up with larger block sizes
after all. But preventing the block size from messing with per-byte
timings is much more interesting from a filesystem design perspective.
It also matches the behavior of artificially increasing block size to
reduce block allocator pressure.
Unfortunately, this also raises concerns with read/prog timing when
varying geometry is involved... Should we stick to the per-byte timing
in such cases? Is there a better timing model out there without too much
additional complexity?
The main changes:
- Added TEST_DEFINES and BENCH_DEFINES to allow overriding the default
test/bench define header:
-DTEST_DEFINES=my_test_defines.h
Note these are VERY different from LFS_DEFINES upstream. They aren't a
typical header file, and are included multiple times with various
query macros.
It's hacky, but works surprisingly well.
Or maybe I'll just do anything to avoid having to write multiline
macros. Ugh, backslashes.
- Moved more logic into bench/test_defines.h, including everything
needed to integrate other filesystems out-of-tree.
This mostly meant moving all of the cfg initialization logic into its
own query macro (replacing the BENCH/TEST_CFG field macros).
But this also includes a bunch of small tweaks encountered while trying
to get external benchmarks running again.
The external benchmarks include several other filesystems (littlefs2,
SPIFFS, Yaffs2), and I'm hoping this injectable/queryable header thing
will do a good job at avoiding a maintenance headache. (At least a
better job than forking bench_runner.c, which was the previous
solution.)
This was already made unconditional upstream as a part of LFS_DEFINES
support.
We should try to avoid conditional definitions that depend on
unintuitive conditions (LFS3_CFG in this case). It just makes the whole
codebase more fragile.
Now -l/--list-fields includes however many results fit in 36 chars:
$ ./scripts/csv.py --list-fields test.csv
i int # 16,17,14,18,19,15,20,13,12,29,27,28,...
suite ? # bench_p26_wt
case ? # bench_p26_wt_linear,bench_p26_wt_ran...
NO_FRUNCATE int # 0
SIZE int # 2097152
SEED int # 42
The whole point of -l/--list-fields is to give a quick information dump
about what's inside a csv file, and we're already parsing everything to
try to figure out types, so why not?
Much easier to read than head:
$ head -n5 test.csv
i,suite,case,NO_FRUNCATE,SIZE,SEED,BLOCK_SIZE,FILE_SIZE,SIM_...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
This extends csv.py's enumerate/accumulate exprs with optional by field
arguments. Each set of by fields gets its own state, allowing multiple
parallel enumerates/accumulates to be processed simultaneously.
This is especially useful when the number of by field sets is unknown.
In theory you could split/merge each by field set with a separate csv.py
call, but it'd be a real pain.
Consider some bench results:
case,n,simtime
bench_rbyd,1,100
bench_rbyd,2,10
bench_rbyd,3,100
bench_btree,1,200
bench_rbyd,4,10
bench_btree,2,20
bench_btree,3,2000
bench_btree,4,200
It was a bit awkward to handle these with csv.py's accumulate, as
accumulate operated strictly per-row, ignoring the case field.
But now with optional by fields:
$ ./scripts/csv.py test.csv \
-bcase -bn \
-fsimtime='accumulate(simtime, case)'
case,n simtime
bench_btree,1 200
bench_btree,2 220
bench_btree,3 2220
bench_btree,4 2420
bench_rbyd,1 100
bench_rbyd,2 110
bench_rbyd,3 210
bench_rbyd,4 220
TOTAL 5700
Note that these by fields are a bit special in csv.py's grammar. So far,
they are the only fields in field exprs that aren't typechecked. The
alternative would be string types in csv.py, but I'm not sure I want to
go that far.
---
It's tempting to try to invert this logic (accumulate(simtime, n)), but
I'm not sure how it would work internally. The duplicate by fields
("case") do get annoying, but specifying them in the expr helps make the
relevant state explicit.
Keep in mind we don't evaluate the actual by fields until much later in
csv.py. Entangling these stages risks confusion (-ba='%(b)s'
-c='enumerate(n)'? hidden by fields? overlapping by+field fields?).
I mean, what would you expect this to do?
max(a) + sum(b)
Whatever your answer is, it's wrong (the way csv.py works, we always
compute folds after expr evaluation). The best option is to error,
matching the behavior of mismatched types.
csv.py's -L/--list-computed was returning some confusing types:
$ ./scripts/csv.py /dev/null -fa='float(1)' -L
a int sum
^-- huh!?
Turns out csv.py's fold typechecking was all broken. Folds can change
the type, but only at the invocation:
$ ./scripts/csv.py /dev/null -fa='sum(float(1))' -L
a int sum
$ ./scripts/csv.py /dev/null -fa='avg(int(1))' -L
a float avg
$ ./scripts/csv.py /dev/null -fa='int(avg(1))' -L
a float avg
This is maybe defensible for explicit folds, since their evaluation is
also lifted, but not so much for things like literals/fields/etc.
---
Fixed by allowing None to indicate a generic fold, and allowing types to
be lazily figured out in csv.compile.
- Off-by-one in test_btree_find_general[_sparse]_fuzz
Because we can only create named btrees via splitting, these always
start with one entry. If all operations are randomly selected to be
splits, this can lead to an overflow of the sim buffer (sounds
unlikely, but relatively easily for small N).
The fix is to use a `for (lfs3_size_t i = 1; i < N; i++)` loop to
account for the initial entry. Note we already use this in the
test_btree_split_* tests.
An alternative is allocating space for N+1 entries, but this seems
unintuitive with N usually being associated with the upper bound on
btree size.
- Off-by-one in our sim rename pattern
When renaming, we don't bother to update sim_size, because after the
rename the sim_size size will be unchanged. But this means the
sim_size is out-of-date during the memmove that reinserts the renamed
entry. Buffer overflow!
To fix we just need to use sim_size-1 to account for the temporarily
deleted entry.
This is messy C code, so not surprised it went unnoticed, even though
this pattern ended up in quite a few tests.
Found while running with HEAP=1. This was just intended to test HEAP=1,
but I guess the injected heap hooks result in a more fragile heap? They
increase all allocations by one word, and maybe this reduces alignment
padding? Not exactly sure.
But it's a good argument for maybe adding heap canaries in the future.
Previously we ran Valgrind on all tests, but it's unclear if this will
still be reasonable with the number of tests we have now.
By "simple" I mean any non-globbing suite/case ids.
Non-globbing suite/case ids can be filtered early in the test_runner.
But globbing ids require, surprise, globbing, which is currently handled
by test/bench.py.
---
This greatly speeds up valgrind testing of specific suites/cases,
otherwise things get bogged down during the initial --list-cases due to
the sheer number of test permutations we've accumulated.
Maybe we shouldn't be running the initial --list-cases under Valgrind,
but oh well. This mostly solves the problem without too many changes.
These have been battle-tested in external benchmarks, and have proven
useful for finding a runtime estimate on stack+heap usage.
Of course, to be realistic they need to be cross-compiled and run under
QEMU (which does work!), but even on x86_64 they provide a nice insight
into RAM usage. In practice the only real difference is pointer width
anyways.
---
Enabled by default for the bench runner, these are available if
TEST/BENCH_YES_HEAP and/or TEST/BENCH_YES_STACK are defined.
(This default is provided by the Makefile. At least heap measurements
rely on linker flags, so it probably doesn't make sense to default
enable in the bench runner itself.)
Stack vs heap rely on slightly different mechanisms:
- Stack: Uses GCC's __builtin_frame_address(0) to measure the current
stack usage on entry to every bd operation.
- Heap: Relies on GCC's -Wl,--wrap flags to intercept every malloc/free
call, to track the current heap usage.
These are available via bench/test macros:
- BENCH_STACK() - Maximum stack usage of the current run
- BENCH_STACK_CURRENT() - Current stack usage
- BENCH_HEAP() - Maximum heap usage of the current run
- BENCH_HEAP_CURRENT() - Current heap usage
Note BENCH_STACK_CURRENT() can be useful for separating out the bench's
ctx from total stack usage, similarly to our static analysis.
---
One surprising outcome is that these heap hooks trivially implement a
memory leak detector. Maybe that could be useful in the test_runner as a
cheaper alternative to Valgrind?
kiwibd has been used extensively in external benchmarks, it makes sense
to make it the default bd for the bench runner:
- test_runner - defaults to emubd - more testing features
- bench_runner - defaults to kiwibd - lighter-weight disks
The benefit of kiwibd is the disk is just one big blob of RAM, so
basically no overhead. This is important when benchmarking on multi-GiB
disks.
emubd is much heavy, but as a tradeoff can do quite a bit more:
bad-block simulation, wear simulation, snapshotting, etc.
---
In theory the bd used by each runner can be controlled at compile-time
by defining -DBENCH_EMUBD, etc, but I have a feeling no one will ever
use this.
This needed a different name, and "bench probe" is sort of reminiscent
of the "debug probes" you can use to measure things in the real world.
Maybe this is just my embedded engineering background poking through,
but honestly anything is better than a single char m, especially for a
non-integer field.
These seem useful enough to have shortform flags:
- -l/--list-fields - Input fields before processing
- -L/--list-computed - Computed fields and expr dependencies
Note while -L/--list-computed has more information, it's also more
likely to trigger an assert/error due to poorly implemented field exprs.
On one hand, only inferring the used input fields is conceptually
correct because that's how csv.py works. On the other, it doesn't really
make sense for --list-computed to show _less_ information than
--list-fields.
So, showing all inferred types now:
$ ./scripts/csv.py --list-computed test.csv \
-bcase='%(case)s+%(m)s' \
-fsimtime='float(bench_simtime)/1.0e9' \
-fsimthroughput='float(n)/max(float(bench_simtime)/1.0e9,1.0e-9)'
i int .--> case ? ?
suite ? |.-> simtime int sum
case ? -+|.> simthroughput int sum
SKIP_WARMUP int |||
FILE_SIZE int |||
SEED int |||
...
m ? -'||
n int ---+
bench_reads int ||
bench_progs int ||
bench_erases int ||
bench_readed int ||
bench_progged int ||
bench_erased int ||
bench_simtime int ---'
I think this makes --list-computed a strict superset of --list-fields
now.
I was expecting -ba -Fa to sort numerically, but it was not. Turns out
hidden field fields (-F/--hidden-field) without exprs were never
typechecked.
This is not an issue for non-hidden field fields (-f/--field), because
we typecheck these explicitly in compile.
Found from some confusing behavior when by/from fields overlap. It turns
out when this happens (-bhi -Fhi, for example), the generated getattr
for the by field would trigger the __getattribute__ for the overlapping
field field, resulting in a fold on _every add operation_.
Hopefully you can see where this is a bit of a problem when summing a
large number of results (O(n^2)?).
---
Fixed by switching getattr to object.__getattribute__ and reconsidering
csv.py's entire design.
Now that we have csv.py's accumulate(), this information is strictly
redundant!
$ ./scripts/csv.py test.csv \
-bspecific_permutation_here \
-fbench_creaded='accumulate(bench_readed)'
The point of adding accumulate() was to drop these. We really shouldn't
be doubling the size of the csvs with redundant/derivable data.
This was a funny issue for external benchmarking, where we've focused
mostly on throughput benchmarking so far.
The current throughput approach is to run a benchmark for a given
simtime, and record the number of bytes written after. This is great for
allowing benchmarks to fail gracefully, but doesn't really work with the
current bench runner, which expected a known n in BENCH_START.
We can work around this by calling BENCH_START/STOP a second time
(making a mess of later scripts), but it would be nice if this was fixed
in the bench runner.
---
Humorously, BENCH_START just stores n to be printed out when BENCH_STOP
is called, so this was an easy fix.
Less useful than --list-fields, but fun.
This shows more of the internal expr eval info: input fields + types,
output fields + types + folds, and a small dependency graph showing what
goes where:
$ ./scripts/csv.py --list-computed test.csv \
-bcase='%(case)s+%(m)s' \
-fsimtime='float(bench_simtime)/1.0e9' \
-fsimthroughput='float(n)/max(float(bench_simtime)/1.0e9,1.0e-9)'
i ? .--> case ? ?
suite ? |.-> simtime int sum
case ? -+|.> simthroughput int sum
SKIP_WARMUP ? |||
FILE_SIZE ? |||
SEED ? |||
...
m ? -'||
n int ---+
bench_reads ? ||
bench_progs ? ||
bench_erases ? ||
bench_readed ? ||
bench_progged ? ||
bench_erased ? ||
bench_simtime int ---'
Maybe I was just itching to write another ascii-art renderer.
One issue I keep running into with csv.py is that it's difficult to get
started with a new/unfamiliar csv file.
csv.py itself doesn't know what to do until you start specifying fields,
but you can't start specifying fields until you know what fields there
are. Add to this the fact that our csv files have so much info shoved in
them that their "human readability" is mostly theoretical.
The --list-fields flag provides a quick solution to this:
$ ./scripts/csv.py --list-fields test.csv
i int
suite ?
case ?
SKIP_WARMUP int
FILE_SIZE int
SEED int
...
csv.py doesn't have much info at this stage, but we can at least include
the best-effort type guessing we use for field exprs.
Now that we have the enumerate expr, -i/--enumerate can be implemented
entirely during expr eval:
- -i/--enumerate => -bi -Fi=enumerate()
- -I/--hidden-enumerate => -Bi -Fi=enumerate()
Instead of internally reimplementing the same behavior.
This is what our help text implies, so might as well put our money where
our mouth is. And the less special internals we have, the better.
I considered removing -i/-I completely, but it's quite a convenient flag
when debugging csv.py expressions.
In an effort to move away from magic usage of -i/--enumerate, this adds
an explicit z field for differentiating -r/--hot results (and for normal
recursive results).
Instead of trying to think of a new flag to control this, this just
piggybacks on -Z/--children, which now accepts a tuple:
- ./scripts/csv.py -z3 -Z
- ./scripts/csv.py -z3 -Zchildren
- ./scripts/csv.py -z3 -Zz,children
The only tricky bit was needing to insert z in front of the by fields,
otherwise it was mostly a simplification from the enumerate mess.
Another positive side-effect: -r/--hot (and -z/--depth) now implies
-Zz,children, removing the annoying/confusing behavior of hotify folding
results by default.
The current... attempt at an approach was broken and becoming horribly
unmaintainable. Two issues found without even looking:
1. Field inference didn't understand prefixes, leading to duplicate
by/field fields when attempting to infer by fields with --prefix.
2. Sort wasn't working for some reason, probably because they behavior
of sort, defines, etc are really weird since they apply to both by
fields and field fields.
I considered just dropping support for --prefix completely, this really
isn't worth the time, but instead found a simple solution of moving
prefix handling to one of the first steps in collect_csv.
This has the downside of creating conflicts when a prefixed/non-prefixed
field has the same name, but I don't care. --prefix is a niche flag that
shouldn't mess with the rest of the code like this, and none of the
other scripts really handle field conflicts correctly anyways.
- Fixed the initial filter using explicit 'children'/'notes' literals
Whoops, how did this happen?
- Fixed fold using default children/notes result attributes
This one is a bit more excusable, self.children is easy to overlook.
But not actual string literals, that's silly.
This adds two new exprs to csv.py, useful for sequential data:
enumerate() A number incremented each result
accumulate(a) A running sum across results
To make these work required adding support for cross-row state, thus the
new state field in CsvExpr.Expr.eval.
Once you have that cross-row state, implementing enumerate/accumulate is
pretty straightforward. The only complication being that we need to hash
state by the unique Python id (`id(self)`), otherwise multiple exprs
would share state, which would be pretty weird.
Note that csv.py's pipeline is now quite complex, and stage order is
important!
input --> define --> expr --> folding --> sorting --> output
filtering eval
As a result, it's unfortunately not possible to organize enumerate/
accumulate by by fields. I poked around with the idea but decided it was
too complex (aren't I supposed be building a filesystem?). The guiding
principle behind csv.py is most problems can be solved with more process
substitution.
---
This is a bit clunky since we can't use the existing fold system, but
csv.py is already a pile of hacks, so what's one more?
The reason for the clunkiness is that the original idea behind csv.py
was to treat each folded row independently and order-agnostic. Not the
greatest idea in hindsight, cross-row operations are useful!
The idea here is to add some sort of accumulate operation to csv.py, so
we can stop cumulative-result clunkiness. It would also be immensely
useful as a general function, and -i/--enumerate already sets a
precedent for this sort of cross-row behavior.
But I'm starting to think using flags here is not the best way, maybe
this would be better as a field expr?
For some reason -F/--hidden-field fields weren't being parsed as a
CsvExpr, breaking any attempt to use exprs with hidden fields. Probably
just broken during a refactor.
Fortunately an easy fix.
The value of simtime isn't actually the simtime value, but the
simulated throughput, which is easy enough for our csv.py script to
calculate (with a daintily placed max to avoid divide-by-zero).
Throughput has the benefit of being somewhat size-agnostic, making
cross-benchmark comparisons easier.
I guess it's technically possible to do something similar with
readed/progged/erased numbers, but conceptually that would be really
confusing...
---
Also renamed test/bench_time -> test/bench_runtime to hopefully prevent
confusion between the two time spaces.
This is based on some work in external benchmarks. What's worked well
there is emulating a global simtime based on per-byte estimates.
This moves the emulated simtime into emubd/kiwibd, and extends the idea
with both per-byte and per-op timing estimates for hopefully more
realistic results.
---
The problem is how NAND flash reads work.
Per-byte timing estimates are surprisingly accurate for NOR flash. There
is some overhead for sending the address, but it's mostly dominated by
bus cost (~20ns/B [1]).
NAND flash, on the otherhand, technically does support byte-level reads,
but first needs to read into 2KiB buffer. Surprisingly, these are pretty
close in cost (~19ns/B bus [2] vs ~12ns/B buffer [2]).
This close-ness makes modeling NAND flash difficult. If we set
read_size=1, we risk hiding the cost of small reads, which littlefs3 is
full of (rbyd lookups). If we set read_size=2048, we unfairly penalize
littlefs3 for the same reason.
---
The solution here is to expose both per-byte and per-op timing
estimates. This lets you model NAND reads using two data points:
^
| realtime --> ...............o
| : .....'''' :
| ...............:'''' ^ :
| :....''''' | :
| ..........:::::: simtime :
| .....:'''' :
|o....:::::.....: :
|: :
|: :
+:-----------------------------------------------------------:>
min read max read
Where:
bus_timing = 19ns
buffer_timing = 25us
buffer_size = 2KiB
erase_size = 128KiB
min_read = buffer_timing
max_read = (erase_size/buffer_size)*buffer_timing - buffer_timing
read_timing = min_read
readed_timing = ((max_read - min_read)/erase_size) + bus_timing
simtime = reads*read_timing + readed*readed_timing
(per-op) (per-byte)
This should correctly penalize small reads without complicating
emubd/kiwibd too much.
That's the idea anyways! It will take some use to understand if this is
a reasonable approach.
As a plus, this is a superset of the per-byte model, so both can be used
for realistic vs idealistic simulations (and to test the bus+buffer
model itself).
1: https://www.winbond.com/resource-files/W25Q256JV%20SPI%20RevQ%2002072025%20Plus.pdf
2: https://www.winbond.com/resource-files/W25N01GV%20Rev%20R%20070323.pdf
The big TEST_IMPLICIT_DEFINES and TEST_CFG macros have been a big
pain-in-the-ass to maintain. Mostly due to C preprocessor annoyances
(bleh escaped newlines) and no-ifdef workarounds, which make a real mess
of things.
This does two things:
1. Moves all the defines out of test_runner.h and into test_defines.h
(same for benches).
2. Inverts the include logic such that test_defines.h gets included many
times with various "query macros" defined.
Currently just two, but can easily add more:
1. TEST_DEFINE(name, value) - name and default value for a define
2. TEST_CFG(name, value) - name and value for a cfg field
This seems to work surprisingly well. It solves all of the above C
preprocessor issues, and provides a flexible method for defining test
defines.
Note an important part of making this work is that test_defines.h
expands to an empty string by default.
I just noticed we weren't testing preerase with non-0xff ecksums at all!
Added to relevant tests:
# test with a number of different erase values
defines.ERASE_VALUE = [0xff, 0x00, -1]
The most important non-0xff value being -1 (noop erases), which should
usually result in fragmented ecksums.
Note this was copied from test_rbyd, where we do something similar to
test non-0xff ecksums in rbyd logs.
Based on what we implemented for the preerase gbmap known window check,
adding one block_count before mod is way simpler than a negative-friendly
mod in C.
Saves a bit of code:
code stack ctx
before: 35260 2136 660
after: 35256 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38616 2144 776
gbmap after: 38612 (-0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39280 2168 796
preerase after: 39276 (-0.0%) 2168 (+0.0%) 796 (+0.0%)
This gets gc tests working with both LFS3_GC=1 and LFS3_PREERASE=1, and
adds a few more tests that should round out the necessary preerase test
coverage:
- test_gc_preerase_progress - A simple test that checks if
LFS3_GC_PREERASE clears the LFS3_I_PREERASE flag, as well as some
checks against emubd's erase counters to see if it actually did
anything (erased >= cycles - preerased, erased < 1.25*cycles -
preerased).
- test_gc_preerase_relaxed - A test with a couple different
GC_PREERASE_COUNTs, and checks against emubd's erase counters to make
sure they demonstrate different levels of pre-erasing (erased >=
cycles - preerased, erased < 1.25*cycles - preerased).
- test_gc_preerase_decreasing - A test with increasing
GC_PREERASE_COUNTs, measuring min/max/avg emubd's erase counters, and
asserting if the avg delta is worst than ~0.75x.
This is probably the most valuable one, if only for the extra analysis
available when debugging.
And, just so we know these tests are working, they found a few more bugs:
- We were calling the implicitly ckpointing variant of lfs3_mdir_commit
in lfs3_allocclaim, when the block we just allocated is still very
much in-flight!
An easy one-character fix (lfs3_mdir_commit -> lfs3_mdir_commit_, the
non-ckpointing variant), but was a pain to track down. I guess the
good news is test_gc_nospc has proven to be a very valuable test.
Added a comment to hopefully discourage a regression.
- Found a wacky catch-22 where the block we just preerased can be
allocated during the gbmap commit that tries to save the preerased
ecksum.
This is somewhat expected during normal operation, the gbmap may need
a few allocations before the preeraser can get ahead, but we need to
make sure not to increment the preeraser's known window if the
preerased block is no longer in the gbmap's known window.
Fortunately(?), our preeraser state is pretty robust to bugs like this
due to being reset (forcing ecksum refetches) during gbmap rebuilds.
However, preeraser state falling out-of-sync risks unnecessary
erases/surprising latency during block allocation.
- Found a typo where we used lfs3->cfg->block_count instead of
lfs3->block_count again... Hopefully this becomes impossible after the
planned config rework...
---
A few other test tweaks:
- Added LFS3_F/M_REVPERTURB flags where necessary to support PREERASE.
Previously the tests only worked with LFS3_YES_REVPERTURB=1.
- Adopt lfs3_handle_isopen over lfs3.handles == lfs3.gc.t.h. With the
logic change to use the traversal handle to track its position in the
open file handles, these simplified isopen checks no longer work.
- Prefer toml lists for multiple ifdefs (hey, these were at least useful
for testing test.py's ifdef exprs).
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38616 2144 776
gbmap after: 38616 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39232 2168 796
preerase after: 39280 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
- Fixed lfs3_alloc_cansyncgbmap ignoring known window changes.
Being able to just call lfs3_btree_cmp(b, b_p) would be nice, but this
ignores known window changes!
Fixed by comparing the on-disk encoding, which is heavy-handed, but
probably the safest approach.
lfs3_alloc_cansyncgbmap will probably never be on the stack hot-path,
and the added code is roughly one function call. The main cost is
CPU-cycles, but fortunately(?) that's not something we really care
about?
- Fixed lfs3_allocclaim accidentally returning lfs3_mdir_commit's return
value instead of the allocated block!
Probably caused by a copy-paste, resulted in lfs3_allocclaim returning
block 0, which is really not good!
- Fixed assert typo in lfs3_trv_open where we assert REVPERTURB in flags
instead of lfs3->flags.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38560 2144 776
gbmap after: 38616 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39168 2168 796
preerase after: 39232 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
- Delayed defines/permutations assignment until after generation. Just a
bit of code smell.
- Expanded all __eq__, __ne__, __lt__, __gt__, etc magic methods, just
to minimize surprises in the future.
This extends our ifdef/ifndef test attributes to support more
complicated logic expressions.
So far we haven't really needed this (ifdef/ifndef accepts an implicitly
anded list, which has covered everything so far), but I realized there's
a simple trick to make this work.
For example, in test.toml:
ifdef = 'A && !(B || C)'
Generated ifdef:
#if (defined(A) && !(defined(B) || defined(C)))
This doesn't require complex parsing or anything, just a simple regex:
s/[a-zA-Z_0-9]\+/defined(&)/g
Is using #if defined(A) everywhere instead of #ifdef A more expensive
for the compiler? Not sure. But it seems like we're heavily dominated by
the single-threaded link time, so I'm not sure we care.
Still needs testing with LFS3_GC=1, and tests that intentionally test
preerasing, but this should at least fix most fsinfo.flag related issues.
Despite no intentional preerase testing, this already found a number of
issues. Most importantly: our ckpoint-agnostic gbmap zeroing was never
going to work with preerasing!
Main fixes:
- Adopted conservative zeroing of gbmap during rebuilds
This was the biggest change. Our previous lfs3_gbmap_zero impl was
never going to work with preerasing because it unconditionally
cleared BMERASED ranges.
Not entirely wrong, but a big waste of any preerase work.
It also causes the whole system to lock up when LFS3_GC_LOOKAHEAD and
LFS3_GC_PREERASE fight to make progress. With LFS3_GC_LOOKAHEAD
clearing BMERASED ranges, and LFS3_GC_PREERASE clearing the lookahead
flag, nothing gets done!
---
The fix was to rewrite lfs3_gbmap_zero[unknown] to only zero BMERASED
(and BMINUSE, though this isn't strictly necessary) ranges in the
unknown window. This keeps any known-preerased blocks around and
avoids throwing that information away.
This is also slightly different from BMBAD ranges, which we want to
keep around forever, even if in the unknown window.
Whether or not was should limit zeroing BMINUSE ranges is an
interesting question. If we already need this logic, I think extending
it to BMINUSE is a good idea because of how it limits gbmap commits
during rebuilds:
- Unfortunately, gbmap rebuilds require quite a few commits to both
(1) zero gbmap state, and (2) set all the in-use blocks to BMINUSE.
This is especially concerning when relying on aggressive gc, such as
gc_lookgbmap=-1, which may trigger rebuilds when only a couple
blocks are allocated.
Limiting zeroing limits gbmap commits in two ways:
1. We only need to update ranges in the unknown window, which
shrinks with more aggressive gbmap rebuilds.
2. By not clearing BMINUSE ranges in the known window, populating
those blocks during the lookgbmap scan should be a noop.
Together, this hopefully makes aggressive gbmap rebuilds relatively
cheap, at least in terms of progs/erases.
- It's slightly simpler if BMINUSE and BMERASED are handled the same.
- Actually increment the preeraser known window in lfs3_alloc_inc.
Otherwise our estimated preeraser.count only ever increases! There was
some trickiness to make sure preeraser.count is only ever decremented
when allocating erased blocks, but fortunately lfs3->gbmap.ecksum's
existence can tell us that information.
- Reset preeraser state during gbmap rebuilds.
Also necessary to avoid unbounded preeraser.count. The simplest
solution is to zero the preeraser, which forces it to rescan the gbmap
for BMERASED ranges. The preeraser strictly avoids redundant erases.
This does require extra gbmap lookups during LFS3_GC_PREERASE, but
that's not the end of the world.
- Avoid erasing corrupted preerased blocks in case there's other
preerased blocks available in our gbmap.
This happens when the ecksum check fails, implying a prog was
attempted, but power was lost.
Before this change (the continue in lfs3_alloc_:11244), we were
erasing corrupt ecksums, which is not _wrong_, but sort of defeats the
purpose of prerasing. Skipping the block and trying another:
1. Is better in terms of wear-leveling (try not to double erase!)
2. Minimizes latency if we have other preerased blocks we can use
- Made lfs3_fs_gc_ preerasing actually conditional on the
LFS3_GC_PREERASE flag.
Before, lfs3_fs_gc_ was unconditionally preerasing, which is wrong!
---
Currently passing:
LFS3_YES_GBMAP=1 \
LFS3_YES_REVPERTURB=1 \
LFS3_PREERASE=1 \
make test-runner -j \
& ./scripts/test.py -j -b
Other test fixes:
- Mostly just adding the necessary LFS3_I_PREERASE flags for all
lfs3_fs_stat calls.
- LFS3_I_PREERASE and LFS3_I_LOOKAHEAD can interact in funny ways. Just
needed testing.
- lfs3_trv_t doesn't actually do anything with LFS3_T_PREERASE, so we
shouldn't try to test it.
- Adopted lfs3_fs_ck instead of explicit traversals where possible.
- test_badblocks_*_btree_many was still running with LFS3_YES_GBMAP, but
it shouldn't be. The gbmap state is undefined during internal btree
tests.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38492 2144 776
gbmap after: 38560 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39036 2168 796
preerase after: 39168 (+0.3%) 2168 (+0.0%) 796 (+0.0%)
This started with adding -d/--diff support to dbgflags.py, which is very
useful for comparing flags during test failure.
The flag asserts in our tests generally look like this:
tests/test_mount.toml:171:assert: assert failed with 33570064,
expected eq 33570576
assert(fsinfo.flags == (
Which can now be quickly compared with dbgflags.py:
$ ./scripts/dbgflags.py +i 33570064 -d 33570576
LFS3_I_GBMAP 0x02000000 Global on-disk block-map in use
LFS3_I_REVPERTURB 0x00000010 Mounted with LFS3_M_REVPERTURB
LFS3_I_MKCONSISTENT 0x00000100 Filesystem needs mkconsistent to write
-LFS3_I_LOOKAHEAD 0x00000200 Lookahead buffer is not full
LFS3_I_PREERASE 0x00000400 Blocks can be pre-erased
LFS3_I_COMPACT 0x00000800 Filesystem may have uncompacted metadata
LFS3_I_CKMETA 0x00001000 Metadata checksums not checked recently
LFS3_I_CKDATA 0x00002000 Data checksums not checked recently
The assert print is a bit more annoying than it needs to be, as it only
prints in decimal. But, since our prettyasserts.py only works at the
syntax layer, it's not possible to make it any smarter.
---
To make this diffing work required a couple more features in our
self-parsing Flag class:
- Keep track of lineno, mainly for ordering things
- Moved find logic into a staticmethod on all classes
- Added _sentinel based defaults to find functions
- Allowed self to be non-class in line functions to deduplicate "Unknown
flag" messages
I went ahead and extended these to the other self-parsing classes (Err
and Tag) in case they're useful in the future.
This was broken. The good news is this was easily detected by our
test_mtree tests.
The problem is that mroot split + drop (resulting in an mtree with one
mdir) is indistinguishable from mroot relocation via mdelta. Both cases
have an mdelta of 0.
This also breaks the later mid-mdir update if we wanted to stay on the
current chain mroot.
The tricky part is we have several entangled cases:
- mdir=active mroot, mid>=0 - follow mdir_
- mdir=active mroot, mid<=-1 - follow mroot_, not mdir_!
- mdir=chain mroot, mid<=-1 - follow mdir_, not mroot_!
---
It's tempting to rely on mid<=-2 vs mid==-1 for chain mroots vs active
mroot, but this doesn't always work! During traversals mid is always
<=-2, in part because we don't actually know if the current mroot is the
active mroot until we try to lookup its child.
Fortunately, what _does_ work is just comparing against the mroot's
blocks, which we know.
Though the continued reliance and reliability of mptr comparisons makes
me wonder if it's possible to simplify said function...
Code changes:
code stack ctx
before: 35224 2136 660
after: 35260 (+0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38464 2144 776
gbmap after: 38492 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39008 2168 796
preerase after: 39036 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
Christopher Haster