| Message ID | 20190216033618.116680-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [-next] IB/qib: Remove unused variable 'locked' | expand |
On Sat, Feb 16, 2019 at 03:36:18AM +0000, YueHaibing wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/infiniband/hw/qib/qib_user_pages.c: In function 'qib_get_user_pages': > drivers/infiniband/hw/qib/qib_user_pages.c:103:16: warning: > variable 'locked' set but not used [-Wunused-but-set-variable] > > It's never used since introduction. > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > drivers/infiniband/hw/qib/qib_user_pages.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c > index ef8bcf366ddc..a5be36c07451 100644 > +++ b/drivers/infiniband/hw/qib/qib_user_pages.c > @@ -100,12 +100,12 @@ int qib_map_page(struct pci_dev *hwdev, struct page *page, dma_addr_t *daddr) > int qib_get_user_pages(unsigned long start_page, size_t num_pages, > struct page **p) > { > - unsigned long locked, lock_limit; > + unsigned long lock_limit; > size_t got; > int ret; > > lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > - locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); > + atomic64_add_return(num_pages, ¤t->mm->pinned_vm); > > if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { > ret = -ENOMEM; This looks just wrong... The algorithm should look like the one in umem Jason
On Fri, 15 Feb 2019, Jason Gunthorpe wrote: >On Sat, Feb 16, 2019 at 03:36:18AM +0000, YueHaibing wrote: >> Fixes gcc '-Wunused-but-set-variable' warning: >> >> drivers/infiniband/hw/qib/qib_user_pages.c: In function 'qib_get_user_pages': >> drivers/infiniband/hw/qib/qib_user_pages.c:103:16: warning: >> variable 'locked' set but not used [-Wunused-but-set-variable] >> >> It's never used since introduction. >> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >> drivers/infiniband/hw/qib/qib_user_pages.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c >> index ef8bcf366ddc..a5be36c07451 100644 >> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c >> @@ -100,12 +100,12 @@ int qib_map_page(struct pci_dev *hwdev, struct page *page, dma_addr_t *daddr) >> int qib_get_user_pages(unsigned long start_page, size_t num_pages, >> struct page **p) >> { >> - unsigned long locked, lock_limit; >> + unsigned long lock_limit; >> size_t got; >> int ret; >> >> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >> - locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); >> + atomic64_add_return(num_pages, ¤t->mm->pinned_vm); >> >> if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { >> ret = -ENOMEM; > >This looks just wrong... The algorithm should look like the one in umem Indeed, but the current check also looks wrong; we wanna check against the new value after the addition of num_pages. Thanks, Davidlohr -----------8<------------------------------------------------------------ [PATCH] drivers/IB,qib: Fix pinned/locked limit check in qib_get_user_pages() The current check does not take into account the previous value of pinned_vm; thus it is quite bogus as is. Fix this by checking the new value after the (optimistic) atomic inc. Signed-off-by: Davidlohr Bueso <dbueso@suse.de> --- drivers/infiniband/hw/qib/qib_user_pages.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c index ef8bcf366ddc..123ca8f64f75 100644 --- a/drivers/infiniband/hw/qib/qib_user_pages.c +++ b/drivers/infiniband/hw/qib/qib_user_pages.c @@ -107,7 +107,7 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages, lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); - if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { ret = -ENOMEM; goto bail; }
On Sat, Feb 16, 2019 at 04:15:52AM -0800, Davidlohr Bueso wrote: > On Fri, 15 Feb 2019, Jason Gunthorpe wrote: > > > On Sat, Feb 16, 2019 at 03:36:18AM +0000, YueHaibing wrote: > > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > > > drivers/infiniband/hw/qib/qib_user_pages.c: In function 'qib_get_user_pages': > > > drivers/infiniband/hw/qib/qib_user_pages.c:103:16: warning: > > > variable 'locked' set but not used [-Wunused-but-set-variable] > > > > > > It's never used since introduction. > > > > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > drivers/infiniband/hw/qib/qib_user_pages.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c > > > index ef8bcf366ddc..a5be36c07451 100644 > > > +++ b/drivers/infiniband/hw/qib/qib_user_pages.c > > > @@ -100,12 +100,12 @@ int qib_map_page(struct pci_dev *hwdev, struct page *page, dma_addr_t *daddr) > > > int qib_get_user_pages(unsigned long start_page, size_t num_pages, > > > struct page **p) > > > { > > > - unsigned long locked, lock_limit; > > > + unsigned long lock_limit; > > > size_t got; > > > int ret; > > > > > > lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > > - locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); > > > + atomic64_add_return(num_pages, ¤t->mm->pinned_vm); > > > > > > if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { > > > ret = -ENOMEM; > > > > This looks just wrong... The algorithm should look like the one in umem > > Indeed, but the current check also looks wrong; we wanna check against the > new value after the addition of num_pages. > > Thanks, > Davidlohr > > [PATCH] drivers/IB,qib: Fix pinned/locked limit check in qib_get_user_pages() > > The current check does not take into account the previous value of > pinned_vm; thus it is quite bogus as is. Fix this by checking the > new value after the (optimistic) atomic inc. > > Signed-off-by: Davidlohr Bueso <dbueso@suse.de> > drivers/infiniband/hw/qib/qib_user_pages.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c > index ef8bcf366ddc..123ca8f64f75 100644 > +++ b/drivers/infiniband/hw/qib/qib_user_pages.c > @@ -107,7 +107,7 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages, > lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); > > - if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { > + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { > ret = -ENOMEM; > goto bail; Missing atomic_sub ? Jason
On Sat, 16 Feb 2019, Jason Gunthorpe wrote: >> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c >> index ef8bcf366ddc..123ca8f64f75 100644 >> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c >> @@ -107,7 +107,7 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages, >> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >> locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); >> >> - if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { >> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { >> ret = -ENOMEM; >> goto bail; > >Missing atomic_sub ? Not really the sub is in thhe bail part when we return non-zero. Thanks, Davidlohr
On Sun, 17 Feb 2019, H?kon Bugge wrote: > > >> On 16 Feb 2019, at 13:15, Davidlohr Bueso <dave@stgolabs.net> wrote: >> >> On Fri, 15 Feb 2019, Jason Gunthorpe wrote: >> >>> On Sat, Feb 16, 2019 at 03:36:18AM +0000, YueHaibing wrote: >>>> Fixes gcc '-Wunused-but-set-variable' warning: >>>> >>>> drivers/infiniband/hw/qib/qib_user_pages.c: In function 'qib_get_user_pages': >>>> drivers/infiniband/hw/qib/qib_user_pages.c:103:16: warning: >>>> variable 'locked' set but not used [-Wunused-but-set-variable] >>>> >>>> It's never used since introduction. >>>> >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>> drivers/infiniband/hw/qib/qib_user_pages.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c >>>> index ef8bcf366ddc..a5be36c07451 100644 >>>> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c >>>> @@ -100,12 +100,12 @@ int qib_map_page(struct pci_dev *hwdev, struct page *page, dma_addr_t *daddr) >>>> int qib_get_user_pages(unsigned long start_page, size_t num_pages, >>>> struct page **p) >>>> { >>>> - unsigned long locked, lock_limit; >>>> + unsigned long lock_limit; >>>> size_t got; >>>> int ret; >>>> >>>> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >>>> - locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); >>>> + atomic64_add_return(num_pages, ¤t->mm->pinned_vm); >>>> >>>> if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { >>>> ret = -ENOMEM; >>> >>> This looks just wrong... The algorithm should look like the one in umem >> >> Indeed, but the current check also looks wrong; we wanna check against the >> new value after the addition of num_pages. >> >> Thanks, >> Davidlohr >> >> -----------8<------------------------------------------------------------ >> [PATCH] drivers/IB,qib: Fix pinned/locked limit check in qib_get_user_pages() >> >> The current check does not take into account the previous value of >> pinned_vm; thus it is quite bogus as is. Fix this by checking the >> new value after the (optimistic) atomic inc. >> >> Signed-off-by: Davidlohr Bueso <dbueso@suse.de <mailto:dbueso@suse.de>> >> --- >> drivers/infiniband/hw/qib/qib_user_pages.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c >> index ef8bcf366ddc..123ca8f64f75 100644 >> --- a/drivers/infiniband/hw/qib/qib_user_pages.c >> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c >> @@ -107,7 +107,7 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages, >> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >> locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); >> - if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { >> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { >> ret = -ENOMEM; >> goto bail; > >But you have still added num_pages to current->mm->pinned_vm. Need to subtract them. bail: atomic64_sub(num_pages, ¤t->mm->pinned_vm); return ret;
On Sun, 17 Feb 2019, H?kon Bugge wrote: >But while looking at the code, the for loop just below looks suspicious: > > for (got = 0; got < num_pages; got += ret) { > >as ret is uninitialized. The first loop will initialize ret with the return of gup_longterm(). Thanks, Davidlohr
On Fri, 15 Feb 2019, Jason Gunthorpe wrote:
>This looks just wrong... The algorithm should look like the one in umem
On another note, and considering infiniband (core) is allowed on most
archs, do we want to relax the ordering guarantees from the compound
atomic64_inc_return(pinned_vm) in ib_umem_get()?
Looking at IB core and some of the drivers I don't see any particular
constraints on the order the pinned_vm is touched - so ll/sc based archs
could benefit with atomic64_inc_return_relaxed().
Thanks,
Davidlohr
On Sat, Feb 16, 2019 at 04:15:52AM -0800, Davidlohr Bueso wrote: > [PATCH] drivers/IB,qib: Fix pinned/locked limit check in qib_get_user_pages() > > The current check does not take into account the previous value of > pinned_vm; thus it is quite bogus as is. Fix this by checking the > new value after the (optimistic) atomic inc. > > Signed-off-by: Davidlohr Bueso <dbueso@suse.de> > -- > drivers/infiniband/hw/qib/qib_user_pages.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied to for-next thanks Jason
On Sun, Feb 17, 2019 at 01:33:36PM -0800, Davidlohr Bueso wrote: > On Fri, 15 Feb 2019, Jason Gunthorpe wrote: > > This looks just wrong... The algorithm should look like the one in umem > > On another note, and considering infiniband (core) is allowed on most > archs, do we want to relax the ordering guarantees from the compound > atomic64_inc_return(pinned_vm) in ib_umem_get()? > Looking at IB core and some of the drivers I don't see any particular > constraints on the order the pinned_vm is touched - so ll/sc based archs > could benefit with atomic64_inc_return_relaxed(). I don't think order matters for any place touching pinned_vm, but it also doesn't seem like any of these are such a performance path the difference matters Jason
diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c index ef8bcf366ddc..a5be36c07451 100644 --- a/drivers/infiniband/hw/qib/qib_user_pages.c +++ b/drivers/infiniband/hw/qib/qib_user_pages.c @@ -100,12 +100,12 @@ int qib_map_page(struct pci_dev *hwdev, struct page *page, dma_addr_t *daddr) int qib_get_user_pages(unsigned long start_page, size_t num_pages, struct page **p) { - unsigned long locked, lock_limit; + unsigned long lock_limit; size_t got; int ret; lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; - locked = atomic64_add_return(num_pages, ¤t->mm->pinned_vm); + atomic64_add_return(num_pages, ¤t->mm->pinned_vm); if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) { ret = -ENOMEM;
Fixes gcc '-Wunused-but-set-variable' warning: drivers/infiniband/hw/qib/qib_user_pages.c: In function 'qib_get_user_pages': drivers/infiniband/hw/qib/qib_user_pages.c:103:16: warning: variable 'locked' set but not used [-Wunused-but-set-variable] It's never used since introduction. Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/infiniband/hw/qib/qib_user_pages.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)