linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
@ 2020-05-27 21:02 Souptick Joarder
  2020-05-28 11:04 ` Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Souptick Joarder @ 2020-05-27 21:02 UTC (permalink / raw)
  To: rspringer, toddpoynor, benchan, gregkh
  Cc: devel, linux-kernel, Souptick Joarder, John Hubbard

This code was using get_user_pages_fast(), in a "Case 2" scenario
(DMA/RDMA), using the categorization from [1]. That means that it's
time to convert the get_user_pages_fast() + put_page() calls to
pin_user_pages_fast() + unpin_user_page() calls.

There is some helpful background in [2]: basically, this is a small
part of fixing a long-standing disconnect between pinning pages, and
file systems' use of those pages.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
	https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>

Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.
---
 drivers/staging/gasket/gasket_page_table.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index f6d7157..d712ad4 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -449,7 +449,7 @@ static bool gasket_release_page(struct page *page)
 
 	if (!PageReserved(page))
 		SetPageDirty(page);
-	put_page(page);
+	unpin_user_page(page);
 
 	return true;
 }
@@ -486,12 +486,12 @@ static int gasket_perform_mapping(struct gasket_page_table *pg_tbl,
 			ptes[i].dma_addr = pg_tbl->coherent_pages[0].paddr +
 					   off + i * PAGE_SIZE;
 		} else {
-			ret = get_user_pages_fast(page_addr - offset, 1,
+			ret = pin_user_pages_fast(page_addr - offset, 1,
 						  FOLL_WRITE, &page);
 
 			if (ret <= 0) {
 				dev_err(pg_tbl->device,
-					"get user pages failed for addr=0x%lx, offset=0x%lx [ret=%d]\n",
+					"pin user pages failed for addr=0x%lx, offset=0x%lx [ret=%d]\n",
 					page_addr, offset, ret);
 				return ret ? ret : -ENOMEM;
 			}
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-27 21:02 [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*() Souptick Joarder
@ 2020-05-28 11:04 ` Dan Carpenter
  2020-05-29  6:16   ` Souptick Joarder
  2020-05-29 11:53   ` Dan Carpenter
  0 siblings, 2 replies; 12+ messages in thread
From: Dan Carpenter @ 2020-05-28 11:04 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: rspringer, toddpoynor, benchan, gregkh, devel, John Hubbard,
	linux-kernel

On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> This code was using get_user_pages_fast(), in a "Case 2" scenario
> (DMA/RDMA), using the categorization from [1]. That means that it's
> time to convert the get_user_pages_fast() + put_page() calls to
> pin_user_pages_fast() + unpin_user_page() calls.

You are saying that the page is used for DIO and not DMA, but it sure
looks to me like it is used for DMA.

   503                          /* Map the page into DMA space. */
   504                          ptes[i].dma_addr =
   505                                  dma_map_page(pg_tbl->device, page, 0, PAGE_SIZE,
   506                                               DMA_BIDIRECTIONAL);

To be honest, that starting paragraph was confusing.  At first I thought
you were saying gasket was an RDMA driver. :P  I shouldn't have to read
a different document to understand the commit message.  It should be
summarized enough and the other documentation is supplemental.

"In 2019 we introduced pin_user_pages() and now we are converting
get_user_pages() to the new API as appropriate".

> 
> There is some helpful background in [2]: basically, this is a small
> part of fixing a long-standing disconnect between pinning pages, and
> file systems' use of those pages.

What is the impact of this patch on runtime?

> 
> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
> 	https://lwn.net/Articles/807108/
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> 
> Hi,
> 
> I'm compile tested this, but unable to run-time test, so any testing
> help is much appriciated.
> ---

The "Hi" part of patch should have been under the "---" cut off line so
this will definitely need to be resent.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-28 11:04 ` Dan Carpenter
@ 2020-05-29  6:16   ` Souptick Joarder
  2020-05-29  6:27     ` Souptick Joarder
  2020-05-29 11:53   ` Dan Carpenter
  1 sibling, 1 reply; 12+ messages in thread
From: Souptick Joarder @ 2020-05-29  6:16 UTC (permalink / raw)
  To: Dan Carpenter, John Hubbard
  Cc: Rob Springer, Todd Poynor, benchan, Greg KH,
	open list:ANDROID DRIVERS, linux-kernel

