-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fs: zms: multiple fixes from previous PR review #80407
Conversation
There was a problem hiding this 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.
6142310
to
7f03d4a
Compare
7f03d4a
to
c93c98e
Compare
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)
doc/services/storage/zms/zms.rst
Outdated
- 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 was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
subsys/fs/zms/zms.c
Outdated
@@ -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); |
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
c93c98e
to
494e0cf
Compare
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)
@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 <[email protected]> (cherry picked from commit 494e0cff17dbccbd8093911f063fdd47e26b35eb)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit.
include/zephyr/fs/zms.h
Outdated
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*addresses
doc/services/storage/zms/zms.rst
Outdated
|
||
.. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. 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 |
494e0cf
to
6d8d090
Compare
There was a problem hiding this 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]>
6d8d090
to
a40ba48
Compare
There was a problem hiding this 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.
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). |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
@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? |
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. |
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 <[email protected]>
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]>
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.