search

seL4 / sel4bench

sel4 benchmarking applications and support library.
Other
18 stars 30 forks source link

allow user to change bench options - riscv #20

Closed malus-brandywine closed 2 years ago

malus-brandywine commented 2 years ago

DefaultBenchDeps is set to TRUE when benchmark suite is being built for RiscV to enable configure benchmark options.

Signed-off-by: Nataliya Korovkina malus.brandywine@gmail.com

axel-h commented 2 years ago

Based on the comment https://github.com/seL4/sel4bench/issues/17#issuecomment-1036763050, it seem the least we could add in the code is a more verbose comment. Is there a fundamental setup conflict now, that block running a custom benchmark set on RISC-V, which needs to be resolved?

malus-brandywine commented 2 years ago

The solution requires some other modifications. I'll take care of it within next few days.

malus-brandywine commented 2 years ago

As follow up of the https://github.com/seL4/sel4bench/issues/17#issuecomment-1036763050 I checked the code.

Currently, event counters are supported only for HiFive, according to sel4bench.h

It means IPC bench which uses event counters will be built, but will NOT be functional for other platforms. IPC benchmark is ON by default.

Back to DefaultBenchDeps. Options depending on DefaultBenchDeps can be redefined in easy-settings.cmake and command line only if DefaultBenchDeps set to TRUE, otherwise benchmark options' values are set to local defaults.

Currently, the options depending on DefaultBenchDeps are only the ones that switch ON/OFF the whole corresponding application.

The patch will allow to manually pick up benchmark applications for RISC-V platforms, therefore to exclude IPC benchmark when needed.

malus-brandywine commented 2 years ago

The recent tests failed because the command line contained -DFAULT=TRUE for RISC-V.

With the option DefaultBenchDeps set to TRUE command line options make an effect now.

Should we change command line parameters in the test?

axel-h commented 2 years ago
malus-brandywine commented 2 years ago
  • Style nitpicking: please change the commit description to: "bench/risc-v: allow user to change options"

I definitely will.

  • concerning the configuration, I'm a bit lost now. So the blocker is that the apps/fault benchmark app fails on RISC-V? This should become a separate issue then, which needs to be fixed. Once that is done, it could be made a know issue where a (temporary) work around is disabling this for RISC-V in the hw tests, so the fine grained user configurability from this PR can be merged.

apps/fault never was supported for risc-v. The tests that use init-build.sh with parameter -DFAULT=TRUE were successful because the parameter was ignored (DefaultBenchDeps was FALSE) and by default apps/fault is not built.

So, the used command line: init-build.sh -DFASTPATH=TRUE -DHARDWARE=TRUE -DFAULT=TRUE -DRISCV64=TRUE -DPLATFORM=hifive was misleading (apps/hardware is off by default as well).

I will open new issue about unsupported apps/fault https://github.com/seL4/sel4bench/issues/25#issue-1141894620 (apps/hardware is successfully been built)

As for this PR, while apps/fault is not supported I would suggest to correct the command line for risc-v to:

init-build.sh -DFASTPATH=TRUE -DHARDWARE=TRUE -DRISCV64=TRUE -DPLATFORM=hifive

axel-h commented 2 years ago

Thanks, now I get it. Seem changing the CI workflow parameters can happen at these places:

malus-brandywine commented 2 years ago

Thank you, I'll fix it

kent-mcleod commented 2 years ago

Currently, the options depending on DefaultBenchDeps are only the ones that switch ON/OFF the whole corresponding application.

One of the configuration requirements of sel4bench is that it will only allow benchmarks to be configured and built that are supported. Otherwise someone who isn't intimately familiar with sel4bench can't tell that the benchmarks they've configured are unsupported. DefaultBenchDeps is sort of a big lock around several benchmarks that requires them to either be all supported by a platform, or none supported by a platform.

Removing the RISCV restriction should require that the minimum set of required benchmarks are now supported. It should then only be removed as a restriction only on the supported platforms that can successfully run these benchmarks.

malus-brandywine commented 2 years ago

In that case I will postpone the PR until the apps/fault is supported.

But I would suggest to remove parameters -DHARDWARE=TRUE -DFAULT=TRUE for HiFive from Performance webpage. For ones who aren't closely familiar with sel4bench it's misleading, they will not get those benchmarks anyway.

As well as from CI tests, to avoid confusion caused by difference of parameters here and there.

axel-h commented 2 years ago

@malus-brandywine Did you intend to close this or just accidently updated the master branch?

malus-brandywine commented 2 years ago

@axel-h I didn't drop the idea, but the commit has been postponed (prev. comment). I'm going to make another commit related to other task soon, it will go as another PR.

The changes I made in this commit are saved in a local branch. I'm going to revive them later.

kent-mcleod commented 2 years ago

But I would suggest to remove parameters -DHARDWARE=TRUE -DFAULT=TRUE for HiFive from Performance webpage. For ones who aren't closely familiar with sel4bench it's misleading, they will not get those benchmarks anyway.

As well as from CI tests, to avoid confusion caused by difference of parameters here and there.

This is a good idea

lsf37 commented 2 years ago

I'll have a look at that.