Refactor TDB internal bounds computation #11629

kyle-cypress · 2019-10-04T01:04:37Z

Summary of changes

Refactor internal TDB flash bounds computation to reduce code duplication and to provide more viable default values on parts where the program and erase block sizes are close or identical in value.

The default TDBStore internal flash size is determined as the larger of two flash sectors or 14 flash pages (it previously defaulted to two sectors). This increases the usability of the default size computation on devices where the flash sector and page sizes are close or equal to each other.

Impact of changes

Applications which use TDBStore (or another class which wraps it), and do not explicitly specify a value for internal_size in storage_tdb_internal, will consume additional flash space for TDBStore.

Migration actions required

If an application does not already specify internal_size for storage_tdb_internal in its mbed_app.json, and the increased storage is not desired, add an entry to mbed_app.json to explicitly specify the desired size.
If an application already specifies internal_size for storage_tdb_internal in its mbed_app.json, no changes are required.

Documentation

N/A

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2019-10-04T03:00:24Z

@kyle-cypress, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

JanneKiiskila · 2019-10-04T09:47:41Z

Attn @SeppoTakalo @ARMmbed/team-st-mcd

VeijoPesonen · 2019-10-07T05:02:10Z

features/storage/kvstore/tdbstore/TDBStore.cpp

+    MBED_ASSERT(curr_addr >= first_wrtbl_sector_addr);
+    if (curr_addr < first_wrtbl_sector_addr) {
+        ret = MBED_ERROR_MEDIA_FULL;
+    } else {
+        *start_address = curr_addr;
+    }


@SeppoTakalo should we maybe use MBED_ERROR here? To replace all the highlighted lines.

Would be too big API change.. maybe for the OS version 6.

VeijoPesonen · 2019-10-07T05:03:35Z

features/storage/kvstore/conf/kv_config.cpp

    }

