diff mbox series

mm/gup_benchmark: update the documentation in Kconfig

Message ID 20200821032546.19992-1-song.bao.hua@hisilicon.com
State Accepted
Commit 4c6cd03ed88cbeed796d840a4f9c8ac082e82409
Headers show
Series mm/gup_benchmark: update the documentation in Kconfig | expand

Commit Message

Song Bao Hua (Barry Song) Aug. 21, 2020, 3:25 a.m. UTC
In the beginning, mm/gup_benchmark.c supported get_user_pages_fast()
only, but right now, it supports the benchmarking of a couple of
get_user_pages() related calls like:
* get_user_pages_fast()
* get_user_pages()
* pin_user_pages_fast()
* pin_user_pages()
The documentation is confusing and needs update.

Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 mm/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

John Hubbard Aug. 21, 2020, 3:51 a.m. UTC | #1
On 8/20/20 8:25 PM, Barry Song wrote:
> In the beginning, mm/gup_benchmark.c supported get_user_pages_fast()
> only, but right now, it supports the benchmarking of a couple of
> get_user_pages() related calls like:
> * get_user_pages_fast()
> * get_user_pages()
> * pin_user_pages_fast()
> * pin_user_pages()
> The documentation is confusing and needs update.

hmmm, it's not that confusing, given that pin_user_pages() and
get_user_pages() use the same underlying get_user_pages()
implementation.

> 
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> ---
>   mm/Kconfig | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 6c974888f86f..f7c9374da7b3 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -831,10 +831,10 @@ config PERCPU_STATS
>   	  be used to help understand percpu memory usage.
>   
>   config GUP_BENCHMARK
> -	bool "Enable infrastructure for get_user_pages_fast() benchmarking"
> +	bool "Enable infrastructure for get_user_pages() and related calls benchmarking"

If we really want to go to the trouble of tweaking this, then I'd go with
something more like:

"Enable infrastructure for get_user_pages() and pin_user_pages benchmarking"

...but I don't think it really warrants a patch just yet. *However*, my
judgment is skewed right now, because I'm planning a small patchset to split
up gup_benchmark a little bit, and to add some more testing and take advantage
of parts of it to do a dump_page() test. At which point "related calls" would
make more sense, but then it would be different enough that this patch would
still need changing.

So I'm inclined to just recommend leaving this alone for a bit, but if others
want to put it in, I'm OK with that too.

>   	help
>   	  Provides /sys/kernel/debug/gup_benchmark that helps with testing
> -	  performance of get_user_pages_fast().
> +	  performance of get_user_pages() and related calls.
>   
>   	  See tools/testing/selftests/vm/gup_benchmark.c
>   
> 

thanks,
Song Bao Hua (Barry Song) Aug. 21, 2020, 4:39 a.m. UTC | #2
> -----Original Message-----
> From: linux-kselftest-owner@vger.kernel.org
> [mailto:linux-kselftest-owner@vger.kernel.org] On Behalf Of John Hubbard
> Sent: Friday, August 21, 2020 3:51 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>;
> akpm@linux-foundation.org; linux-mm@kvack.org;
> linux-kselftest@vger.kernel.org
> Cc: Linuxarm <linuxarm@huawei.com>; linux-kernel@vger.kernel.org; Keith
> Busch <keith.busch@intel.com>; Ira Weiny <ira.weiny@intel.com>; Kirill A .
> Shutemov <kirill.shutemov@linux.intel.com>
> Subject: Re: [PATCH] mm/gup_benchmark: update the documentation in
> Kconfig
> 
> On 8/20/20 8:25 PM, Barry Song wrote:
> > In the beginning, mm/gup_benchmark.c supported get_user_pages_fast()
> > only, but right now, it supports the benchmarking of a couple of
> > get_user_pages() related calls like:
> > * get_user_pages_fast()
> > * get_user_pages()
> > * pin_user_pages_fast()
> > * pin_user_pages()
> > The documentation is confusing and needs update.
> 
> hmmm, it's not that confusing, given that pin_user_pages() and
> get_user_pages() use the same underlying get_user_pages()
> implementation.

get_user_pages_fast() is not get_user_pages(). It is a specific function :-)

In the beginning, gup_benchmark was only for get_user_pages_fast():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/mm/gup_benchmark.c?id=64c349f4ae78


> 
> >
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> > ---
> >   mm/Kconfig | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index 6c974888f86f..f7c9374da7b3 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -831,10 +831,10 @@ config PERCPU_STATS
> >   	  be used to help understand percpu memory usage.
> >
> >   config GUP_BENCHMARK
> > -	bool "Enable infrastructure for get_user_pages_fast() benchmarking"
> > +	bool "Enable infrastructure for get_user_pages() and related calls
> benchmarking"
> 
> If we really want to go to the trouble of tweaking this, then I'd go with
> something more like:
> 
> "Enable infrastructure for get_user_pages() and pin_user_pages benchmarking"
> 
> ...but I don't think it really warrants a patch just yet. *However*, my
> judgment is skewed right now, because I'm planning a small patchset to split
> up gup_benchmark a little bit, and to add some more testing and take
> advantage
> of parts of it to do a dump_page() test. At which point "related calls" would
> make more sense, but then it would be different enough that this patch would
> still need changing.

Ok. Fair enough. I was copying the "related calls" from:
Documentation/core-api/pin_user_pages.rst

> 
> So I'm inclined to just recommend leaving this alone for a bit, but if others
> want to put it in, I'm OK with that too.
> 
> >   	help
> >   	  Provides /sys/kernel/debug/gup_benchmark that helps with testing
> > -	  performance of get_user_pages_fast().
> > +	  performance of get_user_pages() and related calls.
> >
> >   	  See tools/testing/selftests/vm/gup_benchmark.c
> >

Thanks
Barry
diff mbox series

Patch

diff --git a/mm/Kconfig b/mm/Kconfig
index 6c974888f86f..f7c9374da7b3 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -831,10 +831,10 @@  config PERCPU_STATS
 	  be used to help understand percpu memory usage.
 
 config GUP_BENCHMARK
-	bool "Enable infrastructure for get_user_pages_fast() benchmarking"
+	bool "Enable infrastructure for get_user_pages() and related calls benchmarking"
 	help
 	  Provides /sys/kernel/debug/gup_benchmark that helps with testing
-	  performance of get_user_pages_fast().
+	  performance of get_user_pages() and related calls.
 
 	  See tools/testing/selftests/vm/gup_benchmark.c