Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

rghaddab
Copy link
Contributor

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 Show resolved Hide resolved
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

include/zephyr/fs/zms.h Show resolved Hide resolved
include/zephyr/fs/zms.h Outdated Show resolved Hide resolved
include/zephyr/fs/zms.h Outdated Show resolved Hide resolved
include/zephyr/fs/zms.h Show resolved Hide resolved
include/zephyr/fs/zms.h Outdated Show resolved Hide resolved
include/zephyr/fs/zms.h Show resolved Hide resolved
@rghaddab rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch 2 times, most recently from 6142310 to 7f03d4a Compare October 25, 2024 09:40
tomi-font
tomi-font previously approved these changes Oct 25, 2024
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
tomi-font
tomi-font previously approved these changes Oct 25, 2024
rghaddab added a commit to rghaddab/sdk-zephyr that referenced this pull request Oct 25, 2024
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 <[email protected]>
(cherry picked from commit c93c98eb20d21fccef17cb68beee63b7b0925ac8)
- 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
Copy link
Collaborator

@Damian-Nordic Damian-Nordic Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rghaddab rghaddab force-pushed the rghaddab/pr-zms-style-fixup branch from c93c98e to 494e0cf Compare October 25, 2024 15:13
rghaddab added a commit to rghaddab/sdk-zephyr that referenced this pull request Oct 25, 2024
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 <[email protected]>
(cherry picked from commit 494e0cff17dbccbd8093911f063fdd47e26b35eb)
@rghaddab
Copy link
Contributor Author

@frkv I addressed your comments in this PR

rghaddab added a commit to rghaddab/sdk-zephyr that referenced this pull request Oct 25, 2024
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 <[email protected]>
(cherry picked from commit 494e0cff17dbccbd8093911f063fdd47e26b35eb)
tomi-font
tomi-font previously approved these changes Oct 28, 2024
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
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit.

@@ -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 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*addresses


.. 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
tomi-font previously approved these changes Oct 28, 2024
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <[email protected]>
@tomi-font tomi-font added this to the v4.0.0 milestone Oct 29, 2024
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
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).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
doc/services/storage/zms/zms.rst Show resolved Hide resolved
@tomi-font
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

@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.

@mmahadevan108 mmahadevan108 merged commit 46e1635 into zephyrproject-rtos:main Oct 29, 2024
23 checks passed
rghaddab added a commit to rghaddab/zephyr that referenced this pull request Oct 30, 2024
This resolves some addressed comments in this PR zephyrproject-rtos#77930
as well as this PR zephyrproject-rtos#80407

Signed-off-by: Riadh Ghaddab <[email protected]>
rghaddab added a commit to rghaddab/zephyr that referenced this pull request Nov 6, 2024
This resolves some addressed comments in this PR zephyrproject-rtos#77930
as well as this PR zephyrproject-rtos#80407

Signed-off-by: Riadh Ghaddab <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants