Skip to content

Conversation

@sunyuechi
Copy link
Contributor

Optimize mem_is_zero using RISC-V V extension instructions.

On the bpi-f3 device, the output of running
ctest -V -R unittest_memory is as follows:

size default v intrinsic
1024 332ms 92ms
2048 657ms 186ms
4096 1290ms 366ms
8192 2572ms 733ms
65536 24836ms 10004ms

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands

@sunyuechi
Copy link
Contributor Author

@tchaikov Hi, could you please help review the RISC-V part? Thanks.

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

The current commit message uses "v intrinsic" which might be unclear to developers not deeply familiar with RISC-V terminology. Consider expanding this to be more descriptive:

Instead of: "v intrinsic", please consider: "RISC-V Vector (RVV) intrinsics" .

also, please use the imperative mood in the commit title. see https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst#commit-title

@sunyuechi sunyuechi changed the title inline_memory: optimized mem_is_zero for riscv64 by v intrinsic inline_memory: optimize mem_is_zero for riscv using RISC-V Vector (RVV) intrinsics Sep 4, 2025
…V) intrinsics

On the bpi-f3 device, the output of running
ctest -V -R unittest_memory is as follows:

        size    |    default     |  v intrinsic
     --------------------------------------------
        1024    |    332ms       |     92ms
     --------------------------------------------
        2048    |    657ms       |     186ms
     --------------------------------------------
        4096    |    1290ms      |     366ms
     --------------------------------------------
        8192    |    2572ms      |     733ms
     --------------------------------------------
        65536   |    24836ms     |     10004ms

Signed-off-by: Sun Yuechi <sunyuechi@iscas.ac.cn>
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

my only concern is that this might break riscv installations where the RVV instruction set is not available while the building host does support it.

@sunyuechi is it safe to assume its existence on mainstream RISC CPUs?

@sunyuechi
Copy link
Contributor Author

sunyuechi commented Sep 9, 2025

@tchaikov It should not assume that the CPU supports RVV; however, since #elif defined(__riscv_v_intrinsic) will only be compiled when the compiler supports RVV and -march=rv..v is explicitly specified, there should be no issues.

@tchaikov
Copy link
Contributor

tchaikov commented Sep 9, 2025

jenkins test make check arm64

@tchaikov
Copy link
Contributor

tchaikov commented Sep 9, 2025

jenkins test api

@tchaikov
Copy link
Contributor

tchaikov commented Sep 9, 2025

jenkins test windows

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm

return true;
}

#elif defined(__riscv_v_intrinsic)
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, shouldn't affect other architectures.

@rzarzynski
Copy link
Contributor

(needs-qa is here just to honor our procedures; the possibility QA finds a bug here is theoretical – we don't test for this architecture)

@sunyuechi
Copy link
Contributor Author

@rzarzynski @tchaikov Could you please help merge it? Thank you.

@markhpc markhpc merged commit 344159e into ceph:main Sep 25, 2025
13 checks passed
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.

4 participants