Re: [LTP] [PATCH] Add hugemmap41(Migrating the libhugetlbfs/testcases/slbpacaflush.c test)

[Shirisha ganta <shirisha@linux.ibm.com>]

On Mon, 2024-02-19 at 23:10 +0100, Petr Vorel wrote:
> Hi Shirisha,
> 
> > We are verifying ppc64 kernels prior to 2.6.15-rc5 exhibit a bug in
> > the
> Test is from 2005, for 2.6.15-rc5. Is it really relevant now?
yes
> 
> > hugepage SLB flushing path. When opening new hugetlb areas,
> > updating masks
> > in the thread_struct and copying to the PACA only occurs on the CPU
> > where
> > segments are opened, leading to potential stale copies in other
> > CPUs.
> > This bug can be triggered by multiple threads sharing the mm or a
> > single thread
> > migrating between CPUs, particularly evident in a close-to-idle
> > system,
> > as other processes may flush the SLB and prevent the bug from
> > manifesting.
> 
> Please run  make check-hugemmap41 in the test directory and fix
> formatting.
will take care in V2
> 
> > Original test originates from 
> > https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/slbpacaflush.c
> > Signed-off-by: Shirisha G <shirisha@linux.ibm.com>
> > ---
> >  runtest/hugetlb                               |   1 +
> >  testcases/kernel/mem/.gitignore               |   1 +
> >  .../kernel/mem/hugetlb/hugemmap/hugemmap41.c  | 144
> > ++++++++++++++++++
> >  3 files changed, 146 insertions(+)
> >  create mode 100644
> > testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c
> > diff --git a/runtest/hugetlb b/runtest/hugetlb
> > index 299c07ac9..d956866ac 100644
> > --- a/runtest/hugetlb
> > +++ b/runtest/hugetlb
> > @@ -35,6 +35,7 @@ hugemmap29 hugemmap29
> >  hugemmap30 hugemmap30
> >  hugemmap31 hugemmap31
> >  hugemmap32 hugemmap32
> > +hugemmap41 hugemmap41
> nit: Any reason why not add it as hugemmap33? You don't want to clash
> with other
> sent test, right?
yes
> 
> >  hugemmap05_1 hugemmap05 -m
> >  hugemmap05_2 hugemmap05 -s
> >  hugemmap05_3 hugemmap05 -s -m
> > diff --git a/testcases/kernel/mem/.gitignore
> > b/testcases/kernel/mem/.gitignore
> > index c96fe8bfc..b7e108956 100644
> > --- a/testcases/kernel/mem/.gitignore
> > +++ b/testcases/kernel/mem/.gitignore
> > @@ -34,6 +34,7 @@
> >  /hugetlb/hugemmap/hugemmap30
> >  /hugetlb/hugemmap/hugemmap31
> >  /hugetlb/hugemmap/hugemmap32
> > +/hugetlb/hugemmap/hugemmap41
> >  /hugetlb/hugeshmat/hugeshmat01
> >  /hugetlb/hugeshmat/hugeshmat02
> >  /hugetlb/hugeshmat/hugeshmat03
> > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c
> > b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c
> > new file mode 100644
> > index 000000000..3b3388c68
> > --- /dev/null
> > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c
> > @@ -0,0 +1,144 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2005-2006 IBM Corporation.
> > + * Author: David Gibson & Adam Litke
> > + */
> > +/*\
> > + * [Description]
> > + *
> > + * ppc64 kernels (prior to 2.6.15-rc5) have a bug in the hugepage
> > SLB
> > + * flushing path.  After opening new hugetlb areas, we update the
> > + * masks in the thread_struct, copy to the PACA, then do slbies on
> > + * each CPU.  The trouble is we only copy to the PACA on the CPU
> > where
> > + * we're opening the segments, which can leave a stale copy in the
> > + * PACAs on other CPUs.
> > + *
> > + * This can be triggered either with multiple threads sharing the
> > mm,
> > + * or with a single thread which is migrated from one CPU, to
> > another
> > + * (where the mapping occurs), then back again (where we touch the
> > + * stale SLB).  We use the second method in this test, since it's
> > + * easier to force (using sched_setaffinity).  However it relies
> > on a
> > + * close-to-idle system, if any process other than a kernel thread
> > + * runs on the first CPU between runs of the test process, the SLB
> > + * will be flushed and we won't trigger the bug, hence the
> > + * PASS_INCONCLUSIVE().  Obviously, this test won't work on a 1-
> > cpu
> > + * system (should get CONFIG() on the sched_setaffinity)
> > + *
> > + */
> > +#define _GNU_SOURCE
> > +#include "hugetlb.h"
> > +#define SYSFS_CPU_ONLINE_FMT	"/sys/devices/system/cpu/cpu%d/
> > online"
> > +#define MNTPOINT "hugetlbfs/"
> Could you please have these 2 #define above _GNU_SOURCE?
> (readablility)
will take care in V2
> > +
> > +
> > +#include <stdio.h>
> > +#include <sched.h>
> > +
> > +
> > +long hpage_size;
> > +int fd;
> > +void *p;
> > +volatile unsigned long *q;
> > +int online_cpus[2], err;
> > +cpu_set_t cpu0, cpu1;
> > +
> > +
> > +
> Please remove these blank lines above.
Sure.will take care in V2
> 
> > +void check_online_cpus(int online_cpus[], int nr_cpus_needed)
> > +{
> > +	char cpu_state, path_buf[64];
> > +	int total_cpus, cpu_idx, fd, ret, i;
> > +
> > +	total_cpus = get_nprocs_conf();
> > +	cpu_idx = 0;
> > +
> > +	if (get_nprocs() < nr_cpus_needed)
> > +		tst_res(TFAIL, "Atleast online %d cpus are required",
> > nr_cpus_needed);
> > +
> > +	for (i = 0; i < total_cpus && cpu_idx < nr_cpus_needed; i++) {
> nit: Maybe just use get_nprocs_conf() directly?
Sure.
> 
> > +		errno = 0;
> Is it errno reset really needed?
Since based on the error number we are failing the testcase so need it
> 
> > +		sprintf(path_buf, SYSFS_CPU_ONLINE_FMT, i);
> > +		fd = open(path_buf, O_RDONLY);
> > +		if (fd < 0) {
> > +			/* If 'online' is absent, the cpu cannot be
> > offlined */
> > +			if (errno == ENOENT) {
> > +				online_cpus[cpu_idx] = i;
> > +				cpu_idx++;
> > +				continue;
> > +			} else {
> > +				tst_res(TFAIL, "Unable to open %s: %s",
> > path_buf,
> > +				     strerror(errno));
> We have TERRNO:
> 				tst_res(TFAIL | TERRNO, "Unable to open
> %s: %s", path_buf);
will take care in V2
> > +			}
> > +		}
> > +
> > +		ret = read(fd, &cpu_state, 1);
> > +		if (ret < 1)
> > +			tst_res(TFAIL, "Unable to read %s: %s",
> > path_buf,
> > +			     strerror(errno));
> Maybe use SAFE_READ() ?
Am using read here because unable to read may potentially indicate a
test failure
> > +
> > +		if (cpu_state == '1') {
> > +			online_cpus[cpu_idx] = i;
> > +			cpu_idx++;
> > +		}
> > +
> > +		if (fd >= 0)
> > +			SAFE_CLOSE(fd);
> > +	}
> > +
> > +	if (cpu_idx < nr_cpus_needed)
> > +		tst_res(TFAIL, "Atleast %d online cpus were not found",
> > nr_cpus_needed);
> > +}
> > +
> > +
> > +static void run_test(void)
> > +{
> > +	check_online_cpus(online_cpus, 2);
> > +	CPU_ZERO(&cpu0);
> > +	CPU_SET(online_cpus[0], &cpu0);
> > +	CPU_ZERO(&cpu1);
> > +	CPU_SET(online_cpus[1], &cpu1);
> > +
> > +	err = sched_setaffinity(getpid(), CPU_SETSIZE/8, &cpu0);
> > +	if (err != 0)
> > +		tst_res(TFAIL, "sched_setaffinity(cpu%d): %s",
> > online_cpus[0],
> > +				strerror(errno));
> Again, please use TTERRNO.
Sure. will take care in V2
> > +
> > +	err = sched_setaffinity(getpid(), CPU_SETSIZE/8, &cpu1);
> Maybe define CPU_SETSIZE/8 at the top?
Sure.will take care in V2
> > +
> > +	if (err != 0)
> > +		tst_res(TFAIL, "sched_setaffinity(cpu%d): %s",
> > online_cpus[1],
> > +				strerror(errno));
> > +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE,
> > MAP_SHARED, fd, 0);
> > +
> > +	err = sched_setaffinity(getpid(), CPU_SETSIZE/8, &cpu0);
> > +	if (err != 0)
> > +		tst_res(TFAIL, "sched_setaffinity(cpu%d): %s",
> > online_cpus[0],
> > +				strerror(errno));
> > +	q = (volatile unsigned long *)(p + getpagesize());
> > +	*q = 0xdeadbeef;
> Why to set the address before end of testing? (yes, the original does
> it, but
> why?). Wouldn't be better to use guarded buffers instead?
> 
> https://github.com/linux-test-project/ltp/wiki/C-Test-API#131-guarded-buffers
We are trying to access and write into a memory location within the
mapped segment by adding the huge page size to the mapped address and
casting it to a volatile unsigned long pointer to ensure things work
fine.
> 
> Kind regards,
> Petr
> 
> > +
> > +	tst_res(TPASS, "Test Passed inconclusive");
> > +}
> > +
> > +static void setup(void)
> > +{
> > +	hpage_size = tst_get_hugepage_size();
> > +	fd = tst_creat_unlinked(MNTPOINT, 0);
> > +}
> > +
> > +void cleanup(void)
> > +{
> > +	if (fd > 0)
> > +		SAFE_CLOSE(fd);
> > +}
> > +
> > +static struct tst_test test = {
> > +	.needs_root = 1,
> > +	.mntpoint = MNTPOINT,
> > +	.needs_hugetlbfs = 1,
> > +	.needs_tmpdir = 1,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.test_all = run_test,
> > +	.hugepages = {1, TST_NEEDS},
> > +};


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp