Re: [PATCH 2/3] mm/gup_benchmark: support pin_user_pages() and related calls

3 views
Skip to first unread message

kbuild test robot

unread,
Jan 27, 2020, 3:33:22 PM1/27/20
to kbu...@lists.01.org, Nick Desaulniers, clang-bu...@googlegroups.com
CC: kbuil...@lists.01.org
In-Reply-To: <20200125021115.7...@nvidia.com>
References: <20200125021115.7...@nvidia.com>
TO: John Hubbard <jhub...@nvidia.com>

Hi John,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200124]
[cannot apply to mmotm/master kselftest/next linus/master hch-configfs/for-next v5.5-rc7 v5.5-rc6 v5.5-rc5 v5.5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/John-Hubbard/mm-gup-track-FOLL_PIN-pages-follow-on-from-v12/20200126-035513
base: 702ccea170f07783bd002055a353a0866c062267
config: arm64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (git://gitmirror/llvm_project 0b83c5a78fae96dd66150e7a14c8c6d0292de01d)
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <l...@intel.com>

All warnings (new ones prefixed by >>):

>> mm/gup_benchmark.c:37:7: warning: overflow converting case value to switch condition type (3229116165 to 18446744072643700485) [-Wswitch]
case PIN_BENCHMARK:
^
mm/gup_benchmark.c:12:24: note: expanded from macro 'PIN_BENCHMARK'
#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
mm/gup_benchmark.c:36:7: warning: overflow converting case value to switch condition type (3229116164 to 18446744072643700484) [-Wswitch]
case PIN_FAST_BENCHMARK:
^
mm/gup_benchmark.c:11:28: note: expanded from macro 'PIN_FAST_BENCHMARK'
#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
mm/gup_benchmark.c:31:7: warning: overflow converting case value to switch condition type (3229116163 to 18446744072643700483) [-Wswitch]
case GUP_BENCHMARK:
^
mm/gup_benchmark.c:10:24: note: expanded from macro 'GUP_BENCHMARK'
#define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
mm/gup_benchmark.c:30:7: warning: overflow converting case value to switch condition type (3229116162 to 18446744072643700482) [-Wswitch]
case GUP_LONGTERM_BENCHMARK:
^
mm/gup_benchmark.c:9:32: note: expanded from macro 'GUP_LONGTERM_BENCHMARK'
#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
mm/gup_benchmark.c:29:7: warning: overflow converting case value to switch condition type (3229116161 to 18446744072643700481) [-Wswitch]
case GUP_FAST_BENCHMARK:
^
mm/gup_benchmark.c:8:28: note: expanded from macro 'GUP_FAST_BENCHMARK'
#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
mm/gup_benchmark.c:51:7: warning: overflow converting case value to switch condition type (3229116165 to 18446744072643700485) [-Wswitch]
case PIN_BENCHMARK:
^
mm/gup_benchmark.c:12:24: note: expanded from macro 'PIN_BENCHMARK'
#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
mm/gup_benchmark.c:50:7: warning: overflow converting case value to switch condition type (3229116164 to 18446744072643700484) [-Wswitch]
case PIN_FAST_BENCHMARK:
^
mm/gup_benchmark.c:11:28: note: expanded from macro 'PIN_FAST_BENCHMARK'
#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
^
include/uapi/asm-generic/ioctl.h:88:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
^
include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
(((dir) << _IOC_DIRSHIFT) | \
^
7 warnings generated.

vim +37 mm/gup_benchmark.c

23
24 static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
25 {
26 int i;
27
28 switch (cmd) {
29 case GUP_FAST_BENCHMARK:
30 case GUP_LONGTERM_BENCHMARK:
31 case GUP_BENCHMARK:
32 for (i = 0; i < nr_pages; i++)
33 put_page(pages[i]);
34 break;
35
36 case PIN_FAST_BENCHMARK:
> 37 case PIN_BENCHMARK:
38 unpin_user_pages(pages, nr_pages);
39 break;
40 }
41 }
42

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuil...@lists.01.org Intel Corporation
.config.gz

Nathan Chancellor

unread,
Jan 27, 2020, 3:52:54 PM1/27/20
to John Hubbard, Andrew Morton, Al Viro, Christoph Hellwig, Dan Williams, Dave Chinner, Ira Weiny, Jan Kara, Jason Gunthorpe, Jonathan Corbet, Jérôme Glisse, Kirill A . Shutemov, Michal Hocko, Mike Kravetz, Shuah Khan, Vlastimil Babka, linu...@vger.kernel.org, linux-...@vger.kernel.org, linux-k...@vger.kernel.org, linux...@vger.kernel.org, linu...@kvack.org, LKML, clang-bu...@googlegroups.com
Hi John,

On Fri, Jan 24, 2020 at 06:11:14PM -0800, John Hubbard wrote:
> Up until now, gup_benchmark supported testing of the
> following kernel functions:
>
> * get_user_pages(): via the '-U' command line option
> * get_user_pages_longterm(): via the '-L' command line option
> * get_user_pages_fast(): as the default (no options required)
>
> Add test coverage for the new corresponding pin_*() functions:
>
> * pin_user_pages_fast(): via the '-a' command line option
> * pin_user_pages(): via the '-b' command line option
>
> Also, add an option for clarity: '-u' for what is now (still) the
> default choice: get_user_pages_fast().
>
> Also, for the commands that set FOLL_PIN, verify that the pages
> really are dma-pinned, via the new is_dma_pinned() routine.
> Those commands are:
>
> PIN_FAST_BENCHMARK : calls pin_user_pages_fast()
> PIN_BENCHMARK : calls pin_user_pages()
>
> In between the calls to pin_*() and unpin_user_pages(),
> check each page: if page_dma_pinned() returns false, then
> WARN and return.
>
> Do this outside of the benchmark timestamps, so that it doesn't
> affect reported times.
>
> Reviewed-by: Ira Weiny <ira....@intel.com>
> Signed-off-by: John Hubbard <jhub...@nvidia.com>
> ---
> mm/gup_benchmark.c | 70 ++++++++++++++++++++--
> tools/testing/selftests/vm/gup_benchmark.c | 15 ++++-
> 2 files changed, 79 insertions(+), 6 deletions(-)
>
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index 8dba38e79a9f..3d5fb765e4e6 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -8,6 +8,8 @@
> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
> #define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> #define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
> +#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
> +#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
>
> struct gup_benchmark {
> __u64 get_delta_usec;
> @@ -19,6 +21,47 @@ struct gup_benchmark {
> __u64 expansion[10]; /* For future use */
> };
>
> +static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)

We received a Clang build report on this patch because the use of
PIN_FAST_BENCHMARK and PIN_BENCHMARK in the switch statement below will
overflow int; this should be unsigned int to match the cmd parameter in
the ioctls.

The report can be read here if you care for it:

https://groups.google.com/d/msg/clang-built-linux/gyGayC_dnis/D1celSStEgAJ

Cheers,
Nathan

> +{
> + int i;
> +
> + switch (cmd) {
> + case GUP_FAST_BENCHMARK:
> + case GUP_LONGTERM_BENCHMARK:
> + case GUP_BENCHMARK:
> + for (i = 0; i < nr_pages; i++)
> + put_page(pages[i]);
> + break;
> +
> + case PIN_FAST_BENCHMARK:
> + case PIN_BENCHMARK:
> + unpin_user_pages(pages, nr_pages);
> + break;
> + }
> +}
> +
> +static void verify_dma_pinned(int cmd, struct page **pages,
> + unsigned long nr_pages)
> +{
> + int i;
> + struct page *page;
> +
> + switch (cmd) {
> + case PIN_FAST_BENCHMARK:
> + case PIN_BENCHMARK:
> + for (i = 0; i < nr_pages; i++) {
> + page = pages[i];
> + if (WARN(!page_dma_pinned(page),
> + "pages[%d] is NOT dma-pinned\n", i)) {
> +
> + dump_page(page, "gup_benchmark failure");
> + break;
> + }
> + }
> + break;
> + }
> +}
> +
> static int __gup_benchmark_ioctl(unsigned int cmd,
> struct gup_benchmark *gup)
> {
> @@ -66,6 +109,14 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> nr = get_user_pages(addr, nr, gup->flags, pages + i,
> NULL);
> break;
> + case PIN_FAST_BENCHMARK:
> + nr = pin_user_pages_fast(addr, nr, gup->flags,
> + pages + i);
> + break;
> + case PIN_BENCHMARK:
> + nr = pin_user_pages(addr, nr, gup->flags, pages + i,
> + NULL);
> + break;
> default:
> kvfree(pages);
> ret = -EINVAL;
> @@ -78,15 +129,22 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> }
> end_time = ktime_get();
>
> + /* Shifting the meaning of nr_pages: now it is actual number pinned: */
> + nr_pages = i;
> +
> gup->get_delta_usec = ktime_us_delta(end_time, start_time);
> gup->size = addr - gup->addr;
>
> + /*
> + * Take an un-benchmark-timed moment to verify DMA pinned
> + * state: print a warning if any non-dma-pinned pages are found:
> + */
> + verify_dma_pinned(cmd, pages, nr_pages);
> +
> start_time = ktime_get();
> - for (i = 0; i < nr_pages; i++) {
> - if (!pages[i])
> - break;
> - put_page(pages[i]);
> - }
> +
> + put_back_pages(cmd, pages, nr_pages);
> +
> end_time = ktime_get();
> gup->put_delta_usec = ktime_us_delta(end_time, start_time);
>
> @@ -105,6 +163,8 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
> case GUP_FAST_BENCHMARK:
> case GUP_LONGTERM_BENCHMARK:
> case GUP_BENCHMARK:
> + case PIN_FAST_BENCHMARK:
> + case PIN_BENCHMARK:
> break;
> default:
> return -EINVAL;
> diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
> index 389327e9b30a..43b4dfe161a2 100644
> --- a/tools/testing/selftests/vm/gup_benchmark.c
> +++ b/tools/testing/selftests/vm/gup_benchmark.c
> @@ -18,6 +18,10 @@
> #define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> #define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
>
> +/* Similar to above, but use FOLL_PIN instead of FOLL_GET. */
> +#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
> +#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
> +
> /* Just the flags we need, copied from mm.h: */
> #define FOLL_WRITE 0x01 /* check pte is writable */
>
> @@ -40,8 +44,14 @@ int main(int argc, char **argv)
> char *file = "/dev/zero";
> char *p;
>
> - while ((opt = getopt(argc, argv, "m:r:n:f:tTLUwSH")) != -1) {
> + while ((opt = getopt(argc, argv, "m:r:n:f:abtTLUuwSH")) != -1) {
> switch (opt) {
> + case 'a':
> + cmd = PIN_FAST_BENCHMARK;
> + break;
> + case 'b':
> + cmd = PIN_BENCHMARK;
> + break;
> case 'm':
> size = atoi(optarg) * MB;
> break;
> @@ -63,6 +73,9 @@ int main(int argc, char **argv)
> case 'U':
> cmd = GUP_BENCHMARK;
> break;
> + case 'u':
> + cmd = GUP_FAST_BENCHMARK;
> + break;
> case 'w':
> write = 1;
> break;
> --
> 2.25.0
>

John Hubbard

unread,
Jan 27, 2020, 4:32:19 PM1/27/20
to Nathan Chancellor, Andrew Morton, Al Viro, Christoph Hellwig, Dan Williams, Dave Chinner, Ira Weiny, Jan Kara, Jason Gunthorpe, Jonathan Corbet, Jérôme Glisse, Kirill A . Shutemov, Michal Hocko, Mike Kravetz, Shuah Khan, Vlastimil Babka, linu...@vger.kernel.org, linux-...@vger.kernel.org, linux-k...@vger.kernel.org, linux...@vger.kernel.org, linu...@kvack.org, LKML, clang-bu...@googlegroups.com
On 1/27/20 12:52 PM, Nathan Chancellor wrote:
...
>> --- a/mm/gup_benchmark.c
>> +++ b/mm/gup_benchmark.c
>> @@ -8,6 +8,8 @@
>> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
>> #define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
>> #define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
>> +#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
>> +#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
>>
>> struct gup_benchmark {
>> __u64 get_delta_usec;
>> @@ -19,6 +21,47 @@ struct gup_benchmark {
>> __u64 expansion[10]; /* For future use */
>> };
>>
>> +static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
>
> We received a Clang build report on this patch because the use of
> PIN_FAST_BENCHMARK and PIN_BENCHMARK in the switch statement below will
> overflow int; this should be unsigned int to match the cmd parameter in
> the ioctls.


Thanks for the report! Yes, that should have been "unsigned int cmd", to match the
one in the ioctls, just as you said.

I'll apply this diff, for the next version of the series:

diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index 3d5fb765e4e6..7d5573083ab3 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -21,7 +21,8 @@ struct gup_benchmark {
__u64 expansion[10]; /* For future use */
};

-static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
+static void put_back_pages(unsigned int cmd, struct page **pages,
+ unsigned long nr_pages)
{
int i;

@@ -40,7 +41,7 @@ static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
}
}

-static void verify_dma_pinned(int cmd, struct page **pages,
+static void verify_dma_pinned(unsigned int cmd, struct page **pages,
unsigned long nr_pages)
{
int i;



thanks,
--
John Hubbard
NVIDIA
Reply all
Reply to author
Forward
0 new messages