Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Eric Dumazet <hidden>
Date: 2011-03-13 15:53:00
Also in:
kvm, lkml, netdev
Le dimanche 13 mars 2011 à 17:06 +0200, Michael S. Tsirkin a écrit :
quoted hunk ↗ jump to hunk
On Mon, Jan 17, 2011 at 04:11:17PM +0800, Jason Wang wrote:quoted
We can use lock_sock_fast() instead of lock_sock() in order to get speedup in peek_head_len(). Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/vhost/net.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index c32a2e4..50b622a 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c@@ -211,12 +211,12 @@ static int peek_head_len(struct sock *sk) { struct sk_buff *head; int len = 0; + bool slow = lock_sock_fast(sk); - lock_sock(sk); head = skb_peek(&sk->sk_receive_queue); if (head) len = head->len; - release_sock(sk); + unlock_sock_fast(sk, slow); return len; }Wanted to apply this, but looking at the code I think the lock_sock here is wrong. What we really need is to handle the case where the skb is pulled from the receive queue after skb_peek. However this is not the right lock to use for that, sk_receive_queue.lock is. So I expect the following is the right way to handle this. Comments? Signed-off-by: Michael S. Tsirkin <mst@redhat.com>diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 0329c41..5720301 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c@@ -213,12 +213,13 @@ static int peek_head_len(struct sock *sk) { struct sk_buff *head; int len = 0; + unsigned long flags; - lock_sock(sk); + spin_lock_irqsave(&sk->sk_receive_queue.lock, flags); head = skb_peek(&sk->sk_receive_queue); - if (head) + if (likely(head)) len = head->len; - release_sock(sk); + spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags); return len; }
You may be right, only way to be sure is to check the other side. If it uses skb_queue_tail(), then yes, your patch is fine. If other side did not lock socket, then your patch is a bug fix.