Re: [PATCH net-next v20 01/14] mm: page_frag: add a test module for page_frag

[Yunsheng Lin <linyunsheng@huawei.com>]

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 <alexander.duyck@gmail.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
> 
> 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-linyunsheng@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 000000000000..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 000000000000..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.