On Thu, May 28, 2020 at 4:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> > This code was using get_user_pages_fast(), in a "Case 2" scenario
> > (DMA/RDMA), using the categorization from [1]. That means that it's
> > time to convert the get_user_pages_fast() + put_page() calls to
> > pin_user_pages_fast() + unpin_user_page() calls.
>
> You are saying that the page is used for DIO and not DMA, but it sure
> looks to me like it is used for DMA.

No, I was referring to "Case 2" scenario in change log which means  it is
used for DMA, not DIO.

>
>    503                          /* Map the page into DMA space. */
>    504                          ptes[i].dma_addr =
>    505                                  dma_map_page(pg_tbl->device, page, 0, PAGE_SIZE,
>    506                                               DMA_BIDIRECTIONAL);
>
> To be honest, that starting paragraph was confusing.  At first I thought
> you were saying gasket was an RDMA driver. :P  I shouldn't have to read
> a different document to understand the commit message.  It should be
> summarized enough and the other documentation is supplemental.
>
> "In 2019 we introduced pin_user_pages() and now we are converting
> get_user_pages() to the new API as appropriate".

As all other similar conversion have similar change logs, so I was trying
to maintain the same. John might have a different opinion on this.

John, Any further opinion ??

>
> >
> > There is some helpful background in [2]: basically, this is a small
> > part of fixing a long-standing disconnect between pinning pages, and
> > file systems' use of those pages.
>
> What is the impact of this patch on runtime?

I don't have the hardware to validate the runtime impact and will
wait if someone is going to validate it for runtime impact.

>
> >
> > [1] Documentation/core-api/pin_user_pages.rst
> >
> > [2] "Explicit pinning of user-space pages":
> >       https://lwn.net/Articles/807108/
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> >
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> > ---
>
> The "Hi" part of patch should have been under the "---" cut off line so
> this will definitely need to be resent.

Sorry about it.
Will wait for feedback from John before resend it :)

>
> regards,
> dan carpenter
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-29  6:16   ` Souptick Joarder
@ 2020-05-29  6:27     ` Souptick Joarder
  2020-05-29  7:38       ` John Hubbard
  2020-05-29  7:46       ` Dan Carpenter
  0 siblings, 2 replies; 12+ messages in thread
From: Souptick Joarder @ 2020-05-29  6:27 UTC (permalink / raw)
  To: Dan Carpenter, John Hubbard
  Cc: Rob Springer, Todd Poynor, benchan, Greg KH,
	open list:ANDROID DRIVERS, linux-kernel

On Fri, May 29, 2020 at 11:46 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 4:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> > > This code was using get_user_pages_fast(), in a "Case 2" scenario
> > > (DMA/RDMA), using the categorization from [1]. That means that it's
> > > time to convert the get_user_pages_fast() + put_page() calls to
> > > pin_user_pages_fast() + unpin_user_page() calls.
> >
> > You are saying that the page is used for DIO and not DMA, but it sure
> > looks to me like it is used for DMA.
>
> No, I was referring to "Case 2" scenario in change log which means  it is
> used for DMA, not DIO.
>
> >
> >    503                          /* Map the page into DMA space. */
> >    504                          ptes[i].dma_addr =
> >    505                                  dma_map_page(pg_tbl->device, page, 0, PAGE_SIZE,
> >    506                                               DMA_BIDIRECTIONAL);
> >
> > To be honest, that starting paragraph was confusing.  At first I thought
> > you were saying gasket was an RDMA driver. :P  I shouldn't have to read
> > a different document to understand the commit message.  It should be
> > summarized enough and the other documentation is supplemental.
> >
> > "In 2019 we introduced pin_user_pages() and now we are converting
> > get_user_pages() to the new API as appropriate".
>
> As all other similar conversion have similar change logs, so I was trying
> to maintain the same. John might have a different opinion on this.

For example, I was referring to few recent similar commits for change logs.

http://lkml.kernel.org/r/20200519002124.2025955-5-jhubbard@nvidia.com
https://lore.kernel.org/r/20200518015237.1568940-1-jhubbard@nvidia.com


>
> John, Any further opinion ??
>
> >
> > >
> > > There is some helpful background in [2]: basically, this is a small
> > > part of fixing a long-standing disconnect between pinning pages, and
> > > file systems' use of those pages.
> >
> > What is the impact of this patch on runtime?
>
> I don't have the hardware to validate the runtime impact and will
> wait if someone is going to validate it for runtime impact.
>
> >
> > >
> > > [1] Documentation/core-api/pin_user_pages.rst
> > >
> > > [2] "Explicit pinning of user-space pages":
> > >       https://lwn.net/Articles/807108/
> > >
> > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > Cc: John Hubbard <jhubbard@nvidia.com>
> > >
> > > Hi,
> > >
> > > I'm compile tested this, but unable to run-time test, so any testing
> > > help is much appriciated.
> > > ---
> >
> > The "Hi" part of patch should have been under the "---" cut off line so
> > this will definitely need to be resent.
>
> Sorry about it.
> Will wait for feedback from John before resend it :)
>
> >
> > regards,
> > dan carpenter
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-29  6:27     ` Souptick Joarder
@ 2020-05-29  7:38       ` John Hubbard
  2020-05-29  7:45         ` Dan Carpenter
  2020-05-29  7:46       ` Dan Carpenter
  1 sibling, 1 reply; 12+ messages in thread
From: John Hubbard @ 2020-05-29  7:38 UTC (permalink / raw)
  To: Souptick Joarder, Dan Carpenter
  Cc: Rob Springer, Todd Poynor, benchan, Greg KH,
	open list:ANDROID DRIVERS, linux-kernel

On 2020-05-28 23:27, Souptick Joarder wrote:
> On Fri, May 29, 2020 at 11:46 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>>
>> On Thu, May 28, 2020 at 4:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>
>>> On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
>>>> This code was using get_user_pages_fast(), in a "Case 2" scenario
>>>> (DMA/RDMA), using the categorization from [1]. That means that it's
>>>> time to convert the get_user_pages_fast() + put_page() calls to
>>>> pin_user_pages_fast() + unpin_user_page() calls.
>>>
>>> You are saying that the page is used for DIO and not DMA, but it sure
>>> looks to me like it is used for DMA.
>>
>> No, I was referring to "Case 2" scenario in change log which means  it is
>> used for DMA, not DIO.

Hi,

Dan, I also uncertain as to how you read this as referring to DIO. Case 2 is
DMA or RDMA, and in fact the proposed commit log says both of those things:
Case 2 and DMA/RDMA. I don't see "DIO" anywhere here...


>>
>>>
>>>     503                          /* Map the page into DMA space. */
>>>     504                          ptes[i].dma_addr =
>>>     505                                  dma_map_page(pg_tbl->device, page, 0, PAGE_SIZE,
>>>     506                                               DMA_BIDIRECTIONAL);
>>>
>>> To be honest, that starting paragraph was confusing.  At first I thought
>>> you were saying gasket was an RDMA driver. :P  I shouldn't have to read
>>> a different document to understand the commit message.  It should be
>>> summarized enough and the other documentation is supplemental.
>>>
>>> "In 2019 we introduced pin_user_pages() and now we are converting
>>> get_user_pages() to the new API as appropriate".
>>
>> As all other similar conversion have similar change logs, so I was trying
>> to maintain the same. John might have a different opinion on this.
> 
> For example, I was referring to few recent similar commits for change logs.
> 
> http://lkml.kernel.org/r/20200519002124.2025955-5-jhubbard@nvidia.com
> https://lore.kernel.org/r/20200518015237.1568940-1-jhubbard@nvidia.com
> 
> 
>>
>> John, Any further opinion ??


Well, I've gotten away with the current wording for quite a few patches so
far, but that sure doesn't mean it's perfect! :)

Maybe adding the words that Dan suggests, above, will suffice? Here:

 >>> "In 2019 we introduced pin_user_pages() and now we are converting
 >>> get_user_pages() to the new API as appropriate".


thanks,
-- 
John Hubbard
NVIDIA

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-29  7:38       ` John Hubbard
@ 2020-05-29  7:45         ` Dan Carpenter
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2020-05-29  7:45 UTC (permalink / raw)
  To: John Hubbard
  Cc: Souptick Joarder, open list:ANDROID DRIVERS, Greg KH,
	linux-kernel, Rob Springer, Todd Poynor

On Fri, May 29, 2020 at 12:38:20AM -0700, John Hubbard wrote:
> On 2020-05-28 23:27, Souptick Joarder wrote:
> > On Fri, May 29, 2020 at 11:46 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> > > 
> > > On Thu, May 28, 2020 at 4:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > 
> > > > On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> > > > > This code was using get_user_pages_fast(), in a "Case 2" scenario
> > > > > (DMA/RDMA), using the categorization from [1]. That means that it's
> > > > > time to convert the get_user_pages_fast() + put_page() calls to
> > > > > pin_user_pages_fast() + unpin_user_page() calls.
> > > > 
> > > > You are saying that the page is used for DIO and not DMA, but it sure
> > > > looks to me like it is used for DMA.
> > > 
> > > No, I was referring to "Case 2" scenario in change log which means  it is
> > > used for DMA, not DIO.
> 
> Hi,
> 
> Dan, I also uncertain as to how you read this as referring to DIO. Case 2 is
> DMA or RDMA, and in fact the proposed commit log says both of those things:
> Case 2 and DMA/RDMA. I don't see "DIO" anywhere here...

I thought he meant that the original code was appropriate for DMA and he
was fixing it.  :P

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-29  6:27     ` Souptick Joarder
  2020-05-29  7:38       ` John Hubbard
@ 2020-05-29  7:46       ` Dan Carpenter
  2020-05-29  8:00         ` John Hubbard
  1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-29  7:46 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: John Hubbard, open list:ANDROID DRIVERS, Greg KH, linux-kernel,
	Rob Springer, Todd Poynor

On Fri, May 29, 2020 at 11:57:09AM +0530, Souptick Joarder wrote:
> On Fri, May 29, 2020 at 11:46 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> >
> > On Thu, May 28, 2020 at 4:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> > > > This code was using get_user_pages_fast(), in a "Case 2" scenario
> > > > (DMA/RDMA), using the categorization from [1]. That means that it's
> > > > time to convert the get_user_pages_fast() + put_page() calls to
> > > > pin_user_pages_fast() + unpin_user_page() calls.
> > >
> > > You are saying that the page is used for DIO and not DMA, but it sure
> > > looks to me like it is used for DMA.
> >
> > No, I was referring to "Case 2" scenario in change log which means  it is
> > used for DMA, not DIO.

You can't use pin_user_pages() for DMA.  This was second reason that I
was confused.

mm/gup.c
  2863  /**
  2864   * pin_user_pages_fast() - pin user pages in memory without taking locks
  2865   *
  2866   * @start:      starting user address
  2867   * @nr_pages:   number of pages from start to pin
  2868   * @gup_flags:  flags modifying pin behaviour
  2869   * @pages:      array that receives pointers to the pages pinned.
  2870   *              Should be at least nr_pages long.
  2871   *
  2872   * Nearly the same as get_user_pages_fast(), except that FOLL_PIN is set. See
  2873   * get_user_pages_fast() for documentation on the function arguments, because
  2874   * the arguments here are identical.
  2875   *
  2876   * FOLL_PIN means that the pages must be released via unpin_user_page(). Please
  2877   * see Documentation/core-api/pin_user_pages.rst for further details.
  2878   *
  2879   * This is intended for Case 1 (DIO) in Documentation/core-api/pin_user_pages.rst. It
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  2880   * is NOT intended for Case 2 (RDMA: long-term pins).
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  2881   */
  2882  int pin_user_pages_fast(unsigned long start, int nr_pages,
  2883                          unsigned int gup_flags, struct page **pages)
  2884  {
  2885          /* FOLL_GET and FOLL_PIN are mutually exclusive. */
  2886          if (WARN_ON_ONCE(gup_flags & FOLL_GET))
  2887                  return -EINVAL;
  2888  
  2889          gup_flags |= FOLL_PIN;
  2890          return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages);
  2891  }
  2892  EXPORT_SYMBOL_GPL(pin_user_pages_fast);

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-29  7:46       ` Dan Carpenter
@ 2020-05-29  8:00         ` John Hubbard
  0 siblings, 0 replies; 12+ messages in thread
From: John Hubbard @ 2020-05-29  8:00 UTC (permalink / raw)
  To: Dan Carpenter, Souptick Joarder
  Cc: open list:ANDROID DRIVERS, Greg KH, linux-kernel, Rob Springer,
	Todd Poynor

On 2020-05-29 00:46, Dan Carpenter wrote:
> On Fri, May 29, 2020 at 11:57:09AM +0530, Souptick Joarder wrote:
>> On Fri, May 29, 2020 at 11:46 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>>>
>>> On Thu, May 28, 2020 at 4:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>>
>>>> On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
>>>>> This code was using get_user_pages_fast(), in a "Case 2" scenario
>>>>> (DMA/RDMA), using the categorization from [1]. That means that it's
>>>>> time to convert the get_user_pages_fast() + put_page() calls to
>>>>> pin_user_pages_fast() + unpin_user_page() calls.
>>>>
>>>> You are saying that the page is used for DIO and not DMA, but it sure
>>>> looks to me like it is used for DMA.
>>>
>>> No, I was referring to "Case 2" scenario in change log which means  it is
>>> used for DMA, not DIO.
> 
> You can't use pin_user_pages() for DMA.  This was second reason that I
> was confused.


OK, now it is getting interesting!


> 
> mm/gup.c
>    2863  /**
>    2864   * pin_user_pages_fast() - pin user pages in memory without taking locks
>    2865   *
>    2866   * @start:      starting user address
>    2867   * @nr_pages:   number of pages from start to pin
>    2868   * @gup_flags:  flags modifying pin behaviour
>    2869   * @pages:      array that receives pointers to the pages pinned.
>    2870   *              Should be at least nr_pages long.
>    2871   *
>    2872   * Nearly the same as get_user_pages_fast(), except that FOLL_PIN is set. See
>    2873   * get_user_pages_fast() for documentation on the function arguments, because
>    2874   * the arguments here are identical.
>    2875   *
>    2876   * FOLL_PIN means that the pages must be released via unpin_user_page(). Please
>    2877   * see Documentation/core-api/pin_user_pages.rst for further details.
>    2878   *
>    2879   * This is intended for Case 1 (DIO) in Documentation/core-api/pin_user_pages.rst. It
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    2880   * is NOT intended for Case 2 (RDMA: long-term pins).
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


I'm trying to figure out why I wrote that. It seems just wrong, because once the
page is dma-pinned, it will work just fine for either Case 1 or Case 2. hmmm, I
think this was from a few design ideas ago, when we were still working through the
FOLL_LONGTERM and FOLL_PIN thoughts and how the pin_user_pages*() API set should
look.

At this point, it's looking very much like a (my) documentation bug: all 4 of the
"intended for Case 1 (DIO)" comments in mm/gup.c probably need to be simply deleted.
Good catch.



>    2881   */
>    2882  int pin_user_pages_fast(unsigned long start, int nr_pages,
>    2883                          unsigned int gup_flags, struct page **pages)
>    2884  {
>    2885          /* FOLL_GET and FOLL_PIN are mutually exclusive. */
>    2886          if (WARN_ON_ONCE(gup_flags & FOLL_GET))
>    2887                  return -EINVAL;
>    2888
>    2889          gup_flags |= FOLL_PIN;
>    2890          return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages);
>    2891  }
>    2892  EXPORT_SYMBOL_GPL(pin_user_pages_fast);
> 
> regards,
> dan carpenter
> 

thanks,
-- 
John Hubbard
NVIDIA

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-28 11:04 ` Dan Carpenter
  2020-05-29  6:16   ` Souptick Joarder
@ 2020-05-29 11:53   ` Dan Carpenter
  2020-05-29 20:28     ` John Hubbard
  1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-29 11:53 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: devel, gregkh, linux-kernel, rspringer, John Hubbard, toddpoynor

Anyway, can you resend with the commit message re-written.  To me the
information that's most useful is from the lwn article:

   "In short, if pages are being pinned for access to the data
    contained within those pages, pin_user_pages() should be used. For
    cases where the intent is to manipulate the page structures
    corresponding to the pages rather than the data within them,
    get_user_pages() is the correct interface."

What are the runtime implications of this patch?  I'm still not clear on
that honestly.

When I'm reviewing patches, I also want to know how a bug was
introduced.  In this case the original author did everything correctly
but we've just added some new features (cleanups.  whatever).

