>> + }
>> +
>> + fclose(fp);
>> +
>> + return 0;
>> +}
>> +
>> +int get_snc_mode(void)
>> +{
>> + static int snc_mode;
>> +
>> + if (!snc_mode) {
>> + snc_mode = snc_nodes_per_l3_cache();
>> + if (!cpus_offline_empty()) {
>> + ksft_print_msg("Runtime SNC detection unreliable due to offline
>> CPUs!\n");
>> + ksft_print_msg("Setting SNC mode to disabled.\n");
>> + snc_mode = 1;
>> + snc_unreliable = 1;
>> + }
>> + }
>> +
>> + return snc_mode;
>> +}
>
> I think the SNC detection will be easier to understand if it is done in a
> single
> place. Can the static local variable and checks using the offline file
> instead be included in
> existing snc_nodes_per_l3_cache()?
Sure, that sounds good.
>
>> +
>> +/**
>> + * snc_kernel_support - Compare system reported cache size and resctrl
>> + * reported cache size to get an idea if SNC is supported on the kernel
>> side.
>
> This comment does not seem to match what the function does.
Oops, sorry, will fix it.
>
>> + *
>> + * Return: 0 if not supported, 1 if SNC is disabled or SNC is both enabled
>> and
>> + * supported, < 0 on failure.
>> + */
>> +int snc_kernel_support(void)
>> +{
>> + char node_path[PATH_MAX];
>> + struct stat statbuf;
>> + int ret;
>> +
>> + ret = get_snc_mode();
>> + /*
>> + * If SNC is disabled then its kernel support isn't important. If value
>> + * is smaller than 1 an error happened.
>
> How can a value smaller than 1 be returned?
I think I left it here by accident because I was experimenting with other ways
of detecting the snc mode and then it could return errors. Will remove it.
>
>> + */
>> + if (ret <= 1)
>> + return ret;
>> +
>> + snprintf(node_path, sizeof(node_path), "%s/%s/%s", RESCTRL_PATH,
>> "mon_data",
>> + "mon_L3_00/mon_sub_L3_00");
>> +
>> + if (!stat(node_path, &statbuf))
>> + return 1;
>> +
>> + return 0;
>> +}
>
>
> Reinette
--
Kind regards
Maciej Wieczór-Retman
Hi Reinette!
On 2024-01-23 at 09:42:07 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/22/2024 11:58 PM, Maciej Wieczór-Retman wrote:
>> On 2024-01-22 at 08:32:36 -0800, Reinette Chatre wrote:
>>> Hi Maciej,
>>>
>>> On 1/21/2024 11:56 PM, Maciej Wieczó
On 2024-01-22 at 08:32:36 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/21/2024 11:56 PM, Maciej Wieczór-Retman wrote:
>> Hi!
>>
>> On 2024-01-19 at 08:39:31 -0800, Reinette Chatre wrote:
>>> Hi Maciej,
>>>
>>> On 1/18/2024 11:37 PM, Macie
Hi!
On 2024-01-19 at 08:39:31 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/18/2024 11:37 PM, Maciej Wieczór-Retman wrote:
>> On 2024-01-18 at 09:15:46 -0800, Reinette Chatre wrote:
>>> On 1/18/2024 4:02 AM, Maciej Wieczór-Retman wrote:
>>>> On 2024-01-1
Hello!
On 2024-01-18 at 09:15:46 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/18/2024 4:02 AM, Maciej Wieczór-Retman wrote:
>> On 2024-01-17 at 10:49:06 -0800, Reinette Chatre wrote:
>>> On 1/17/2024 12:26 AM, Maciej Wieczór-Retman wrote:
>>>> On 2024-01-0
On 2024-01-17 at 10:49:06 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/17/2024 12:26 AM, Maciej Wieczór-Retman wrote:
>> On 2024-01-08 at 14:42:11 -0800, Reinette Chatre wrote:
>>> On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
>
>
>>>> +
&
* resctrl_test:resctrl test definition
>> * @name: Test name
>> + * @group: Test group (e.g., L2 and L3 CAT test belong to CAT
>> group), can be NULL
>
>Could you please remove references to L2 CAT test that is not available yet? A
>detailed description of what a test group is will be helpful.
Sure, thanks for catching this!
>Reinette
>
--
Kind regards
Maciej Wieczór-Retman
esult of the request. If
>resource_info_unsigned_get()
>returns 0 then @val will contain the value read.
Right, that might be confusing. Will fix according to your comment, thanks!
>> +perror("Could not get sparse_masks contents\n");
>> +fclose(fp);
>> +return -1;
>> +}
>> +
>> +fclose(fp);
>> +return ret;
>> +}
>> +
>> /*
>> * create_bit_mask- Create bit mask from start, len pair
>> * @start: LSB of the mask
>
>Reinette
--
Kind regards
Maciej Wieczór-Retman
s", INFO_PATH, resource,
>> + feature);
>> +
>> +if (stat(res_path, &statbuf))
>> +return false;
>
>I think it will be more robust to look at statbuf to learn if the file type
>is correct and the file is actually readable.
Could that file be unreadable or of wrong type?
Also I thought that since read_info_res_file() opens and reads that file any
errors should be handled there. Shouldn't this part of the test only return
whether the file is there or not since that indicates if something is supported
in the kernel?
>> +
>> +return true;
>> +}
>> +
>> bool test_resource_feature_check(const struct resctrl_test *test)
>> {
>> -return validate_resctrl_feature_request(test->resource, NULL);
>> +return resctrl_resource_exists(test->resource);
>> }
>>
>> int filter_dmesg(void)
>
>Reinette
--
Kind regards
Maciej Wieczór-Retman
trlc_handler(int signum, siginfo_t *info, void *ptr);
>> int signal_handler_register(void);
>> @@ -201,5 +202,7 @@ extern struct resctrl_test mbm_test;
>> extern struct resctrl_test mba_test;
>> extern struct resctrl_test cmt_test;
>> extern struct resctrl_test l3_cat_test;
>> +extern struct resctrl_test l3_noncont_cat_test;
>> +extern struct resctrl_test l2_noncont_cat_test;
>>
>> #endif /* RESCTRL_H */
>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c
>> b/tools/testing/selftests/resctrl/resctrl_tests.c
>> index 3044179ee6e9..f3dc1b9696e7 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> @@ -19,6 +19,8 @@ static struct resctrl_test *resctrl_tests[] = {
>> &mba_test,
>> &cmt_test,
>> &l3_cat_test,
>> +&l3_noncont_cat_test,
>> +&l2_noncont_cat_test,
>> };
>>
>> static int detect_vendor(void)
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
>> b/tools/testing/selftests/resctrl/resctrlfs.c
>> index 8546421f0940..8bd30973fec3 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -100,7 +100,7 @@ int umount_resctrlfs(void)
>> *
>> * Return: cache level as integer or -1 if @cache_type is invalid.
>> */
>> -static int get_cache_level(const char *cache_type)
>> +int get_cache_level(const char *cache_type)
>> {
>> if (!strcmp(cache_type, "L3"))
>> return 3;
>
>
>Reinette
--
Kind regards
Maciej Wieczór-Retman
itionally call a function inside validate_resctrl_feature_request()
that would check for the existance of a particular file that would indicate if a
feature is supported or not?
Does implementing it as a new entry in resctrl_test struct that would hold the
desired filename seem reasonable? Or would it be better to pass it as a new
function argument (similiar to how "const char *feature" is used now)?
>> +return test_resource_feature_check(test);
>> +}
>> +
>> struct resctrl_test l3_cat_test = {
>> .name = "L3_CAT",
>> .group = "CAT",
>> @@ -357,3 +438,19 @@ struct resctrl_test l2_cat_test = {
>> .feature_check = test_resource_feature_check,
>> .run_test = cat_run_test,
>> };
>> +
>> +struct resctrl_test l3_noncont_cat_test = {
>> +.name = "L3_NONCONT_CAT",
>> +.group = "NONCONT_CAT",
>
>> +struct resctrl_test l2_noncont_cat_test = {
>> +.name = "L2_NONCONT_CAT",
>> +.group = "NONCONT_CAT",
>
>I think these should be grouped among "CAT" group because it well, tests
>CAT functionality. Why you think a separate group for them is needed?
It was convenient for my test purposes and I guess I didn't rethink that part
later. So you're right, it fits better grouped with CAT, I'll change it.
--
Kind regards
Maciej Wieczór-Retman
On 2023-10-27 at 16:30:38 +0300, Ilpo Järvinen wrote:
>On Fri, 27 Oct 2023, Maciej Wieczór-Retman wrote:
>
>> On 2023-10-24 at 12:26:31 +0300, Ilpo Järvinen wrote:
>> >Resource id is acquired differently depending on CPU. AMD tests use id
>> >from L3 cache, whereas CP
On 2023-10-27 at 15:32:58 +0300, Ilpo Järvinen wrote:
>On Fri, 27 Oct 2023, Maciej Wieczór-Retman wrote:
>> On 2023-10-24 at 12:26:26 +0300, Ilpo Järvinen wrote:
>> >- ksft_print_msg("%s Check cache miss rate within %lu%%\n",
>> >- ret
uld be possible to allow negative values for those cases, it
>would be more confusing to user.
"to user" -> "to the user"?
>
>Ignore failures from the tests where <= 2 were used to avoid false
>negative results.
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
years from now it could be handy to have something more specific instead of
"some newer CPUs".
>
>Add L2 CAT selftest. As measuring L2 misses is not easily available
>with perf, use L3 accesses as a proxy for L2 CAT working or not.
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
uot;CAT"
>group will enable to both L3 and L2 CAT tests.
"enable to both" -> "enable both"?
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
ed-off-by: Ilpo Järvinen
>---
> tools/testing/selftests/resctrl/resctrl.h | 2 +-
> tools/testing/selftests/resctrl/resctrl_val.c | 4 +--
> tools/testing/selftests/resctrl/resctrlfs.c | 31 ---
> 3 files changed, 23 insertions(+), 14 deletions(-)
>
--
Kind regards
Maciej Wieczór-Retman
n to make
>it reusable.
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
ruct input_params to hold are input parameters. It can be easily
"to hold are input parameters" -> "to hold input parameters"?
>passed to every test without varying the call signature.
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
t is passed.
>- ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
>+ ksft_print_msg("Percent diff=%.1f\n", avg_diff * 100);
>+ }
>+ *prev_avg_llc_val = avg_llc_val;
>
> show_cache_info(no_of_bits, avg_llc_val, cache_span, true);
>
> return ret;
> }
>
>@@ -143,54 +168,64 @@ static int cat_test(struct resctrl_val_param *param,
>size_t span)
> if (ret)
> return ret;
>
>+ buf = alloc_buffer(span, 1);
>+ if (buf == NULL)
Similiar to patch 01/24, wouldn't this:
if (!buf)
be better?
>+ return -1;
>+
--
Kind regards
Maciej Wieczór-Retman
;+ ret = perf_event_measure(pe_fd, &pe_read, param, bm_pid);
>+ if (ret)
>+ goto pe_close;
>+
>+ close(pe_fd);
>+ }
>+
>+ return ret;
>+
>+pe_close:
>+ close(pe_fd);
>+ return ret;
>+}
>+
> int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> {
> unsigned long l_mask, l_mask_1;
--
Kind regards
Maciej Wieczór-Retman
nd
>by doing it inside cat_val().
>
>Make also sizeof()s use safer way determine the right struct.
"use safer way determine" -> "use safer way to determine"?
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
ables are moved to local variables).
>
>Signed-off-by: Ilpo Järvinen
--
Kind regards
Maciej Wieczór-Retman
er get_cbm_mask() to construct the filename for
>get_bit_mask().
>
>Also mark cache_type const while at it.
>
>Co-developed-by: Fenghua Yu
>Signed-off-by: Fenghua Yu
>Signed-off-by: Ilpo Järvinen
>---
--
Kind regards
Maciej Wieczór-Retman
t64_t)rand();
>+ p64 += (CL_SIZE / sizeof(uint64_t));
>+ s64 -= (CL_SIZE / sizeof(uint64_t));
>+ }
>+
> /* Flush the memory before using to avoid "cache hot pages" effect */
> if (memflush)
>- mem_flush(buf, buf_size);
>+ mem_flush(p, buf_size);
Wouldn't renaming "p" to "buf" keep this relationship with "buf_size" more
explicit?
Or is naming void pointers "buffers" not appropriate?
>
>- return buf;
>+ return p;
> }
--
Kind regards
Maciej Wieczór-Retman
gt;+ unsigned char *buf;
>+ int ret;
>+
>+ buf = alloc_buffer(buf_size, memflush);
>+ if (buf == NULL)
Maybe just do:
if (!buf)?
Checkpatch also seems to suggest this approach:
CHECK: Comparison to NULL could be written "!buf"
#65: FILE: tools/testing/selftests/resctrl/fill_buf.c:159:
+ if (buf == NULL)
>+ return -1;
>+
--
Kind regards
Maciej Wieczór-Retman
rg/all/20231002094813.6633-1-ilpo.jarvi...@linux.intel.com/
>[3]
>https://lore.kernel.org/all/20230904095339.11321-1-ilpo.jarvi...@linux.intel.com/
>
>thanks,
>-- Shuah
>
Thank you!
--
Kind regards
Maciej Wieczór-Retman
On 2023-10-12 at 08:30:37 -0600, Shuah Khan wrote:
>On 10/12/23 01:32, Maciej Wieczór-Retman wrote:
>> On 2023-10-11 at 13:40:48 -0600, Shuah wrote:
>> > On 10/11/23 02:23, Maciej Wieczor-Retman wrote:
>> > > Kselftest header defines multiple variadic functions tha
"Prefer $new over __attribute__(($orig_attr$params))\n"
Please correct me if my train of thought is wrong but I think checkpatch
sees __printf() macro defined and it sees it's raw version
"__attribute__((format(printf, a, b)))" which it wants to replace with
the macro. But since the raw version is found in the define line that is
obviously not possible.
>thanks,
>-- Shuah
--
Kind regards
Maciej Wieczór-Retman
f (write(fd, schema, schema_len) < 0) {
>> +snprintf(reason, sizeof(reason),
>> + "write() failed : %s", strerror(errno));
>> +close(fd);
>> ret = -1;
>>
>> goto out;
>> }
>> -fclose(fp);
>> +close(fd);
>> +schema[schema_len - 1] = 0;
>>
>> out:
>> ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
>
>
>As changelog states, the newline is removed from schema to
>ensure it is printed correctly. Note that this is not done when an
>error is encountered during open() or write() so when an error is
>encountered in these places then the print does not look as intended.
>
>I think a new goto label inserted just before the newline removal
>should be sufficient, with the open() and write() error paths jumping
>to it.
>
>With that addressed:
>
>Reviewed-by: Reinette Chatre
>
>Reinette
Thanks, I'll make the correction and resend.
--
Kind regards
Maciej Wieczór-Retman
tion
>how these problems are found in the commit logs.
>
>thanks,
>-- Shuah
I wrote the first patch that adds the check to functions with format
specifiers and I compiled all selftests. Then I just corrected any
warnings that were found by the __printf attribute.
Should I mention the methodology in the cover letter?
--
Kind regards
Maciej Wieczór-Retman
schema) < 0) {
>> -sprintf(reason, "Failed to write schemata in control group");
>> - fclose(fp);
>> +if (write(fd, schema, schema_len) < 0) {
>> +snprintf(reason, sizeof(reason),
>> + "write() failed : %s", strerror(errno));
>> +close(fd);
>> ret = -1;
>>
>> goto out;
>> }
>> -fclose(fp);
>> +close(fd);
>> +schema[schema_len - 1] = 0;
>>
>> out:
>> ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
>
>Also please note that _if_ there is an early exit then uninitialized schema
>will be printed. Maybe this also needs a schema[1024] = {} ?
>
>Reinette
Thanks for pointing it out, I'll fix it for the next version.
--
Kind regards
Maciej Wieczór-Retman
On 2023-09-28 at 14:25:23 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/27/2023 11:46 PM, Maciej Wieczór-Retman wrote:
>> On 2023-09-27 at 15:15:06 -0700, Reinette Chatre wrote:
>>> On 9/22/2023 1:10 AM, Maciej Wieczor-Retman wrote:
>
>>>> diff
:", resource_id, '=', schemata);
>> +schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n",
>> + "MB:", resource_id, '=', schemata);
>>
>> -fp = fopen(controlgroup, "w");
>> -if (!fp) {
>> -sprintf(reason, "Failed to open control group");
>> +fd = open(controlgroup, O_WRONLY);
>> +if (!fd) {
>
>Be careful ... the error checking appropriate to the original
>pointer needs a double check with this new usage.
>According to "man 2 open" - open() returns -1 on error so I expect
>that this should rather be:
> if (fd < 0) {
>or
> if (fd == -1) {
>
>The rest looks good to me.
Thanks for catching this, that is very helpful.
--
Kind regards
Maciej Wieczór-Retman
is based on [2] which are based on
>> ksefltest next branch.
>
>ksefltest -> kselftest
>
>Reinette
--
Kind regards
Maciej Wieczór-Retman
Hi, looks good to me. Also ran it without problems on Intel(R) Xeon(R)
Platinum 8360Y.
Reviewed-by: Maciej Wieczor-Retman
Tested-by: Maciej Wieczor-Retman
--
Kind regards
Maciej Wieczór-Retman
On 2023-09-14 at 08:14:25 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/13/2023 11:01 PM, Maciej Wieczór-Retman wrote:
>> On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
>>> On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
>>>> On 2023-09-12 at 0
Hi,
On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
>> On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
>>> Hi Maciej,
>>>
>>> On 9/11/2023 11:32 PM, Maciej Wieczór-Retman w
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
>> On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
>>> Hi Maciej,
>>> When I build the tests with this applied I encounter the f
en multiple files anymore.
>>
>> Remove return comment from kernel-doc since the function is type void.
>>
>> Signed-off-by: Wieczor-Retman Maciej
>> Reviewed-by: Ilpo Järvinen
>
>Reviewed-by: Reinette Chatre
>
>Reinette
Thank you for the review!
--
Kind regards
Maciej Wieczór-Retman
eof(schema), "%s%d%c%s\n",
>> + "MB:", resource_id, '=', schemata);
>>
>> -fp = fopen(controlgroup, "w");
>> -if (!fp) {
>> +fd = open(controlgroup, O_WRONLY);
>> +if (!fd) {
>> sprintf(reason, "Failed to open control group");
>
>It makes code easier to understand and maintain if it is kept
>consistent. It is thus unexpected for open() error handling to
>be untouched while write() error handling is modified. I think
>the addition of errno in error handling of write() is helpful.
>Could you do the same for open()?
Okay, I'll add that, thanks.
--
Kind regards
Maciej Wieczór-Retman
41 matches
Mail list logo
