Skip to content

sys/random/fortuna: change interval ressed to ms#16594

Merged
leandrolanzieri merged 2 commits intoRIOT-OS:masterfrom
fjmolinas:pr_sys_fortuna_reseed_ms
Sep 27, 2021
Merged

sys/random/fortuna: change interval ressed to ms#16594
leandrolanzieri merged 2 commits intoRIOT-OS:masterfrom
fjmolinas:pr_sys_fortuna_reseed_ms

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented Jun 29, 2021

Contribution description

fortuna suggests a 100ms reseed interval, therefore msec resolution should be enough for the reseed value which. This PR also uses a timer to set a flag when a reseed is necessary this allows using ztimer without the need of including now64 and ticks an item off #13667.

To do that I realized that Fortuna is actually relying on an xtimer call before the module is initialized, therefore I had to move timer initialization up, I'll split this part out.

Testing procedure

I applied the following patch:

diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile
index 258d8e9baf..f389bea7ca 100644
--- a/examples/hello-world/Makefile
+++ b/examples/hello-world/Makefile
@@ -15,4 +15,11 @@ DEVELHELP ?= 1
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1

+USEMODULE += xtimer
+USEMODULE += prng_fortuna
+USEMODULE += fortuna_reseed
+USEMODULE += random
+
+CFLAGS=-DFORTUNA_RESEED_INTERVAL_MS=1000
+
 include $(RIOTBASE)/Makefile.include
diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index f51bf8c0a0..168cbdcfdc 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -20,6 +20,8 @@
  */

 #include <stdio.h>
+#include "xtimer.h"
+#include "random.h"

 int main(void)
 {
@@ -28,5 +30,10 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s MCU.\n", RIOT_MCU);

+    while(1) {
+        printf("random number: %"PRIu32"\n", random_uint32());
+        xtimer_usleep(2000000);
+    }
+
     return 0;
 }
diff --git a/sys/random/fortuna/fortuna.c b/sys/random/fortuna/fortuna.c
index ac096eb360..5f9bcc2a58 100644
--- a/sys/random/fortuna/fortuna.c
+++ b/sys/random/fortuna/fortuna.c
@@ -199,8 +199,8 @@ int fortuna_random_data(fortuna_state_t *state, uint8_t *out, size_t bytes)

     /* reseed the generator if needed, before returning data */
 #if FORTUNA_RESEED_INTERVAL_MS > 0 && IS_USED(MODULE_FORTUNA_RESEED)
