[PATCH v1 rdma-next] RDMA/siw: Relax from kmap_atomic() use in TX path

[PATCH v1 rdma-next] RDMA/siw: Relax from kmap_atomic() use in TX path

[Bernard Metzler]

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);
 			} 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;
-- 
2.17.2

Re: [PATCH v1 rdma-next] RDMA/siw: Relax from kmap_atomic() use in TX path

[Potnuri Bharat Teja]

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
> 

Re: Re: [PATCH v1 rdma-next] RDMA/siw: Relax from kmap_atomic() use in TX path

[Bernard Metzler]

-----"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
>> 
>
>