RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: David Miller [mailto:da...@davemloft.net] > Sent: Wednesday, July 27, 2016 1:45 > To: Dexuan Cui > > From: Dexuan Cui > Date: Tue, 26 Jul 2016 07:09:41 + > > > 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
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: netdev-ow...@vger.kernel.org [mailto:netdev- > ow...@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
Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
From: Dexuan Cui Date: Tue, 26 Jul 2016 07:09:41 + > 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.
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: Michal Kubecek [mailto:mkube...@suse.cz] > Sent: Tuesday, July 26, 2016 17:57 > ... > On Tue, Jul 26, 2016 at 07:09:41AM +, 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
Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
On Tue, Jul 26, 2016 at 07:09:41AM +, 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
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On > Behalf Of Dexuan Cui > ... > > From: David Miller [mailto:da...@davemloft.net] > > ... > > From: Dexuan Cui > > Date: Tue, 26 Jul 2016 03:09:16 + > > > > > 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): in AF_VOSCK vs. 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 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
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: David Miller [mailto:da...@davemloft.net] > ... > From: Dexuan Cui > Date: Tue, 26 Jul 2016 03:09:16 + > > > 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
Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
From: Dexuan Cui Date: Tue, 26 Jul 2016 03:09:16 + > 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.
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: David Miller [mailto:da...@davemloft.net]
>
> From: Dexuan Cui
> Date: Sat, 23 Jul 2016 01:35:51 +
>
> > +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
Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
From: Dexuan Cui
Date: Sat, 23 Jul 2016 01:35:51 +
> +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.
