RE: [PATCH v16 net-next 1/1] hv_sock: introduce Hyper-V Sockets

From: Michal Kubecek [mailto:mkubecek@suse.cz]

quoted

......
+static struct sock *hvsock_find_connected_socket_by_channel(
+	const struct vmbus_channel *channel)
+{
+	struct hvsock_sock *hvsk;
+
+	list_for_each_entry(hvsk, &hvsock_connected_list, connected_list) {
+		if (hvsk->channel == channel)
+			return hvsock_to_sk(hvsk);
+	}
+	return NULL;
+}

How does this work from performance point of view if there are many
connected sockets and/or high frequency of new connections? AFAICS most
other families use a hash table for socket lookup.

Hi Michal,
Per the current design of the feature in the host, there is actually an implicit
inherent limit of the number of the per-guest connections: a guest can't
have more than 2048 connections.  This is because 1 connection takes a
VMBus channel ID and at most 2048 channel IDs per guest are supported.

And I don't think the lookup function is a bottleneck because the
whole process of creating or closing a connection is actually doing lots of
things, which need several extra rounds of interactions between the host and
the guest, taking much more cycles than the lookup here.

quoted

+static void get_ringbuffer_rw_status(struct vmbus_channel *channel,
+				     bool *can_read, bool *can_write)
......
+	if (can_write) {
+		hv_get_ringbuffer_availbytes(&channel->outbound,
+					     &dummy,
+					     &avl_write_bytes);
+
+		/* We only write if there is enough space */
+		*can_write = avl_write_bytes > HVSOCK_PKT_LEN(PAGE_SIZE);

I'm not sure where does this come from but is this really supposed to be
PAGE_SIZE (not the fixed 4KB PAGE_SIZE_4K)?

Thanks for pointing this out!
I'll replace it with PAGE_SIZE_4K.

quoted

+	/* see get_ringbuffer_rw_status() */
+	set_channel_pending_send_size(channel, HVSOCK_PKT_LEN(PAGE_SIZE)

+ 1);

Same question.

I'll replace it with PAGE_SIZE_4K too.

quoted

+static int hvsock_create_sock(struct net *net, struct socket *sock,
+			      int protocol, int kern)
+{
+	struct sock *sk;
+
+	if (!capable(CAP_SYS_ADMIN) && !capable(CAP_NET_ADMIN))
+		return -EPERM;

Looks like any application wanting to use hyper-v sockets will need
rather high privileges. It would make sense if these sockets were
reserved for privileged tasks like VM management. But according to the
commit message, hv_sock is supposed to be used for regular application
to application communication. Requiring CAP_{SYS,NET}_ADMIN looks like
an overkill to me.

I agree with you. Let me remove this check.

BTW, the check was supposed to prevent regular app from using the socket,
because the current design by the host has a drawback: a connection consumes
at least 40KB unswapable memory as the host<->guest shared ring and we
don't want malicious regular apps to be able to consume all the memory.

Later I realized the per-guest number of connections couldn't exceed 2048,
so at most the host<->guest rings consume 2K * 40KB = 80MB memory and
this isn't a big concern to me.

Thanks,
-- Dexuan