Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Tue, Jul 16, 2019 at 06:01:33AM -0400, Michael S. Tsirkin wrote: > On Tue, Jul 16, 2019 at 11:40:24AM +0200, Stefano Garzarella wrote: > > On Mon, Jul 15, 2019 at 01:50:28PM -0400, Michael S. Tsirkin wrote: > > > On Mon, Jul 15, 2019 at 09:44:16AM +0200, Stefano Garzarella wrote: > > > > On Fri, Jul 12, 2019 at 06:14:39PM +0800, Jason Wang wrote: > > > > [...] > > > > > > > > > > > > > > > > > I think it's just a branch, for ethernet, go for networking stack. > > > > > otherwise > > > > > go for vsock core? > > > > > > > > > > > > > Yes, that should work. > > > > > > > > So, I should refactor the functions that can be called also from the > > > > vsock > > > > core, in order to remove "struct net_device *dev" parameter. > > > > Maybe creating some wrappers for the network stack. > > > > > > > > Otherwise I should create a fake net_device for vsock_core. > > > > > > > > What do you suggest? > > > > > > Neither. > > > > > > I think what Jason was saying all along is this: > > > > > > virtio net doesn't actually lose packets, at least most > > > of the time. And it actually most of the time > > > passes all packets to host. So it's possible to use a virtio net > > > device (possibly with a feature flag that says "does not lose packets, > > > all packets go to host") and build vsock on top. > > > > Yes, I got it after the latest Jason's reply. > > > > > > > > and all of this is nice, but don't expect anything easy, > > > or any quick results. > > > > I expected this... :-( > > > > > > > > Also, in a sense it's a missed opportunity: we could cut out a lot > > > of fat and see just how fast can a protocol that is completely > > > new and separate from networking stack go. > > > > In this case, if we will try to do a PoC, what do you think is better? > > 1. new AF_VSOCK + network-stack + virtio-net modified > > Maybe it is allow us to reuse a lot of stuff already written, > > but we will go through the network stack > > > > 2. new AF_VSOCK + glue + virtio-net modified > > Intermediate approach, similar to Jason's proposal > > > > 3, new AF_VSOCK + new virtio-vsock > > Can be the thinnest, but we have to rewrite many things, with the > > risk > > of making the same mistakes as the current implementation. > > > > 1 or 3 imho. I wouldn't expect a lot from 2. I slightly favor 3 and > Jason 1. So take your pick :) > Yes, I agree :) Maybe "Jason 1" could be the short term (and an opportunity to study better the code and sources of overhead) and "new AF_VSOCK + new virtio-vsock" the long term goal with the multi-transport support in mind. Thank you so much for your guidance and useful advice, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Tue, Jul 16, 2019 at 11:40:24AM +0200, Stefano Garzarella wrote: > On Mon, Jul 15, 2019 at 01:50:28PM -0400, Michael S. Tsirkin wrote: > > On Mon, Jul 15, 2019 at 09:44:16AM +0200, Stefano Garzarella wrote: > > > On Fri, Jul 12, 2019 at 06:14:39PM +0800, Jason Wang wrote: > > [...] > > > > > > > > > > > > > I think it's just a branch, for ethernet, go for networking stack. > > > > otherwise > > > > go for vsock core? > > > > > > > > > > Yes, that should work. > > > > > > So, I should refactor the functions that can be called also from the vsock > > > core, in order to remove "struct net_device *dev" parameter. > > > Maybe creating some wrappers for the network stack. > > > > > > Otherwise I should create a fake net_device for vsock_core. > > > > > > What do you suggest? > > > > Neither. > > > > I think what Jason was saying all along is this: > > > > virtio net doesn't actually lose packets, at least most > > of the time. And it actually most of the time > > passes all packets to host. So it's possible to use a virtio net > > device (possibly with a feature flag that says "does not lose packets, > > all packets go to host") and build vsock on top. > > Yes, I got it after the latest Jason's reply. > > > > > and all of this is nice, but don't expect anything easy, > > or any quick results. > > I expected this... :-( > > > > > Also, in a sense it's a missed opportunity: we could cut out a lot > > of fat and see just how fast can a protocol that is completely > > new and separate from networking stack go. > > In this case, if we will try to do a PoC, what do you think is better? > 1. new AF_VSOCK + network-stack + virtio-net modified > Maybe it is allow us to reuse a lot of stuff already written, > but we will go through the network stack > > 2. new AF_VSOCK + glue + virtio-net modified > Intermediate approach, similar to Jason's proposal > > 3, new AF_VSOCK + new virtio-vsock > Can be the thinnest, but we have to rewrite many things, with the risk > of making the same mistakes as the current implementation. > 1 or 3 imho. I wouldn't expect a lot from 2. I slightly favor 3 and Jason 1. So take your pick :) > > Instead vsock implementation carries so much baggage from both > > networking stack - such as softirq processing - and itself such as > > workqueues, global state and crude locking - to the point where > > it's actually slower than TCP. > > I agree, and I'm finding new issues while I'm trying to support nested > VMs, allowing multiple vsock transports (virtio-vsock and vhost-vsock in > the KVM case) at runtime. > > > > > [...] > > > > > > > > > I suggest to do this step by step: > > > > > > > > 1) use virtio-net but keep some protocol logic > > > > > > > > 2) separate protocol logic and merge it to exist Linux networking stack > > > > > > Make sense, thanks for the suggestions, I'll try to do these steps! > > > > > > Thanks, > > > Stefano > > > > > > An alternative is look at sources of overhead in vsock and get rid of > > them, or rewrite it from scratch focusing on performance. > > I started looking at virtio-vsock and vhost-vsock trying to do very > simple changes [1] to increase the performance. I should send a v4 of that > series as a very short term, then I'd like to have a deeper look to understand > if it is better to try to optimize or rewrite it from scratch. > > > Thanks, > Stefano > > [1] https://patchwork.kernel.org/cover/10970145/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Mon, Jul 15, 2019 at 01:50:28PM -0400, Michael S. Tsirkin wrote: > On Mon, Jul 15, 2019 at 09:44:16AM +0200, Stefano Garzarella wrote: > > On Fri, Jul 12, 2019 at 06:14:39PM +0800, Jason Wang wrote: [...] > > > > > > > > > I think it's just a branch, for ethernet, go for networking stack. > > > otherwise > > > go for vsock core? > > > > > > > Yes, that should work. > > > > So, I should refactor the functions that can be called also from the vsock > > core, in order to remove "struct net_device *dev" parameter. > > Maybe creating some wrappers for the network stack. > > > > Otherwise I should create a fake net_device for vsock_core. > > > > What do you suggest? > > Neither. > > I think what Jason was saying all along is this: > > virtio net doesn't actually lose packets, at least most > of the time. And it actually most of the time > passes all packets to host. So it's possible to use a virtio net > device (possibly with a feature flag that says "does not lose packets, > all packets go to host") and build vsock on top. Yes, I got it after the latest Jason's reply. > > and all of this is nice, but don't expect anything easy, > or any quick results. I expected this... :-( > > Also, in a sense it's a missed opportunity: we could cut out a lot > of fat and see just how fast can a protocol that is completely > new and separate from networking stack go. In this case, if we will try to do a PoC, what do you think is better? 1. new AF_VSOCK + network-stack + virtio-net modified Maybe it is allow us to reuse a lot of stuff already written, but we will go through the network stack 2. new AF_VSOCK + glue + virtio-net modified Intermediate approach, similar to Jason's proposal 3, new AF_VSOCK + new virtio-vsock Can be the thinnest, but we have to rewrite many things, with the risk of making the same mistakes as the current implementation. > Instead vsock implementation carries so much baggage from both > networking stack - such as softirq processing - and itself such as > workqueues, global state and crude locking - to the point where > it's actually slower than TCP. I agree, and I'm finding new issues while I'm trying to support nested VMs, allowing multiple vsock transports (virtio-vsock and vhost-vsock in the KVM case) at runtime. > [...] > > > > > > I suggest to do this step by step: > > > > > > 1) use virtio-net but keep some protocol logic > > > > > > 2) separate protocol logic and merge it to exist Linux networking stack > > > > Make sense, thanks for the suggestions, I'll try to do these steps! > > > > Thanks, > > Stefano > > > An alternative is look at sources of overhead in vsock and get rid of > them, or rewrite it from scratch focusing on performance. I started looking at virtio-vsock and vhost-vsock trying to do very simple changes [1] to increase the performance. I should send a v4 of that series as a very short term, then I'd like to have a deeper look to understand if it is better to try to optimize or rewrite it from scratch. Thanks, Stefano [1] https://patchwork.kernel.org/cover/10970145/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Mon, Jul 15, 2019 at 09:44:16AM +0200, Stefano Garzarella wrote: > On Fri, Jul 12, 2019 at 06:14:39PM +0800, Jason Wang wrote: > > > > On 2019/7/12 下午6:00, Stefano Garzarella wrote: > > > On Thu, Jul 11, 2019 at 03:52:21PM -0400, Michael S. Tsirkin wrote: > > > > On Thu, Jul 11, 2019 at 01:41:34PM +0200, Stefano Garzarella wrote: > > > > > On Thu, Jul 11, 2019 at 03:37:00PM +0800, Jason Wang wrote: > > > > > > On 2019/7/10 下午11:37, Stefano Garzarella wrote: > > > > > > > Hi, > > > > > > > as Jason suggested some months ago, I looked better at the > > > > > > > virtio-net driver to > > > > > > > understand if we can reuse some parts also in the virtio-vsock > > > > > > > driver, since we > > > > > > > have similar challenges (mergeable buffers, page allocation, small > > > > > > > packets, etc.). > > > > > > > > > > > > > > Initially, I would add the skbuff in the virtio-vsock in order to > > > > > > > re-use > > > > > > > receive_*() functions. > > > > > > > > > > > > Yes, that will be a good step. > > > > > > > > > > > Okay, I'll go on this way. > > > > > > > > > > > > Then I would move receive_[small, big, mergeable]() and > > > > > > > add_recvbuf_[small, big, mergeable]() outside of virtio-net > > > > > > > driver, in order to > > > > > > > call them also from virtio-vsock. I need to do some refactoring > > > > > > > (e.g. leave the > > > > > > > XDP part on the virtio-net driver), but I think it is feasible. > > > > > > > > > > > > > > The idea is to create a virtio-skb.[h,c] where put these > > > > > > > functions and a new > > > > > > > object where stores some attributes needed (e.g. hdr_len ) and > > > > > > > status (e.g. > > > > > > > some fields of struct receive_queue). > > > > > > > > > > > > My understanding is we could be more ambitious here. Do you see any > > > > > > blocker > > > > > > for reusing virtio-net directly? It's better to reuse not only the > > > > > > functions > > > > > > but also the logic like NAPI to avoid re-inventing something buggy > > > > > > and > > > > > > duplicated. > > > > > > > > > > > These are my concerns: > > > > > - virtio-vsock is not a "net_device", so a lot of code related to > > > > >ethtool, net devices (MAC address, MTU, speed, VLAN, XDP, > > > > > offloading) will be > > > > >not used by virtio-vsock. > > > > > > Linux support device other than ethernet, so it should not be a problem. > > > > > > > > > > > > > > - virtio-vsock has a different header. We can consider it as part of > > > > >virtio_net payload, but it precludes the compatibility with old > > > > > hosts. This > > > > >was one of the major doubts that made me think about using only the > > > > >send/recv skbuff functions, that it shouldn't break the > > > > > compatibility. > > > > > > We can extend the current vnet header helper for it to work for vsock. > > Okay, I'll do it. > > > > > > > > > > > > > > > > > This is an idea of virtio-skb.h that > > > > > > > I have in mind: > > > > > > > struct virtskb; > > > > > > > > > > > > What fields do you want to store in virtskb? It looks to be exist > > > > > > sk_buff is > > > > > > flexible enough to us? > > > > > My idea is to store queues information, like struct receive_queue or > > > > > struct send_queue, and some device attributes (e.g. hdr_len ). > > > > > > If you reuse skb or virtnet_info, there is not necessary. > > > > Okay. > > > > > > > > > > > > > > > > > > > > > struct sk_buff *virtskb_receive_small(struct virtskb *vs, > > > > > > > ...); > > > > > > > struct sk_buff *virtskb_receive_big(struct virtskb *vs, > > > > > > > ...); > > > > > > > struct sk_buff *virtskb_receive_mergeable(struct virtskb > > > > > > > *vs, ...); > > > > > > > > > > > > > > int virtskb_add_recvbuf_small(struct virtskb*vs, ...); > > > > > > > int virtskb_add_recvbuf_big(struct virtskb *vs, ...); > > > > > > > int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); > > > > > > > > > > > > > > For the Guest->Host path it should be easier, so maybe I can add a > > > > > > > "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a > > > > > > > part of the code > > > > > > > of xmit_skb(). > > > > > > > > > > > > I may miss something, but I don't see any thing that prevents us > > > > > > from using > > > > > > xmit_skb() directly. > > > > > > > > > > > Yes, but my initial idea was to make it more parametric and not > > > > > related to the > > > > > virtio_net_hdr, so the 'hdr_len' could be a parameter and the > > > > > 'num_buffers' should be handled by the caller. > > > > > > > > > > > > Let me know if you have in mind better names or if I should put > > > > > > > these function > > > > > > > in another place. > > > > > > > > > > > > > > I would like to leave the control part completely separate, so, > > > > > > > for example, > > > > > > > the two drivers will negotiate the features independently and > > > > > > > they
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Mon, Jul 15, 2019 at 05:16:09PM +0800, Jason Wang wrote: > > > > > > > > >struct sk_buff *virtskb_receive_small(struct virtskb > > > > > > > > *vs, ...); > > > > > > > >struct sk_buff *virtskb_receive_big(struct virtskb *vs, > > > > > > > > ...); > > > > > > > >struct sk_buff *virtskb_receive_mergeable(struct virtskb > > > > > > > > *vs, ...); > > > > > > > > > > > > > > > >int virtskb_add_recvbuf_small(struct virtskb*vs, ...); > > > > > > > >int virtskb_add_recvbuf_big(struct virtskb *vs, ...); > > > > > > > >int virtskb_add_recvbuf_mergeable(struct virtskb *vs, > > > > > > > > ...); > > > > > > > > > > > > > > > > For the Guest->Host path it should be easier, so maybe I can > > > > > > > > add a > > > > > > > > "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a > > > > > > > > part of the code > > > > > > > > of xmit_skb(). > > > > > > > I may miss something, but I don't see any thing that prevents us > > > > > > > from using > > > > > > > xmit_skb() directly. > > > > > > > > > > > > > Yes, but my initial idea was to make it more parametric and not > > > > > > related to the > > > > > > virtio_net_hdr, so the 'hdr_len' could be a parameter and the > > > > > > 'num_buffers' should be handled by the caller. > > > > > > > > > > > > > > Let me know if you have in mind better names or if I should put > > > > > > > > these function > > > > > > > > in another place. > > > > > > > > > > > > > > > > I would like to leave the control part completely separate, so, > > > > > > > > for example, > > > > > > > > the two drivers will negotiate the features independently and > > > > > > > > they will call > > > > > > > > the right virtskb_receive_*() function based on the negotiation. > > > > > > > If it's one the issue of negotiation, we can simply change the > > > > > > > virtnet_probe() to deal with different devices. > > > > > > > > > > > > > > > > > > > > > > I already started to work on it, but before to do more steps > > > > > > > > and send an RFC > > > > > > > > patch, I would like to hear your opinion. > > > > > > > > Do you think that makes sense? > > > > > > > > Do you see any issue or a better solution? > > > > > > > I still think we need to seek a way of adding some codes on > > > > > > > virtio-net.c > > > > > > > directly if there's no huge different in the processing of TX/RX. > > > > > > > That would > > > > > > > save us a lot time. > > > > > > After the reading of the buffers from the virtqueue I think the > > > > > > process > > > > > > is slightly different, because virtio-net will interface with the > > > > > > network > > > > > > stack, while virtio-vsock will interface with the vsock-core > > > > > > (socket). > > > > > > So the virtio-vsock implements the following: > > > > > > - control flow mechanism to avoid to loose packets, informing the > > > > > > peer > > > > > > about the amount of memory available in the receive queue using > > > > > > some > > > > > > fields in the virtio_vsock_hdr > > > > > > - de-multiplexing parsing the virtio_vsock_hdr and choosing the > > > > > > right > > > > > > socket depending on the port > > > > > > - socket state handling > > > > > > I think it's just a branch, for ethernet, go for networking stack. > > > otherwise > > > go for vsock core? > > > > > Yes, that should work. > > > > So, I should refactor the functions that can be called also from the vsock > > core, in order to remove "struct net_device *dev" parameter. > > Maybe creating some wrappers for the network stack. > > > > Otherwise I should create a fake net_device for vsock_core. > > > > What do you suggest? > > > I'm not quite sure I get the question. Can you just use the one that created > by virtio_net? Sure, sorry but I missed that it is allocated in the virtnet_probe()! Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); int virtskb_add_recvbuf_small(struct virtskb*vs, ...); int virtskb_add_recvbuf_big(struct virtskb *vs, ...); int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); For the Guest->Host path it should be easier, so maybe I can add a "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of the code of xmit_skb(). I may miss something, but I don't see any thing that prevents us from using xmit_skb() directly. Yes, but my initial idea was to make it more parametric and not related to the virtio_net_hdr, so the 'hdr_len' could be a parameter and the 'num_buffers' should be handled by the caller. Let me know if you have in mind better names or if I should put these function in another place. I would like to leave the control part completely separate, so, for example, the two drivers will negotiate the features independently and they will call the right virtskb_receive_*() function based on the negotiation. If it's one the issue of negotiation, we can simply change the virtnet_probe() to deal with different devices. I already started to work on it, but before to do more steps and send an RFC patch, I would like to hear your opinion. Do you think that makes sense? Do you see any issue or a better solution? I still think we need to seek a way of adding some codes on virtio-net.c directly if there's no huge different in the processing of TX/RX. That would save us a lot time. After the reading of the buffers from the virtqueue I think the process is slightly different, because virtio-net will interface with the network stack, while virtio-vsock will interface with the vsock-core (socket). So the virtio-vsock implements the following: - control flow mechanism to avoid to loose packets, informing the peer about the amount of memory available in the receive queue using some fields in the virtio_vsock_hdr - de-multiplexing parsing the virtio_vsock_hdr and choosing the right socket depending on the port - socket state handling I think it's just a branch, for ethernet, go for networking stack. otherwise go for vsock core? Yes, that should work. So, I should refactor the functions that can be called also from the vsock core, in order to remove "struct net_device *dev" parameter. Maybe creating some wrappers for the network stack. Otherwise I should create a fake net_device for vsock_core. What do you suggest? I'm not quite sure I get the question. Can you just use the one that created by virtio_net? Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Fri, Jul 12, 2019 at 06:14:39PM +0800, Jason Wang wrote: > > On 2019/7/12 下午6:00, Stefano Garzarella wrote: > > On Thu, Jul 11, 2019 at 03:52:21PM -0400, Michael S. Tsirkin wrote: > > > On Thu, Jul 11, 2019 at 01:41:34PM +0200, Stefano Garzarella wrote: > > > > On Thu, Jul 11, 2019 at 03:37:00PM +0800, Jason Wang wrote: > > > > > On 2019/7/10 下午11:37, Stefano Garzarella wrote: > > > > > > Hi, > > > > > > as Jason suggested some months ago, I looked better at the > > > > > > virtio-net driver to > > > > > > understand if we can reuse some parts also in the virtio-vsock > > > > > > driver, since we > > > > > > have similar challenges (mergeable buffers, page allocation, small > > > > > > packets, etc.). > > > > > > > > > > > > Initially, I would add the skbuff in the virtio-vsock in order to > > > > > > re-use > > > > > > receive_*() functions. > > > > > > > > > > Yes, that will be a good step. > > > > > > > > > Okay, I'll go on this way. > > > > > > > > > > Then I would move receive_[small, big, mergeable]() and > > > > > > add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, > > > > > > in order to > > > > > > call them also from virtio-vsock. I need to do some refactoring > > > > > > (e.g. leave the > > > > > > XDP part on the virtio-net driver), but I think it is feasible. > > > > > > > > > > > > The idea is to create a virtio-skb.[h,c] where put these functions > > > > > > and a new > > > > > > object where stores some attributes needed (e.g. hdr_len ) and > > > > > > status (e.g. > > > > > > some fields of struct receive_queue). > > > > > > > > > > My understanding is we could be more ambitious here. Do you see any > > > > > blocker > > > > > for reusing virtio-net directly? It's better to reuse not only the > > > > > functions > > > > > but also the logic like NAPI to avoid re-inventing something buggy and > > > > > duplicated. > > > > > > > > > These are my concerns: > > > > - virtio-vsock is not a "net_device", so a lot of code related to > > > >ethtool, net devices (MAC address, MTU, speed, VLAN, XDP, > > > > offloading) will be > > > >not used by virtio-vsock. > > > Linux support device other than ethernet, so it should not be a problem. > > > > > > > > > > - virtio-vsock has a different header. We can consider it as part of > > > >virtio_net payload, but it precludes the compatibility with old > > > > hosts. This > > > >was one of the major doubts that made me think about using only the > > > >send/recv skbuff functions, that it shouldn't break the > > > > compatibility. > > > We can extend the current vnet header helper for it to work for vsock. Okay, I'll do it. > > > > > > > > > > > > This is an idea of virtio-skb.h that > > > > > > I have in mind: > > > > > > struct virtskb; > > > > > > > > > > What fields do you want to store in virtskb? It looks to be exist > > > > > sk_buff is > > > > > flexible enough to us? > > > > My idea is to store queues information, like struct receive_queue or > > > > struct send_queue, and some device attributes (e.g. hdr_len ). > > > If you reuse skb or virtnet_info, there is not necessary. > Okay. > > > > > > > > > > > > > > > > struct sk_buff *virtskb_receive_small(struct virtskb *vs, > > > > > > ...); > > > > > > struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); > > > > > > struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, > > > > > > ...); > > > > > > > > > > > > int virtskb_add_recvbuf_small(struct virtskb*vs, ...); > > > > > > int virtskb_add_recvbuf_big(struct virtskb *vs, ...); > > > > > > int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); > > > > > > > > > > > > For the Guest->Host path it should be easier, so maybe I can add a > > > > > > "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part > > > > > > of the code > > > > > > of xmit_skb(). > > > > > > > > > > I may miss something, but I don't see any thing that prevents us from > > > > > using > > > > > xmit_skb() directly. > > > > > > > > > Yes, but my initial idea was to make it more parametric and not related > > > > to the > > > > virtio_net_hdr, so the 'hdr_len' could be a parameter and the > > > > 'num_buffers' should be handled by the caller. > > > > > > > > > > Let me know if you have in mind better names or if I should put > > > > > > these function > > > > > > in another place. > > > > > > > > > > > > I would like to leave the control part completely separate, so, for > > > > > > example, > > > > > > the two drivers will negotiate the features independently and they > > > > > > will call > > > > > > the right virtskb_receive_*() function based on the negotiation. > > > > > > > > > > If it's one the issue of negotiation, we can simply change the > > > > > virtnet_probe() to deal with different devices. > > > > > > > > > > > > > > > > I already started to work on it, but before to do more steps and > > > > >
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On 2019/7/12 下午6:00, Stefano Garzarella wrote: On Thu, Jul 11, 2019 at 03:52:21PM -0400, Michael S. Tsirkin wrote: On Thu, Jul 11, 2019 at 01:41:34PM +0200, Stefano Garzarella wrote: On Thu, Jul 11, 2019 at 03:37:00PM +0800, Jason Wang wrote: On 2019/7/10 下午11:37, Stefano Garzarella wrote: Hi, as Jason suggested some months ago, I looked better at the virtio-net driver to understand if we can reuse some parts also in the virtio-vsock driver, since we have similar challenges (mergeable buffers, page allocation, small packets, etc.). Initially, I would add the skbuff in the virtio-vsock in order to re-use receive_*() functions. Yes, that will be a good step. Okay, I'll go on this way. Then I would move receive_[small, big, mergeable]() and add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, in order to call them also from virtio-vsock. I need to do some refactoring (e.g. leave the XDP part on the virtio-net driver), but I think it is feasible. The idea is to create a virtio-skb.[h,c] where put these functions and a new object where stores some attributes needed (e.g. hdr_len ) and status (e.g. some fields of struct receive_queue). My understanding is we could be more ambitious here. Do you see any blocker for reusing virtio-net directly? It's better to reuse not only the functions but also the logic like NAPI to avoid re-inventing something buggy and duplicated. These are my concerns: - virtio-vsock is not a "net_device", so a lot of code related to ethtool, net devices (MAC address, MTU, speed, VLAN, XDP, offloading) will be not used by virtio-vsock. Linux support device other than ethernet, so it should not be a problem. - virtio-vsock has a different header. We can consider it as part of virtio_net payload, but it precludes the compatibility with old hosts. This was one of the major doubts that made me think about using only the send/recv skbuff functions, that it shouldn't break the compatibility. We can extend the current vnet header helper for it to work for vsock. This is an idea of virtio-skb.h that I have in mind: struct virtskb; What fields do you want to store in virtskb? It looks to be exist sk_buff is flexible enough to us? My idea is to store queues information, like struct receive_queue or struct send_queue, and some device attributes (e.g. hdr_len ). If you reuse skb or virtnet_info, there is not necessary. struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); int virtskb_add_recvbuf_small(struct virtskb*vs, ...); int virtskb_add_recvbuf_big(struct virtskb *vs, ...); int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); For the Guest->Host path it should be easier, so maybe I can add a "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of the code of xmit_skb(). I may miss something, but I don't see any thing that prevents us from using xmit_skb() directly. Yes, but my initial idea was to make it more parametric and not related to the virtio_net_hdr, so the 'hdr_len' could be a parameter and the 'num_buffers' should be handled by the caller. Let me know if you have in mind better names or if I should put these function in another place. I would like to leave the control part completely separate, so, for example, the two drivers will negotiate the features independently and they will call the right virtskb_receive_*() function based on the negotiation. If it's one the issue of negotiation, we can simply change the virtnet_probe() to deal with different devices. I already started to work on it, but before to do more steps and send an RFC patch, I would like to hear your opinion. Do you think that makes sense? Do you see any issue or a better solution? I still think we need to seek a way of adding some codes on virtio-net.c directly if there's no huge different in the processing of TX/RX. That would save us a lot time. After the reading of the buffers from the virtqueue I think the process is slightly different, because virtio-net will interface with the network stack, while virtio-vsock will interface with the vsock-core (socket). So the virtio-vsock implements the following: - control flow mechanism to avoid to loose packets, informing the peer about the amount of memory available in the receive queue using some fields in the virtio_vsock_hdr - de-multiplexing parsing the virtio_vsock_hdr and choosing the right socket depending on the port - socket state handling I think it's just a branch, for ethernet, go for networking stack. otherwise go for vsock core? We can use the virtio-net as transport, but we should add a lot of code to skip "net device" stuff when it is used by the virtio-vsock. This could be another choice, but consider it was not transparent to the admin and
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Thu, Jul 11, 2019 at 03:52:21PM -0400, Michael S. Tsirkin wrote: > On Thu, Jul 11, 2019 at 01:41:34PM +0200, Stefano Garzarella wrote: > > On Thu, Jul 11, 2019 at 03:37:00PM +0800, Jason Wang wrote: > > > > > > On 2019/7/10 下午11:37, Stefano Garzarella wrote: > > > > Hi, > > > > as Jason suggested some months ago, I looked better at the virtio-net > > > > driver to > > > > understand if we can reuse some parts also in the virtio-vsock driver, > > > > since we > > > > have similar challenges (mergeable buffers, page allocation, small > > > > packets, etc.). > > > > > > > > Initially, I would add the skbuff in the virtio-vsock in order to re-use > > > > receive_*() functions. > > > > > > > > > Yes, that will be a good step. > > > > > > > Okay, I'll go on this way. > > > > > > > > > Then I would move receive_[small, big, mergeable]() and > > > > add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, in > > > > order to > > > > call them also from virtio-vsock. I need to do some refactoring (e.g. > > > > leave the > > > > XDP part on the virtio-net driver), but I think it is feasible. > > > > > > > > The idea is to create a virtio-skb.[h,c] where put these functions and > > > > a new > > > > object where stores some attributes needed (e.g. hdr_len ) and status > > > > (e.g. > > > > some fields of struct receive_queue). > > > > > > > > > My understanding is we could be more ambitious here. Do you see any > > > blocker > > > for reusing virtio-net directly? It's better to reuse not only the > > > functions > > > but also the logic like NAPI to avoid re-inventing something buggy and > > > duplicated. > > > > > > > These are my concerns: > > - virtio-vsock is not a "net_device", so a lot of code related to > > ethtool, net devices (MAC address, MTU, speed, VLAN, XDP, offloading) > > will be > > not used by virtio-vsock. > > > > - virtio-vsock has a different header. We can consider it as part of > > virtio_net payload, but it precludes the compatibility with old hosts. > > This > > was one of the major doubts that made me think about using only the > > send/recv skbuff functions, that it shouldn't break the compatibility. > > > > > > > > > This is an idea of virtio-skb.h that > > > > I have in mind: > > > > struct virtskb; > > > > > > > > > What fields do you want to store in virtskb? It looks to be exist sk_buff > > > is > > > flexible enough to us? > > > > My idea is to store queues information, like struct receive_queue or > > struct send_queue, and some device attributes (e.g. hdr_len ). > > > > > > > > > > > > > > > > struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); > > > > struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); > > > > struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); > > > > > > > > int virtskb_add_recvbuf_small(struct virtskb*vs, ...); > > > > int virtskb_add_recvbuf_big(struct virtskb *vs, ...); > > > > int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); > > > > > > > > For the Guest->Host path it should be easier, so maybe I can add a > > > > "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of > > > > the code > > > > of xmit_skb(). > > > > > > > > > I may miss something, but I don't see any thing that prevents us from > > > using > > > xmit_skb() directly. > > > > > > > Yes, but my initial idea was to make it more parametric and not related to > > the > > virtio_net_hdr, so the 'hdr_len' could be a parameter and the > > 'num_buffers' should be handled by the caller. > > > > > > > > > > > > > Let me know if you have in mind better names or if I should put these > > > > function > > > > in another place. > > > > > > > > I would like to leave the control part completely separate, so, for > > > > example, > > > > the two drivers will negotiate the features independently and they will > > > > call > > > > the right virtskb_receive_*() function based on the negotiation. > > > > > > > > > If it's one the issue of negotiation, we can simply change the > > > virtnet_probe() to deal with different devices. > > > > > > > > > > > > > > I already started to work on it, but before to do more steps and send > > > > an RFC > > > > patch, I would like to hear your opinion. > > > > Do you think that makes sense? > > > > Do you see any issue or a better solution? > > > > > > > > > I still think we need to seek a way of adding some codes on virtio-net.c > > > directly if there's no huge different in the processing of TX/RX. That > > > would > > > save us a lot time. > > > > After the reading of the buffers from the virtqueue I think the process > > is slightly different, because virtio-net will interface with the network > > stack, while virtio-vsock will interface with the vsock-core (socket). > > So the virtio-vsock implements the following: > > - control flow mechanism to avoid to loose packets, informing the peer > >
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Thu, Jul 11, 2019 at 01:41:34PM +0200, Stefano Garzarella wrote: > On Thu, Jul 11, 2019 at 03:37:00PM +0800, Jason Wang wrote: > > > > On 2019/7/10 下午11:37, Stefano Garzarella wrote: > > > Hi, > > > as Jason suggested some months ago, I looked better at the virtio-net > > > driver to > > > understand if we can reuse some parts also in the virtio-vsock driver, > > > since we > > > have similar challenges (mergeable buffers, page allocation, small > > > packets, etc.). > > > > > > Initially, I would add the skbuff in the virtio-vsock in order to re-use > > > receive_*() functions. > > > > > > Yes, that will be a good step. > > > > Okay, I'll go on this way. > > > > > > Then I would move receive_[small, big, mergeable]() and > > > add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, in > > > order to > > > call them also from virtio-vsock. I need to do some refactoring (e.g. > > > leave the > > > XDP part on the virtio-net driver), but I think it is feasible. > > > > > > The idea is to create a virtio-skb.[h,c] where put these functions and a > > > new > > > object where stores some attributes needed (e.g. hdr_len ) and status > > > (e.g. > > > some fields of struct receive_queue). > > > > > > My understanding is we could be more ambitious here. Do you see any blocker > > for reusing virtio-net directly? It's better to reuse not only the functions > > but also the logic like NAPI to avoid re-inventing something buggy and > > duplicated. > > > > These are my concerns: > - virtio-vsock is not a "net_device", so a lot of code related to > ethtool, net devices (MAC address, MTU, speed, VLAN, XDP, offloading) will > be > not used by virtio-vsock. > > - virtio-vsock has a different header. We can consider it as part of > virtio_net payload, but it precludes the compatibility with old hosts. This > was one of the major doubts that made me think about using only the > send/recv skbuff functions, that it shouldn't break the compatibility. > > > > > > This is an idea of virtio-skb.h that > > > I have in mind: > > > struct virtskb; > > > > > > What fields do you want to store in virtskb? It looks to be exist sk_buff is > > flexible enough to us? > > My idea is to store queues information, like struct receive_queue or > struct send_queue, and some device attributes (e.g. hdr_len ). > > > > > > > > > > > struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); > > > struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); > > > struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); > > > > > > int virtskb_add_recvbuf_small(struct virtskb*vs, ...); > > > int virtskb_add_recvbuf_big(struct virtskb *vs, ...); > > > int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); > > > > > > For the Guest->Host path it should be easier, so maybe I can add a > > > "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of > > > the code > > > of xmit_skb(). > > > > > > I may miss something, but I don't see any thing that prevents us from using > > xmit_skb() directly. > > > > Yes, but my initial idea was to make it more parametric and not related to the > virtio_net_hdr, so the 'hdr_len' could be a parameter and the > 'num_buffers' should be handled by the caller. > > > > > > > > > Let me know if you have in mind better names or if I should put these > > > function > > > in another place. > > > > > > I would like to leave the control part completely separate, so, for > > > example, > > > the two drivers will negotiate the features independently and they will > > > call > > > the right virtskb_receive_*() function based on the negotiation. > > > > > > If it's one the issue of negotiation, we can simply change the > > virtnet_probe() to deal with different devices. > > > > > > > > > > I already started to work on it, but before to do more steps and send an > > > RFC > > > patch, I would like to hear your opinion. > > > Do you think that makes sense? > > > Do you see any issue or a better solution? > > > > > > I still think we need to seek a way of adding some codes on virtio-net.c > > directly if there's no huge different in the processing of TX/RX. That would > > save us a lot time. > > After the reading of the buffers from the virtqueue I think the process > is slightly different, because virtio-net will interface with the network > stack, while virtio-vsock will interface with the vsock-core (socket). > So the virtio-vsock implements the following: > - control flow mechanism to avoid to loose packets, informing the peer > about the amount of memory available in the receive queue using some > fields in the virtio_vsock_hdr > - de-multiplexing parsing the virtio_vsock_hdr and choosing the right > socket depending on the port > - socket state handling > > We can use the virtio-net as transport, but we should add a lot of > code to skip "net device" stuff when it is used by
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On Thu, Jul 11, 2019 at 03:37:00PM +0800, Jason Wang wrote: > > On 2019/7/10 下午11:37, Stefano Garzarella wrote: > > Hi, > > as Jason suggested some months ago, I looked better at the virtio-net > > driver to > > understand if we can reuse some parts also in the virtio-vsock driver, > > since we > > have similar challenges (mergeable buffers, page allocation, small > > packets, etc.). > > > > Initially, I would add the skbuff in the virtio-vsock in order to re-use > > receive_*() functions. > > > Yes, that will be a good step. > Okay, I'll go on this way. > > > Then I would move receive_[small, big, mergeable]() and > > add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, in > > order to > > call them also from virtio-vsock. I need to do some refactoring (e.g. leave > > the > > XDP part on the virtio-net driver), but I think it is feasible. > > > > The idea is to create a virtio-skb.[h,c] where put these functions and a new > > object where stores some attributes needed (e.g. hdr_len ) and status (e.g. > > some fields of struct receive_queue). > > > My understanding is we could be more ambitious here. Do you see any blocker > for reusing virtio-net directly? It's better to reuse not only the functions > but also the logic like NAPI to avoid re-inventing something buggy and > duplicated. > These are my concerns: - virtio-vsock is not a "net_device", so a lot of code related to ethtool, net devices (MAC address, MTU, speed, VLAN, XDP, offloading) will be not used by virtio-vsock. - virtio-vsock has a different header. We can consider it as part of virtio_net payload, but it precludes the compatibility with old hosts. This was one of the major doubts that made me think about using only the send/recv skbuff functions, that it shouldn't break the compatibility. > > > This is an idea of virtio-skb.h that > > I have in mind: > > struct virtskb; > > > What fields do you want to store in virtskb? It looks to be exist sk_buff is > flexible enough to us? My idea is to store queues information, like struct receive_queue or struct send_queue, and some device attributes (e.g. hdr_len ). > > > > > > struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); > > struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); > > struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); > > > > int virtskb_add_recvbuf_small(struct virtskb*vs, ...); > > int virtskb_add_recvbuf_big(struct virtskb *vs, ...); > > int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); > > > > For the Guest->Host path it should be easier, so maybe I can add a > > "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of the > > code > > of xmit_skb(). > > > I may miss something, but I don't see any thing that prevents us from using > xmit_skb() directly. > Yes, but my initial idea was to make it more parametric and not related to the virtio_net_hdr, so the 'hdr_len' could be a parameter and the 'num_buffers' should be handled by the caller. > > > > > Let me know if you have in mind better names or if I should put these > > function > > in another place. > > > > I would like to leave the control part completely separate, so, for example, > > the two drivers will negotiate the features independently and they will call > > the right virtskb_receive_*() function based on the negotiation. > > > If it's one the issue of negotiation, we can simply change the > virtnet_probe() to deal with different devices. > > > > > > I already started to work on it, but before to do more steps and send an RFC > > patch, I would like to hear your opinion. > > Do you think that makes sense? > > Do you see any issue or a better solution? > > > I still think we need to seek a way of adding some codes on virtio-net.c > directly if there's no huge different in the processing of TX/RX. That would > save us a lot time. After the reading of the buffers from the virtqueue I think the process is slightly different, because virtio-net will interface with the network stack, while virtio-vsock will interface with the vsock-core (socket). So the virtio-vsock implements the following: - control flow mechanism to avoid to loose packets, informing the peer about the amount of memory available in the receive queue using some fields in the virtio_vsock_hdr - de-multiplexing parsing the virtio_vsock_hdr and choosing the right socket depending on the port - socket state handling We can use the virtio-net as transport, but we should add a lot of code to skip "net device" stuff when it is used by the virtio-vsock. This could break something in virtio-net, for this reason, I thought to reuse only the send/recv functions starting from the idea to split the virtio-net driver in two parts: a. one with all stuff related to the network stack b. one with the stuff needed to communicate with the host And use skbuff to communicate between parts. In this
Re: [RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
On 2019/7/10 下午11:37, Stefano Garzarella wrote: Hi, as Jason suggested some months ago, I looked better at the virtio-net driver to understand if we can reuse some parts also in the virtio-vsock driver, since we have similar challenges (mergeable buffers, page allocation, small packets, etc.). Initially, I would add the skbuff in the virtio-vsock in order to re-use receive_*() functions. Yes, that will be a good step. Then I would move receive_[small, big, mergeable]() and add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, in order to call them also from virtio-vsock. I need to do some refactoring (e.g. leave the XDP part on the virtio-net driver), but I think it is feasible. The idea is to create a virtio-skb.[h,c] where put these functions and a new object where stores some attributes needed (e.g. hdr_len ) and status (e.g. some fields of struct receive_queue). My understanding is we could be more ambitious here. Do you see any blocker for reusing virtio-net directly? It's better to reuse not only the functions but also the logic like NAPI to avoid re-inventing something buggy and duplicated. This is an idea of virtio-skb.h that I have in mind: struct virtskb; What fields do you want to store in virtskb? It looks to be exist sk_buff is flexible enough to us? struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); int virtskb_add_recvbuf_small(struct virtskb*vs, ...); int virtskb_add_recvbuf_big(struct virtskb *vs, ...); int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); For the Guest->Host path it should be easier, so maybe I can add a "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of the code of xmit_skb(). I may miss something, but I don't see any thing that prevents us from using xmit_skb() directly. Let me know if you have in mind better names or if I should put these function in another place. I would like to leave the control part completely separate, so, for example, the two drivers will negotiate the features independently and they will call the right virtskb_receive_*() function based on the negotiation. If it's one the issue of negotiation, we can simply change the virtnet_probe() to deal with different devices. I already started to work on it, but before to do more steps and send an RFC patch, I would like to hear your opinion. Do you think that makes sense? Do you see any issue or a better solution? I still think we need to seek a way of adding some codes on virtio-net.c directly if there's no huge different in the processing of TX/RX. That would save us a lot time. Thanks Thanks in advance, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC] virtio-net: share receive_*() and add_recvbuf_*() with virtio-vsock
Hi, as Jason suggested some months ago, I looked better at the virtio-net driver to understand if we can reuse some parts also in the virtio-vsock driver, since we have similar challenges (mergeable buffers, page allocation, small packets, etc.). Initially, I would add the skbuff in the virtio-vsock in order to re-use receive_*() functions. Then I would move receive_[small, big, mergeable]() and add_recvbuf_[small, big, mergeable]() outside of virtio-net driver, in order to call them also from virtio-vsock. I need to do some refactoring (e.g. leave the XDP part on the virtio-net driver), but I think it is feasible. The idea is to create a virtio-skb.[h,c] where put these functions and a new object where stores some attributes needed (e.g. hdr_len ) and status (e.g. some fields of struct receive_queue). This is an idea of virtio-skb.h that I have in mind: struct virtskb; struct sk_buff *virtskb_receive_small(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_big(struct virtskb *vs, ...); struct sk_buff *virtskb_receive_mergeable(struct virtskb *vs, ...); int virtskb_add_recvbuf_small(struct virtskb*vs, ...); int virtskb_add_recvbuf_big(struct virtskb *vs, ...); int virtskb_add_recvbuf_mergeable(struct virtskb *vs, ...); For the Guest->Host path it should be easier, so maybe I can add a "virtskb_send(struct virtskb *vs, struct sk_buff *skb)" with a part of the code of xmit_skb(). Let me know if you have in mind better names or if I should put these function in another place. I would like to leave the control part completely separate, so, for example, the two drivers will negotiate the features independently and they will call the right virtskb_receive_*() function based on the negotiation. I already started to work on it, but before to do more steps and send an RFC patch, I would like to hear your opinion. Do you think that makes sense? Do you see any issue or a better solution? Thanks in advance, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
