Skip to content

benchmarks/mtd: Add MTD transfer rates benchmark. #3033

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

Merged
merged 1 commit into from
Mar 25, 2025

Conversation

tmedicci
Copy link
Contributor

@tmedicci tmedicci commented Mar 21, 2025

Summary

This test allows measuring write and read operations on an MTD flash device, evaluating its transfer rates.

Impact

Impact on user: YES, by providing a testing application for MTD devices (without any kind of buffering or file system).

Impact on build: NO.

Impact on hardware: NO.

Impact on documentation: NO.

Impact on security: NO.

Impact on compatibility: NO.

Testing

To test it, create an MTD partition and register it as a block driver. Using esp32s3-devkit:spiflash defconfig, let's try it:

Building

First of all, apply the following patch (which will be submitted to the nuttx repository):

diff --git a/boards/xtensa/esp32s3/common/src/esp32s3_board_spiflash.c b/boards/xtensa/esp32s3/common/src/esp32s3_board_spiflash.c
index 7d0ceecc6d..37c15357c2 100644
--- a/boards/xtensa/esp32s3/common/src/esp32s3_board_spiflash.c
+++ b/boards/xtensa/esp32s3/common/src/esp32s3_board_spiflash.c
@@ -333,6 +333,14 @@ static int init_storage_partition(void)
       return ret;
     }
 
+  ret = ftl_initialize(0, mtd);
+  if (ret < 0)
+    {
+      syslog(LOG_ERR, "ERROR: Failed to initialize the FTL layer: %d\n",
+              ret);
+      return ret;
+    }
+
 #ifdef CONFIG_MTD_PARTITION
   ret = parse_mtd_partition(mtd, NULL, NULL);
   if (ret < 0)

After applying it, build the defconfig with the following additional configs:

make -j distclean && ./tools/configure.sh esp32s3-devkit:spiflash && kconfig-tweak -d ESP32S3_SPIFLASH_SMARTFS && kconfig-tweak --set-val ESP32S3_SPIFLASH_MTD_BLKSIZE 4096 && kconfig-tweak -e BENCHMARK_MTD &&  make olddefconfig && make flash ESPTOOL_PORT=/dev/ttyUSB0 -s -j$(nproc) && minicom -D /dev/ttyUSB0

Running

Run the MTD test application:

nsh> mtd /dev/mtdblock0

Results

nsh> mtd /dev/mtdblock0
FLASH Test on device with:
   Sector size:        4096
   Sector count:        256
   Erase block:        4096
   Total size:      1048576

Starting write operation...

Write operation completed in 4.29 seconds
Total bytes written: 1048576
Transfer rate [write]: 238.69 KiB/s

Starting read operation...

Read operation completed in 0.43 seconds
Total bytes read: 1048576
Transfer rate [read]: 2381.40 KiB/s

Data verification successful: read data matches written data

@nuttxpr
Copy link

nuttxpr commented Mar 21, 2025

[Experimental Bot, please feedback here]

This PR description mostly meets the NuttX requirements, but could be improved in a few key areas:

Summary:

  • Needs more detail on why this change is necessary. What problem does this solve for users? Is it for benchmarking, debugging, or something else? Simply stating it "allows measuring" isn't enough justification. Explain the use case.
  • More explicit about the functional change. Instead of "Add MTD test...", be more specific, e.g., "Add a new application to the testing/fs directory that performs sequential read/write tests on MTD devices to measure transfer rates."

Impact:

  • User impact is understated. While it provides a testing application, the patch to esp32s3_board_spiflash.c suggests more than just adding an application. Explain why that patch is necessary for this test to work. Does it enable some functionality needed by the test? If so, that's a larger impact than simply adding a test application. If the patch is unrelated, it shouldn't be included in this PR.
  • Build impact is likely YES. Adding a new application and potentially modifying board configuration files usually impacts the build system, at least by adding a new target. Clarify this. Even if the user has to explicitly enable the test application, it's still a build impact.
  • Documentation impact is likely YES. A new test application should have at least some minimal documentation (e.g., in the README.txt or a help message within the application itself).

Testing:

  • Build Host(s) is missing. Specify the OS, CPU architecture, and compiler version used for testing.
  • Patch application should be avoided. Submitting a patch that needs another patch is confusing. Either integrate the necessary changes into the main PR or submit the board modification as a separate, prerequisite PR.
  • Testing logs before change are irrelevant. Since this is a new feature, there shouldn't be any prior logs. Remove that section.
  • "Data verification successful" is crucial, but needs more detail. How is data verification performed? Byte-by-byte comparison? Checksum? Be explicit.
  • More test cases are needed. Test with different MTD configurations (different block sizes, different drivers, etc.) to ensure robustness. Testing on only one configuration is insufficient. Ideally, test on both a real target and a simulator if possible.

Example of Improved Description:

Summary

This PR adds a new test application to testing/fs to benchmark the read/write performance of MTD devices. This is useful for characterizing flash performance, identifying bottlenecks, and verifying driver correctness. This allows developers to easily measure transfer rates on raw MTD devices without the overhead of a filesystem. This PR also modifies the esp32s3_board_spiflash.c file to [explain why this change is necessary, e.g., to expose the MTD device in a way accessible by the test application].

