[v1,rdma-next] RDMA/siw: Relax from kmap_atomic() use in TX path
diff mbox series

Message ID 20190909132945.30462-1-bmt@zurich.ibm.com
State Accepted
Commit 4db8fd4973329810c8549b08df12907e6fe921c2
Headers show
Series
  • [v1,rdma-next] RDMA/siw: Relax from kmap_atomic() use in TX path
Related show

Commit Message

Bernard Metzler Sept. 9, 2019, 1:29 p.m. UTC
Since the transmit path is never executed in an atomic
context, we do not need kmap_atomic() and can always
use less demanding kmap().

Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Potnuri Bharat Teja Sept. 9, 2019, 2:54 p.m. UTC | #1
On Monday, September 09/09/19, 2019 at 18:59:45 +0530, Bernard Metzler wrote:
> Since the transmit path is never executed in an atomic
> context, we do not need kmap_atomic() and can always
> use less demanding kmap().
> 
> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
>  drivers/infiniband/sw/siw/siw_qp_tx.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index 8e72f955921d..5d97bba0ce6d 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -76,16 +76,15 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
>  			if (unlikely(!p))
>  				return -EFAULT;
>  
> -			buffer = kmap_atomic(p);
> +			buffer = kmap(p);
>  
>  			if (likely(PAGE_SIZE - off >= bytes)) {
>  				memcpy(paddr, buffer + off, bytes);
> -				kunmap_atomic(buffer);
missing kunmap()
>  			} else {
>  				unsigned long part = bytes - (PAGE_SIZE - off);
>  
>  				memcpy(paddr, buffer + off, part);
> -				kunmap_atomic(buffer);
> +				kunmap(p);
kunmap(buffer)
>  
>  				if (!mem->is_pbl)
>  					p = siw_get_upage(mem->umem,
> @@ -97,11 +96,10 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
>  				if (unlikely(!p))
>  					return -EFAULT;
>  
> -				buffer = kmap_atomic(p);
> -				memcpy(paddr + part, buffer,
> -				       bytes - part);
> -				kunmap_atomic(buffer);
> +				buffer = kmap(p);
> +				memcpy(paddr + part, buffer, bytes - part);
>  			}
> +			kunmap(p);
Can this be out of if()? the buffers seem to be different.
>  		}
>  	}
>  	return (int)bytes;
> -- 
> 2.17.2
>
Bernard Metzler Sept. 10, 2019, 8:13 a.m. UTC | #2
-----"Potnuri Bharat Teja" <bharat@chelsio.com> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Potnuri Bharat Teja" <bharat@chelsio.com>
>Date: 09/09/2019 04:54PM
>Cc: "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
>"dledford@redhat.com" <dledford@redhat.com>
>Subject: [EXTERNAL] Re: [PATCH v1 rdma-next] RDMA/siw: Relax from
>kmap_atomic() use in TX path
>
>On Monday, September 09/09/19, 2019 at 18:59:45 +0530, Bernard
>Metzler wrote:
>> Since the transmit path is never executed in an atomic
>> context, we do not need kmap_atomic() and can always
>> use less demanding kmap().
>> 
>> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
>> ---
>>  drivers/infiniband/sw/siw/siw_qp_tx.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>> index 8e72f955921d..5d97bba0ce6d 100644
>> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>> @@ -76,16 +76,15 @@ static int siw_try_1seg(struct siw_iwarp_tx
>*c_tx, void *paddr)
>>  			if (unlikely(!p))
>>  				return -EFAULT;
>>  
>> -			buffer = kmap_atomic(p);
>> +			buffer = kmap(p);
>>  
>>  			if (likely(PAGE_SIZE - off >= bytes)) {
>>  				memcpy(paddr, buffer + off, bytes);
>> -				kunmap_atomic(buffer);
>missing kunmap()

No it's not missing. we are in the if-path,
and we unmap 'p' at the next line (skipping
the else-path).

>>  			} else {
>>  				unsigned long part = bytes - (PAGE_SIZE - off);
>>  
>>  				memcpy(paddr, buffer + off, part);
>> -				kunmap_atomic(buffer);
>> +				kunmap(p);
>kunmap(buffer)
>>  
>>  				if (!mem->is_pbl)
>>  					p = siw_get_upage(mem->umem,
>> @@ -97,11 +96,10 @@ static int siw_try_1seg(struct siw_iwarp_tx
>*c_tx, void *paddr)
>>  				if (unlikely(!p))
>>  					return -EFAULT;
>>  
>> -				buffer = kmap_atomic(p);
>> -				memcpy(paddr + part, buffer,
>> -				       bytes - part);
>> -				kunmap_atomic(buffer);
>> +				buffer = kmap(p);
>> +				memcpy(paddr + part, buffer, bytes - part);
>>  			}
>> +			kunmap(p);
>Can this be out of if()? the buffers seem to be different.

the page pointer gets reassigned since the data
span two pages. so we unmap the first page after
copying out of it what we need, get the second
page into 'p', and map it. at the end, the current
page p gets unmapped. if we do not have data spanning
two pages, we unmap the first page. If we do have,
we unmap the second page.

It should be all good.

Thanks & best regards,
Bernard.
>>  		}
>>  	}
>>  	return (int)bytes;
>> -- 
>> 2.17.2
>> 
>
>

Patch
diff mbox series

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 8e72f955921d..5d97bba0ce6d 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -76,16 +76,15 @@  static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 			if (unlikely(!p))
 				return -EFAULT;
 
-			buffer = kmap_atomic(p);
+			buffer = kmap(p);
 
 			if (likely(PAGE_SIZE - off >= bytes)) {
 				memcpy(paddr, buffer + off, bytes);
-				kunmap_atomic(buffer);
 			} else {
 				unsigned long part = bytes - (PAGE_SIZE - off);
 
 				memcpy(paddr, buffer + off, part);
-				kunmap_atomic(buffer);
+				kunmap(p);
 
 				if (!mem->is_pbl)
 					p = siw_get_upage(mem->umem,
@@ -97,11 +96,10 @@  static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 				if (unlikely(!p))
 					return -EFAULT;
 
-				buffer = kmap_atomic(p);
-				memcpy(paddr + part, buffer,
-				       bytes - part);
-				kunmap_atomic(buffer);
+				buffer = kmap(p);
+				memcpy(paddr + part, buffer, bytes - part);
 			}
+			kunmap(p);
 		}
 	}
 	return (int)bytes;