Skip to contentSkip to conversation

core: enable more fine-grained datagram reception control

PATCH Applied Rainer Weikusat 2 participants 10 messages Dec 6, 2015 – Dec 8, 2015

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Rainer Weikusat

author
#0 lore raw

PATCH 1/2 core: enable more fine-grained datagram reception control

The __skb_recv_datagram routine in core/ datagram.c provides a general skb reception factility supposed to be utilized by protocol modules providing datagram sockets. It encompasses both the actual recvmsg code and a surrounding 'sleep until data is available' loop. This is inconvenient if a protocol module has to use additional locking in order to maintain some per-socket state the generic datagram socket code is unaware of (as the af_unix code does). The patch below moves the recvmsg proper code into a new __skb_try_recv_datagram routine which doesn't sleep and renames wait_for_more_packets to __skb_wait_for_more_packets, both routines being exported interfaces. The original __skb_recv_datagram routine is reimplemented on top of these two functions such that its user-visible behaviour remains unchanged.

Signed-off-by Rainer Weikusat <[email protected]>

---

include/linux/skbuff.h +6 -0
index 4355129..dce91ac 100644
@@ -2788,6 +2788,12 @@ static inline void skb_frag_list_init(struct sk_buff *skb)
 #define skb_walk_frags(skb, iter)	\
 	for (iter = skb_shinfo(skb)->frag_list; iter; iter = iter->next)
 
+
+int __skb_wait_for_more_packets(struct sock *sk, int *err, long *timeo_p,
+				const struct sk_buff *skb);
+struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned flags,
+					int *peeked, int *off, int *err,
+					struct sk_buff **last);
 struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned flags,
 				    int *peeked, int *off, int *err);
 struct sk_buff *skb_recv_datagram(struct sock *sk, unsigned flags, int noblock,
net/core/datagram.c +48 -29

148 lines not on this page · raw message

maintainer Applied
#1 lore raw

From: Rainer Weikusat <[email protected]>
Date: Sun, 06 Dec 2015 21:11:34 +0000

Quoted 13 lines

The __skb_recv_datagram routine in core/ datagram.c provides a general skb reception factility supposed to be utilized by protocol modules providing datagram sockets. It encompasses both the actual recvmsg code and a surrounding 'sleep until data is available' loop. This is inconvenient if a protocol module has to use additional locking in order to maintain some per-socket state the generic datagram socket code is unaware of (as the af_unix code does). The patch below moves the recvmsg proper code into a new __skb_try_recv_datagram routine which doesn't sleep and renames wait_for_more_packets to __skb_wait_for_more_packets, both routines being exported interfaces. The original __skb_recv_datagram routine is reimplemented on top of these two functions such that its user-visible behaviour remains unchanged.

Quoted 1 line
Signed-off-by Rainer Weikusat <[email protected]>

Applied to net-next.

author
#2 lore raw

breaks blocking receive for other users (was: [PATCH 01/02] core: enable more fine-grained datagram reception control)

Quoted 16 lines · David Miller

From: Rainer Weikusat <[email protected]>
Date: Sun, 06 Dec 2015 21:11:34 +0000

Quoted 13 lines

The __skb_recv_datagram routine in core/ datagram.c provides a general skb reception factility supposed to be utilized by protocol modules providing datagram sockets. It encompasses both the actual recvmsg code and a surrounding 'sleep until data is available' loop. This is inconvenient if a protocol module has to use additional locking in order to maintain some per-socket state the generic datagram socket code is unaware of (as the af_unix code does). The patch below moves the recvmsg proper code into a new __skb_try_recv_datagram routine which doesn't sleep and renames wait_for_more_packets to __skb_wait_for_more_packets, both routines being exported interfaces. The original __skb_recv_datagram routine is reimplemented on top of these two functions such that its user-visible behaviour remains unchanged.

Quoted 3 lines
Quoted 1 line
Signed-off-by Rainer Weikusat <[email protected]>

Applied to net-next.

Because of an oversight, this change breaks blocking datagram reception for anyone but the AF_UNIX SOCK_DGRAM code. I've noticed this by chance while running a test program for SOCK_STREAM sockets. The problem seems to be (fix not yet tested) that a - is missing in the *err check in __skb_recv_datagram.

Proposed fix:

------------

net/core/datagram.c +1 -1
index 7daff66..fa9dc64 100644
@@ -275,7 +275,7 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
                if (skb)
                        return skb;
 
-               if (*err != EAGAIN)
+               if (*err != -EAGAIN)
                        break;
        } while (timeo &&
                !__skb_wait_for_more_packets(sk, err, &timeo, last));

-------------

Provided this works (very likely), I'll send a real patch for that ASAP.

author
#3 lore raw

PATCH 1/1 fix inverted test in __skb_recv_datagram

As the kernel generally uses negated error numbers, *err needs to be compared with -EAGAIN (d'oh).

Signed-off-by Rainer Weikusat <[email protected]>
Fixes ea3793ee29d3

---

net/core/datagram.c +1 -1
index 7daff66..fa9dc64 100644
@@ -275,7 +275,7 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 		if (skb)
 			return skb;
 
-		if (*err != EAGAIN)
+		if (*err != -EAGAIN)
 			break;
 	} while (timeo &&
 		!__skb_wait_for_more_packets(sk, err, &timeo, last));