-    //Check that internal flash has 2 or more sectors
-    if (_calculate_blocksize_match_tdbstore(kvstore_config.internal_bd) != MBED_SUCCESS) {
+    //Check if TDBStore has at least 2 sector.


*s or 10 pages.

Why would 10 pages work instead of a full sector? I think TDBSTore uses the flash sectors to do the garbage collection, in sort of A vs. B, one of them being active and the other one as the spare?

Yes, that matches my understanding of TDBStore's garbage collection - it divides the available flash area in half and alternates between the two to do copying GC.
The problem is that the default approach breaks down when the sector size and page sizes are the same (or close), because many of the KVStore operations consume an entire page. Based on our analysis of the KVStore greentea tests as a proxy for typical usage, the minimum page count appeared to be around 10 i.e. 5 pages in active use, 5 in reserve for GC (or 2 sectors if that is larger, so that GC continues to work). If there's a different number of pages that more directly corresponds to something intrinsic in the driver I'd be happy to go with that instead.

@VeijoPesonen, with regard to:

*s or 10 pages.

Wouldn't expanding this check produce a backwards compatibility concern? Or does this qualify as a situation where it would probably have always failed, just in a less explicit manner, and therefore it is okay to add a new error condition?

@VeijoPesonen, can you please confirm that the expanded check is not a backwards compatibility concern?

Hi, was browsing some PRs randomly, sorry for jumping in.

The number of pages TDBStore needs depends on the number of KV pairs+1. So for 4 KV pairs it needs 5 pages. (Actually it may need 2 pages per KV pair, not sure).

Unfortunately I think this means you can't make this check unless you know the number of KV pairs beforehand.

I agree that you can't compute a minimum that is guaranteed to be enough unless you know how much is intended to be stored. However, there is a bare minimum size (e.g. space for internal bookkeeping, a second sector for garbage collection) below which TDBStore cannot function. That is what this check is intended to achieve. Are you saying that the bare minimum page count calculation should be different?

Ah, my understanding was that the theoretical minimum would be 4 pages total (2 for the active master record, 2 for the old master record, where pages = program size'd units). But my understand could be out of date.

Out of curiousity do you know where the other 2x3 pages come from?

I not able to determine precisely where that came from.
However, after further consideration I've removed the hard-fail on too few pages given that the exact minimum is application specific. Full reasoning is in the commit message for a3a4eef59939741999101f1eb5b4367f422f9e6e

features/storage/kvstore/tdbstore/TDBStore.h

0xc0170 · 2019-10-21T12:36:26Z

@ARMmbed/mbed-os-storage Please help reviewing this PR

VeijoPesonen · 2019-10-22T06:29:55Z

@0xc0170 work in progress, let's give @kyle-cypress time to address the remaining issues. He seems to have a higher priority PR to finish first.

0xc0170 · 2019-11-04T14:47:49Z

@kyle-cypress This PR was updated, what is the current status?

kyle-cypress · 2019-11-04T17:50:11Z

There are two items left that I know of:

Need a response to my question in Refactor TDB internal bounds computation #11629 (comment) about whether it is okay to add the additional check as suggested
There is an open question to @SeppoTakalo in Refactor TDB internal bounds computation #11629 (comment) ; I'm not sure if that gates this PR or not.

features/storage/kvstore/conf/filesystem/mbed_lib.json

SeppoTakalo · 2019-11-05T13:39:54Z

features/storage/kvstore/conf/filesystem/mbed_lib.json

@@ -2,11 +2,11 @@
    "name": "storage_filesystem",
    "config": {
        "rbp_internal_size": {
-            "help": "Default is the size of the 2 last sectors of internal flash",
+            "help": "Default is the larger of the last 2 sectors or last 10 pages of flash.",


You later specify TDBStore ::STORE_PAGES to 14, so why this is 10?

Because STORE_PAGES was increased and the comment update was missed. I will fix.

SeppoTakalo · 2019-11-05T13:41:38Z

features/storage/kvstore/conf/kv_config.cpp

        tr_warning("KV Config: There are less than two sectors - TDBStore will not work.");
        return -1;
    }

+    if (number_of_page < TDBStore::STORE_PAGES) {
+        tr_warning("KV Config: There are less than fourteen pages - TDBStore will not work.");


Should be tr_error(), and preferably change the previous as well.

Also not sure whether TDBStore::STORE_PAGES can change in the future, so perhaps use less than %d pages.

SeppoTakalo · 2019-11-05T13:45:01Z

features/storage/kvstore/conf/kv_config.cpp

 #else
    return NULL;
 #endif
 }

-BlockDevice *_get_blockdevice_SPIF(bd_addr_t start_address, bd_size_t size)
+BlockDevice *_get_blockdevice_SPIF(bd_addr_t &start_address, bd_size_t &size)


If we change the API, should we use pointers instead of references?
Makes it more obvious for caller that the values can change. I don't really like references for trivial types.

I looked through the current changeset and nothing uses the returned value anymore. I will revert the argument type change.

SeppoTakalo · 2019-11-05T13:55:51Z

features/storage/kvstore/tdbstore/TDBStore.cpp

+    MBED_ASSERT(curr_addr >= first_wrtbl_sector_addr);
+    if (curr_addr < first_wrtbl_sector_addr) {
+        ret = MBED_ERROR_MEDIA_FULL;
+    } else {
+        *start_address = curr_addr;
+    }


Would be too big API change.. maybe for the OS version 6.

0xc0170 · 2019-11-08T10:52:28Z

@kyle-cypress Please review the latest comments from @SeppoTakalo

kyle-cypress · 2019-11-09T02:22:41Z

I've pushed up changes that I believe address all of the remaining change requests. I still need confirmation that the change suggested in #11629 (comment) (it is included in the current changeset) is not a backwards compatibility problem.
I also rebased on master to pick up the fix in #11810.

SeppoTakalo · 2019-11-11T10:25:17Z

Looks OK.

This has already been labeled as 6.0, so I would not worry about being backward compatible.
Using storage, without explicitly setting its location should already be documented as a "experimental development feature" and nobody goes into production with setup like that.

VeijoPesonen

Looks good to me

Increase minimum page count from 10 to 14 based on further experiments with features-storage-tests-kvstore-static_tests.

No callers make use of the modified argument values, so change them to a more straightforward pass by value.

Replace custom caluation that always assumed two sectors with the standard calculation exposed on TDBStore.

STORE_SECTORS is a hard requirement. If there are fewer than 2 pages then the kvstore will not work, because the garbage collection process relies on having at least two sectors to work with. STORE_PAGES is a heuristic. It is a reasonable default to use if the application does not specify the amount of flash to use for TDBStore. But if an application knows that a smaller number of pages will suffice for its specific needs, then that is valid and should be permitted.

The unittests compile DirectAccessDeviceKey.cpp which depends on kv_config.h, which lives in features/storage/kvstore/conf

kyle-cypress · 2020-03-24T19:41:56Z

The unittest failure is because kv_config.h is included from DirectAccessDeviceKey.cpp, but its containing folder is not in the include path for the unittest build. I pushed up 85d2e8f (rebasing again in the process) which adds an entry to UNITTESTS/CMakeLists.txt. This fixed the build when I tested it out locally, but I'm not sure if that is the right place to make this change.

kyle-cypress · 2020-03-24T19:43:04Z

Also, FYI, the "Build Artifacts" link posted above by mbed-ci still doesn't contain any logs related to failing tests.

0xc0170 · 2020-03-25T09:07:53Z

CI restarted

mbed-ci · 2020-03-25T18:57:37Z

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

kyle-cypress · 2020-03-25T19:04:44Z

The failures for NUCLEO_F429ZI are in network and netsocket, which this PR doesn't touch. Is this a CI issue?
As far as I can tell, that is the only failure, but please let me know if there are others that I'm not seeing in the build artifacts.

0xc0170 · 2020-03-26T17:25:00Z

CI restarted

kyle-cypress · 2020-03-26T17:57:42Z

Updated PR summary to match the latest template from https://os.mbed.com/docs/mbed-os/v5.15/contributing/workflow.html#pull-request-template

adbridge · 2020-03-27T12:24:04Z

CI restarted

adbridge · 2020-03-27T12:25:19Z

hi @kyle-cypress you're still slightly out, see #12472 for current template. May be our docs need a tweak too

kyle-cypress · 2020-03-27T15:27:41Z

Yes, I think the template in the workflow doc is slightly off. Should be up to date now.

mbed-ci · 2020-03-27T16:32:55Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 8
Build artifacts

ciarmcom requested review from a team October 4, 2019 03:00

ciarmcom added the needs: review label Oct 4, 2019

kyle-cypress force-pushed the pr/tdb-bounds-refactor branch from 2814f11 to d6a1ac6 Compare October 4, 2019 14:41

VeijoPesonen reviewed Oct 7, 2019

View reviewed changes

adbridge added needs: work and removed needs: review labels Oct 7, 2019

0xc0170 requested a review from SeppoTakalo October 17, 2019 13:23

0xc0170 added needs: review and removed needs: work labels Oct 18, 2019

0xc0170 requested a review from VeijoPesonen October 18, 2019 13:32

0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Oct 18, 2019

0xc0170 added devices: cypress needs: work and removed needs: review labels Oct 25, 2019

kyle-cypress force-pushed the pr/tdb-bounds-refactor branch from 7455372 to 2899562 Compare October 31, 2019 00:03

SeppoTakalo reviewed Nov 5, 2019

View reviewed changes

maclobdell added the priority: high label Nov 7, 2019

kyle-cypress force-pushed the pr/tdb-bounds-refactor branch from 2899562 to c52d452 Compare November 9, 2019 02:21

SeppoTakalo approved these changes Nov 11, 2019

View reviewed changes

VeijoPesonen approved these changes Dec 2, 2019

View reviewed changes

Kyle Kearney added 8 commits March 24, 2020 12:27

TDBStore: Increase min pages to 14

3ef04db

Increase minimum page count from 10 to 14 based on further experiments with features-storage-tests-kvstore-static_tests.

Remove unnecessary reference-typed arguments

e1b8570

No callers make use of the modified argument values, so change them to a more straightforward pass by value.

KV_CONFIG: Change errors to use tr_error not tr_warning

622a50f

Reuse TDBStore default size computation in devicekey test

926423c

Replace custom caluation that always assumed two sectors with the standard calculation exposed on TDBStore.

Move flash bounds helpers from TDBStore to kv_config

7f18a6c

Remove stray include of TDBStore.h

ab8ac88

Add kvstore/conf to unittest includes

85d2e8f

The unittests compile DirectAccessDeviceKey.cpp which depends on kv_config.h, which lives in features/storage/kvstore/conf

kyle-cypress force-pushed the pr/tdb-bounds-refactor branch from 6d730b4 to 85d2e8f Compare March 24, 2020 19:38

mergify bot added needs: work and removed needs: CI labels Mar 25, 2020

0xc0170 added needs: CI and removed needs: work labels Mar 26, 2020

0xc0170 added the release-version: 6.0.0-beta-1 label Mar 26, 2020

0xc0170 approved these changes Mar 30, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Mar 30, 2020

0xc0170 merged commit 4c6e59d into ARMmbed:master Mar 30, 2020

mergify bot removed the ready for merge label Mar 30, 2020

Refactor TDB internal bounds computation #11629

Refactor TDB internal bounds computation #11629

Uh oh!

Conversation

kyle-cypress commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type (required)

Test results (required)

Reviewers

Uh oh!

ciarmcom commented Oct 4, 2019

Uh oh!

JanneKiiskila commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyle-cypress Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

VeijoPesonen commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 4, 2019

Uh oh!

kyle-cypress commented Nov 4, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 8, 2019

Uh oh!

kyle-cypress commented Nov 9, 2019

Uh oh!

SeppoTakalo commented Nov 11, 2019

Uh oh!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

kyle-cypress commented Mar 24, 2020

Uh oh!

kyle-cypress commented Mar 24, 2020

Uh oh!

0xc0170 commented Mar 25, 2020

Uh oh!

kyle-cypress commented Oct 4, 2019 •

edited

Loading

kyle-cypress Oct 29, 2019 •

edited

Loading

VeijoPesonen commented Oct 22, 2019 •

edited

Loading