| Message ID | 1371720033-19714-5-git-send-email-asias@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
> [0-250] is for bound sockets, [251] is for unbound sockets, no? Well caught. Acked-by: Andy King <acking@vmware.com> Thanks! - Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) \
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(-)