diff mbox

[2/5] crypto: chtls: wait for memory sendmsg, sendpage

Message ID 1526295659-29839-3-git-send-email-atul.gupta@chelsio.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Atul Gupta May 14, 2018, 11 a.m. UTC 
Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: Atul Gupta <atul.gupta@chelsio.com>
---
 drivers/crypto/chelsio/chtls/chtls.h      |  1 +
 drivers/crypto/chelsio/chtls/chtls_io.c   | 90 +++++++++++++++++++++++++++++--
 drivers/crypto/chelsio/chtls/chtls_main.c |  1 +
 3 files changed, 89 insertions(+), 3 deletions(-)

Comments

Dan Carpenter May 14, 2018, 11:29 a.m. UTC | #1 
On Mon, May 14, 2018 at 04:30:56PM +0530, Atul Gupta wrote:
> Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Signed-off-by: Atul Gupta <atul.gupta@chelsio.com>

There isn't a commit message for this.  It should say what the user
visible effects of this bug are.  I haven't seen Gustavo's bug report,
but probably copy and pasting that would be good?

> ---
>  drivers/crypto/chelsio/chtls/chtls.h      |  1 +
>  drivers/crypto/chelsio/chtls/chtls_io.c   | 90 +++++++++++++++++++++++++++++--
>  drivers/crypto/chelsio/chtls/chtls_main.c |  1 +
>  3 files changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls.h b/drivers/crypto/chelsio/chtls/chtls.h
> index f4b8f1e..778c194 100644
> --- a/drivers/crypto/chelsio/chtls/chtls.h
> +++ b/drivers/crypto/chelsio/chtls/chtls.h
> @@ -149,6 +149,7 @@ struct chtls_dev {
>  	struct list_head rcu_node;
>  	struct list_head na_node;
>  	unsigned int send_page_order;
> +	int max_host_sndbuf;
>  	struct key_map kmap;
>  };
>  
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 5a75be4..a4c7d2d 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -914,6 +914,78 @@ static u16 tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)
>  	return (__force u16)cpu_to_be16(thdr->length);
>  }
>  
> +static int csk_mem_free(struct chtls_dev *cdev, struct sock *sk)
> +{
> +	return (cdev->max_host_sndbuf - sk->sk_wmem_queued) > 0;

Why not just say:

	return (max_host_sndbuf > sk->sk_wmem_queued);

> +}
> +
> +static int csk_wait_memory(struct chtls_dev *cdev,
> +			   struct sock *sk, long *timeo_p)
> +{
> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> +	int sndbuf, err = 0;
> +	long current_timeo;
> +	long vm_wait = 0;
> +	bool noblock;
> +
> +	current_timeo = *timeo_p;
> +	noblock = (*timeo_p ? false : true);
> 	sndbuf = cdev->max_host_sndbuf;
> +	if (sndbuf > sk->sk_wmem_queued) {

You could use it here:

	if (csk_mem_free(cdev, sk)) {


> +		current_timeo = (prandom_u32() % (HZ / 5)) + 2;
> +		vm_wait = (prandom_u32() % (HZ / 5)) + 2;
> +	}
> +
> +	add_wait_queue(sk_sleep(sk), &wait);
> +	while (1) {
> +		sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> +
> +		if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
> +			goto do_error;
> +		if (!*timeo_p) {
> +			if (noblock)
> +				set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> +				goto do_nonblock;

There are missing curly braces here.  I feel like these gotos make the
code worse.  They don't remove duplicate lines of code.  They just
spread things out so that you have to jump around to understand this
code.  It's like being a kangaroo.

			if (noblock) {
				set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
				err = -EAGAIN;
				goto remove_queue;
			}

I always like picking a descriptive label names instead of "out:"

> +		}
> +		if (signal_pending(current))
> +			goto do_interrupted;
> +		sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> +		if (sndbuf > sk->sk_wmem_queued && !vm_wait)
> +			break;

		if (csk_mem_free(cdev, sk) && !vm_wait)


> +
> +		set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> +		sk->sk_write_pending++;
> +		sk_wait_event(sk, &current_timeo, sk->sk_err ||
> +			      (sk->sk_shutdown & SEND_SHUTDOWN) ||
> +			      (sndbuf > sk->sk_wmem_queued && !vm_wait), &wait);


			      (csk_mem_free(cdev, sk) && !vm_wait), &wait);


> +		sk->sk_write_pending--;
> +
> +		if (vm_wait) {
> +			vm_wait -= current_timeo;
> +			current_timeo = *timeo_p;
> +			if (current_timeo != MAX_SCHEDULE_TIMEOUT) {
> +				current_timeo -= vm_wait;
> +				if (current_timeo < 0)
> +					current_timeo = 0;
> +			}
> +			vm_wait = 0;
> +		}
> +		*timeo_p = current_timeo;
> +	}
> +out:
> +	remove_wait_queue(sk_sleep(sk), &wait);
> +	return err;
> +do_error:
> +	err = -EPIPE;
> +	goto out;
> +do_nonblock:
> +	err = -EAGAIN;
> +	goto out;
> +do_interrupted:
> +	err = sock_intr_errno(*timeo_p);
> +	goto out;
> +}
> +

regards,
dan carpenter
kernel test robot May 14, 2018, 2:04 p.m. UTC | #2 
Hi Atul,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on cryptodev/master]
[also build test WARNING on v4.17-rc5 next-20180514]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Atul-Gupta/build-warnings-cleanup/20180514-213306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: i386-randconfig-x009-201819 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/crypto/chelsio/chtls/chtls_io.c: In function 'csk_wait_memory':
>> drivers/crypto/chelsio/chtls/chtls_io.c:946:4: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
       if (noblock)
       ^~
   drivers/crypto/chelsio/chtls/chtls_io.c:948:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
        goto do_nonblock;
        ^~~~

vim +/if +946 drivers/crypto/chelsio/chtls/chtls_io.c

   921	
   922	static int csk_wait_memory(struct chtls_dev *cdev,
   923				   struct sock *sk, long *timeo_p)
   924	{
   925		DEFINE_WAIT_FUNC(wait, woken_wake_function);
   926		int sndbuf, err = 0;
   927		long current_timeo;
   928		long vm_wait = 0;
   929		bool noblock;
   930	
   931		current_timeo = *timeo_p;
   932		noblock = (*timeo_p ? false : true);
   933		sndbuf = cdev->max_host_sndbuf;
   934		if (sndbuf > sk->sk_wmem_queued) {
   935			current_timeo = (prandom_u32() % (HZ / 5)) + 2;
   936			vm_wait = (prandom_u32() % (HZ / 5)) + 2;
   937		}
   938	
   939		add_wait_queue(sk_sleep(sk), &wait);
   940		while (1) {
   941			sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
   942	
   943			if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
   944				goto do_error;
   945			if (!*timeo_p) {
 > 946				if (noblock)
   947					set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
   948					goto do_nonblock;
   949			}
   950			if (signal_pending(current))
   951				goto do_interrupted;
   952			sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
   953			if (sndbuf > sk->sk_wmem_queued && !vm_wait)
   954				break;
   955	
   956			set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
   957			sk->sk_write_pending++;
   958			sk_wait_event(sk, &current_timeo, sk->sk_err ||
   959				      (sk->sk_shutdown & SEND_SHUTDOWN) ||
   960				      (sndbuf > sk->sk_wmem_queued && !vm_wait), &wait);
   961			sk->sk_write_pending--;
   962	
   963			if (vm_wait) {
   964				vm_wait -= current_timeo;
   965				current_timeo = *timeo_p;
   966				if (current_timeo != MAX_SCHEDULE_TIMEOUT) {
   967					current_timeo -= vm_wait;
   968					if (current_timeo < 0)
   969						current_timeo = 0;
   970				}
   971				vm_wait = 0;
   972			}
   973			*timeo_p = current_timeo;
   974		}
   975	out:
   976		remove_wait_queue(sk_sleep(sk), &wait);
   977		return err;
   978	do_error:
   979		err = -EPIPE;
   980		goto out;
   981	do_nonblock:
   982		err = -EAGAIN;
   983		goto out;
   984	do_interrupted:
   985		err = sock_intr_errno(*timeo_p);
   986		goto out;
   987	}
   988	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/crypto/chelsio/chtls/chtls.h b/drivers/crypto/chelsio/chtls/chtls.h
index f4b8f1e..778c194 100644
--- a/drivers/crypto/chelsio/chtls/chtls.h
+++ b/drivers/crypto/chelsio/chtls/chtls.h
@@ -149,6 +149,7 @@  struct chtls_dev {
 	struct list_head rcu_node;
 	struct list_head na_node;
 	unsigned int send_page_order;
+	int max_host_sndbuf;
 	struct key_map kmap;
 };
 
diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
index 5a75be4..a4c7d2d 100644
--- a/drivers/crypto/chelsio/chtls/chtls_io.c
+++ b/drivers/crypto/chelsio/chtls/chtls_io.c
@@ -914,6 +914,78 @@  static u16 tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)
 	return (__force u16)cpu_to_be16(thdr->length);
 }
 