maintainer
#4 lore raw

Re: [PATCH] fix inverted test in __skb_recv_datagram

From: Rainer Weikusat <[email protected]>
Date: Mon, 07 Dec 2015 23:30:58 +0000

Quoted 5 lines

As the kernel generally uses negated error numbers, *err needs to be compared with -EAGAIN (d'oh).

Signed-off-by Rainer Weikusat <[email protected]>
Fixes ea3793ee29d3

Improperly formatted Fixes: tag, you must also include the commit header line, in parenthesis and double quotes, after the SHA_ID.

Futhermore this is the wrong SHA_ID.

author
#5 lore raw

Re: [PATCH] fix inverted test in __skb_recv_datagram

Quoted 6 lines · David Miller

From: Rainer Weikusat <[email protected]>
Date: Mon, 07 Dec 2015 23:30:58 +0000

Quoted 3 lines

As the kernel generally uses negated error numbers, *err needs to be compared with -EAGAIN (d'oh).

Quoted 7 lines
Quoted 2 lines
Signed-off-by Rainer Weikusat <[email protected]>
Fixes ea3793ee29d3

Improperly formatted Fixes: tag, you must also include the commit header line, in parenthesis and double quotes, after the SHA_ID.

Futhermore this is the wrong SHA_ID.

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=ea3793ee29d3

displays the commit I was referring to, namely, the one containing this

+ timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT); +
+ do {
+ skb = __skb_try_recv_datagram(sk, flags, peeked, off, err,
+ &last);
+ if (skb)
+ return skb;
+
+ if (*err != EAGAIN)
+ break;
+ } while (timeo &&
+ !__skb_wait_for_more_packets(sk, err, &timeo, last));

which added the inverted test, IOW, if this is the wrong hash, I have no idea what the right one could be. I'll resubmit this with the 'one line summary' added. After noticing the issue around 23:10 UK time yesterday, I was in a bit of a hurry and stopped reading the 'Fixes' text in SubmittingPatches after the "with the first 12 characters".

maintainer
#6 lore raw

Re: [PATCH] fix inverted test in __skb_recv_datagram

From: Rainer Weikusat <[email protected]>
Date: Tue, 08 Dec 2015 14:46:36 +0000

Quoted 10 lines
Quoted 9 lines · David Miller

From: Rainer Weikusat <[email protected]>
Date: Mon, 07 Dec 2015 23:30:58 +0000

Quoted 5 lines

As the kernel generally uses negated error numbers, *err needs to be compared with -EAGAIN (d'oh).

Signed-off-by Rainer Weikusat <[email protected]>
Fixes ea3793ee29d3
Quoted 8 lines
Quoted 4 lines

Improperly formatted Fixes: tag, you must also include the commit header line, in parenthesis and double quotes, after the SHA_ID.

Futhermore this is the wrong SHA_ID.

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=ea3793ee29d3

displays the commit I was referring to, namely, the one containing this

My bad.

author
#7 lore raw

Re: [PATCH] fix inverted test in __skb_recv_datagram

In reply to David Miller

As the kernel generally uses negated error numbers, *err needs to be compared with -EAGAIN (d'oh).

Signed-off-by Rainer Weikusat <[email protected]>
Fixes ea3793ee29d3 ("core: enable more fine-grained datagram reception control")

---

net/core/datagram.c +1 -1
index 7daff66..fa9dc64 100644
@@ -275,7 +275,7 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 		if (skb)
 			return skb;
 
-		if (*err != EAGAIN)
+		if (*err != -EAGAIN)
 			break;
 	} while (timeo &&
 		!__skb_wait_for_more_packets(sk, err, &timeo, last));
maintainer Applied
#8 lore raw

Re: [PATCH] fix inverted test in __skb_recv_datagram

From: Rainer Weikusat <[email protected]>
Date: Tue, 08 Dec 2015 14:47:56 +0000

Quoted 5 lines

As the kernel generally uses negated error numbers, *err needs to be compared with -EAGAIN (d'oh).

Signed-off-by Rainer Weikusat <[email protected]>
Fixes ea3793ee29d3 ("core: enable more fine-grained datagram reception control")

Applied, thanks.

Please, in the future, place proper subsystem prefixes in your Subject lines. In this case, "net: " would have been appropriate and it wouldn't be the end of the world if you capitalized your commit header line too.

I fixed both of these things while installing your change.

Thanks.

author
#9 lore raw

Re: [PATCH] fix inverted test in __skb_recv_datagram

David Miller <[email protected]> writes:

[...]

Quoted 6 lines

Please, in the future, place proper subsystem prefixes in your Subject lines. In this case, "net: " would have been appropriate and it wouldn't be the end of the world if you capitalized your commit header line too.

I fixed both of these things while installing your change.

Sorry. Will try to keep this in mind.