Re: [PATCH net-next v20 01/14] mm: page_frag: add a test module for page_frag
On 10/8/24 21:59, Yunsheng Lin wrote:
On 2024/10/9 3:56, Shuah Khan wrote:
On 10/8/24 05:20, Yunsheng Lin wrote:
The testing is done by ensuring that the fragment allocated
from a frag_frag_cache instance is pushed into a ptr_ring
instance in a kthread binded to a specified cpu, and a kthread
binded to a specified cpu will pop the fragment from the
ptr_ring and free the fragment.
CC: Alexander Duyck
Signed-off-by: Yunsheng Lin
Reviewed-by: Alexander Duyck
Signed-off-by should be last. Same comment on all the other
Hi, Shuah
I used 'git am' to collect those tag, it seems that is the order
the tool applied, and I checking other applied commit, it seems
only Signed-off-by from the committer is the last, like the below
recent mm commit:
6901cf55de22
ff7f5ad7bce4
okay.
patches in this series. When you have 4 patches, it is a good
practice to add cover-letter.
I guess the cover-letter meant below?
https://lore.kernel.org/all/20241008112049.2279307-1-linyunsh...@huawei.com/
Somehow this isn't in my Inbox.
[snip]
...
+function run_manual_check()
+{
+ #
+ # Validate passed parameters. If there is wrong one,
+ # the script exists and does not execute further.
+ #
+ validate_passed_args $@
+
+ echo "Run the test with following parameters: $@"
Is this marker good enough to isolate the test results in the
dmesg? Include the test name in the message.
+ insmod $DRIVER $@ > /dev/null 2>&1
+ echo "Done."
Is this marker good enough to isolate the test results in the
dmesg? Include the test name in the message.
+ echo "Check the kernel ring buffer to see the summary."
Usually the test would run dmesg and filter out the test results
from the dmesg and include them in the test script output.
You can refer to other tests that do that: powerpc/scripts/hmi.sh
is one example.
Thanks, will check that.
thanks,
-- Shuah
Re: [PATCH net-next v20 01/14] mm: page_frag: add a test module for page_frag
On 2024/10/9 3:56, Shuah Khan wrote:
> On 10/8/24 05:20, Yunsheng Lin wrote:
>> The testing is done by ensuring that the fragment allocated
>> from a frag_frag_cache instance is pushed into a ptr_ring
>> instance in a kthread binded to a specified cpu, and a kthread
>> binded to a specified cpu will pop the fragment from the
>> ptr_ring and free the fragment.
>>
>> CC: Alexander Duyck
>> Signed-off-by: Yunsheng Lin
>> Reviewed-by: Alexander Duyck
>
> Signed-off-by should be last. Same comment on all the other
Hi, Shuah
I used 'git am' to collect those tag, it seems that is the order
the tool applied, and I checking other applied commit, it seems
only Signed-off-by from the committer is the last, like the below
recent mm commit:
6901cf55de22
ff7f5ad7bce4
> patches in this series. When you have 4 patches, it is a good
> practice to add cover-letter.
I guess the cover-letter meant below?
https://lore.kernel.org/all/20241008112049.2279307-1-linyunsh...@huawei.com/
>
>> ---
>> tools/testing/selftests/mm/Makefile | 3 +
>> tools/testing/selftests/mm/page_frag/Makefile | 18 ++
>> .../selftests/mm/page_frag/page_frag_test.c | 173 ++
>> tools/testing/selftests/mm/run_vmtests.sh | 8 +
>> tools/testing/selftests/mm/test_page_frag.sh | 171 +
>> 5 files changed, 373 insertions(+)
>> create mode 100644 tools/testing/selftests/mm/page_frag/Makefile
>> create mode 100644 tools/testing/selftests/mm/page_frag/page_frag_test.c
>> create mode 100755 tools/testing/selftests/mm/test_page_frag.sh
>>
>> diff --git a/tools/testing/selftests/mm/Makefile
>> b/tools/testing/selftests/mm/Makefile
>> index 02e1204971b0..acec529baaca 100644
>> --- a/tools/testing/selftests/mm/Makefile
>> +++ b/tools/testing/selftests/mm/Makefile
>> @@ -36,6 +36,8 @@ MAKEFLAGS += --no-builtin-rules
>> CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
>> $(TOOLS_INCLUDES)
>> LDLIBS = -lrt -lpthread -lm
>> +TEST_GEN_MODS_DIR := page_frag
>> +
>> TEST_GEN_FILES = cow
>> TEST_GEN_FILES += compaction_test
>> TEST_GEN_FILES += gup_longterm
>> @@ -126,6 +128,7 @@ TEST_FILES += test_hmm.sh
>> TEST_FILES += va_high_addr_switch.sh
>> TEST_FILES += charge_reserved_hugetlb.sh
>> TEST_FILES += hugetlb_reparenting_test.sh
>> +TEST_FILES += test_page_frag.sh
>> # required by charge_reserved_hugetlb.sh
>> TEST_FILES += write_hugetlb_memory.sh
>> diff --git a/tools/testing/selftests/mm/page_frag/Makefile
>> b/tools/testing/selftests/mm/page_frag/Makefile
>> new file mode 100644
>> index ..58dda74d50a3
>> --- /dev/null
>> +++ b/tools/testing/selftests/mm/page_frag/Makefile
>> @@ -0,0 +1,18 @@
>> +PAGE_FRAG_TEST_DIR := $(realpath $(dir $(abspath $(lastword
>> $(MAKEFILE_LIST)
>> +KDIR ?= $(abspath $(PAGE_FRAG_TEST_DIR)/../../../../..)
>> +
>> +ifeq ($(V),1)
>> +Q =
>> +else
>> +Q = @
>> +endif
>> +
>> +MODULES = page_frag_test.ko
>> +
>> +obj-m += page_frag_test.o
>> +
>> +all:
>> + +$(Q)make -C $(KDIR) M=$(PAGE_FRAG_TEST_DIR) modules
>> +
>> +clean:
>> + +$(Q)make -C $(KDIR) M=$(PAGE_FRAG_TEST_DIR) clean
>> diff --git a/tools/testing/selftests/mm/page_frag/page_frag_test.c
>> b/tools/testing/selftests/mm/page_frag/page_frag_test.c
>> new file mode 100644
>> index ..eeb2b6bc681a
>> --- /dev/null
>> +++ b/tools/testing/selftests/mm/page_frag/page_frag_test.c
>> @@ -0,0 +1,173 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> I think this would throw a checkpatch warning about
> comment should be "/*" and not "//"
using "git grep 'SPDX-License' mm", "//" seems like a more common
case.
And I did using './scripts/checkpatch.pl --strict --codespell', and
it does not throw a checkpatch warning.
>> +
>> +/*
>> + * Test module for page_frag cache
>> + *
...
>> +function run_manual_check()
>> +{
>> + #
>> + # Validate passed parameters. If there is wrong one,
>> + # the script exists and does not execute further.
>> + #
>> + validate_passed_args $@
>> +
>> + echo "Run the test with following parameters: $@"
>
> Is this marker good enough to isolate the test results in the
> dmesg? Include the test name in the message.
>
>
>> + insmod $DRIVER $@ > /dev/null 2>&1
>> + echo "Done."
>
> Is this marker good enough to isolate the test results in the
> dmesg? Include the test name in the message.
>
>> + echo "Check the kernel ring buffer to see the summary."
>
> Usually the test would run dmesg and filter out the test results
> from the dmesg and include them in the test script output.
>
> You can refer to other tests that do that: powerpc/scripts/hmi.sh
> is one example.
Thanks, will check that.
Re: [PATCH net-next v20 01/14] mm: page_frag: add a test module for page_frag
On 10/8/24 05:20, Yunsheng Lin wrote:
The testing is done by ensuring that the fragment allocated
from a frag_frag_cache instance is pushed into a ptr_ring
instance in a kthread binded to a specified cpu, and a kthread
binded to a specified cpu will pop the fragment from the
ptr_ring and free the fragment.
CC: Alexander Duyck
Signed-off-by: Yunsheng Lin
Reviewed-by: Alexander Duyck
Signed-off-by should be last. Same comment on all the other
patches in this series. When you have 4 patches, it is a good
practice to add cover-letter.
---
tools/testing/selftests/mm/Makefile | 3 +
tools/testing/selftests/mm/page_frag/Makefile | 18 ++
.../selftests/mm/page_frag/page_frag_test.c | 173 ++
tools/testing/selftests/mm/run_vmtests.sh | 8 +
tools/testing/selftests/mm/test_page_frag.sh | 171 +
5 files changed, 373 insertions(+)
create mode 100644 tools/testing/selftests/mm/page_frag/Makefile
create mode 100644 tools/testing/selftests/mm/page_frag/page_frag_test.c
create mode 100755 tools/testing/selftests/mm/test_page_frag.sh
diff --git a/tools/testing/selftests/mm/Makefile
b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..acec529baaca 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -36,6 +36,8 @@ MAKEFLAGS += --no-builtin-rules
CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
$(TOOLS_INCLUDES)
LDLIBS = -lrt -lpthread -lm
+TEST_GEN_MODS_DIR := page_frag
+
TEST_GEN_FILES = cow
TEST_GEN_FILES += compaction_test
TEST_GEN_FILES += gup_longterm
@@ -126,6 +128,7 @@ TEST_FILES += test_hmm.sh
TEST_FILES += va_high_addr_switch.sh
TEST_FILES += charge_reserved_hugetlb.sh
TEST_FILES += hugetlb_reparenting_test.sh
+TEST_FILES += test_page_frag.sh
# required by charge_reserved_hugetlb.sh
TEST_FILES += write_hugetlb_memory.sh
diff --git a/tools/testing/selftests/mm/page_frag/Makefile
b/tools/testing/selftests/mm/page_frag/Makefile
new file mode 100644
index ..58dda74d50a3
--- /dev/null
+++ b/tools/testing/selftests/mm/page_frag/Makefile
@@ -0,0 +1,18 @@
+PAGE_FRAG_TEST_DIR := $(realpath $(dir $(abspath $(lastword
$(MAKEFILE_LIST)
+KDIR ?= $(abspath $(PAGE_FRAG_TEST_DIR)/../../../../..)
+
+ifeq ($(V),1)
+Q =
+else
+Q = @
+endif
+
+MODULES = page_frag_test.ko
+
+obj-m += page_frag_test.o
+
+all:
+ +$(Q)make -C $(KDIR) M=$(PAGE_FRAG_TEST_DIR) modules
+
+clean:
+ +$(Q)make -C $(KDIR) M=$(PAGE_FRAG_TEST_DIR) clean
diff --git a/tools/testing/selftests/mm/page_frag/page_frag_test.c
b/tools/testing/selftests/mm/page_frag/page_frag_test.c
new file mode 100644
index ..eeb2b6bc681a
--- /dev/null
+++ b/tools/testing/selftests/mm/page_frag/page_frag_test.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0
I think this would throw a checkpatch warning about
comment should be "/*" and not "//"
+
+/*
+ * Test module for page_frag cache
+ *
+ * Copyright (C) 2024 Yunsheng Lin
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+static struct ptr_ring ptr_ring;
+static int nr_objs = 512;
+static atomic_t nthreads;
+static struct completion wait;
+static struct page_frag_cache test_nc;
+static int test_popped;
+static int test_pushed;
+
+static int nr_test = 200;
+module_param(nr_test, int, 0);
+MODULE_PARM_DESC(nr_test, "number of iterations to test");
+
+static bool test_align;
+module_param(test_align, bool, 0);
+MODULE_PARM_DESC(test_align, "use align API for testing");
+
+static int test_alloc_len = 2048;
+module_param(test_alloc_len, int, 0);
+MODULE_PARM_DESC(test_alloc_len, "alloc len for testing");
+
+static int test_push_cpu;
+module_param(test_push_cpu, int, 0);
+MODULE_PARM_DESC(test_push_cpu, "test cpu for pushing fragment");
+
+static int test_pop_cpu;
+module_param(test_pop_cpu, int, 0);
+MODULE_PARM_DESC(test_pop_cpu, "test cpu for popping fragment");
+
+static int page_frag_pop_thread(void *arg)
+{
+ struct ptr_ring *ring = arg;
+
+ pr_info("page_frag pop test thread begins on cpu %d\n",
+ smp_processor_id());
+
+ while (test_popped < nr_test) {
+ void *obj = __ptr_ring_consume(ring);
+
+ if (obj) {
+ test_popped++;
+ page_frag_free(obj);
+ } else {
+ cond_resched();
+ }
+ }
+
+ if (atomic_dec_and_test(&nthreads))
+ complete(&wait);
+
+ pr_info("page_frag pop test thread exits on cpu %d\n",
+ smp_processor_id());
+
+ return 0;
+}
+
+static int page_frag_push_thread(void *arg)
+{
+ struct ptr_ring *ring = arg;
+
+ pr_info("page_frag push test thread begins on cpu %d\n",
+ smp_processor_id());
+
+ while (test_pushed < nr_test) {
+ void *va;
+ int ret;
+
+ if (test_align) {
+ va = page
