fs: zms: multiple fixes from previous PR review #80407

rghaddab · 2024-10-25T01:24:47Z

This resolves some addressed comments in this PR
#77930

It adds as well a section in the documentation about some recommendations to increase ZMS performance.

subsys/fs/zms/zms.c

tomi-font

Thanks for addressing my first batch of comments! Just a few more related remarks for now.

include/zephyr/fs/zms.h

subsys/fs/zms/Kconfig

This resolves some addressed comments in this PR zephyrproject-rtos/zephyr#77930 It adds as well a section in the documentation about some recommendations to increase ZMS performance. Upstream PR: zephyrproject-rtos/zephyr#80407 Signed-off-by: Riadh Ghaddab <rghaddab@baylibre.com> (cherry picked from commit c93c98eb20d21fccef17cb68beee63b7b0925ac8)

Damian-Nordic · 2024-10-25T11:20:18Z

doc/services/storage/zms/zms.rst

+- Each additional cache entry will add 8 bytes to your RAM usage. Cache size should be carefully
+  chosen.
+- If you use ZMS through :ref:`Settings <settings_api>`, you have to take into account that each Settings entry is
+  divided into two ZMS entries. The perfect cache size should be double that of the Settings


There's no perfect cache size :) The more you can afford the better as even for a random hash function, if you have N values and cache of size N, the expected number of collisions is > 0. Maybe you should state instead that the cache size should be at least twice the number of settings entries?

you are right, the cache is using a random hash function

Damian-Nordic · 2024-10-25T11:35:50Z

subsys/fs/zms/zms.c

@@ -263,7 +264,7 @@ static int zms_flash_cmp_const(struct zms_fs *fs, uint64_t addr, uint8_t value,
 	size_t bytes_to_cmp, block_size;
 	uint8_t cmp[ZMS_BLOCK_SIZE];

-	block_size = ZMS_BLOCK_SIZE & ~(fs->flash_parameters->write_block_size - 1U);
+	block_size = ROUND_DOWN(ZMS_BLOCK_SIZE, fs->flash_parameters->write_block_size);


fyi: This macro now uses a division (as it doesn't assume powers of two anymore) so it's pretty inefficient for use when the divisor is unknown at compile-time :).

@Damian-Nordic I thought GCC and Clang optimizes automatically the divisions of an integer when it is a power of 2.
Isn't this the case ?

But in this case the compiler doesn't know if write_block_size is a power of 2, right?

I will change it to an inline function to explicitly optimize it

This resolves some addressed comments in this PR zephyrproject-rtos/zephyr#77930 It adds as well a section in the documentation about some recommendations to increase ZMS performance. Upstream PR: zephyrproject-rtos/zephyr#80407 Signed-off-by: Riadh Ghaddab <rghaddab@baylibre.com> (cherry picked from commit 494e0cff17dbccbd8093911f063fdd47e26b35eb)

rghaddab · 2024-10-25T15:41:20Z

@frkv I addressed your comments in this PR

This resolves some addressed comments in this PR zephyrproject-rtos/zephyr#77930 It adds as well a section in the documentation about some recommendations to increase ZMS performance. Upstream PR: zephyrproject-rtos/zephyr#80407 Signed-off-by: Riadh Ghaddab <rghaddab@baylibre.com> (cherry picked from commit 494e0cff17dbccbd8093911f063fdd47e26b35eb)

subsys/fs/zms/zms.c

nordicjm

Minor nit.

nordicjm · 2024-10-28T07:29:16Z

include/zephyr/fs/zms.h

@@ -65,7 +61,7 @@ struct zms_fs {
 	/** Size of an Allocation Table Entry */
 	size_t ate_size;
 #if CONFIG_ZMS_LOOKUP_CACHE
-	/** Lookup table used to cache ATE address of a written ID */
+	/** Lookup table used to cache ATE addressese of written IDs */


nordicjm · 2024-10-28T08:55:16Z

doc/services/storage/zms/zms.rst


-.. note:: Enabling the data CRC feature on a previously existing ZMS content without
-   data CRC will make all existing data invalid.
+.. note:: Enabling the CRC feature on a previously existing ZMS content without CRC enabled


Suggested change

.. note:: Enabling the CRC feature on a previously existing ZMS content without CRC enabled

.. note:: Enabling the CRC feature on previously existing ZMS content without CRC enabled

tomi-font

Nice!

This resolves some addressed comments in this PR zephyrproject-rtos#77930 It adds as well a section in the documentation about some recommendations to increase ZMS performance. Signed-off-by: Riadh Ghaddab <rghaddab@baylibre.com>

tomi-font

Some more comments. Nothing blocking if you want to address them in a follow-up PR.

doc/services/storage/zms/zms.rst

tomi-font · 2024-10-29T09:11:49Z

doc/services/storage/zms/zms.rst

+  occur less frequently.
+  Decreasing its size, in the opposite, will make the garbage collection operation faster
+  which will occur more frequently.
+- For some subsystems like :ref:`Settings <settings_api>`, all path-value pairs are split into two ZMS entries (ATEs).


Why some subsystems like? Isn't it only about settings?

ZMS could be used directly by any subsystem (bluetooth, secure storage, ...)
This is an example of one subsystem which is Settings

But those are not gonna reinvent the wheel, are they?

doc/services/storage/zms/zms.rst

tomi-font · 2024-10-29T10:32:14Z

@dkalowsk @mmahadevan108 We'll need this in for 4.0. It's just minor review fixes, but there are two breaking changes (struct field order and Kconfig option name) that would make it bad to only have this in later.

nordicjm · 2024-10-29T10:33:58Z

@dkalowsk @mmahadevan108 We'll need this in for 4.0. It's just minor review fixes, but there are two breaking changes (struct field order and Kconfig option name) that would make it bad to only have this in later.

Is it really a breaking change to merge something into 4.0 that updates something that was only added to 4.0 anyway?

tomi-font · 2024-10-29T10:59:22Z

Is it really a breaking change to merge something into 4.0 that updates something that was only added to 4.0 anyway?

No, it's not, and that's the point. To have this in before 4.1.

mmahadevan108 · 2024-10-29T11:36:39Z

Is it really a breaking change to merge something into 4.0 that updates something that was only added to 4.0 anyway?

No, it's not, and that's the point. To have this in before 4.1.

@tomi-font I see some comments from you that may have not been addressed. I see you have approved, can you please confirm.

tomi-font · 2024-10-29T12:01:44Z

@tomi-font I see some comments from you that may have not been addressed. I see you have approved, can you please confirm.

Correct, this PR is good to go in. I have open comments both in this and the original implementation PR that will be addressed by @rghaddab in yet another follow-up PR that doesn't need to go in before 4.0.

This resolves some addressed comments in this PR zephyrproject-rtos#77930 as well as this PR zephyrproject-rtos#80407 Signed-off-by: Riadh Ghaddab <rghaddab@baylibre.com>

zephyrbot added area: File System area: Samples Samples area: Storage Storage subsystem labels Oct 25, 2024

zephyrbot requested review from de-nordic, kartben, Laczen and nashif October 25, 2024 01:25

zephyrbot assigned de-nordic Oct 25, 2024

Laczen removed their request for review October 25, 2024 04:14

nordicjm requested changes Oct 25, 2024

View reviewed changes

subsys/fs/zms/zms.c Show resolved Hide resolved

tomi-font requested review from carlescufi, Damian-Nordic and tomi-font October 25, 2024 06:12

tomi-font reviewed Oct 25, 2024

View reviewed changes

rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch 2 times, most recently from 6142310 to 7f03d4a Compare October 25, 2024 09:40

rghaddab requested review from nordicjm and tomi-font October 25, 2024 09:46

tomi-font previously approved these changes Oct 25, 2024

View reviewed changes

include/zephyr/fs/zms.h Outdated Show resolved Hide resolved

include/zephyr/fs/zms.h Outdated Show resolved Hide resolved

subsys/fs/zms/Kconfig Outdated Show resolved Hide resolved

rghaddab dismissed tomi-font’s stale review via c93c98e October 25, 2024 10:23

rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch from 7f03d4a to c93c98e Compare October 25, 2024 10:23

tomi-font previously approved these changes Oct 25, 2024

View reviewed changes

Damian-Nordic approved these changes Oct 25, 2024

View reviewed changes

rghaddab dismissed tomi-font’s stale review via 494e0cf October 25, 2024 15:13

rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch from c93c98e to 494e0cf Compare October 25, 2024 15:13

rghaddab requested review from Damian-Nordic and tomi-font October 25, 2024 15:18

tomi-font previously approved these changes Oct 28, 2024

View reviewed changes

subsys/fs/zms/zms.c Outdated Show resolved Hide resolved

subsys/fs/zms/zms.c Outdated Show resolved Hide resolved

subsys/fs/zms/zms.c Outdated Show resolved Hide resolved

subsys/fs/zms/zms.c Outdated Show resolved Hide resolved

nordicjm reviewed Oct 28, 2024

View reviewed changes

rghaddab dismissed tomi-font’s stale review via 6d8d090 October 28, 2024 09:20

rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch from 494e0cf to 6d8d090 Compare October 28, 2024 09:20

rghaddab requested review from nordicjm and tomi-font October 28, 2024 09:20

tomi-font previously approved these changes Oct 28, 2024

View reviewed changes

fs: zms: multiple fixes from previous PR review

a40ba48

This resolves some addressed comments in this PR zephyrproject-rtos#77930 It adds as well a section in the documentation about some recommendations to increase ZMS performance. Signed-off-by: Riadh Ghaddab <rghaddab@baylibre.com>

rghaddab dismissed tomi-font’s stale review via a40ba48 October 28, 2024 09:29

rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch from 6d8d090 to a40ba48 Compare October 28, 2024 09:29

nordicjm approved these changes Oct 28, 2024

View reviewed changes

tomi-font approved these changes Oct 28, 2024

View reviewed changes

de-nordic approved these changes Oct 28, 2024

View reviewed changes

tomi-font added this to the v4.0.0 milestone Oct 29, 2024

tomi-font reviewed Oct 29, 2024

View reviewed changes

tomi-font requested review from dkalowsk and mmahadevan108 October 29, 2024 10:30

mmahadevan108 merged commit 46e1635 into zephyrproject-rtos:main Oct 29, 2024
23 checks passed

rghaddab mentioned this pull request Oct 30, 2024

fs: zms: multiple style fixes from previous PR review #80628

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: zms: multiple fixes from previous PR review #80407

fs: zms: multiple fixes from previous PR review #80407

rghaddab commented Oct 25, 2024

tomi-font left a comment

Damian-Nordic Oct 25, 2024 •

edited

Loading

rghaddab Oct 25, 2024

Damian-Nordic Oct 25, 2024

rghaddab Oct 25, 2024

Damian-Nordic Oct 25, 2024

rghaddab Oct 25, 2024

rghaddab commented Oct 25, 2024

nordicjm left a comment

nordicjm Oct 28, 2024

nordicjm Oct 28, 2024

tomi-font left a comment

tomi-font left a comment

tomi-font Oct 29, 2024

rghaddab Oct 29, 2024

tomi-font Nov 5, 2024

tomi-font commented Oct 29, 2024

nordicjm commented Oct 29, 2024

tomi-font commented Oct 29, 2024

mmahadevan108 commented Oct 29, 2024

tomi-font commented Oct 29, 2024

	.. note:: Enabling the CRC feature on a previously existing ZMS content without CRC enabled
	.. note:: Enabling the CRC feature on previously existing ZMS content without CRC enabled

fs: zms: multiple fixes from previous PR review #80407

fs: zms: multiple fixes from previous PR review #80407

Conversation

rghaddab commented Oct 25, 2024

tomi-font left a comment

Choose a reason for hiding this comment

Damian-Nordic Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rghaddab commented Oct 25, 2024

nordicjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomi-font left a comment

Choose a reason for hiding this comment

tomi-font left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomi-font commented Oct 29, 2024

nordicjm commented Oct 29, 2024

tomi-font commented Oct 29, 2024

mmahadevan108 commented Oct 29, 2024

tomi-font commented Oct 29, 2024

Damian-Nordic Oct 25, 2024 •

edited

Loading