Re: [PATCH v2 2/2] add ksm test for smart-scan feature

[Stefan Roesch <shr@devkernel.io>]

Petr Vorel <pvorel@suse.cz> writes:

> Hi Stefan,
>
> ...
>> +++ b/testcases/kernel/mem/ksm/ksm07.c
>> @@ -0,0 +1,69 @@
>> +/*
>> + * Copyright (C) 2010-2017  Red Hat, Inc.
>> + *
>
>> + * This program is free software;  you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY;  without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>> + * the GNU General Public License for more details.
> NOTE: we use SPDX instead of verbose GPL (see ksm06.c).
>> + *
>
> NOTE: we have special doc format which starts like this (see ksm06.c):
> /*\
>  * [Description]
>  *
>  * ...
>> + * Kernel Samepage Merging (KSM)
>> + *
>> + * This adds a new ksm (kernel samepage merging) test to evaluate the new
>> + * smart scan feature. It allocates a page and fills it with 'a'
>> + * characters. It captures the pages_skipped counter, waits for a few
>> + * iterations and captures the pages_skipped counter again. The expectation
>> + * is that over 50% of the page scans are skipped (There is only one page
>> + * that has KSM enabled and it gets scanned during each iteration and it
>> + * cannot be de-duplicated).
>> + *
>> + * Prerequisites:
>> + *
>> + * 1) ksm and ksmtuned daemons need to be disabled. Otherwise, it could
>> + *    distrub the testing as they also change some ksm tunables depends
>> + *    on current workloads.
> Hm, we don't have to any helper in LTP API to detect this, so at least we
> document this.
>> + *
>> + */
>
> The result is then uploaded:
> https://github.com/linux-test-project/ltp/releases/download/20230929/metadata.20230929.html
>
> Therefore please use:
>
> // SPDX-License-Identifier: GPL-2.0-or-later
> /*
>  * Copyright (C) 2010-2017  Red Hat, Inc.
>  */
> /*\
>  * [Description]
>  *
>  * Kernel Samepage Merging (KSM)
>  *
>  * This adds a new ksm (kernel samepage merging) test to evaluate the new
>  * smart scan feature. It allocates a page and fills it with 'a'
>  * characters. It captures the pages_skipped counter, waits for a few
>  * iterations and captures the pages_skipped counter again. The expectation
>  * is that over 50% of the page scans are skipped (There is only one page
>  * that has KSM enabled and it gets scanned during each iteration and it
>  * cannot be de-duplicated).
>  *
>  * Prerequisites:
>  *
>  * 1) ksm and ksmtuned daemons need to be disabled. Otherwise, it could
>  *    distrub the testing as they also change some ksm tunables depends
>  *    on current workloads.
>  */
>

The next version will use the above comment as documentation.

>> +
>> +#include <sys/types.h>
>> +#include <sys/mman.h>
>> +#include <sys/stat.h>
>> +#include <sys/wait.h>
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <signal.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include "../include/mem.h"
>> +#include "ksm_common.h"
>> +
>> +static void verify_ksm(void)
>> +{
>> +	create_memory_for_smartscan();
> I wonder, if we reusable create_memory_for_smartscan() later. Maybe it should be
> put into ksm07 for the start.
>

I moved it over to ksm07.c and renamed it to create_memory.

> Also, the test needs to run on older kernel - exit with TCONF (which is not
> considered as a failure) instead of TBROK which does now:
> mem.c:488: TBROK: Failed to open FILE '/sys/kernel/mm/ksm/pages_skipped' for reading: ENOENT (2)
>

Changed it to TCONF.

> If the testing code stays in testcases/kernel/mem/lib/mem.c, you will have to
> stat() it. But if it's really just this test specific and you move it to
> ksm07.c, then you can simply add the handling via .save_restore.
>
>> +}
>> +
>> +static struct tst_test test = {
>> +	.needs_root = 1,
>> +	.forks_child = 1,
>> +	.options = (struct tst_option[]) {
>> +		{}
>> +	},
>> +	.save_restore = (const struct tst_path_val[]) {
>> +		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_TBROK},
>> +		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TBROK},
>> +		{"/sys/kernel/mm/ksm/smart_scan", "1",
>> +			TST_SR_SKIP_MISSING | TST_SR_TBROK_RO},
>> +		{}
>> +	},
>> +	.needs_kconfigs = (const char *const[]){
>> +		"CONFIG_KSM=y",
>> +		NULL
>> +	},
>> +	.test_all = verify_ksm,
>> +};
>
> Also, there are missing static:
>

These are declared as non-static in ksm_common.h.

> $ cd testcases/kernel/mem/ksm/; make check-ksm07
> CHECK testcases/kernel/mem/ksm/ksm07.c
> ksm07.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> make: [../../../../include/mk/rules.mk:56: check-ksm07] Error 1 (ignored)
> ksm07.c: note: in included file:
> ksm_common.h:14:5: warning: Symbol 'size' has no prototype or library ('tst_') prefix. Should it be static?
> ksm_common.h:14:29: warning: Symbol 'num' has no prototype or library ('tst_') prefix. Should it be static?
> ksm_common.h:14:38: warning: Symbol 'unit' has no prototype or library ('tst_') prefix. Should it be static?
> ksm_common.h:15:6: warning: Symbol 'opt_sizestr' has no prototype or library
> ('tst_') prefix. Should it be static?
> ksm_common.h:15:20: warning: Symbol 'opt_numstr' has no prototype or library
> ('tst_') prefix. Should it be static?
> ksm_common.h:15:33: warning: Symbol 'opt_unitstr' has no prototype or library
> ('tst_') prefix. Should it be static?
>
> Kind regards,
> Petr