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

[Dexuan Cui]

> From: Michal Kubecek [mailto:mkubecek@xxxxxxx]
> > ......
> > +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.

> > +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.

> > +	/* 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.

> > +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