Received-SPF: pass (google.com: domain of linux-kernel+bounces-20635-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 9 Jan 2024 11:13:16 +0200 (EET)
From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
cc: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>, 
    Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>, 
    LKML <linux-kernel@vger.kernel.org>, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v2 4/4] selftests/resctrl: Add non-contiguous CBMs CAT
 test
In-Reply-To: <fd6acf6a-2610-406b-b363-220121a45aee@intel.com>
Message-ID: <33787043-5823-6de4-4e5c-a24a136ba541@linux.intel.com>
References: <cover.1702392177.git.maciej.wieczor-retman@intel.com> <10c3afd7f62c63db31a3d4af86529144a5d7bbf9.1702392177.git.maciej.wieczor-retman@intel.com> <fd6acf6a-2610-406b-b363-220121a45aee@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Mon, 8 Jan 2024, Reinette Chatre wrote:

> Hi Maciej,
> 
> On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
> > Add tests for both L2 and L3 CAT to verify the return values
> > generated by writing non-contiguous CBMs don't contradict the
> > reported non-contiguous support information.
> > 
> > Use a logical XOR to confirm return value of write_schemata() and
> > non-contiguous CBMs support information match.
> > 
> > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> > ---
> > Changelog v2:
> > - Redo the patch message. (Ilpo)
> > - Tidy up __cpuid_count calls. (Ilpo)
> > - Remove redundant AND in noncont_mask calculations (Ilpo)
> > - Fix bit_center offset.
> > - Add newline before function return. (Ilpo)
> > - Group non-contiguous tests with CAT tests. (Ilpo)
> > - Use a helper for reading sparse_masks file. (Ilpo)
> > - Make get_cache_level() available in other source files. (Ilpo)
> > 
> >  tools/testing/selftests/resctrl/cat_test.c    | 75 +++++++++++++++++++
> >  tools/testing/selftests/resctrl/resctrl.h     |  3 +
> >  .../testing/selftests/resctrl/resctrl_tests.c |  2 +
> >  tools/testing/selftests/resctrl/resctrlfs.c   |  2 +-
> >  4 files changed, 81 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> > index 7dc7206b3b99..ecf553a89aae 100644
> > --- a/tools/testing/selftests/resctrl/cat_test.c
> > +++ b/tools/testing/selftests/resctrl/cat_test.c
> > @@ -292,6 +292,65 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
> >  	return ret;
> >  }
> >  
> > +static int noncont_cat_run_test(const struct resctrl_test *test,
> > +				const struct user_params *uparams)
> > +{
> > +	unsigned long full_cache_mask, cont_mask, noncont_mask;
> > +	unsigned int eax, ebx, ecx, edx, ret;
> > +	int level, bit_center, sparse_masks;
> > +	char schemata[64];
> > +
> > +	/* Check to compare sparse_masks content to cpuid output. */
> 
> "cpuid" -> "CPUID" (to note it is an instruction)
> 
> > +	sparse_masks = read_info_res_file(test->resource, "sparse_masks");
> > +	if (sparse_masks < 0)
> > +		return sparse_masks;
> > +
> > +	level = get_cache_level(test->resource);
> > +	if (level < 0)
> > +		return -EINVAL;
> > +	__cpuid_count(0x10, 4 - level, eax, ebx, ecx, edx);
> 
> Please do not invent relationships. Please replace the "4 - level" with
> specific index used that depends on particular cache. The cache level
> may not even be needed, just use the resource to determine the correct
> index.

This is actually my fault, I suggested Maciej could use arithmetics there.

> > +
> > +	return !ret == !sparse_masks;
> 
> Please return negative on error. Ilpo just did a big cleanup to address this.

Test failure is not same as an error. So tests should return negative for 
errors which prevent even running test at all, and 0/1 for test 
success/fail.

-- 
 i.