datagram: Extend the datagram queue MSG_PEEK-ing
Subsystems (4)
PATCH 1/1 datagram: Extend the datagram queue MSG_PEEK-ing
We're working on the checkpoint-restore project. To checkpoint a unix socket we need to read its skb queue. Analogous task for TCP sockets Tejun proposed to solve with parasite + recvmsg + MSG_PEEK. That's nice, but doesn't work for unix sockets, because for them MSG_PEEK always peeks a single skb from the head of the queue.
I propose to extend the MSG_PEEK functionality with two more flags that peek either next not picked skb in queue or pick the last picked one. The latter ability is required to make it possible to re-read a message if MSG_TRUNC was reported on it.
These two flags can be used for unix stream sockets, since making the MSG_PEEK just report all data that fits the buffer length is bad -- we may have scms in queue thus turning stream socket into dgram one.
--- include/linux/socket.h | 3 ++ net/core/datagram.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 4 deletions(-)
include/linux/socket.h +3 -0
@@ -266,6 +266,9 @@ struct ucred {#define MSG_MORE 0x8000/* Sender will send more */ #define MSG_WAITFORONE 0x10000/* recvmmsg(): block until 1+ packets avail */ +#define MSG_PEEK_MORE 0x20000 +#define MSG_PEEK_AGAIN 0x40000 +#define MSG_EOF MSG_FIN#define MSG_CMSG_CLOEXEC 0x40000000/* Set close_on_exit for file
net/core/datagram.c +49 -5
73 lines not on this page · raw message
Signature · 1 line
1.5.5.6
Quoted 11 lines · Pavel Emelyanov
We're working on the checkpoint-restore project. To checkpoint a unix socket we need to read its skb queue. Analogous task for TCP sockets Tejun proposed to solve with parasite + recvmsg +
MSG_PEEK. That's nice, but doesn't work for unix sockets, because for themMSG_PEEKalways peeks a single skb from the head of the queue.I propose to extend the
MSG_PEEKfunctionality with two more flags that peek either next not picked skb in queue or pick the last picked one. The latter ability is required to make it possible to re-read a message ifMSG_TRUNCwas reported on it.
Quoted 7 lines
These two flags can be used for unix stream sockets, since making the
MSG_PEEKjust report all data that fits the buffer length is bad -- we may have scms in queue thus turning stream socket into dgram one.Signed-off-by Pavel Emelyanov <[email protected]>---
Nice !
So this CR stuff assumes an application wont use itself MSG_PEEK / MORE / AGAIN ?
(skb->peeked can only be set, never unset)
Quoted 16 lines · Eric Dumazet
Quoted 15 lines · Pavel Emelyanov
We're working on the checkpoint-restore project. To checkpoint a unix socket we need to read its skb queue. Analogous task for TCP sockets Tejun proposed to solve with parasite + recvmsg +
MSG_PEEK. That's nice, but doesn't work for unix sockets, because for themMSG_PEEKalways peeks a single skb from the head of the queue.I propose to extend the
MSG_PEEKfunctionality with two more flags that peek either next not picked skb in queue or pick the last picked one. The latter ability is required to make it possible to re-read a message ifMSG_TRUNCwas reported on it.These two flags can be used for unix stream sockets, since making the
MSG_PEEKjust report all data that fits the buffer length is bad -- we may have scms in queue thus turning stream socket into dgram one.
Quoted 8 lines
Quoted 3 lines
Signed-off-by Pavel Emelyanov <[email protected]>---
Nice !
So this CR stuff assumes an application wont use itself
MSG_PEEK/ MORE / AGAIN ?
:( Not exactly. MSG_PEEK will still work, but PEEK_MORE/PEEK_AGAIN will not.
Hm... This means that the state of "what was peek-ed already" should be on the user side. Can we pass some "offset" (in bytes for stream and in packets for datagram) to the recvmsg?
Quoted 1 line
(
skb->peekedcan only be set, never unset)
David Miller
maintainerFrom: Pavel Emelyanov <[email protected]>
Date: Fri, 10 Feb 2012 17:54:10 +0400
Quoted 11 lines
We're working on the checkpoint-restore project. To checkpoint a unix socket we need to read its skb queue. Analogous task for TCP sockets Tejun proposed to solve with parasite + recvmsg +
MSG_PEEK. That's nice, but doesn't work for unix sockets, because for themMSG_PEEKalways peeks a single skb from the head of the queue.I propose to extend the
MSG_PEEKfunctionality with two more flags that peek either next not picked skb in queue or pick the last picked one. The latter ability is required to make it possible to re-read a message ifMSG_TRUNCwas reported on it.
Quoted 5 lines
These two flags can be used for unix stream sockets, since making the
MSG_PEEKjust report all data that fits the buffer length is bad -- we may have scms in queue thus turning stream socket into dgram one.Signed-off-by Pavel Emelyanov <[email protected]>
I'm still thinking about how I feel about this.
But meanwhile you can fix some thing up, for example I really feel that you should make sure that multiple bits aren't set at the same time.
You either mean MSG_PEEK_MORE or MSG_PEEK_AGAIN, so setting both makes no sense and should be flagged as an error.
Quoted 8 lines
I'm still thinking about how I feel about this.
But meanwhile you can fix some thing up, for example I really feel that you should make sure that multiple bits aren't set at the same time.
You either mean
MSG_PEEK_MOREorMSG_PEEK_AGAIN, so setting both makes no sense and should be flagged as an error.
Actually Eric has pointed out the critical issue with this -- if a task (whose socket's queue we're about to peek) is currently peek-ing this queue himself, we will peek only the queue's tail and will potentially break this task's peeking in case he uses the _MORE/_AGAIN macros :(
I was thinking about another option of doing the same, how about introducing the peek offset member on sock (get/set via sockopt) which works like
* if == -1, then peek works as before
* if >= 0, then each peek/recvmsg will increase/decrease the value so that the next peek peeks next data
It's questionable what to do if the peek_off points into the middle of a datagram however. Here's an example of how this looks for datagram sockets (tested on pf_unix), for stream this requires more patching.
What do you think? Does it make sense to go on with this making other ->recvmsg handlers support peeking offset?
---
include/asm-generic/socket.h +1 -0
@@ -66,5 +66,6 @@#define SO_RXQ_OVFL 40#define SO_WIFI_STATUS 41 +#define SO_PEEK_OFF 42#define SCM_WIFI_STATUS SO_WIFI_STATUS#endif /* __ASM_GENERIC_SOCKET_H */
include/net/sock.h +1 -0
@@ -357,6 +357,7 @@ struct sock {struct page *sk_sndmsg_page;struct sk_buff *sk_send_head;__u32 sk_sndmsg_off; +__s32 sk_peek_off;int sk_write_pending;#ifdef CONFIG_SECURITY void *sk_security;
net/core/datagram.c +22 -6
@@ -180,21 +180,37 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned flags,* However, this function was correct in any case. 8) */ unsigned long cpu_flags; +long skip;spin_lock_irqsave (&sk->sk_receive_queue.lock, cpu_flags); - skb =skb_peek (&sk->sk_receive_queue); -if (skb) { + skip = sk->sk_peek_off; + skb_queue_walk(&sk->sk_receive_queue, skb) { *peeked = skb->peeked;if (flags &MSG_PEEK ) { skb->peeked =1 ;atomic_inc (&skb->users); - }else +if (skip >= skb->len) { + skip -= skb->len; +continue ; + } + +if (sk->sk_peek_off >=0 ) + sk->sk_peek_off += (skb->len - skip); + }else {__skb_unlink (skb, &sk->sk_receive_queue); - } -spin_unlock_irqrestore (&sk->sk_receive_queue.lock, cpu_flags); -if (skb) +if (sk->sk_peek_off >=0 ) { +if (sk->sk_peek_off >= skb->len) + sk->sk_peek_off -= skb->len; +else + sk->sk_peek_off =0 ; + } + } + +spin_unlock_irqrestore (&sk->sk_receive_queue.lock, cpu_flags);return skb; + } +spin_unlock_irqrestore (&sk->sk_receive_queue.lock, cpu_flags);/* User doesn't want to wait */ error = -EAGAIN ;
net/core/sock.c +7 -1
@@ -792,7 +792,9 @@ set_rcvbuf:case SO_WIFI_STATUS :sock_valbool_flag (sk,SOCK_WIFI_STATUS , valbool);break ; - +case SO_PEEK_OFF : + sk->sk_peek_off = val; +break ;default : ret = -ENOPROTOOPT ;break ;
@@ -1017,6 +1019,9 @@ int sock_getsockopt(struct socket *sock, int level, int optname,case SO_WIFI_STATUS : v.val = !!sock_flag (sk,SOCK_WIFI_STATUS );break ; +case SO_PEEK_OFF : + v.val = sk->sk_peek_off; +break ;default :return -ENOPROTOOPT ;
@@ -2092,6 +2097,7 @@ void sock_init_data(struct socket *sock, struct sock *sk) sk->sk_sndmsg_page =NULL ; sk->sk_sndmsg_off =0 ; + sk->sk_peek_off =-1 ; sk->sk_peer_pid =NULL ; sk->sk_peer_cred =NULL ;
David Miller
maintainerFrom: Pavel Emelyanov <[email protected]>
Date: Mon, 20 Feb 2012 16:17:11 +0400
Quoted 7 lines
I was thinking about another option of doing the same, how about introducing the peek offset member on sock (get/set via sockopt) which works like
* if == -1, then peek works as before
* if >= 0, then each peek/recvmsg will increase/decrease the value so that the next peek peeks next data
Quoted 6 lines
It's questionable what to do if the
peek_offpoints into the middle of a datagram however. Here's an example of how this looks for datagram sockets (tested onpf_unix), for stream this requires more patching.What do you think? Does it make sense to go on with this making other ->recvmsg handlers support peeking offset?
I think you need to add some kind of locking to this for sanity, for example right now you're doing test/decide/store decisions over sk_peek_off inside of __skb_recv_datagram() without the socket lock, yet another thread can set the SO_PEEK_OFF in parallel.
The locking decision will probably be worse for stream sockets.