I did skim the LWN article back in December but I don't remember the
details so I really want all this stuff re-stated in each commit
message.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-29 11:53   ` Dan Carpenter
@ 2020-05-29 20:28     ` John Hubbard
  0 siblings, 0 replies; 12+ messages in thread
From: John Hubbard @ 2020-05-29 20:28 UTC (permalink / raw)
  To: Dan Carpenter, Souptick Joarder
  Cc: devel, gregkh, linux-kernel, rspringer, toddpoynor

On 2020-05-29 04:53, Dan Carpenter wrote:
...
> What are the runtime implications of this patch?  I'm still not clear on
> that honestly.

Instead of incrementing each page's refcount by 1 (with get_user_pages()),
pin_user_pages*() will increment by GUP_PIN_COUNTING_BIAS, which is 1024.
That by itself should not have any performance impact, of course, but
there's a couple more things:

For compound pages of more than 2 page size, it will also increment
a separate struct page's field, via hpage_pincount_add().

And finally, it will update /proc/vmstat counters on pin and unpin, via
the optimized mod_node_page_state() call.

So it's expected to be very light. And, for DMA (as opposed to DIO)
situations, the DMA setup time is inevitably much greater than any of
the above overheads, so I expect that this patch will be completely
invisible from a performance point of view.

It would be a "nice to have", though, if anyone were able to do a
performance comparison on the gasket driver for this patch, and/or
basic runtime verification, since I'm sure it's a specialized setup.


thanks,
-- 
John Hubbard
NVIDIA

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
  2020-05-31  7:23 Souptick Joarder
@ 2020-05-31  8:59 ` Dan Carpenter
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2020-05-31  8:59 UTC (permalink / raw)
  To: Souptick Joarder
  Cc: rspringer, toddpoynor, benchan, gregkh, devel, John Hubbard,
	linux-kernel

On Sun, May 31, 2020 at 12:53:11PM +0530, Souptick Joarder wrote:
> In 2019, we introduced pin_user_pages*() and now we are converting
> get_user_pages*() to the new API as appropriate. [1] & [2] could
> be referred for more information.
> 
> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
> 	https://lwn.net/Articles/807108/
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> 

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()
@ 2020-05-31  7:23 Souptick Joarder
  2020-05-31  8:59 ` Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Souptick Joarder @ 2020-05-31  7:23 UTC (permalink / raw)
  To: rspringer, toddpoynor, benchan, gregkh
  Cc: devel, linux-kernel, Souptick Joarder, John Hubbard, Dan Carpenter

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
	https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>

---
Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.

 drivers/staging/gasket/gasket_page_table.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index f6d7157..d712ad4 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -449,7 +449,7 @@ static bool gasket_release_page(struct page *page)
 
 	if (!PageReserved(page))
 		SetPageDirty(page);
-	put_page(page);
+	unpin_user_page(page);
 
 	return true;
 }
@@ -486,12 +486,12 @@ static int gasket_perform_mapping(struct gasket_page_table *pg_tbl,
 			ptes[i].dma_addr = pg_tbl->coherent_pages[0].paddr +
 					   off + i * PAGE_SIZE;
 		} else {
-			ret = get_user_pages_fast(page_addr - offset, 1,
+			ret = pin_user_pages_fast(page_addr - offset, 1,
 						  FOLL_WRITE, &page);
 
 			if (ret <= 0) {
 				dev_err(pg_tbl->device,
-					"get user pages failed for addr=0x%lx, offset=0x%lx [ret=%d]\n",
+					"pin user pages failed for addr=0x%lx, offset=0x%lx [ret=%d]\n",
 					page_addr, offset, ret);
 				return ret ? ret : -ENOMEM;
 			}
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-05-31  9:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 21:02 [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*() Souptick Joarder
2020-05-28 11:04 ` Dan Carpenter
2020-05-29  6:16   ` Souptick Joarder
2020-05-29  6:27     ` Souptick Joarder
2020-05-29  7:38       ` John Hubbard
2020-05-29  7:45         ` Dan Carpenter
2020-05-29  7:46       ` Dan Carpenter
2020-05-29  8:00         ` John Hubbard
2020-05-29 11:53   ` Dan Carpenter
2020-05-29 20:28     ` John Hubbard
2020-05-31  7:23 Souptick Joarder
2020-05-31  8:59 ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).