Impact

  • User impact: YES. Users can now benchmark MTD performance using the provided test application. The modification to esp32s3_board_spiflash.c [explain user impact, e.g., now exposes a new block device]. Users may need to enable the TESTING_MTD_TRANSFER_RATE Kconfig option to build the test.
  • Build impact: YES. The build system now includes the new mtd_test application.
  • Hardware impact: NO.
  • Documentation impact: YES. This PR adds documentation for the mtd_test application within the application's help output (accessible via mtd_test -h).
  • Security impact: NO.
  • Compatibility impact: NO.

Testing

  • Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
  • Target: esp32s3-devkit:spiflash, sim:qemu-esp32s3

Testing on esp32s3-devkit:spiflash:

nsh> mtd_test /dev/mtdblock0
... (output as before, but include the -h output as well)

Testing on sim:qemu-esp32s3:

nsh> mtd_test /dev/mtd0  (or appropriate device)
...

Data verification is performed via a byte-by-byte comparison of the written and read data. Additional tests were performed with block sizes of [list sizes] and on [list other configurations].

By addressing these points, the PR description will be clearer, more complete, and easier for reviewers to assess. Remember to clearly justify each change and provide comprehensive testing results.

@tmedicci
Copy link
Contributor Author

Related documentation PR: apache/nuttx#16032

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @tmedicci very nice tool :-)

  • Would it be possible to provite this utiulity as apps/benchmarks/mtd ? Recently we agreed that apps/testing will only hold test scenarios and fameworks, while utilities go to apps/system and benchmarks to apps/benchmarks.
  • Would it be possible to provide even a simple documentation too?

@tmedicci
Copy link
Contributor Author

tmedicci commented Mar 24, 2025

Thank you @tmedicci very nice tool :-)

  • Would it be possible to provite this utiulity as apps/benchmarks/mtd ? Recently we agreed that apps/testing will only hold test scenarios and fameworks, while utilities go to apps/system and benchmarks to apps/benchmarks.

I'll move it!

  • Would it be possible to provide even a simple documentation too?

I provided it: #3033 (comment)

@tmedicci
Copy link
Contributor Author

Thank you @tmedicci very nice tool :-)

  • Would it be possible to provite this utiulity as apps/benchmarks/mtd ? Recently we agreed that apps/testing will only hold test scenarios and fameworks, while utilities go to apps/system and benchmarks to apps/benchmarks.

I'll move it!

Moved!

@tmedicci tmedicci requested a review from cederom March 24, 2025 12:53
@cederom cederom changed the title testing/fs/mtd_test: Add MTD test to evaluate transfer rates benchmarks/mtd: Add MTD transfer rates benchmark. Mar 24, 2025
@cederom
Copy link
Contributor

cederom commented Mar 24, 2025

Thank you @tmedicci :-) Some minor updates of test -> benchmark noted :-) When ready please also update build and runtime logs and we are ready to go, thanks!! :-)

@tmedicci
Copy link
Contributor Author

Thank you @tmedicci :-) Some minor updates of test -> benchmark noted :-) When ready please also update build and runtime logs and we are ready to go, thanks!! :-)

I had updated the build log, forgot about the runtime logs. Now we are fine here!

@cederom
Copy link
Contributor

cederom commented Mar 24, 2025

Lets just update the testing to benchmarks nomenclature.. as in documentation so folks will search this app in benchmarks not tests.. thank you @tmedicci :-)

Tests are specific programs to return PASS/FAIL results. This application is mostly aimed at memory benchmarking but also can validate read/write and provide information about memory layout.. probably more functionalities will show up in future right? If this will be more generic application to work with mtd then maybe apps/system would be a good location :D

@tmedicci
Copy link
Contributor Author

Lets just update the testing to benchmarks nomenclature.. as in documentation so folks will search this app in benchmarks not tests.. thank you @tmedicci :-)

Tests are specific programs to return PASS/FAIL results. This application is mostly aimed at memory benchmarking but also can validate read/write and provide information about memory layout.. probably more functionalities will show up in future right? If this will be more generic application to work with mtd then maybe apps/system would be a good location :D

I'm sorry, can you be more specific?

I already updated the location and the related Kconfig files. I also updated the commit and PR descriptions. What am I missing here?

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thanks @tmedicci.. this github works strangely recently.. did you see in code notes that I made before or these are visible after I click request changes again? o_O

Hmm these in code reviews had "pending" label, not some have no label and some have "outdated" label o_O

@tmedicci
Copy link
Contributor Author

Thanks @tmedicci.. this github works strangely recently.. did you see in code notes that I made before or these are visible after I click request changes again? o_O

Now I can see the notes! Thanks!

This test allows measuring write and read operations on an MTD
flash device, evaluating its transfer rates.

Signed-off-by: Tiago Medicci <[email protected]>
@tmedicci
Copy link
Contributor Author

Thanks @tmedicci.. this github works strangely recently.. did you see in code notes that I made before or these are visible after I click request changes again? o_O

Now I can see the notes! Thanks!

Updated! Thank you very much, @cederom !

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Great work @tmedicci Big Thank You!! :-)

@cederom
Copy link
Contributor

cederom commented Mar 24, 2025

@cederom: Thanks @tmedicci.. this github works strangely recently.. did you see in code notes that I made before or these are visible after I click request changes again? o_O

@tmedicci: Now I can see the notes! Thanks!

@tmedicci: Updated! Thank you very much, @cederom !

I probably clicked notes in review mode and now on GH we have to close review so these are visible, my bad, sorry! :-)

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.

6 participants