[PATCH net-next] strparser: Return if socket does not have required number of bytes

[PATCH net-next] strparser: Return if socket does not have required number of bytes

[Vakul Garg]

Function strp_data_ready() should peek the associated socket to check
whether it has the required number of bytes available before queueing
work or initiating socket read via strp_read_sock(). This saves cpu
cycles because strp_read_sock() is called only when required amount of
data is available.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/strparser/strparser.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index da1a676860ca..38f8d8d8f4ad 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -384,6 +384,14 @@ void strp_data_ready(struct strparser *strp)
 	if (unlikely(strp->stopped) || strp->paused)
 		return;
 
+	/* If the socket does not contain the number bytes required by
+	 * stream parser context to proceed, return silently.
+	 */
+	if (strp->need_bytes) {
+		if (strp_peek_len(strp) < strp->need_bytes)
+			return;
+	}
+
 	/* This check is needed to synchronize with do_strp_work.
 	 * do_strp_work acquires a process lock (lock_sock) whereas
 	 * the lock held here is bh_lock_sock. The two locks can be
@@ -396,11 +404,6 @@ void strp_data_ready(struct strparser *strp)
 		return;
 	}
 
-	if (strp->need_bytes) {
-		if (strp_peek_len(strp) < strp->need_bytes)
-			return;
-	}
-
 	if (strp_read_sock(strp) == -ENOMEM)
 		queue_work(strp_wq, &strp->work);
 }
-- 
2.13.6

Re: [PATCH net-next] strparser: Return if socket does not have required number of bytes

[David Miller]

From: Vakul Garg <vakul.garg@nxp.com>
Date: Wed, 30 Jan 2019 07:31:44 +0000

> Function strp_data_ready() should peek the associated socket to check
> whether it has the required number of bytes available before queueing
> work or initiating socket read via strp_read_sock(). This saves cpu
> cycles because strp_read_sock() is called only when required amount of
> data is available.
> 
> Signed-off-by: Vakul Garg <vakul.garg@nxp.com>

You can't do this, I think.  It's racy and the user socket owned check
is absolutely necessary before you do the need bytes check.

RE: [PATCH net-next] strparser: Return if socket does not have required number of bytes

[Vakul Garg]

> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Thursday, January 31, 2019 11:30 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCH net-next] strparser: Return if socket does not have
> required number of bytes
> 
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Wed, 30 Jan 2019 07:31:44 +0000
> 
> > Function strp_data_ready() should peek the associated socket to check
> > whether it has the required number of bytes available before queueing
> > work or initiating socket read via strp_read_sock(). This saves cpu
> > cycles because strp_read_sock() is called only when required amount of
> > data is available.
> >
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> 
> You can't do this, I think.  It's racy and the user socket owned check is
> absolutely necessary before you do the need bytes check.

Do you mean to say that 'peek_len' operation can't be invoked if the socket is already owned by some other context?

My understanding by reading following link is that we can to peek_len operation without acquiring sock lock.

https://elixir.bootlin.com/linux/v5.0-rc4/source/include/linux/net.h#L191

Can you please kindly elaborate the problem?