-    if (state->pools[0].len >= FORTUNA_MIN_POOL_SIZE &&
-        atomic_load_u8(&state->reseed)) {
+    if (state-atomic_load_u8(&state->needs_reseed)) {
+        puts("reseed");
 #else
     if (state->pools[0].len >= FORTUNA_MIN_POOL_SIZE) {
 #endif
  • make -C examples/hello-world/ all term
main(): This is RIOT! (Version: 2021.07-devel-485-g63b849-pr_sys_fortuna_reseed_ms)
Hello World!
You are running RIOT on a(n) native board.
This board features a(n) native MCU.
reseed
random number: 3113564991
reseed
random number: 984556921
reseed
random number: 44374394
reseed
random number: 4139781756
reseed
random number: 3529685450
reseed
random number: 2868101182

Issues/PRs references

Ticks an item in #13667

@fjmolinas
Copy link
Copy Markdown
Contributor Author

ping @leandrolanzieri @PeterKietzmann

@PeterKietzmann
Copy link
Copy Markdown
Member

Though I doubt a 100ms reseed interval is reasonable for us (not a decision of this PR), moving to a ms resolution is fine. The new variable name reseed in the state struct is suboptimal since reseeds already exists. Maybe needs_reseed or similar + documentation. Otherwise, as the logic doesn't break, I would leave the review up to timer people.

Side note: Moving the order in auto_init is sometimes dangerous. Hence, I would suggest to run some "standard examples" on this PR even if they don't relate to fortuna in particular.

@fjmolinas fjmolinas force-pushed the pr_sys_fortuna_reseed_ms branch from 63b8496 to 53a344d Compare September 2, 2021 07:23
@fjmolinas
Copy link
Copy Markdown
Contributor Author

Though I doubt a 100ms reseed interval is reasonable for us (not a decision of this PR), moving to a ms resolution is fine. The new variable name reseed in the state struct is suboptimal since reseeds already exists. Maybe needs_reseed or similar + documentation. Otherwise, as the logic doesn't break, I would leave the review up to timer people.

Side note: Moving the order in auto_init is sometimes dangerous. Hence, I would suggest to run some "standard examples" on this PR even if they don't relate to fortuna in particular.

@PeterKietzmann renamed the variable as suggested, the xtimer/ztimer change was merged a while ago, so unrelated to this PR now.

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 2, 2021
@fjmolinas
Copy link
Copy Markdown
Contributor Author

@leandrolanzieri might be able to take a look as well?

@fjmolinas
Copy link
Copy Markdown
Contributor Author

I think I've addressed your changes @leandrolanzieri!

@fjmolinas fjmolinas changed the title sys/random/fortune: change interval ressed to ms sys/random/fortuna: change interval ressed to ms Sep 13, 2021
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Sep 24, 2021
Comment on lines +3 to +5
CONFIG_MODULE_EMBUNIT=y
CONFIG_MODULE_TEST_UTILS_INTERACTIVE_SYNC=y
CONFIG_MODULE_XTIMER=y
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
CONFIG_MODULE_EMBUNIT=y
CONFIG_MODULE_TEST_UTILS_INTERACTIVE_SYNC=y
CONFIG_MODULE_XTIMER=y
CONFIG_MODULE_ATOMIC_UTILS=y
CONFIG_MODULE_TEST_UTILS_INTERACTIVE_SYNC=y
CONFIG_MODULE_XTIMER=y

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this change got lost

@fjmolinas
Copy link
Copy Markdown
Contributor Author

@leandrolanzieri murdock is green, OK to squash?

@fjmolinas fjmolinas force-pushed the pr_sys_fortuna_reseed_ms branch from 0c4c80f to 7c215d2 Compare September 27, 2021 06:21
#ifndef FORTUNA_RESEED_INTERVAL
#define FORTUNA_RESEED_INTERVAL (0)
#ifndef FORTUNA_RESEED_INTERVAL_MS
#define FORTUNA_RESEED_INTERVAL_MS 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that we have the pseudomodule as the extra switch for this feature, I'd say let's set the recommended value of 100 ms as default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm seems I bodged the rebase

@fjmolinas fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 27, 2021
@fjmolinas
Copy link
Copy Markdown
Contributor Author

Addressed changes redid the testing procedur:

main(): This is RIOT! (Version: 2021.10-devel-534-gcc71e-pr_sys_fortuna_reseed_ms)
Hello World!
You are running RIOT on a(n) native board.
This board features a(n) native MCU.
random number: 301462354
reseed
random number: 2900410742
reseed
random number: 1588391176
reseed
random number: 2563216097

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Sep 27, 2021
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Changes look good, @fjmolinas provided test results. ACK!

Please squash @fjmolinas

@fjmolinas fjmolinas force-pushed the pr_sys_fortuna_reseed_ms branch from 5412d46 to c14313d Compare September 27, 2021 12:08
@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 27, 2021
@fjmolinas fjmolinas force-pushed the pr_sys_fortuna_reseed_ms branch from c14313d to f4db917 Compare September 27, 2021 14:47
@fjmolinas
Copy link
Copy Markdown
Contributor Author

Hmm I bodged the rebase

Use a timer to required a reseed, enable use of plain ztimer (no now64)
@fjmolinas fjmolinas force-pushed the pr_sys_fortuna_reseed_ms branch from f4db917 to c307cad Compare September 27, 2021 15:30
@fjmolinas
Copy link
Copy Markdown
Contributor Author

Should be back on track

@fjmolinas
Copy link
Copy Markdown
Contributor Author

All green finally :)

@leandrolanzieri
Copy link
Copy Markdown
Contributor

All green finally :)

Then let's go!

@leandrolanzieri leandrolanzieri merged commit 0129cbe into RIOT-OS:master Sep 27, 2021
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
@fjmolinas fjmolinas deleted the pr_sys_fortuna_reseed_ms branch January 31, 2022 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: build system Area: Build system Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants