| Message ID | 20190207053740.26915-2-dave@stgolabs.net |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [1/2] xsk: do not use mmap_sem | expand |
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(¤t->mm->mmap_sem); > - npgs = get_user_pages(umem->address, umem->npgs, > - gup_flags, &umem->pgs[0], NULL); > - up_write(¤t->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 >
[ +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(¤t->mm->mmap_sem); >> - npgs = get_user_pages(umem->address, umem->npgs, >> - gup_flags, &umem->pgs[0], NULL); >> - up_write(¤t->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
[ 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(¤t->mm->mmap_sem); > >> - npgs = get_user_pages(umem->address, umem->npgs, > >> - gup_flags, &umem->pgs[0], NULL); > >> - up_write(¤t->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.
> > >> --- > > >> 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(¤t->mm->mmap_sem); > > >> - npgs = get_user_pages(umem->address, umem->npgs, > > >> - gup_flags, &umem->pgs[0], NULL); > > >> - up_write(¤t->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
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(¤t->mm->mmap_sem); - npgs = get_user_pages(umem->address, umem->npgs, - gup_flags, &umem->pgs[0], NULL); - up_write(¤t->mm->mmap_sem); + npgs = get_user_pages_fast(umem->address, umem->npgs, + gup_flags, &umem->pgs[0]); if (npgs != umem->npgs) { if (npgs >= 0) {
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(-)