Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450269 > unrolled thread

Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets David Miller <davem@davemloft.net> - 2016-07-26 02:40 +0200
    RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-07-26 05:10 +0200
      Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets David Miller <davem@davemloft.net> - 2016-07-26 06:40 +0200
        RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-07-26 07:30 +0200
          RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-07-26 09:10 +0200
            Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Michal Kubecek <mkubecek@suse.cz> - 2016-07-26 12:00 +0200
              RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-07-26 15:30 +0200
                RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-07-27 11:30 +0200
            Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets David Miller <davem@davemloft.net> - 2016-07-26 19:50 +0200
              RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-07-27 11:30 +0200

#1450269 — Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets

From: Dexuan Cui <decui@microsoft.com>
Date: Sat, 23 Jul 2016 01:35:51 +0000

> +static struct sock *hvsock_create(struct net *net, struct socket *sock,
> +				  gfp_t priority, unsigned short type)
> +{
> +	struct hvsock_sock *hvsk;
> +	struct sock *sk;
> +
> +	sk = sk_alloc(net, AF_HYPERV, priority, &hvsock_proto, 0);
> +	if (!sk)
> +		return NULL;
 ...
> +	/* Looks stream-based socket doesn't need this. */
> +	sk->sk_backlog_rcv = NULL;
> +
> +	sk->sk_state = 0;
> +	sock_reset_flag(sk, SOCK_DONE);

All of these are unnecessary initializations, since sk_alloc() zeroes
out the 'sk' object for you.

[toc] | [next] | [standalone]

#1450343

> From: David Miller [mailto:davem@davemloft.net]
> 
> From: Dexuan Cui <decui@microsoft.com>
> Date: Sat, 23 Jul 2016 01:35:51 +0000
> 
> > +static struct sock *hvsock_create(struct net *net, struct socket *sock,
> > +				  gfp_t priority, unsigned short type)
> > +{
> > +	struct hvsock_sock *hvsk;
> > +	struct sock *sk;
> > +
> > +	sk = sk_alloc(net, AF_HYPERV, priority, &hvsock_proto, 0);
> > +	if (!sk)
> > +		return NULL;
>  ...
> > +	/* Looks stream-based socket doesn't need this. */
> > +	sk->sk_backlog_rcv = NULL;
> > +
> > +	sk->sk_state = 0;
> > +	sock_reset_flag(sk, SOCK_DONE);
> 
> All of these are unnecessary initializations, since sk_alloc() zeroes
> out the 'sk' object for you.

Hi David,
Thanks for the comment!  I'll remove the 3 lines.

May I know if you have more comments?

BTW, during the past month, at least 7 other people also reviewed
the patch and gave me quite a few good comments, which have
been addressed. Though only one of them gave the Reviewed-by
line for now, I guess I would get more if I ping them to have a look
at the latest version of the patch, i.e., v19 -- I'm going to post it
with the aforementioned 3 lines removed and if you've more 
comments, I'm ready to address them too. :-)

Thanks,
-- Dexuan

[toc] | [prev] | [next] | [standalone]

#1450367

From: Dexuan Cui <decui@microsoft.com>
Date: Tue, 26 Jul 2016 03:09:16 +0000

> BTW, during the past month, at least 7 other people also reviewed
> the patch and gave me quite a few good comments, which have
> been addressed.

Correction: Several people gave coding style and simple corrections
to your patch.

Very few gave any review of the _SUBSTANCE_ of your changes.

And the one of the few who did, and suggested you build your
facilities using the existing S390 hypervisor socket infrastructure,
you brushed off _IMMEDIATELY_.

That drives me crazy.  The one person who gave you real feedback
you basically didn't consider seriously at all.

I know why you don't want to consider alternative implementations,
and it's because you guys have so much invested in what you've
implemented already.

But that's tough and not our problem.

And until this changes, yes, this submission will be stuck in the
mud and continue slogging on like this.

Sorry.

[toc] | [prev] | [next] | [standalone]

#1450383

> From: David Miller [mailto:davem@davemloft.net]
> ...
> From: Dexuan Cui <decui@microsoft.com>
> Date: Tue, 26 Jul 2016 03:09:16 +0000
> 
> > BTW, during the past month, at least 7 other people also reviewed
> > the patch and gave me quite a few good comments, which have
> > been addressed.
> 
> Correction: Several people gave coding style and simple corrections
> to your patch.
> 
> Very few gave any review of the _SUBSTANCE_ of your changes.
> 
> And the one of the few who did, and suggested you build your
> facilities using the existing S390 hypervisor socket infrastructure,
> you brushed off _IMMEDIATELY_.
>
> That drives me crazy.  The one person who gave you real feedback
> you basically didn't consider seriously at all.

Hi David,
I'm very sorry -- I guess I must have missed something here -- I don't
remember somebody replied with S390 hypervisor socket
infrastructure... I'm re-reading all the replies, trying to locate the
reply and I'll find out why I didn't take it seriously. Sorry in advance.

> I know why you don't want to consider alternative implementations,
> and it's because you guys have so much invested in what you've
> implemented already.
This is not true. I'm absolutely open to any possibility to have an
alternative better implementation.
Please allow me to find the "S390 hypervisor socket infrastructure" reply
first and I'll report back ASAP.
 
> But that's tough and not our problem.
> 
> And until this changes, yes, this submission will be stuck in the
> mud and continue slogging on like this.

I definitely agree and understand.

Thanks,
-- Dexuan

[toc] | [prev] | [next] | [standalone]

#1450428

> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of Dexuan Cui
> ...
> > From: David Miller [mailto:davem@davemloft.net]
> > ...
> > From: Dexuan Cui <decui@microsoft.com>
> > Date: Tue, 26 Jul 2016 03:09:16 +0000
> >
> > > BTW, during the past month, at least 7 other people also reviewed
> > > the patch and gave me quite a few good comments, which have
> > > been addressed.
> >
> > Correction: Several people gave coding style and simple corrections
> > to your patch.
> >
> > Very few gave any review of the _SUBSTANCE_ of your changes.
> >
> > And the one of the few who did, and suggested you build your
> > facilities using the existing S390 hypervisor socket infrastructure,
> > you brushed off _IMMEDIATELY_.
> >
> > That drives me crazy.  The one person who gave you real feedback
> > you basically didn't consider seriously at all.
>
> Hi David,
> I'm very sorry -- I guess I must have missed something here -- I don't
> remember somebody replied with S390 hypervisor socket
> infrastructure... I'm re-reading all the replies, trying to locate the
> reply and I'll find out why I didn't take it seriously. Sorry in advance.

Hi, David,
I checked all the comments I received and all my replies (at least I really
tried my best to check my Inbox) , but couldn't find the "S390 hypervisor
socket infrastructure" mail.

I googled "S390 hypervisor socket" but didn't find anything related (I think).

I'm really sorry -- could you please give a little more info about it?

If you meant https://lkml.org/lkml/2016/7/13/382, I don't think Michal
Kubecek was suggesting I build my code using the existing AF_VSOCK
code(?)  I think he was only asking me to clarify the way I used to write
the text to explain why I can't fit my code into the existing AF_VSOCK
code. BTW, AF_VSOCK is not on S390, I think.

If this is the case, I'm sorry I didn't explain the reason clearer.
My replies last year explained the reason with more info:
https://lkml.org/lkml/2015/7/7/1162
https://lkml.org/lkml/2015/7/17/67
And I thought people agreed that a new address family is justified.

Please let me excerpt the most related snippets in my old replies:

--------------------------------------
The biggest difference is the size of the endpoint (u128 vs. u32):
<u32 ContextID, u32 Port> in AF_VOSCK
vs.
<u128 GUID_VM_ID, u128 GUID_ServiceID> in AF_HYPERV.

In the AF_VSOCK code and the related transport layer (the wrapper
ops of VMware's VMCI), the size is widely used in kernel space
(and user space application). If I have to fit my code to AF_VSOCK
code, I would have to mess up the AF_VSOCK code in many places
by adding ugly code like:

IF the endpoint size is <u32, u32> THEN
        use the existing logic;
ELSE
        use the new logic;

And the user space application has to explicitly handle the
different endpoint sizes too.
--------------------------------------

Looking forward to your reply!

Thanks,
-- Dexuan

[toc] | [prev] | [next] | [standalone]

#1450510

On Tue, Jul 26, 2016 at 07:09:41AM +0000, Dexuan Cui wrote:
> If you meant https://lkml.org/lkml/2016/7/13/382, I don't think Michal
> Kubecek was suggesting I build my code using the existing AF_VSOCK
> code(?)  I think he was only asking me to clarify the way I used to write
> the text to explain why I can't fit my code into the existing AF_VSOCK
> code. BTW, AF_VSOCK is not on S390, I think.

Actually, I believe building on top of existing AF_VSOCK should be the
first thought and only if this way shows unfeasible, one should consider
a completely new implementation from scratch. After all, when VMware was
upstreaming vsock, IIRC they had to work hard on making it a generic
solution rather than a one purpose tool tailored for their specific use
case.

What I wanted to say in that mail was that I didn't find the reasoning
very convincing. The only point that wasn't like "AF_VSOCK has many
features we don't need" was the incompatible addressing scheme. The
cover letter text didn't convince me it was given as much thought as it
deserved. I felt - and it still feel - that the option of building on
top of vsock wasn't considered seriously enough.

I must also admit I'm a bit confused by your response to the issue of
socket lookup performance. I always thought the main reason to use
special hypervisor sockets instead of TCP/IP over virtual network
devices was efficiency (to avoid the overhead of network protocol
processing). The fact that traversing a linear linked list under
a global mutex for each socket lookup is not an issue as opening
a connection is going to be slow anyway surprised me therefore. But
maybe it's fine as the typical use case is going to be small number of
long running connections and traffic performance is going to make for
the connection latency. Or there are other advantages, I don't know.
But if that is the case, it would IMHO deserve to be explained.

                                                       Michal Kubecek

[toc] | [prev] | [next] | [standalone]

#1450602

> From: Michal Kubecek [mailto:mkubecek@suse.cz]
> Sent: Tuesday, July 26, 2016 17:57
>  ...
> On Tue, Jul 26, 2016 at 07:09:41AM +0000, Dexuan Cui wrote:
> > ... I don't think Michal
> > Kubecek was suggesting I build my code using the existing AF_VSOCK
> > code(?)  I think he was only asking me to clarify the way I used to write
> > the text to explain why I can't fit my code into the existing AF_VSOCK
> > code. BTW, AF_VSOCK is not on S390, I think.
> 
> Actually, I believe building on top of existing AF_VSOCK should be the
> first thought and only if this way shows unfeasible, one should consider
> a completely new implementation from scratch. After all, when VMware
> was upstreaming vsock, IIRC they had to work hard on making it
> a generic solution rather than a one purpose tool tailored for their specific use
> case.
> 
> What I wanted to say in that mail was that I didn't find the reasoning
> very convincing. The only point that wasn't like "AF_VSOCK has many
> features we don't need" was the incompatible addressing scheme. The
> cover letter text didn't convince me it was given as much thought as it
> deserved. I felt - and it still feel - that the option of building on
> top of vsock wasn't considered seriously enough.
Hi Michal,
Thank you very much for the detailed explanation!

Just now I read your previous reply again and I think I actually failed to
get your point and my reply was inappropriate. I'm sorry about that.
 
When I firstly made the patch last July, I did try to build it on AF_VSOCK, 
but my feeling was that I had to made big changes to AF_VSOCK
code and its related transport layer driver's code. My feeling was that
the AF_VSOCK solution's implementation is not so generic that I can fit
mine in (easily).

To make my feeling more concrete so I can answer your question
properly, I'll be figuring out exactly how big the required changes will
be -- I'm afraid this would take non-trivial time, but I'll try to finish the
investigation ASAP.

The biggest challenge is the incompatible addressing scheme.
If you could give some advice, I would be very grateful.

> I must also admit I'm a bit confused by your response to the issue of
> socket lookup performance. I always thought the main reason to use
> special hypervisor sockets instead of TCP/IP over virtual network
> devices was efficiency (to avoid the overhead of network protocol
> processing). 
Yes, I agree with you.

BTW, IMO hypervisor sockets have an advantage of "zero-configuration".
To make TCP/IP work between host/guest, we need to add a NIC to
the guest, configure the NIC properly in the guest and find a way to
let the host/guest know each other's IP address, etc.

With hypervisor sockets, there is almost no such configuration effort.

> The fact that traversing a linear linked list under
> a global mutex for each socket lookup is not an issue as opening
> a connection is going to be slow anyway surprised me therefore. 
This is because, the design of AF_HYPERV in the Hyper-V host side is
suboptimal IMHO (the current host side design requires the least
change in the host side, but it makes my life difficult. :-(  It may
change in the future, but luckily we have to live with it at present):

1) A new connection is treated as a new Hyper-V device, so it has to
go through the slow device_register(). Please see
vmbus_device_register().

2) A connection/device must have its own ringbuffer that is shared
between host/guest. Allocating the ringbuffer memory in the VM 
and sharing the memory with the host by messages are both slow,
though I didn't measure the exact cost. Please see
hvsock_open_connection() -> vmbus_open().

3) The max length of the linear linked list is 2048, and in practice,
typically I guess the length should be small, so my gut feeling is that
the list traversing shouldn't be the bottleneck.
Having said that, I agree it's good to use some mechanism, like 
hash table, to speed up the lookup. I'll add this.

> But
> maybe it's fine as the typical use case is going to be small number of
> long running connections and traffic performance is going to make for
> the connection latency. 
Yeah, IMO it seems traffic performance and zero-configuration came
first when the current host side design was made.

> Or there are other advantages, I don't know.
> But if that is the case, it would IMHO deserve to be explained.
> 
>                                 Michal Kubecek

Thanks,
-- Dexuan

[toc] | [prev] | [next] | [standalone]

#1451166

> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Dexuan Cui
> Sent: Tuesday, July 26, 2016 21:22
> ...
> This is because, the design of AF_HYPERV in the Hyper-V host side is
> suboptimal IMHO (the current host side design requires the least
> change in the host side, but it makes my life difficult. :-(  It may
> change in the future, but luckily we have to live with it at present):
BTW, sorry for my typo: "luckily" should be "unluckily".

Thanks,
-- Dexuan

[toc] | [prev] | [next] | [standalone]

#1450720

From: Dexuan Cui <decui@microsoft.com>
Date: Tue, 26 Jul 2016 07:09:41 +0000

> I googled "S390 hypervisor socket" but didn't find anything related (I think).

That would be net/iucv/

There's also VMWare's stuff under net/vmw_vsock

It's just absolutely rediculous to make a new hypervisor socket
interface over and over again, so much code duplication and
replication.

[toc] | [prev] | [next] | [standalone]

#1451147

> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, July 27, 2016 1:45
> To: Dexuan Cui <decui@microsoft.com>
> 
> From: Dexuan Cui <decui@microsoft.com>
> Date: Tue, 26 Jul 2016 07:09:41 +0000
> 
> > I googled "S390 hypervisor socket" but didn't find anything related (I think).
> 
> That would be net/iucv/
Thanks for the info! I'll look into this.
 
> There's also VMWare's stuff under net/vmw_vsock
> 
> It's just absolutely rediculous to make a new hypervisor socket
> interface over and over again, so much code duplication and
> replication.
I agree on this principle of avoiding duplication.
However my feeling is: IMHO different hypervisor sockets were developed
independently without coordination and the implementation details could be
so different that an enough generic framework/infrastructure is difficult,
e.g., at first glance, it looks AF_IUCV is quite different from AF_VSOCK and
this might explain why AF_VSOCK wasn't built on AF_IUCV(?).

I'll dig more into AF_IUCV, AF_VSOCK and AF_HYPERV and figure out what
is the best direction I should go.

Thanks,
-- Dexuan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web