Re: [PATCH 0/7] selftests/resctrl: Add resctrl selftest

[Fenghua Yu]

On Wed, Oct 17, 2018 at 11:04 AM Moger, Babu <Babu.Moger@xxxxxxx> wrote:
> 
>     Hi Fenghua,
>      My few comments.
> 
>     On 10/17/2018 09:40 AM, Moger, Babu wrote:
>     > On 10/16/2018 03:32 PM, Fenghua Yu wrote:
>     >>> From: Moger, Babu [mailto:Babu.Moger@xxxxxxx]
>     >>> On 10/16/2018 11:56 AM, Fenghua Yu wrote:
>     >>>> With more and more resctrl features are being added by Intel, AMD and
>     >>>> ARM, a test tool is becoming more and more useful to validate that
>     >>>> both hardware and software functionalities work as expected.
>     >>>
>     >>> I like the initiative here. It is always good to have a single code
>     base.
>     >>>
>     >>> One question. I see that there is a tool at https://github.com/intel/
>     intel-cmt-cat to test and verify the functionality of resctrl feature. I
>     also see some of the distros have this tool already.
>     >>> Is this tool going to replace intel-cmt-cat? I have not looked at the
>     >>> patches closely yet.
>     >>
>     >> No, the selftest in this patch set will not replace intel-cmt-cat or
>     >> vice versa.
>     >>
>     >> The selftest in this patch set has a different purpose from
>     intel-cmt-cat:
>     >> the selftest is a test tool which validates resctrl functionalities
>     while
>     >> intel-cmt-cat is mainly a utility that provides base library for higher
>     >> level applications including performance analysis tools, benchmark
>     measurement
>     >> tools, and potential resctrl tests. For example, running MBA test in the
>     >> selftests tells MBA working or not working (fail/pass) right way. The
>     >
>     > Ok. Sure. Let me take a look at selftest closely. Will send my feedback
>     soon.

Thanks.

>     >
>     >> intel-cmt-cat doesn't have this testing capability unless we extend the
>     >> tool.
>     >>
>     >> And intel-cmt-cat is maintained and developed by Intel. I don't think
>     it's
>     >> easy to extend it to AMD and ARM features. The selftest will be
>     maintained
>     >
>     > We1l.. We were hoping to have a common tool across. It makes it easy for
>     > distros. Probably, we can have a separate discussion on this.
>     >
>     >> and developed by the community and will hopefully cover all
>     architectures.
>     >>
>     >> We have seen a few issues recently in resctrl and may see more issues
>     >> while expending the features. A convevient selftest may be useful to
>     help
>     >> identify and fix those potential issues.
> 
>     I don't know the rules for selftest. Here are my general comments.
> 
>      1. File names are not consistent.
>       # ls *.c
>        fill_buf.c  mba.c  mbm.c  resctrl.c  resctrl_membw.c  resctrl_tests.c
>       Few files start with resctrl_ prefix and others don't.

All files are in resctrl directory. It's redundant to have resctrl prefix for
each file. Probably we will remove the resctrl prefix for all files to
keep uniform naming style.

> 
>      2. Do we need README(or USAGE) here? I had too

We can add that in next version.

> 
>      3. I saw lots of these errors.
>         "mba.c:111:2: error: âforâ loop initial declarations are only allowed
>     in C99 mode"
>       for (int i = 0; i < 10; i++) {
>       ^
> 
>       I had to change it to
>        int i;
>        for (i = 0; i < 10; i++) {

Will fix the issues.

Thanks.

-Fenghua