+static int csk_mem_free(struct chtls_dev *cdev, struct sock *sk)
+{
+	return (cdev->max_host_sndbuf - sk->sk_wmem_queued) > 0;
+}
+
+static int csk_wait_memory(struct chtls_dev *cdev,
+			   struct sock *sk, long *timeo_p)
+{
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	int sndbuf, err = 0;
+	long current_timeo;
+	long vm_wait = 0;
+	bool noblock;
+
+	current_timeo = *timeo_p;
+	noblock = (*timeo_p ? false : true);
+	sndbuf = cdev->max_host_sndbuf;
+	if (sndbuf > sk->sk_wmem_queued) {
+		current_timeo = (prandom_u32() % (HZ / 5)) + 2;
+		vm_wait = (prandom_u32() % (HZ / 5)) + 2;
+	}
+
+	add_wait_queue(sk_sleep(sk), &wait);
+	while (1) {
+		sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
+
+		if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
+			goto do_error;
+		if (!*timeo_p) {
+			if (noblock)
+				set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
+				goto do_nonblock;
+		}
+		if (signal_pending(current))
+			goto do_interrupted;
+		sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
+		if (sndbuf > sk->sk_wmem_queued && !vm_wait)
+			break;
+
+		set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
+		sk->sk_write_pending++;
+		sk_wait_event(sk, &current_timeo, sk->sk_err ||
+			      (sk->sk_shutdown & SEND_SHUTDOWN) ||
+			      (sndbuf > sk->sk_wmem_queued && !vm_wait), &wait);
+		sk->sk_write_pending--;
+
+		if (vm_wait) {
+			vm_wait -= current_timeo;
+			current_timeo = *timeo_p;
+			if (current_timeo != MAX_SCHEDULE_TIMEOUT) {
+				current_timeo -= vm_wait;
+				if (current_timeo < 0)
+					current_timeo = 0;
+			}
+			vm_wait = 0;
+		}
+		*timeo_p = current_timeo;
+	}
+out:
+	remove_wait_queue(sk_sleep(sk), &wait);
+	return err;
+do_error:
+	err = -EPIPE;
+	goto out;
+do_nonblock:
+	err = -EAGAIN;
+	goto out;
+do_interrupted:
+	err = sock_intr_errno(*timeo_p);
+	goto out;
+}
+
 int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 {
 	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
@@ -952,6 +1024,8 @@  int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 			copy = mss - skb->len;
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 		}
+		if (!csk_mem_free(cdev, sk))
+			goto wait_for_sndbuf;
 
 		if (is_tls_tx(csk) && !csk->tlshws.txleft) {
 			struct tls_hdr hdr;
@@ -1099,8 +1173,10 @@  int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 		if (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_NO_APPEND)
 			push_frames_if_head(sk);
 		continue;
+wait_for_sndbuf:
+		set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
 wait_for_memory:
-		err = sk_stream_wait_memory(sk, &timeo);
+		err = csk_wait_memory(cdev, sk, &timeo);
 		if (err)
 			goto do_error;
 	}
@@ -1131,6 +1207,7 @@  int chtls_sendpage(struct sock *sk, struct page *page,
 		   int offset, size_t size, int flags)
 {
 	struct chtls_sock *csk;
+	struct chtls_dev *cdev;
 	int mss, err, copied;
 	struct tcp_sock *tp;
 	long timeo;
@@ -1138,6 +1215,7 @@  int chtls_sendpage(struct sock *sk, struct page *page,
 	tp = tcp_sk(sk);
 	copied = 0;
 	csk = rcu_dereference_sk_user_data(sk);
+	cdev = csk->cdev;
 	timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
 
 	err = sk_stream_wait_connect(sk, &timeo);
@@ -1156,6 +1234,8 @@  int chtls_sendpage(struct sock *sk, struct page *page,
 		if (!skb || (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_NO_APPEND) ||
 		    copy <= 0) {
 new_buf:
+			if (!csk_mem_free(cdev, sk))
+				goto wait_for_sndbuf;
 
 			if (is_tls_tx(csk)) {
 				skb = get_record_skb(sk,
@@ -1167,7 +1247,7 @@  int chtls_sendpage(struct sock *sk, struct page *page,
 				skb = get_tx_skb(sk, 0);
 			}
 			if (!skb)
-				goto do_error;
+				goto wait_for_memory;
 			copy = mss;
 		}
 		if (copy > size)
@@ -1206,8 +1286,12 @@  int chtls_sendpage(struct sock *sk, struct page *page,
 		if (unlikely(ULP_SKB_CB(skb)->flags & ULPCB_FLAG_NO_APPEND))
 			push_frames_if_head(sk);
 		continue;
-
+wait_for_sndbuf:
 		set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
+wait_for_memory:
+		err = csk_wait_memory(cdev, sk, &timeo);
+		if (err)
+			goto do_error;
 	}
 out:
 	csk_reset_flag(csk, CSK_TX_MORE_DATA);
diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c b/drivers/crypto/chelsio/chtls/chtls_main.c
index 5b9dd58..e9ffc3d 100644
--- a/drivers/crypto/chelsio/chtls/chtls_main.c
+++ b/drivers/crypto/chelsio/chtls/chtls_main.c
@@ -238,6 +238,7 @@  static void *chtls_uld_add(const struct cxgb4_lld_info *info)
 	spin_lock_init(&cdev->idr_lock);
 	cdev->send_page_order = min_t(uint, get_order(32768),
 				      send_page_order);
+	cdev->max_host_sndbuf = 48 * 1024;
 
 	if (lldi->vr->key.size)
 		if (chtls_init_kmap(cdev, lldi))