[PATCH -next] IB/qib: Remove unused variable 'locked'

[PATCH -next] IB/qib: Remove unused variable 'locked'

[YueHaibing]

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
--- 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, &current->mm->pinned_vm);
+	atomic64_add_return(num_pages, &current->mm->pinned_vm);
 
 	if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) {
 		ret = -ENOMEM;

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Jason Gunthorpe]

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, &current->mm->pinned_vm);
> +	atomic64_add_return(num_pages, &current->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

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Davidlohr Bueso]

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, &current->mm->pinned_vm);
>> +	atomic64_add_return(num_pages, &current->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, &current->mm->pinned_vm);
 
-	if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) {
+	if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
 		ret = -ENOMEM;
 		goto bail;
 	}
-- 
2.16.4

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Jason Gunthorpe]

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, &current->mm->pinned_vm);
> > > +	atomic64_add_return(num_pages, &current->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, &current->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

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Davidlohr Bueso]

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, &current->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

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Davidlohr Bueso]

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, &current->mm->pinned_vm);
>>>> +	atomic64_add_return(num_pages, &current->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, &current->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, &current->mm->pinned_vm);
	return ret;

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Davidlohr Bueso]

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

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Davidlohr Bueso]

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

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Jason Gunthorpe]

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

Re: [PATCH -next] IB/qib: Remove unused variable 'locked'

[Jason Gunthorpe]

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