Re: [PATCH V2 00/19] Miscellaneous fixes for resctrl selftests

Re: [PATCH V2 00/19] Miscellaneous fixes for resctrl selftests

[Reinette Chatre]

Hi Sai,

On 5/18/2020 3:08 PM, Sai Praneeth Prakhya wrote:
> Fenghua Yu (1):
>   selftests/resctrl: Fix missing options "-n" and "-p"
> 
> Reinette Chatre (3):
>   selftests/resctrl: Fix typo
>   selftests/resctrl: Fix typo in help text
>   selftests/resctrl: Ensure sibling CPU is not same as original CPU
> 
> Sai Praneeth Prakhya (15):
>   selftests/resctrl: Rename CQM test as CMT test
>   selftests/resctrl: Declare global variables as extern
>   selftests/resctrl: Return if resctrl file system is not supported
>   selftests/resctrl: Check for resctrl mount point only if resctrl FS is
>     supported
>   selftests/resctrl: Use resctrl/info for feature detection
>   selftests/resctrl: Fix MBA/MBM results reporting format
>   selftests/resctrl: Abort running tests if not root user
>   selftests/resctrl: Enable gcc checks to detect buffer overflows
>   selftests/resctrl: Dynamically select buffer size for CAT test
>   selftests/resctrl: Skip the test if requested resctrl feature is not
>     supported
>   selftests/resctrl: Change return type of umount_resctrlfs() to void
>   selftests/resctrl: Umount resctrl FS only if mounted
>   selftests/resctrl: Unmount resctrl FS after running all tests
>   selftests/resctrl: Fix incorrect parsing of iMC counters
>   selftests/resctrl: Fix checking for < 0 for unsigned values
> 
>  tools/testing/selftests/resctrl/Makefile      |  2 +-
>  tools/testing/selftests/resctrl/README        |  4 +-
>  tools/testing/selftests/resctrl/cache.c       |  4 +-
>  tools/testing/selftests/resctrl/cat_test.c    |  8 +-
>  .../resctrl/{cqm_test.c => cmt_test.c}        | 23 +++---
>  tools/testing/selftests/resctrl/mba_test.c    | 23 +++---
>  tools/testing/selftests/resctrl/mbm_test.c    | 16 ++--
>  tools/testing/selftests/resctrl/resctrl.h     | 20 +++--
>  .../testing/selftests/resctrl/resctrl_tests.c | 69 ++++++++++++-----
>  tools/testing/selftests/resctrl/resctrl_val.c | 67 ++++++++++------
>  tools/testing/selftests/resctrl/resctrlfs.c   | 77 +++++++++++++------
>  11 files changed, 195 insertions(+), 118 deletions(-)
>  rename tools/testing/selftests/resctrl/{cqm_test.c => cmt_test.c} (88%)
> 

Thank you very much for creating these fixes. There are a few to which I
responded directly, the rest look good to me.

Reinette

RE: [PATCH V2 00/19] Miscellaneous fixes for resctrl selftests

[Prakhya, Sai Praneeth]

Hi Reinette,

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Thursday, May 21, 2020 9:12 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>;
> shuah@kernel.org; skhan@linuxfoundation.org; linux-kselftest@vger.kernel.org
> Cc: tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; Luck, Tony
> <tony.luck@intel.com>; babu.moger@amd.com; james.morse@arm.com;
> Shankar, Ravi V <ravi.v.shankar@intel.com>; Yu, Fenghua
> <fenghua.yu@intel.com>; x86@kernel.org; LKML <linux-
> kernel@vger.kernel.org>; dan.carpenter@oracle.com; dcb314@hotmail.com
> Subject: Re: [PATCH V2 00/19] Miscellaneous fixes for resctrl selftests
> 
> Hi Sai,
> 
> On 5/18/2020 3:08 PM, Sai Praneeth Prakhya wrote:
> > Fenghua Yu (1):
> >   selftests/resctrl: Fix missing options "-n" and "-p"

[SNIP]

> Thank you very much for creating these fixes. There are a few to which I
> responded directly, the rest look good to me.

Thanks a lot! for reviewing the patches. I will post a V3 addressing your comments.

Regards,
Sai

Re: [PATCH V2 15/19] selftests/resctrl: Change return type of umount_resctrlfs() to void

[Reinette Chatre]

Hi Sai,

On 5/21/2020 10:19 AM, Prakhya, Sai Praneeth wrote:
> Hi Reinette,
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Wednesday, May 20, 2020 4:52 PM
>> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>;
>> shuah@kernel.org; skhan@linuxfoundation.org; linux-kselftest@vger.kernel.org
>> Cc: tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; Luck, Tony
>> <tony.luck@intel.com>; babu.moger@amd.com; james.morse@arm.com;
>> Shankar, Ravi V <ravi.v.shankar@intel.com>; Yu, Fenghua
>> <fenghua.yu@intel.com>; x86@kernel.org; linux-kernel@vger.kernel;
>> dan.carpenter@oracle.com; dcb314@hotmail.com
>> Subject: Re: [PATCH V2 15/19] selftests/resctrl: Change return type of
>> umount_resctrlfs() to void
>>
>> Hi Sai,
>>
>> On 5/18/2020 3:08 PM, Sai Praneeth Prakhya wrote:
>>> umount_resctrlfs() is used only during tear down path and there is
>>> nothing much to do if unmount of resctrl file system fails, so, all
>>> the callers of this function are not checking for the return value.
>>> Hence, change the return type of this function from int to void.
>>
>> Should the callers be ignoring the return value? From what I can tell the
>> filesystem is unmounted between test runs so I wonder if it may help if the
>> return code is used and the test exits with an appropriate error to user space for
>> possible investigation instead of attempting to run a new test on top of the
>> resctrl filesystem that could potentially be having issues at the time.
> 
> Makes sense to me to check for the return value of umount() and take appropriate
> action rather than ignoring it. But, since this might happen very rarely (I haven't
> noticed umount() failing till now), I am thinking to queue this up for cleanup series.
> What do you think?

That sounds good.

> 
> This bug fixes series will then have patches 16 and 17 because they are fixing a bug
> that could be easily noticed. Please let me know if you think otherwise.

I don't, dropping this change that makes it easy to ignore an error in
this round so that any errors could be dealt with better in a later
patch sounds good to me.

Thank you

Reinette