Netdev Archive on lore.kernel.org
 help / Atom feed
* [PATCH 1/2] xsk: do not use mmap_sem
       [not found] <20190207053740.26915-1-dave@stgolabs.net>
@ 2019-02-07  5:37 ` Davidlohr Bueso
  2019-02-07  7:43   ` Björn Töpel
  2019-02-11 16:15   ` [PATCH v2] xsk: share the mmap_sem for page pinning Davidlohr Bueso
  0 siblings, 2 replies; 8+ messages in thread
From: Davidlohr Bueso @ 2019-02-07  5:37 UTC (permalink / raw)
  To: akpm
  Cc: linux-mm, dave, linux-kernel, David S . Miller, Bjorn Topel,
	Magnus Karlsson, netdev, Davidlohr Bueso

Holding mmap_sem exclusively for a gup() is an overkill.
Lets replace the call for gup_fast() and let the mm take
it if necessary.

Cc: David S. Miller <davem@davemloft.net>
Cc: Bjorn Topel <bjorn.topel@intel.com>
Cc: Magnus Karlsson <magnus.karlsson@intel.com>
CC: netdev@vger.kernel.org
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 net/xdp/xdp_umem.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 5ab236c5c9a5..25e1e76654a8 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -265,10 +265,8 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)
 	if (!umem->pgs)
 		return -ENOMEM;
 
-	down_write(&current->mm->mmap_sem);
-	npgs = get_user_pages(umem->address, umem->npgs,
-			      gup_flags, &umem->pgs[0], NULL);
-	up_write(&current->mm->mmap_sem);
+	npgs = get_user_pages_fast(umem->address, umem->npgs,
+				   gup_flags, &umem->pgs[0]);
 
 	if (npgs != umem->npgs) {
 		if (npgs >= 0) {
-- 
2.16.4


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

* Re: [PATCH 1/2] xsk: do not use mmap_sem
  2019-02-07  5:37 ` [PATCH 1/2] xsk: do not use mmap_sem Davidlohr Bueso
@ 2019-02-07  7:43   ` Björn Töpel
  2019-02-11 15:33     ` Daniel Borkmann
  2019-02-11 16:15   ` [PATCH v2] xsk: share the mmap_sem for page pinning Davidlohr Bueso
  1 sibling, 1 reply; 8+ messages in thread
From: Björn Töpel @ 2019-02-07  7:43 UTC (permalink / raw)
  To: Davidlohr Bueso
  Cc: akpm, linux-mm, LKML, David S . Miller, Bjorn Topel,
	Magnus Karlsson, Netdev, Davidlohr Bueso

Den tors 7 feb. 2019 kl 06:38 skrev Davidlohr Bueso <dave@stgolabs.net>:
>
> Holding mmap_sem exclusively for a gup() is an overkill.
> Lets replace the call for gup_fast() and let the mm take
> it if necessary.
>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Bjorn Topel <bjorn.topel@intel.com>
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  net/xdp/xdp_umem.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 5ab236c5c9a5..25e1e76654a8 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -265,10 +265,8 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)
>         if (!umem->pgs)
>                 return -ENOMEM;
>
> -       down_write(&current->mm->mmap_sem);
> -       npgs = get_user_pages(umem->address, umem->npgs,
> -                             gup_flags, &umem->pgs[0], NULL);
> -       up_write(&current->mm->mmap_sem);
> +       npgs = get_user_pages_fast(umem->address, umem->npgs,
> +                                  gup_flags, &umem->pgs[0]);
>

Thanks for the patch!

The lifetime of the pinning is similar to RDMA umem mapping, so isn't
gup_longterm preferred?


Björn

>         if (npgs != umem->npgs) {
>                 if (npgs >= 0) {
> --
> 2.16.4
>

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

* Re: [PATCH 1/2] xsk: do not use mmap_sem
  2019-02-07  7:43   ` Björn Töpel
@ 2019-02-11 15:33     ` Daniel Borkmann
  2019-02-11 16:37       ` Dan Williams
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Borkmann @ 2019-02-11 15:33 UTC (permalink / raw)
  To: Björn Töpel, Davidlohr Bueso
  Cc: akpm, linux-mm, LKML, David S . Miller, Bjorn Topel,
	Magnus Karlsson, Netdev, Davidlohr Bueso, dan.j.williams

[ +Dan ]

On 02/07/2019 08:43 AM, Björn Töpel wrote:
> Den tors 7 feb. 2019 kl 06:38 skrev Davidlohr Bueso <dave@stgolabs.net>:
>>
>> Holding mmap_sem exclusively for a gup() is an overkill.
>> Lets replace the call for gup_fast() and let the mm take
>> it if necessary.
>>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Bjorn Topel <bjorn.topel@intel.com>
>> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
>> CC: netdev@vger.kernel.org
>> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>> ---
>>  net/xdp/xdp_umem.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
>> index 5ab236c5c9a5..25e1e76654a8 100644
>> --- a/net/xdp/xdp_umem.c
>> +++ b/net/xdp/xdp_umem.c
>> @@ -265,10 +265,8 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)
>>         if (!umem->pgs)
>>                 return -ENOMEM;
>>
>> -       down_write(&current->mm->mmap_sem);
>> -       npgs = get_user_pages(umem->address, umem->npgs,
>> -                             gup_flags, &umem->pgs[0], NULL);
>> -       up_write(&current->mm->mmap_sem);
>> +       npgs = get_user_pages_fast(umem->address, umem->npgs,
>> +                                  gup_flags, &umem->pgs[0]);
>>
> 
> Thanks for the patch!
> 
> The lifetime of the pinning is similar to RDMA umem mapping, so isn't
> gup_longterm preferred?

Seems reasonable from reading what gup_longterm seems to do. Davidlohr
or Dan, any thoughts on the above?

Thanks,
Daniel

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

* [PATCH v2] xsk: share the mmap_sem for page pinning
  2019-02-07  5:37 ` [PATCH 1/2] xsk: do not use mmap_sem Davidlohr Bueso
  2019-02-07  7:43   ` Björn Töpel
@ 2019-02-11 16:15   ` Davidlohr Bueso
  2019-02-11 16:21     ` Björn Töpel
  2019-02-11 19:53     ` Daniel Borkmann
  1 sibling, 2 replies; 8+ messages in thread
From: Davidlohr Bueso @ 2019-02-11 16:15 UTC (permalink / raw)
  To: akpm
  Cc: linux-mm, linux-kernel, David S . Miller, Bjorn Topel,
	Magnus Karlsson, netdev, Davidlohr Bueso

Holding mmap_sem exclusively for a gup() is an overkill.
Lets share the lock and replace the gup call for gup_longterm(),
as it is better suited for the lifetime of the pinning.

Cc: David S. Miller <davem@davemloft.net>
Cc: Bjorn Topel <bjorn.topel@intel.com>
Cc: Magnus Karlsson <magnus.karlsson@intel.com>
CC: netdev@vger.kernel.org
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 net/xdp/xdp_umem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 5ab236c5c9a5..e7fa8d0d7090 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -265,10 +265,10 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)
 	if (!umem->pgs)
 		return -ENOMEM;
 
-	down_write(&current->mm->mmap_sem);
-	npgs = get_user_pages(umem->address, umem->npgs,
+	down_read(&current->mm->mmap_sem);
+	npgs = get_user_pages_longterm(umem->address, umem->npgs,
 			      gup_flags, &umem->pgs[0], NULL);
-	up_write(&current->mm->mmap_sem);
+	up_read(&current->mm->mmap_sem);
 
 	if (npgs != umem->npgs) {
 		if (npgs >= 0) {
-- 
2.16.4


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

* Re: [PATCH v2] xsk: share the mmap_sem for page pinning
  2019-02-11 16:15   ` [PATCH v2] xsk: share the mmap_sem for page pinning Davidlohr Bueso
@ 2019-02-11 16:21     ` Björn Töpel
  2019-02-11 19:53     ` Daniel Borkmann
  1 sibling, 0 replies; 8+ messages in thread
From: Björn Töpel @ 2019-02-11 16:21 UTC (permalink / raw)
  To: akpm, linux-mm, LKML, David S . Miller, Bjorn Topel,
	Magnus Karlsson, Netdev, Davidlohr Bueso

Den mån 11 feb. 2019 kl 17:15 skrev Davidlohr Bueso <dave@stgolabs.net>:
>
> Holding mmap_sem exclusively for a gup() is an overkill.
> Lets share the lock and replace the gup call for gup_longterm(),
> as it is better suited for the lifetime of the pinning.
>

Thanks for the cleanup!

Acked-by: Björn Töpel <bjorn.topel@intel.com>

> Cc: David S. Miller <davem@davemloft.net>
> Cc: Bjorn Topel <bjorn.topel@intel.com>
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  net/xdp/xdp_umem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 5ab236c5c9a5..e7fa8d0d7090 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -265,10 +265,10 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)
>         if (!umem->pgs)
>                 return -ENOMEM;
>
> -       down_write(&current->mm->mmap_sem);
> -       npgs = get_user_pages(umem->address, umem->npgs,
> +       down_read(&current->mm->mmap_sem);
> +       npgs = get_user_pages_longterm(umem->address, umem->npgs,
>                               gup_flags, &umem->pgs[0], NULL);
> -       up_write(&current->mm->mmap_sem);
> +       up_read(&current->mm->mmap_sem);
>
>         if (npgs != umem->npgs) {
>                 if (npgs >= 0) {
> --
> 2.16.4
>

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

* Re: [PATCH 1/2] xsk: do not use mmap_sem
  2019-02-11 15:33     ` Daniel Borkmann
@ 2019-02-11 16:37       ` Dan Williams
  2019-02-11 17:16         ` Weiny, Ira
  0 siblings, 1 reply; 8+ messages in thread
From: Dan Williams @ 2019-02-11 16:37 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Björn Töpel, Davidlohr Bueso, Andrew Morton, Linux MM,
	LKML, David S . Miller, Bjorn Topel, Magnus Karlsson, Netdev,
	Davidlohr Bueso, Weiny, Ira

[ add Ira ]

On Mon, Feb 11, 2019 at 7:33 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> [ +Dan ]
>
> On 02/07/2019 08:43 AM, Björn Töpel wrote:
> > Den tors 7 feb. 2019 kl 06:38 skrev Davidlohr Bueso <dave@stgolabs.net>:
> >>
> >> Holding mmap_sem exclusively for a gup() is an overkill.
> >> Lets replace the call for gup_fast() and let the mm take
> >> it if necessary.
> >>
> >> Cc: David S. Miller <davem@davemloft.net>
> >> Cc: Bjorn Topel <bjorn.topel@intel.com>
> >> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> >> CC: netdev@vger.kernel.org
> >> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> >> ---
> >>  net/xdp/xdp_umem.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> >> index 5ab236c5c9a5..25e1e76654a8 100644
> >> --- a/net/xdp/xdp_umem.c
> >> +++ b/net/xdp/xdp_umem.c
> >> @@ -265,10 +265,8 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)
> >>         if (!umem->pgs)
> >>                 return -ENOMEM;
> >>
> >> -       down_write(&current->mm->mmap_sem);
> >> -       npgs = get_user_pages(umem->address, umem->npgs,
> >> -                             gup_flags, &umem->pgs[0], NULL);
> >> -       up_write(&current->mm->mmap_sem);
> >> +       npgs = get_user_pages_fast(umem->address, umem->npgs,
> >> +                                  gup_flags, &umem->pgs[0]);
> >>
> >
> > Thanks for the patch!
> >
> > The lifetime of the pinning is similar to RDMA umem mapping, so isn't
> > gup_longterm preferred?
>
> Seems reasonable from reading what gup_longterm seems to do. Davidlohr
> or Dan, any thoughts on the above?

Yes, any gup where the lifetime of the corresponding put_page() is
under direct control of userspace should be using the _longterm flavor
to coordinate be careful in the presence of dax. In the dax case there
is no page cache indirection to coordinate truncate vs page pins. Ira
is looking to add a "fast + longterm" flavor for cases like this.

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

* RE: [PATCH 1/2] xsk: do not use mmap_sem
  2019-02-11 16:37       ` Dan Williams
@ 2019-02-11 17:16         ` Weiny, Ira
  0 siblings, 0 replies; 8+ messages in thread
From: Weiny, Ira @ 2019-02-11 17:16 UTC (permalink / raw)
  To: Williams, Dan J, Daniel Borkmann
  Cc: Björn Töpel, Davidlohr Bueso, Andrew Morton, Linux MM,
	LKML, David S . Miller, Topel, Bjorn, Karlsson, Magnus, Netdev,
	Davidlohr Bueso

> > >> ---
> > >>  net/xdp/xdp_umem.c | 6 ++----
> > >>  1 file changed, 2 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c index
> > >> 5ab236c5c9a5..25e1e76654a8 100644
> > >> --- a/net/xdp/xdp_umem.c
> > >> +++ b/net/xdp/xdp_umem.c
> > >> @@ -265,10 +265,8 @@ static int xdp_umem_pin_pages(struct
> xdp_umem *umem)
> > >>         if (!umem->pgs)
> > >>                 return -ENOMEM;
> > >>
> > >> -       down_write(&current->mm->mmap_sem);
> > >> -       npgs = get_user_pages(umem->address, umem->npgs,
> > >> -                             gup_flags, &umem->pgs[0], NULL);
> > >> -       up_write(&current->mm->mmap_sem);
> > >> +       npgs = get_user_pages_fast(umem->address, umem->npgs,
> > >> +                                  gup_flags, &umem->pgs[0]);
> > >>
> > >
> > > Thanks for the patch!
> > >
> > > The lifetime of the pinning is similar to RDMA umem mapping, so
> > > isn't gup_longterm preferred?
> >
> > Seems reasonable from reading what gup_longterm seems to do. Davidlohr
> > or Dan, any thoughts on the above?
> 
> Yes, any gup where the lifetime of the corresponding put_page() is under
> direct control of userspace should be using the _longterm flavor to
> coordinate be careful in the presence of dax. In the dax case there is no page
> cache indirection to coordinate truncate vs page pins. Ira is looking to add a
> "fast + longterm" flavor for cases like this.

Yes I'm just about ready with a small patch set to add a GUP fast longterm.

I think this should wait for that series.

Ira
 

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

* Re: [PATCH v2] xsk: share the mmap_sem for page pinning
  2019-02-11 16:15   ` [PATCH v2] xsk: share the mmap_sem for page pinning Davidlohr Bueso
  2019-02-11 16:21     ` Björn Töpel
@ 2019-02-11 19:53     ` Daniel Borkmann
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2019-02-11 19:53 UTC (permalink / raw)
  To: akpm, linux-mm, linux-kernel, David S . Miller, Bjorn Topel,
	Magnus Karlsson, netdev, Davidlohr Bueso

On 02/11/2019 05:15 PM, Davidlohr Bueso wrote:
> Holding mmap_sem exclusively for a gup() is an overkill.
> Lets share the lock and replace the gup call for gup_longterm(),
> as it is better suited for the lifetime of the pinning.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Bjorn Topel <bjorn.topel@intel.com>
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>

Applied, thanks!

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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190207053740.26915-1-dave@stgolabs.net>
2019-02-07  5:37 ` [PATCH 1/2] xsk: do not use mmap_sem Davidlohr Bueso
2019-02-07  7:43   ` Björn Töpel
2019-02-11 15:33     ` Daniel Borkmann
2019-02-11 16:37       ` Dan Williams
2019-02-11 17:16         ` Weiny, Ira
2019-02-11 16:15   ` [PATCH v2] xsk: share the mmap_sem for page pinning Davidlohr Bueso
2019-02-11 16:21     ` Björn Töpel
2019-02-11 19:53     ` Daniel Borkmann

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox