* [PATCH 1/4] VSOCK: Introduce vsock_auto_bind helper
2013-06-20 9:20 [PATCH 0/4] VSOCK fix and cleanup Asias He
@ 2013-06-20 9:20 ` Asias He
2013-06-20 15:16 ` Andy King
2013-06-20 9:20 ` [PATCH 2/4] VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb Asias He
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Asias He @ 2013-06-20 9:20 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Andy King, Dmitry Torokhov, Reilly Grant,
Asias He
This peace of code is called tree times, let's have a helper for it.
Signed-off-by: Asias He <asias@redhat.com>
---
net/vmw_vsock/af_vsock.c | 49 +++++++++++++++++++++---------------------------
1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 3f77f42..b0b362a 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -165,6 +165,18 @@ static struct list_head vsock_bind_table[VSOCK_HASH_SIZE + 1];
static struct list_head vsock_connected_table[VSOCK_HASH_SIZE];
static DEFINE_SPINLOCK(vsock_table_lock);
+/* Autobind this socket to the local address if necessary. */
+static int vsock_auto_bind(struct vsock_sock *vsk)
+{
+ struct sock *sk = sk_vsock(vsk);
+ struct sockaddr_vm local_addr;
+
+ if (vsock_addr_bound(&vsk->local_addr))
+ return 0;
+ vsock_addr_init(&local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
+ return __vsock_bind(sk, &local_addr);
+}
+
static void vsock_init_tables(void)
{
int i;
@@ -956,15 +968,10 @@ static int vsock_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
lock_sock(sk);
- if (!vsock_addr_bound(&vsk->local_addr)) {
- struct sockaddr_vm local_addr;
-
- vsock_addr_init(&local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
- err = __vsock_bind(sk, &local_addr);
- if (err != 0)
- goto out;
+ err = vsock_auto_bind(vsk);
+ if (err)
+ goto out;
- }
/* If the provided message contains an address, use that. Otherwise
* fall back on the socket's remote handle (if it has been connected).
@@ -1038,15 +1045,9 @@ static int vsock_dgram_connect(struct socket *sock,
lock_sock(sk);
- if (!vsock_addr_bound(&vsk->local_addr)) {
- struct sockaddr_vm local_addr;
-
- vsock_addr_init(&local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
- err = __vsock_bind(sk, &local_addr);
- if (err != 0)
- goto out;
-
- }
+ err = vsock_auto_bind(vsk);
+ if (err)
+ goto out;
if (!transport->dgram_allow(remote_addr->svm_cid,
remote_addr->svm_port)) {
@@ -1163,17 +1164,9 @@ static int vsock_stream_connect(struct socket *sock, struct sockaddr *addr,
memcpy(&vsk->remote_addr, remote_addr,
sizeof(vsk->remote_addr));
- /* Autobind this socket to the local address if necessary. */
- if (!vsock_addr_bound(&vsk->local_addr)) {
- struct sockaddr_vm local_addr;
-
- vsock_addr_init(&local_addr, VMADDR_CID_ANY,
- VMADDR_PORT_ANY);
- err = __vsock_bind(sk, &local_addr);
- if (err != 0)
- goto out;
-
- }
+ err = vsock_auto_bind(vsk);
+ if (err)
+ goto out;
sk->sk_state = SS_CONNECTING;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/4] VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb
2013-06-20 9:20 [PATCH 0/4] VSOCK fix and cleanup Asias He
2013-06-20 9:20 ` [PATCH 1/4] VSOCK: Introduce vsock_auto_bind helper Asias He
@ 2013-06-20 9:20 ` Asias He
2013-06-20 15:15 ` Andy King
2013-06-20 9:20 ` [PATCH 3/4] VSOCK: Remove unnecessary label Asias He
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Asias He @ 2013-06-20 9:20 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Andy King, Dmitry Torokhov, Reilly Grant,
Asias He
vmci_transport_recv_dgram_cb always return VMCI_SUCESS even if we fail
to allocate skb, return VMCI_ERROR_NO_MEM instead.
Signed-off-by: Asias He <asias@redhat.com>
---
net/vmw_vsock/vmci_transport.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index daff752..99b511d 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -625,13 +625,14 @@ static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
/* Attach the packet to the socket's receive queue as an sk_buff. */
skb = alloc_skb(size, GFP_ATOMIC);
- if (skb) {
- /* sk_receive_skb() will do a sock_put(), so hold here. */
- sock_hold(sk);
- skb_put(skb, size);
- memcpy(skb->data, dg, size);
- sk_receive_skb(sk, skb, 0);
- }
+ if (!skb)
+ return VMCI_ERROR_NO_MEM;
+
+ /* sk_receive_skb() will do a sock_put(), so hold here. */
+ sock_hold(sk);
+ skb_put(skb, size);
+ memcpy(skb->data, dg, size);
+ sk_receive_skb(sk, skb, 0);
return VMCI_SUCCESS;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 3/4] VSOCK: Remove unnecessary label
2013-06-20 9:20 [PATCH 0/4] VSOCK fix and cleanup Asias He
2013-06-20 9:20 ` [PATCH 1/4] VSOCK: Introduce vsock_auto_bind helper Asias He
2013-06-20 9:20 ` [PATCH 2/4] VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb Asias He
@ 2013-06-20 9:20 ` Asias He
2013-06-20 15:13 ` Andy King
2013-06-20 9:20 ` [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH Asias He
2013-06-24 6:52 ` [PATCH 0/4] VSOCK fix and cleanup David Miller
4 siblings, 1 reply; 12+ messages in thread
From: Asias He @ 2013-06-20 9:20 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Andy King, Dmitry Torokhov, Reilly Grant,
Asias He
Signed-off-by: Asias He <asias@redhat.com>
---
net/vmw_vsock/vmci_transport.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 99b511d..ffc11df 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -940,10 +940,9 @@ static void vmci_transport_recv_pkt_work(struct work_struct *work)
* reset to prevent that.
*/
vmci_transport_send_reset(sk, pkt);
- goto out;
+ break;
}
-out:
release_sock(sk);
kfree(recv_pkt_info);
/* Release reference obtained in the stream callback when we fetched
--
1.8.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH
2013-06-20 9:20 [PATCH 0/4] VSOCK fix and cleanup Asias He
` (2 preceding siblings ...)
2013-06-20 9:20 ` [PATCH 3/4] VSOCK: Remove unnecessary label Asias He
@ 2013-06-20 9:20 ` Asias He
2013-06-20 15:12 ` Andy King
2013-06-24 6:52 ` [PATCH 0/4] VSOCK fix and cleanup David Miller
4 siblings, 1 reply; 12+ messages in thread
From: Asias He @ 2013-06-20 9:20 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Andy King, Dmitry Torokhov, Reilly Grant,
Asias He
If we mod with VSOCK_HASH_SIZE -1, we get 0, 1, .... 249. Actually, we
have vsock_bind_table[0 ... 250] and vsock_connected_table[0 .. 250].
In this case the last entry will never be used.
We should mod with VSOCK_HASH_SIZE instead.
Signed-off-by: Asias He <asias@redhat.com>
---
net/vmw_vsock/af_vsock.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index b0b362a..593071d 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -144,18 +144,18 @@ EXPORT_SYMBOL_GPL(vm_sockets_get_local_cid);
* VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
* vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
* vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets. The hash function
- * mods with VSOCK_HASH_SIZE - 1 to ensure this.
+ * mods with VSOCK_HASH_SIZE to ensure this.
*/
#define VSOCK_HASH_SIZE 251
#define MAX_PORT_RETRIES 24
-#define VSOCK_HASH(addr) ((addr)->svm_port % (VSOCK_HASH_SIZE - 1))
+#define VSOCK_HASH(addr) ((addr)->svm_port % VSOCK_HASH_SIZE)
#define vsock_bound_sockets(addr) (&vsock_bind_table[VSOCK_HASH(addr)])
#define vsock_unbound_sockets (&vsock_bind_table[VSOCK_HASH_SIZE])
/* XXX This can probably be implemented in a better way. */
#define VSOCK_CONN_HASH(src, dst) \
- (((src)->svm_cid ^ (dst)->svm_port) % (VSOCK_HASH_SIZE - 1))
+ (((src)->svm_cid ^ (dst)->svm_port) % VSOCK_HASH_SIZE)
#define vsock_connected_sockets(src, dst) \
(&vsock_connected_table[VSOCK_CONN_HASH(src, dst)])
#define vsock_connected_sockets_vsk(vsk) \
--
1.8.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH
2013-06-20 9:20 ` [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH Asias He
@ 2013-06-20 15:12 ` Andy King
2013-06-21 0:22 ` Asias He
0 siblings, 1 reply; 12+ messages in thread
From: Andy King @ 2013-06-20 15:12 UTC (permalink / raw)
To: Asias He; +Cc: netdev, David S. Miller, Dmitry Torokhov, Reilly Grant
> If we mod with VSOCK_HASH_SIZE -1, we get 0, 1, .... 249. Actually, we
> have vsock_bind_table[0 ... 250] and vsock_connected_table[0 .. 250].
> In this case the last entry will never be used.
If I remember correctly, we did this on purpose. There's actually a
comment about it:
> * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
> * vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
> * vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets. The hash
[250] is for unbound sockets. If you hash on that, you'll mistakenly
get an unbound socket when looking for a bound one.
It is confusing, so perhaps a better way is just to move unbound into
its own table.
Thanks!
- Andy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH
2013-06-20 15:12 ` Andy King
@ 2013-06-21 0:22 ` Asias He
2013-06-21 12:48 ` Andy King
0 siblings, 1 reply; 12+ messages in thread
From: Asias He @ 2013-06-21 0:22 UTC (permalink / raw)
To: Andy King; +Cc: netdev, David S. Miller, Dmitry Torokhov, Reilly Grant
On Thu, Jun 20, 2013 at 08:12:13AM -0700, Andy King wrote:
> > If we mod with VSOCK_HASH_SIZE -1, we get 0, 1, .... 249. Actually, we
> > have vsock_bind_table[0 ... 250] and vsock_connected_table[0 .. 250].
> > In this case the last entry will never be used.
>
> If I remember correctly, we did this on purpose. There's actually a
> comment about it:
>
> > * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
> > * vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
> > * vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets. The hash
>
> [250] is for unbound sockets. If you hash on that, you'll mistakenly
> get an unbound socket when looking for a bound one.
We have
#define VSOCK_HASH_SIZE 251
static struct list_head vsock_bind_table[VSOCK_HASH_SIZE + 1];
#define vsock_bound_sockets(addr) (&vsock_bind_table[VSOCK_HASH(addr)])
#define vsock_unbound_sockets (&vsock_bind_table[VSOCK_HASH_SIZE])
So
vsock_bind_table[251 + 1]
[0-250] is for bound sockets, [251] is for unbound sockets, no?
> It is confusing, so perhaps a better way is just to move unbound into
> its own table.
This isn't that confusing, but it would be clearer to have a own unbound table.
> Thanks!
> - Andy
--
Asias
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/4] VSOCK fix and cleanup
2013-06-20 9:20 [PATCH 0/4] VSOCK fix and cleanup Asias He
` (3 preceding siblings ...)
2013-06-20 9:20 ` [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH Asias He
@ 2013-06-24 6:52 ` David Miller
4 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2013-06-24 6:52 UTC (permalink / raw)
To: asias; +Cc: netdev, acking, dtor, grantr
From: Asias He <asias@redhat.com>
Date: Thu, 20 Jun 2013 17:20:29 +0800
>
> Asias He (4):
> VSOCK: Introduce vsock_auto_bind helper
> VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb
> VSOCK: Remove unnecessary label
> VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH
Series applied to net-next, thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread