lkml.org 
[lkml]   [2016]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v16 net-next 1/1] hv_sock: introduce Hyper-V Sockets
Date
> From: Michal Kubecek [mailto:mkubecek@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

\
 
 \ /
  Last update: 2016-07-13 14:01    [W:0.055 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site