> From: Michal Kubecek [mailto:mkube...@suse.cz]
> > ......
> > +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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
