RE: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
Hi, Michael That's a great job. We are now working on support VMDq on KVM, and since the VMDq hardware presents L2 sorting based on MAC addresses and VLAN tags, our target is to implement a zero copy solution using VMDq. We stared from the virtio-net architecture. What we want to proposal is to use AIO combined with direct I/O: 1) Modify virtio-net Backend service in Qemu to submit aio requests composed from virtqueue. 2) Modify TUN/TAP device to support aio operations and the user space buffer directly mapping into the host kernel. 3) Let a TUN/TAP device binds to single rx/tx queue from the NIC. 4) Modify the net_dev and skb structure to permit allocated skb to use user space directly mapped payload buffer address rather then kernel allocated. As zero copy is also your goal, we are interested in what's in your mind, and would like to collaborate with you if possible. BTW, we will send our VMDq write-up very soon. Thanks Xiaohui -Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Michael S. Tsirkin Sent: Wednesday, August 19, 2009 11:03 PM To: net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com Subject: [PATCHv4 2/2] vhost_net: a kernel-level virtio server What it is: vhost net is a character device that can be used to reduce the number of system calls involved in virtio networking. Existing virtio net code is used in the guest without modification. There's similarity with vringfd, with some differences and reduced scope - uses eventfd for signalling - structures can be moved around in memory at any time (good for migration) - support memory table and not just an offset (needed for kvm) common virtio related code has been put in a separate file vhost.c and can be made into a separate module if/when more backends appear. I used Rusty's lguest.c as the source for developing this part : this supplied me with witty comments I wouldn't be able to write myself. What it is not: vhost net is not a bus, and not a generic new system call. No assumptions are made on how guest performs hypercalls. Userspace hypervisors are supported as well as kvm. How it works: Basically, we connect virtio frontend (configured by userspace) to a backend. The backend could be a network device, or a tun-like device. In this version I only support raw socket as a backend, which can be bound to e.g. SR IOV, or to macvlan device. Backend is also configured by userspace, including vlan/mac etc. Status: This works for me, and I haven't see any crashes. I have not run any benchmarks yet, compared to userspace, I expect to see improved latency (as I save up to 4 system calls per packet) but not bandwidth/CPU (as TSO and interrupt mitigation are not supported). Features that I plan to look at in the future: - TSO - interrupt mitigation - zero copy Acked-by: Arnd Bergmann Signed-off-by: Michael S. Tsirkin --- MAINTAINERS| 10 + arch/x86/kvm/Kconfig |1 + drivers/Makefile |1 + drivers/vhost/Kconfig | 11 + drivers/vhost/Makefile |2 + drivers/vhost/net.c| 429 drivers/vhost/vhost.c | 664 drivers/vhost/vhost.h | 108 +++ include/linux/Kbuild |1 + include/linux/miscdevice.h |1 + include/linux/vhost.h | 100 +++ 11 files changed, 1328 insertions(+), 0 deletions(-) create mode 100644 drivers/vhost/Kconfig create mode 100644 drivers/vhost/Makefile create mode 100644 drivers/vhost/net.c create mode 100644 drivers/vhost/vhost.c create mode 100644 drivers/vhost/vhost.h create mode 100644 include/linux/vhost.h diff --git a/MAINTAINERS b/MAINTAINERS index b1114cf..de4587f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5431,6 +5431,16 @@ S: Maintained F: Documentation/filesystems/vfat.txt F: fs/fat/ +VIRTIO HOST (VHOST) +P: Michael S. Tsirkin +M: m...@redhat.com +L: kvm@vger.kernel.org +L: virtualizat...@lists.osdl.org +L: net...@vger.kernel.org +S: Maintained +F: drivers/vhost/ +F: include/linux/vhost.h + VIA RHINE NETWORK DRIVER M: Roger Luethi S: Maintained diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index b84e571..94f44d9 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -64,6 +64,7 @@ config KVM_AMD # OK, it's a little counter-intuitive to do this, but it puts it neatly under # the virtualization menu. +source drivers/vhost/Kconfig source drivers/lguest/Kconfig source drivers/virtio/Kconfig diff --git a/drivers/Makefile b/drivers/Makefile index bc4205d..1551ae1 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -105,6 +105,7 @@ obj-$(CONFIG_HID) += hid/ obj-$(CONFIG_PPC_PS3) += ps3/ obj-$(CONFIG_O
RE: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
> One way to share the effort is to make vmdq queues available as normal kernel interfaces. It would take quite a bit of work, but the end result is that no other components need to be change, and it makes vmdq useful outside kvm. It also greatly reduces the amount of integration work needed throughout the stack (kvm/qemu/libvirt). Yes. The common queue pair interface which we want to present will also apply to normal hardware, and try to leave other components unknown. Thanks Xiaohui -Original Message- From: Avi Kivity [mailto:a...@redhat.com] Sent: Tuesday, September 01, 2009 1:52 AM To: Xin, Xiaohui Cc: m...@redhat.com; net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com Subject: Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server On 08/31/2009 02:42 PM, Xin, Xiaohui wrote: > Hi, Michael > That's a great job. We are now working on support VMDq on KVM, and since the > VMDq hardware presents L2 sorting based on MAC addresses and VLAN tags, our > target is to implement a zero copy solution using VMDq. We stared from the > virtio-net architecture. What we want to proposal is to use AIO combined with > direct I/O: > 1) Modify virtio-net Backend service in Qemu to submit aio requests composed > from virtqueue. > 2) Modify TUN/TAP device to support aio operations and the user space buffer > directly mapping into the host kernel. > 3) Let a TUN/TAP device binds to single rx/tx queue from the NIC. > 4) Modify the net_dev and skb structure to permit allocated skb to use user > space directly mapped payload buffer address rather then kernel allocated. > > As zero copy is also your goal, we are interested in what's in your mind, and > would like to collaborate with you if possible. > One way to share the effort is to make vmdq queues available as normal kernel interfaces. It would take quite a bit of work, but the end result is that no other components need to be change, and it makes vmdq useful outside kvm. It also greatly reduces the amount of integration work needed throughout the stack (kvm/qemu/libvirt). -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] Virtual Machine Device Queues(VMDq) support on KVM
[RFC] Virtual Machine Device Queues (VMDq) support on KVM Network adapter with VMDq technology presents multiple pairs of tx/rx queues, and renders network L2 sorting mechanism based on MAC addresses and VLAN tags for each tx/rx queue pair. Here we present a generic framework, in which network traffic to/from a tx/rx queue pair can be directed from/to a KVM guest without any software copy. Actually this framework can apply to traditional network adapters which have just one tx/rx queue pair. And applications using the same user/kernel interface can utilize this framework to send/receive network traffic directly thru a tx/rx queue pair in a network adapter. We use virtio-net architecture to illustrate the framework. || pop add_buf|| |Qemu process| <-TX <-- | Guest Kernel | || -> --> || |Virtio-net | push get_buf|| | (Backend service) | ->RX --> | Virtio-net| || <- <-- |driver | || push get_buf|| || || | | | AIO (read & write) combined with Direct I/O | (which substitute synced file operations) |---| | Host kernel | read: copy-less with directly mapped user | | | space to kernel, payload directly DMAed | | | into user space | | | write: copy-less with directly mapped user | | | space to kernel, payload directly hooked | | | to a skb | | || | (a likely || | queue pair || | instance) || | | || | NIC driver <--> TUN/TAP driver | |---| | | traditional adapter or a tx/rx queue pair The basic idea is to utilize the kernel Asynchronous I/O combined with Direct I/O to implements copy-less TUN/TAP device. AIO and Direct I/O is not new to kernel, we still can see it in SCSI tape driver. With traditional file operations, a copying of payload contents from/to the kernel DMA address to/from a user buffer is needed. That's what the copying we want to save. The proposed framework is like this: A TUN/TAP device is bound to a traditional NIC adapter or a tx/rx queue pair in host side. KVM virto-net Backend service, the user space program submits asynchronous read/write I/O requests to the host kernel through TUN/TAP device. The requests are corresponding to the vqueue elements include both transmission & receive. They can be queued in one AIO request and later, the completion will be notified through the underlying packets tx/rx processing of the rx/tx queue pair. Detailed path: To guest Virtio-net driver, packets receive corresponding to asynchronous read I/O requests of Backend service. 1) Guest Virtio-net driver provides header and payload address through the receive vqueue to Virtio-net backend service. 2) Virtio-net backend service encapsulates multiple vqueue elements into multiple AIO control blocks and composes them into one AIO read request. 3) Virtio-net backend service uses io_submit() syscall to pass the request to the TUN/TAP device. 4) Virtio-net backend service uses io_getevents() syscall to check the completion of the request. 5) The TUN/TAP driver receives packets from the queue pair of NIC, and prepares for Direct I/O. A modified NIC driver may render a skb which header is allocated in host kernel, but the payload buffer is directly mapped from user space buffer which are rendered through the AIO request by the Backend service. get_user_pages() may do this. For one AIO read request, the TUN/TAP driver maintains a list for the directly mapped buffers, and a NIC driver tries to get the buffers as payload buffer to compose the new skbs. Of course, if getting the buffers fails, then kernel allocated buffers are used. 6) Modern NIC cards now mostly have the header split feature. The NIC queue pair then may directly DMA the payload into the user spaces mapped payload buffers. Thus a zero-copy for payload is implemented in packet receiving. 7) The TUN/TAP driver manually copy the host header to space user
RE: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
>I don't think we should do that with the tun/tap driver. By design, tun/tap is >a way to interact >with the >networking stack as if coming from a device. The only way this connects to an >external >adapter is through >a bridge or through IP routing, which means that it does not correspond to a >specific NIC. >I have worked on a driver I called 'macvtap' in lack of a better name, to add >a new tap >frontend to >the 'macvlan' driver. Since macvlan lets you add slaves to a single NIC >device, this gives you >a direct >connection between one or multiple tap devices to an external NIC, which works >a lot better >than when >you have a bridge inbetween. There is also work underway to add a bridging >capability to >macvlan, so >you can communicate directly between guests like you can do with a bridge. >Michael's vhost_net can plug into the same macvlan infrastructure, so the work >is >complementary. We use TUN/TAP device to implement the prototype, and agree that it's not the only choice here. We'd compare the two if possible. And what we cares more about is the modification in the kernel like the net_dev and skb structures' modifications, thanks. Thanks Xiaohui -----Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: Monday, August 31, 2009 11:24 PM To: Xin, Xiaohui Cc: m...@redhat.com; net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com Subject: Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server On Monday 31 August 2009, Xin, Xiaohui wrote: > > Hi, Michael > That's a great job. We are now working on support VMDq on KVM, and since the > VMDq hardware presents L2 sorting > based on MAC addresses and VLAN tags, our target is to implement a zero copy > solution using VMDq. I'm also interested in helping there, please include me in the discussions. > We stared > from the virtio-net architecture. What we want to proposal is to use AIO > combined with direct I/O: > 1) Modify virtio-net Backend service in Qemu to submit aio requests composed > from virtqueue. right, that sounds useful. > 2) Modify TUN/TAP device to support aio operations and the user space buffer > directly mapping into the host kernel. > 3) Let a TUN/TAP device binds to single rx/tx queue from the NIC. I don't think we should do that with the tun/tap driver. By design, tun/tap is a way to interact with the networking stack as if coming from a device. The only way this connects to an external adapter is through a bridge or through IP routing, which means that it does not correspond to a specific NIC. I have worked on a driver I called 'macvtap' in lack of a better name, to add a new tap frontend to the 'macvlan' driver. Since macvlan lets you add slaves to a single NIC device, this gives you a direct connection between one or multiple tap devices to an external NIC, which works a lot better than when you have a bridge inbetween. There is also work underway to add a bridging capability to macvlan, so you can communicate directly between guests like you can do with a bridge. Michael's vhost_net can plug into the same macvlan infrastructure, so the work is complementary. > 4) Modify the net_dev and skb structure to permit allocated skb to use user > space directly mapped payload > buffer address rather then kernel allocated. yes. > As zero copy is also your goal, we are interested in what's in your mind, and > would like to collaborate with you if possible. > BTW, we will send our VMDq write-up very soon. Ok, cool. Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
>It may be possible to make vmdq appear like an sr-iov capable device >from userspace. sr-iov provides the userspace interfaces to allocate >interfaces and assign mac addresses. To make it useful, you would have >to handle tx multiplexing in the driver but that would be much easier to >consume for kvm What we have thought is to support multiple net_dev structures according to multiple queue pairs of one vmdq adapter and presents multiple mac address in user space and each one mac can be used by a guest. What does the tx multiplexing in the driver exactly mean? Thanks Xiaohui -Original Message- From: Anthony Liguori [mailto:anth...@codemonkey.ws] Sent: Tuesday, September 01, 2009 5:57 AM To: Avi Kivity Cc: Xin, Xiaohui; m...@redhat.com; net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com Subject: Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server Avi Kivity wrote: > On 08/31/2009 02:42 PM, Xin, Xiaohui wrote: >> Hi, Michael >> That's a great job. We are now working on support VMDq on KVM, and >> since the VMDq hardware presents L2 sorting based on MAC addresses >> and VLAN tags, our target is to implement a zero copy solution using >> VMDq. We stared from the virtio-net architecture. What we want to >> proposal is to use AIO combined with direct I/O: >> 1) Modify virtio-net Backend service in Qemu to submit aio requests >> composed from virtqueue. >> 2) Modify TUN/TAP device to support aio operations and the user space >> buffer directly mapping into the host kernel. >> 3) Let a TUN/TAP device binds to single rx/tx queue from the NIC. >> 4) Modify the net_dev and skb structure to permit allocated skb to >> use user space directly mapped payload buffer address rather then >> kernel allocated. >> >> As zero copy is also your goal, we are interested in what's in your >> mind, and would like to collaborate with you if possible. >> > > One way to share the effort is to make vmdq queues available as normal > kernel interfaces. It may be possible to make vmdq appear like an sr-iov capable device from userspace. sr-iov provides the userspace interfaces to allocate interfaces and assign mac addresses. To make it useful, you would have to handle tx multiplexing in the driver but that would be much easier to consume for kvm. Regards, Anthony Liguori -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] Virtual Machine Device Queues(VMDq) support on KVM
>* Code is easier to review than bullet points. Yes. We'd send the code soon. >* Direct I/O has to be safe when page is shared by multiple threads, > and has to be non-blocking since network I/O can take indeterminately > long (think big queue's, tunneling, ...) In the situation, one queue pair NIC is assigned to only one guest, the pages are locked and a KVM guest will not swapped out. >* In the past attempts at Direct I/O on network have always had SMP > TLB issues. The page has to be flipped or marked as COW on all CPU's > and the cost of the Inter Processor Interrupt to steal the page has > been slower than copying It may be, we have not thought about this more . Thanks. Thanks Xiaohui -Original Message- From: Stephen Hemminger [mailto:shemmin...@vyatta.com] Sent: Wednesday, September 02, 2009 12:05 AM To: Xin, Xiaohui Cc: m...@redhat.com; net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com Subject: Re: [RFC] Virtual Machine Device Queues(VMDq) support on KVM On Tue, 1 Sep 2009 14:58:19 +0800 "Xin, Xiaohui" wrote: > [RFC] Virtual Machine Device Queues (VMDq) support on KVM > > Network adapter with VMDq technology presents multiple pairs of tx/rx queues, > and renders network L2 sorting mechanism based on MAC addresses and VLAN tags > for each tx/rx queue pair. Here we present a generic framework, in which > network > traffic to/from a tx/rx queue pair can be directed from/to a KVM guest without > any software copy. > > Actually this framework can apply to traditional network adapters which have > just one tx/rx queue pair. And applications using the same user/kernel > interface > can utilize this framework to send/receive network traffic directly thru a > tx/rx > queue pair in a network adapter. > > We use virtio-net architecture to illustrate the framework. > > > || pop add_buf|| > |Qemu process| <-TX <-- | Guest Kernel | > || -> --> || > |Virtio-net | push get_buf|| > | (Backend service) | ->RX --> | Virtio-net| > || <- <-- |driver | > || push get_buf|| > || || >| >| >| AIO (read & write) combined with Direct I/O >| (which substitute synced file operations) > |---| > | Host kernel | read: copy-less with directly mapped user | > | | space to kernel, payload directly DMAed | > | | into user space | > | | write: copy-less with directly mapped user | > | | space to kernel, payload directly hooked | > | | to a skb | > | || > | (a likely || > | queue pair || > | instance) || > | | || > | NIC driver <--> TUN/TAP driver | > |---| >| >| >traditional adapter or a tx/rx queue pair > > The basic idea is to utilize the kernel Asynchronous I/O combined with Direct > I/O to implements copy-less TUN/TAP device. AIO and Direct I/O is not new to > kernel, we still can see it in SCSI tape driver. > > With traditional file operations, a copying of payload contents from/to the > kernel DMA address to/from a user buffer is needed. That's what the copying we > want to save. > > The proposed framework is like this: > A TUN/TAP device is bound to a traditional NIC adapter or a tx/rx queue pair > in > host side. KVM virto-net Backend service, the user space program submits > asynchronous read/write I/O requests to the host kernel through TUN/TAP > device. > The requests are corresponding to the vqueue elements include both > transmission
RE: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
Michael, We are very interested in your patch and want to have a try with it. I have collected your 3 patches in kernel side and 4 patches in queue side. The patches are listed here: PATCHv5-1-3-mm-export-use_mm-unuse_mm-to-modules.patch PATCHv5-2-3-mm-reduce-atomic-use-on-use_mm-fast-path.patch PATCHv5-3-3-vhost_net-a-kernel-level-virtio-server.patch PATCHv3-1-4-qemu-kvm-move-virtio-pci[1].o-to-near-pci.o.patch PATCHv3-2-4-virtio-move-features-to-an-inline-function.patch PATCHv3-3-4-qemu-kvm-vhost-net-implementation.patch PATCHv3-4-4-qemu-kvm-add-compat-eventfd.patch I applied the kernel patches on v2.6.31-rc4 and the qemu patches on latest kvm qemu. But seems there are some patches are needed at least irqfd and ioeventfd patches on current qemu. I cannot create a kvm guest with "-net nic,model=virtio,vhost=vethX". May you kindly advice us the patch lists all exactly to make it work? Thanks a lot. :-) Thanks Xiaohui -Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Michael S. Tsirkin Sent: Wednesday, September 09, 2009 4:14 AM To: Ira W. Snyder Cc: net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com; Rusty Russell; s.he...@linux-ag.com Subject: Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server On Tue, Sep 08, 2009 at 10:20:35AM -0700, Ira W. Snyder wrote: > On Mon, Sep 07, 2009 at 01:15:37PM +0300, Michael S. Tsirkin wrote: > > On Thu, Sep 03, 2009 at 11:39:45AM -0700, Ira W. Snyder wrote: > > > On Thu, Aug 27, 2009 at 07:07:50PM +0300, Michael S. Tsirkin wrote: > > > > What it is: vhost net is a character device that can be used to reduce > > > > the number of system calls involved in virtio networking. > > > > Existing virtio net code is used in the guest without modification. > > > > > > > > There's similarity with vringfd, with some differences and reduced scope > > > > - uses eventfd for signalling > > > > - structures can be moved around in memory at any time (good for > > > > migration) > > > > - support memory table and not just an offset (needed for kvm) > > > > > > > > common virtio related code has been put in a separate file vhost.c and > > > > can be made into a separate module if/when more backends appear. I used > > > > Rusty's lguest.c as the source for developing this part : this supplied > > > > me with witty comments I wouldn't be able to write myself. > > > > > > > > What it is not: vhost net is not a bus, and not a generic new system > > > > call. No assumptions are made on how guest performs hypercalls. > > > > Userspace hypervisors are supported as well as kvm. > > > > > > > > How it works: Basically, we connect virtio frontend (configured by > > > > userspace) to a backend. The backend could be a network device, or a > > > > tun-like device. In this version I only support raw socket as a backend, > > > > which can be bound to e.g. SR IOV, or to macvlan device. Backend is > > > > also configured by userspace, including vlan/mac etc. > > > > > > > > Status: > > > > This works for me, and I haven't see any crashes. > > > > I have done some light benchmarking (with v4), compared to userspace, I > > > > see improved latency (as I save up to 4 system calls per packet) but not > > > > bandwidth/CPU (as TSO and interrupt mitigation are not supported). For > > > > ping benchmark (where there's no TSO) troughput is also improved. > > > > > > > > Features that I plan to look at in the future: > > > > - tap support > > > > - TSO > > > > - interrupt mitigation > > > > - zero copy > > > > > > > > > > Hello Michael, > > > > > > I've started looking at vhost with the intention of using it over PCI to > > > connect physical machines together. > > > > > > The part that I am struggling with the most is figuring out which parts > > > of the rings are in the host's memory, and which parts are in the > > > guest's memory. > > > > All rings are in guest's memory, to match existing virtio code. > > Ok, this makes sense. > > > vhost > > assumes that the memory space of the hypervisor userspace process covers > > the whole of guest memory. > > Is this necessary? Why? Because with virtio ring can give us arbitrary guest addresses. If guest was limited to using a subset of addresses, hypervisor would only have to map these. > The assumption seems very wrong when you're > doing data transport between two physical systems via PCI. > I know vhost has not been designed for this specific situation, but it > is good to be looking toward other possible uses. > > > And there's a translation table. > > Ring addresses are userspace addresses, they do not undergo translation. > > > > > If I understand everything correctly, the rings are all userspace > > > addresses, which means that they can be moved around in physical memory, > > > and get pushed out t
RE: [PATCHv5 3/3] vhost_net: a kernel-level virtio server
>The irqfd/ioeventfd patches are part of Avi's kvm.git tree: >git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git > >I expect them to be merged by 2.6.32-rc1 - right, Avi? Michael, I think I have the kernel patch for kvm_irqfd and kvm_ioeventfd, but missed the qemu side patch for irqfd and ioeventfd. I met the compile error when I compiled virtio-pci.c file in qemu-kvm like this: /root/work/vmdq/vhost/qemu-kvm/hw/virtio-pci.c:384: error: `KVM_IRQFD` undeclared (first use in this function) /root/work/vmdq/vhost/qemu-kvm/hw/virtio-pci.c:400: error: `KVM_IOEVENTFD` undeclared (first use in this function) Which qemu tree or patch do you use for kvm_irqfd and kvm_ioeventfd? Thanks Xiaohui -Original Message- From: Michael S. Tsirkin [mailto:m...@redhat.com] Sent: Sunday, September 13, 2009 1:46 PM To: Xin, Xiaohui Cc: Ira W. Snyder; net...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; linux...@kvack.org; a...@linux-foundation.org; h...@zytor.com; gregory.hask...@gmail.com; Rusty Russell; s.he...@linux-ag.com; a...@redhat.com Subject: Re: [PATCHv5 3/3] vhost_net: a kernel-level virtio server On Fri, Sep 11, 2009 at 11:17:33PM +0800, Xin, Xiaohui wrote: > Michael, > We are very interested in your patch and want to have a try with it. > I have collected your 3 patches in kernel side and 4 patches in queue side. > The patches are listed here: > > PATCHv5-1-3-mm-export-use_mm-unuse_mm-to-modules.patch > PATCHv5-2-3-mm-reduce-atomic-use-on-use_mm-fast-path.patch > PATCHv5-3-3-vhost_net-a-kernel-level-virtio-server.patch > > PATCHv3-1-4-qemu-kvm-move-virtio-pci[1].o-to-near-pci.o.patch > PATCHv3-2-4-virtio-move-features-to-an-inline-function.patch > PATCHv3-3-4-qemu-kvm-vhost-net-implementation.patch > PATCHv3-4-4-qemu-kvm-add-compat-eventfd.patch > > I applied the kernel patches on v2.6.31-rc4 and the qemu patches on latest > kvm qemu. > But seems there are some patches are needed at least irqfd and ioeventfd > patches on > current qemu. I cannot create a kvm guest with "-net > nic,model=virtio,vhost=vethX". > > May you kindly advice us the patch lists all exactly to make it work? > Thanks a lot. :-) > > Thanks > Xiaohui The irqfd/ioeventfd patches are part of Avi's kvm.git tree: git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git I expect them to be merged by 2.6.32-rc1 - right, Avi? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH v2 0/3] Provide a zero-copy method on KVM virtio-net.
> Michael, > >>> The idea is simple, just to pin the guest VM user space and then > >>> let host NIC driver has the chance to directly DMA to it. > >>> The patches are based on vhost-net backend driver. We add a device > >>> which provides proto_ops as sendmsg/recvmsg to vhost-net to > >>> send/recv directly to/from the NIC driver. KVM guest who use the > >>> vhost-net backend may bind any ethX interface in the host side to > >>> get copyless data transfer thru guest virtio-net frontend. > >>> > >>> The scenario is like this: > >>> > >>> The guest virtio-net driver submits multiple requests thru vhost-net > >>> backend driver to the kernel. And the requests are queued and then > >>> completed after corresponding actions in h/w are done. > >>> > >>> For read, user space buffers are dispensed to NIC driver for rx when > >>> a page constructor API is invoked. Means NICs can allocate user buffers > >>> from a page constructor. We add a hook in netif_receive_skb() function > >>> to intercept the incoming packets, and notify the zero-copy device. > >>> > >>> For write, the zero-copy deivce may allocates a new host skb and puts > >>> payload on the skb_shinfo(skb)->frags, and copied the header to skb->data. > >>> The request remains pending until the skb is transmitted by h/w. > >>> > >>> Here, we have ever considered 2 ways to utilize the page constructor > >>> API to dispense the user buffers. > >>> > >>> One: Modify __alloc_skb() function a bit, it can only allocate a > >>> structure of sk_buff, and the data pointer is pointing to a > >>> user buffer which is coming from a page constructor API. > >>> Then the shinfo of the skb is also from guest. > >>> When packet is received from hardware, the skb->data is filled > >>> directly by h/w. What we have done is in this way. > >>> > >>> Pros: We can avoid any copy here. > >>> Cons: Guest virtio-net driver needs to allocate skb as almost > >>> the same method with the host NIC drivers, say the size > >>> of netdev_alloc_skb() and the same reserved space in the > >>> head of skb. Many NIC drivers are the same with guest and > >>> ok for this. But some lastest NIC drivers reserves special > >>> room in skb head. To deal with it, we suggest to provide > >>> a method in guest virtio-net driver to ask for parameter > >>> we interest from the NIC driver when we know which device > >>> we have bind to do zero-copy. Then we ask guest to do so. > >>> Is that reasonable? > >>Unfortunately, this would break compatibility with existing virtio. > >>This also complicates migration. >> You mean any modification to the guest virtio-net driver will break the >> compatibility? We tried to enlarge the virtio_net_config to contains the >> 2 parameter, and add one VIRTIO_NET_F_PASSTHRU flag, virtionet_probe() >> will check the feature flag, and get the parameters, then virtio-net driver >> use >> it to allocate buffers. How about this? >This means that we can't, for example, live-migrate between different systems >without flushing outstanding buffers. Ok. What we have thought about now is to do something with skb_reserve(). If the device is binded by mp, then skb_reserve() will do nothing with it. > >>What is the room in skb head used for? > >I'm not sure, but the latest ixgbe driver does this, it reserves 32 bytes > >compared to >> NET_IP_ALIGN. >Looking at code, this seems to do with alignment - could just be >a performance optimization. > >>> Two: Modify driver to get user buffer allocated from a page > >>> constructor > >>> API(to substitute alloc_page()), the user buffer are used as payload > >>> buffers and filled by h/w directly when packet is received. Driver > >>> should associate the pages with skb (skb_shinfo(skb)->frags). For > >>> the head buffer side, let host allocates skb, and h/w fills it. > >>> After that, the data filled in host skb header will be copied into > >>> guest header buffer which is submitted together with the payload buffer. > >>> > >>> Pros: We could less care the way how guest or host allocates their > >>> buffers. > >>> Cons: We still need a bit copy here for the skb header. > >>> > >>> We are not sure which way is the better here. > >>The obvious question would be whether you see any speed difference > >>with the two approaches. If no, then the second approach would be > >>better. > >> I remember the second approach is a bit slower in 1500MTU. >> But we did not tested too much. >Well, that's an important datapoint. By the way, you'll need >header copy to activate LRO in host, so that's a good >reason to go with option 2 as well. > >>> This is the first thing we want > >>> to get comments from the community. We wish the modification to the > >>> network > >>> part will be generic which not used by vhost-net backend only, but a user > >>> application may use it as well when the
RE: [RFC][PATCH v2 0/3] Provide a zero-copy method on KVM virtio-net.
Michael, >> What we have not done yet: >> packet split support >> >What does this mean, exactly? We can support 1500MTU, but for jumbo frame, since vhost driver before don't support mergeable buffer, we cannot try it for multiple sg. >>>I do not see why, vhost currently supports 64K buffers with indirect >>>descriptors. >>> >> The receive_skb() in guest virtio-net driver will merge the multiple sg to >> skb frags, how >>can indirect descriptors to that? >See add_recvbuf_big. I don't mean this, it's for buffer submission. I mean when packet is received, in receive_buf(), mergeable buffer knows which pages received can be hooked in skb frags, it's receive_mergeable() which do this. When a NIC driver supports packet split mode, then each ring descriptor contains a skb and a page. When packet is received, if the status is not EOP, then hook the page of the next descriptor to the prev skb. We don't how many frags belongs to one skb. So when guest submit buffers, it should submit multiple pages, and when receive, the guest should know which pages are belongs to one skb and hook them together. I think receive_mergeable() can do this, but I don't see how big->packets handle this. May I miss something here? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Re:[RFC][PATCH v3 1/3] A device for zero-copy based on KVM virtio-net.
Michael, Sorry, it's based on the suggestion to hook an iocb completion callback to handle the iocb list in vhost-net. Thanks Xiaohui -Original Message- From: Xin, Xiaohui Sent: Thursday, April 22, 2010 4:24 PM To: m...@redhat.com Cc: a...@arndb.de; net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; da...@davemloft.net; jd...@linux.intel.com; Xin, Xiaohui Subject: Re:[RFC][PATCH v3 1/3] A device for zero-copy based on KVM virtio-net. From: Xin Xiaohui Add a device to utilize the vhost-net backend driver for copy-less data transfer between guest FE and host NIC. It pins the guest user space to the host memory and provides proto_ops as sendmsg/recvmsg to vhost-net. Signed-off-by: Xin Xiaohui Signed-off-by: Zhao Yu Reviewed-by: Jeff Dike --- Michael, Thanks. I have updated the patch with your suggestion. It looks much clean now. Please have a review. Thanks Xiaohui drivers/vhost/Kconfig | 10 + drivers/vhost/Makefile|2 + drivers/vhost/mpassthru.c | 1239 + include/linux/mpassthru.h | 29 + 4 files changed, 1280 insertions(+), 0 deletions(-) create mode 100644 drivers/vhost/mpassthru.c create mode 100644 include/linux/mpassthru.h diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig index 9f409f4..91806b1 100644 --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -9,3 +9,13 @@ config VHOST_NET To compile this driver as a module, choose M here: the module will be called vhost_net. +config MEDIATE_PASSTHRU + tristate "mediate passthru network driver (EXPERIMENTAL)" + depends on VHOST_NET + ---help--- + zerocopy network I/O support, we call it as mediate passthru to + be distiguish with hardare passthru. + + To compile this driver as a module, choose M here: the module will + be called mpassthru. + diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile index 72dd020..c18b9fc 100644 --- a/drivers/vhost/Makefile +++ b/drivers/vhost/Makefile @@ -1,2 +1,4 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o vhost_net-y := vhost.o net.o + +obj-$(CONFIG_MEDIATE_PASSTHRU) += mpassthru.o diff --git a/drivers/vhost/mpassthru.c b/drivers/vhost/mpassthru.c new file mode 100644 index 000..cc99b14 --- /dev/null +++ b/drivers/vhost/mpassthru.c @@ -0,0 +1,1239 @@ +/* + * MPASSTHRU - Mediate passthrough device. + * Copyright (C) 2009 ZhaoYu, XinXiaohui, Dike, Jeffery G + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#define DRV_NAME"mpassthru" +#define DRV_DESCRIPTION "Mediate passthru device driver" +#define DRV_COPYRIGHT "(C) 2009 ZhaoYu, XinXiaohui, Dike, Jeffery G" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +/* Uncomment to enable debugging */ +/* #define MPASSTHRU_DEBUG 1 */ + +#ifdef MPASSTHRU_DEBUG +static int debug; + +#define DBG if (mp->debug) printk +#define DBG1 if (debug == 2) printk +#else +#define DBG(a...) +#define DBG1(a...) +#endif + +#define COPY_THRESHOLD (L1_CACHE_BYTES * 4) +#define COPY_HDR_LEN (L1_CACHE_BYTES < 64 ? 64 : L1_CACHE_BYTES) + +struct frag { + u16 offset; + u16 size; +}; + +struct page_ctor { + struct list_headreadq; + int w_len; + int r_len; + spinlock_t read_lock; + struct kmem_cache *cache; + /* record the locked pages */ + int lock_pages; + struct rlimit o_rlim; + struct net_device *dev; + struct mpassthru_port port; +}; + +struct page_info { + struct list_headlist; + int header; + /* indicate the actual length of bytes +* send/recv in the user space buffers +*/ + int total; + int offset; + struct page *pages[MAX_SKB_FRAGS+1]; + struct skb_frag_struct frag[MAX_SKB_FRAGS+1]; + struct sk_buff *skb; + struct page_ctor*ctor; + + /* The pointer relayed to skb, to indicate +* it's a user space allocate
RE: [RFC][PATCH v2 0/3] Provide a zero-copy method on KVM virtio-net.
Michael, >Yes, I think this packet split mode probably maps well to mergeable buffer >support. Note that >1. Not all devices support large packets in this way, others might map > to indirect buffers better Do the indirect buffers accord to deal with the skb->frag_list? > So we have to figure out how migration is going to work Yes, different guest virtio-net driver may contain different features. Does the qemu migration work with different features supported by virtio-net driver now? >2. It's up to guest driver whether to enable features such as > mergeable buffers and indirect buffers > So we have to figure out how to notify guest which mode > is optimal for a given device Yes. When a device is binded, the mp device may query the capabilities from driver. Actually, there is a structure now in mp device can do this, we can add some field to support more. >3. We don't want to depend on jumbo frames for decent performance > So we probably should support GSO/GRO GSO is for the tx side, right? I think driver can handle it itself. For GRO, I'm not sure it's easy or not. Basically, the mp device now we have support is doing what raw socket is doing. The packets are not going to host stack. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH v4 00/18] Provide a zero-copy method on KVM virtio-net.
> > The idea is simple, just to pin the guest VM user space and then let > > host NIC driver has the chance to directly DMA to it. > > > >Isn't it much easier to map the RX ring of the network device into the > >guest's address space, have DMA map calls translate guest addresses to > >physical/DMA addresses as well as do all of this crazy page pinning > >stuff, and provide the translations and protections via the IOMMU? >This means we need guest know how the specific network device works. >So we won't be able to, for example, move guest between different hosts. >There are other problems: many physical systems do not have an iommu, >some guest OS-es do not support DMA map calls, doing VM exit >on each DMA map call might turn out to be very slow. And so on. This solution is what now we can think of to implement zero-copy. Some modifications are made to net core to try to avoid network device driver changes. The major change is to __alloc_skb(), in which we added a dev parameter to indicate whether the device will DMA to/from guest/user buffer which is pointed by host skb->data. We also modify skb_release_data() and skb_reserve(). We made it now works with ixgbe driver with PS mode disabled, and got some performance data with it. using netperf with GSO/TSO disabled, 10G NIC, disabled packet split mode, with raw socket case compared to vhost. bindwidth will be from 1.1Gbps to 1.7Gbps CPU % from 120%-140% to 140%-160% We are now trying to get decent performance data with advanced features. Do you have any other concerns with this solution? >> What's being proposed here looks a bit over-engineered. >This is an attempt to reduce overhead for virtio (paravirtualization). >'Don't use PV' is kind of an alternative, but I do not >think it's a simpler one. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH v4 05/18] Add a function to indicate if device use external buffer.
> +static int dev_is_mpassthru(struct net_device *dev) > >bool return value should be better here. > >-- >Regards, >Changli Gao(xiao...@gmail.com) Thanks, would fix that. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH v4 00/18] Provide a zero-copy method on KVM virtio-net.
Michael, Sorry, somehow I missed this mail. :-( >> Here, we have ever considered 2 ways to utilize the page constructor >> API to dispense the user buffers. >> >> One: Modify __alloc_skb() function a bit, it can only allocate a >> structure of sk_buff, and the data pointer is pointing to a >> user buffer which is coming from a page constructor API. >> Then the shinfo of the skb is also from guest. >> When packet is received from hardware, the skb->data is filled >> directly by h/w. What we have done is in this way. >> >> Pros: We can avoid any copy here. >> Cons: Guest virtio-net driver needs to allocate skb as almost >> the same method with the host NIC drivers, say the size >> of netdev_alloc_skb() and the same reserved space in the >> head of skb. Many NIC drivers are the same with guest and >> ok for this. But some lastest NIC drivers reserves special >> room in skb head. To deal with it, we suggest to provide >> a method in guest virtio-net driver to ask for parameter >> we interest from the NIC driver when we know which device >> we have bind to do zero-copy. Then we ask guest to do so. >> Is that reasonable? >Do you still do this? Currently, we still use the first way. But we now ignore the room which host skb_reserve() required when device is doing zero-copy. Now we don't taint guest virtio-net driver with a new method by this way. >> Two: Modify driver to get user buffer allocated from a page constructor >> API(to substitute alloc_page()), the user buffer are used as payload >> buffers and filled by h/w directly when packet is received. Driver >> should associate the pages with skb (skb_shinfo(skb)->frags). For >> the head buffer side, let host allocates skb, and h/w fills it. >> After that, the data filled in host skb header will be copied into >> guest header buffer which is submitted together with the payload buffer. >> >> Pros: We could less care the way how guest or host allocates their >> buffers. >> Cons: We still need a bit copy here for the skb header. >> >> We are not sure which way is the better here. This is the first thing we want >> to get comments from the community. We wish the modification to the network >> part will be generic which not used by vhost-net backend only, but a user >> application may use it as well when the zero-copy device may provides async >> read/write operations later. >I commented on this in the past. Do you still want comments? Now we continue with the first way and try to push it. But any comments about the two methods are still welcome. >That's nice. The thing to do is probably to enable GSO/TSO >and see what we get this way. Also, mergeable buffer support >was recently posted and I hope to merge it for 2.6.35. >You might want to take a look. I'm looking at the mergeable buffer. I think GSO/GRO support with zero-copy also needs it. Currently, GSO/TSO is still not supported by vhost-net? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
issue about virtio-net driver to suppoprt vhost mergeable buffer with zero-copy to support PS mode
Michael, I'm now looking into the vhost mergeable buffer, and I tried to use it to support PS mode with zero-copy. And I found an issue there that I have to modify the guest virito-net driver. When guest virtio-net driver submits mergeable buffers, it submits multiple pages outside. In zero-copy case, vhost cannot know which page is used to put header, and which page is used to put payload. Then vhost can only reserves 12 bytes for each page. That means, the page_offset of the payload DMAed into the guest buffer is always 12 bytes. But guest virtio-net driver always use offset 0 to put the data (See receive_mergeable()). That's where the zero-copy use mergeable buffer must modify. Have I missed something here? And how do you think about it? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: issue about virtio-net driver to suppoprt vhost mergeable buffer with zero-copy to support PS mode
Michael, What's you have suggested could avoid to taint the guest virtio-net driver. Really thanks! For the two alternative, the first will do more trick with driver or net-core stuff. So currently, I prefer to try the second one. Anyway, let me have a good think of it. Thanks! Thanks Xiaohui >-Original Message- >From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of >Michael S. Tsirkin >Sent: Thursday, May 27, 2010 4:20 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >Herbert >Xu >Subject: Re: issue about virtio-net driver to suppoprt vhost mergeable buffer >with zero-copy >to support PS mode > >On Thu, May 27, 2010 at 09:21:02AM +0800, Xin, Xiaohui wrote: >> Michael, >> I'm now looking into the vhost mergeable buffer, and I tried to use it to >> support PS mode >with zero-copy. And I found an issue there that I have to modify the guest >virito-net driver. >> >> When guest virtio-net driver submits mergeable buffers, it submits multiple >> pages outside. >In zero-copy case, vhost cannot know which page is used to put header, and >which page is >used to put payload. Then vhost can only reserves 12 bytes for each page. That >means, the >page_offset of the payload DMAed into the guest buffer is always 12 bytes. But >guest >virtio-net driver always use offset 0 to put the data (See >receive_mergeable()). That's where >the zero-copy use mergeable buffer must modify. >> >> Have I missed something here? And how do you think about it? >> >> Thanks >> Xiaohui > >Maybe you can teach the hardware skip the first 12 bytes: qemu will >call an ioctl telling hardware what the virtio header size is. >This is how we plan to do it for tap. > >Alternatively, buffers can be used in any order. >So we can have hardware use N buffers for the packet, and then >have vhost put the header in buffer N+1. > >-- >MST >-- >To unsubscribe from this list: send the line "unsubscribe kvm" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 11/19] Use callback to deal with skb_release_data() specially.
>-Original Message- >From: Eric Dumazet [mailto:eric.duma...@gmail.com] >Sent: Saturday, June 05, 2010 10:56 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 11/19] Use callback to deal with skb_release_data() >specially. > >Le samedi 05 juin 2010 à 18:14 +0800, xiaohui....@intel.com a écrit : >> From: Xin Xiaohui >> >> If buffer is external, then use the callback to destruct >> buffers. >> >> Signed-off-by: Xin Xiaohui >> Signed-off-by: Zhao Yu >> Reviewed-by: Jeff Dike >> --- >> net/core/skbuff.c | 11 +++ >> 1 files changed, 11 insertions(+), 0 deletions(-) >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index 37587f0..418457c 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -385,6 +385,11 @@ static void skb_clone_fraglist(struct sk_buff *skb) >> >> static void skb_release_data(struct sk_buff *skb) >> { >> +/* check if the skb has external buffers, we have use destructor_arg >> + * here to indicate >> + */ >> +struct skb_external_page *ext_page = skb_shinfo(skb)->destructor_arg; >> + > >Oh well. This is v7 of your series, and nobody complained yet ? > >This is a new cache miss on a _critical_ path. Ok, I would remove the declaration here to avoid the new cache miss. > > >> if (!skb->cloned || >> !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1, >> &skb_shinfo(skb)->dataref)) { >> @@ -397,6 +402,12 @@ static void skb_release_data(struct sk_buff *skb) >> if (skb_has_frags(skb)) >> skb_drop_fraglist(skb); >> >> +/* if the skb has external buffers, use destructor here, >> + * since after that skb->head will be kfree, in case skb->head >> + * from external buffer cannot use kfree to destroy. >> + */ > >Why not deferring here the access to skb_shinfo(skb)->destructor_arg ? And references skb_shinfo(skb)->destructor_arg here. > >> +if (dev_is_mpassthru(skb->dev) && ext_page && ext_page->dtor) >> +ext_page->dtor(ext_page); >> kfree(skb->head); >> } >> } > >if (dev_is_mpassthru(skb->dev)) { > struct skb_external_page *ext_page = > skb_shinfo(skb)->destructor_arg; > if (ext_page && ext_page->dtor) > ext_page->dtor(ext_page); >} > >destructor_arg should me moved before frags[] if you really want to use it. Thanks for the patch. But why destructor_arg before frags[] is better than after frags[]? skb_release_data() will reference both of them > >diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >index bf243fc..b136d90 100644 >--- a/include/linux/skbuff.h >+++ b/include/linux/skbuff.h >@@ -202,10 +202,11 @@ struct skb_shared_info { > */ >atomic_tdataref; > >- skb_frag_t frags[MAX_SKB_FRAGS]; >/* Intermediate layers must ensure that destructor_arg > * remains valid until skb destructor */ >void * destructor_arg; >+ >+ skb_frag_t frags[MAX_SKB_FRAGS]; > }; > > /* We divide dataref into two halves. The higher 16 bits hold references > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 08/19] Make __alloc_skb() to get external buffer.
>-Original Message- >From: Eric Dumazet [mailto:eric.duma...@gmail.com] >Sent: Saturday, June 05, 2010 10:53 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 08/19] Make __alloc_skb() to get external buffer. > >Le samedi 05 juin 2010 à 18:14 +0800, xiaohui....@intel.com a écrit : >> From: Xin Xiaohui >> child->fclone = SKB_FCLONE_UNAVAILABLE; >> } >> +/* Record the external buffer info in this field. It's not so good, >> + * but we cannot find another place easily. >> + */ >> +shinfo->destructor_arg = ext_page; >> + > > >Yes this is a big problem, its basically using a cache line that was not >touched before. > Did your patch which moves destructor_arg before frags[] also fix this? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Stephen Hemminger [mailto:shemmin...@vyatta.com] >Sent: Monday, June 07, 2010 7:14 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >Still not sure this is a good idea for a couple of reasons: > >1. We already have lots of special cases with skb's (frags and fraglist), > and skb's travel through a lot of different parts of the kernel. So any > new change like this creates lots of exposed points for new bugs. Look > at cases like MD5 TCP and netfilter, and forwarding these SKB's to ipsec > and ppp and ... > Yes, I agree on your concern at some extent. But the skbs which use external pages in our cases have short travels which start from NIC driver and then forward to the guest immediately. It will not travel into host kernel stack or any filters which may avoid many problems you have mentioned here. Another point is that we try to make the solution more generic to different NIC drivers, then many drivers may use the way without modifications. >2. SKB's can have infinite lifetime in the kernel. If these buffers come from > a fixed size pool in an external device, they can easily all get tied up > if you have a slow listener. What happens then? The pool is not fixed size, it's the size of usable buffers submitted by guest virtio-net driver. Guest virtio-net will check how much buffers are filled and do resubmit. What a slow listener mean? A slow NIC driver? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf >Of Andi >Kleen >Sent: Monday, June 07, 2010 3:51 PM >To: Stephen Hemminger >Cc: Xin, Xiaohui; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >herb...@gondor.apana.org.au; jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >Stephen Hemminger writes: > >> Still not sure this is a good idea for a couple of reasons: >> >> 1. We already have lots of special cases with skb's (frags and fraglist), >>and skb's travel through a lot of different parts of the kernel. So any >>new change like this creates lots of exposed points for new bugs. Look >>at cases like MD5 TCP and netfilter, and forwarding these SKB's to ipsec >>and ppp and ... >> >> 2. SKB's can have infinite lifetime in the kernel. If these buffers come from >>a fixed size pool in an external device, they can easily all get tied up >>if you have a slow listener. What happens then? > >3. If they come from an internal pool what happens when the kernel runs >low on memory? How is that pool balanced against other kernel >memory users? > The size of the pool is limited by the virtqueue capacity now. If the virtqueue is really huge, then how to balance on memory is a problem. I did not consider it clearly how to tune it dynamically currently... >-Andi > >-- >a...@linux.intel.com -- Speaking for myself only. >-- >To unsubscribe from this list: send the line "unsubscribe kvm" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Mitchell Erblich [mailto:erbli...@earthlink.net] >Sent: Monday, June 07, 2010 4:17 PM >To: Andi Kleen >Cc: Stephen Hemminger; Xin, Xiaohui; net...@vger.kernel.org; >kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >herb...@gondor.apana.org.au; jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > > >On Jun 7, 2010, at 12:51 AM, Andi Kleen wrote: > >> Stephen Hemminger writes: >> >>> Still not sure this is a good idea for a couple of reasons: >>> >>> 1. We already have lots of special cases with skb's (frags and fraglist), >>> and skb's travel through a lot of different parts of the kernel. So any >>> new change like this creates lots of exposed points for new bugs. Look >>> at cases like MD5 TCP and netfilter, and forwarding these SKB's to ipsec >>> and ppp and ... >>> >>> 2. SKB's can have infinite lifetime in the kernel. If these buffers come >>> from >>> a fixed size pool in an external device, they can easily all get tied up >>> if you have a slow listener. What happens then? >> >> 3. If they come from an internal pool what happens when the kernel runs >> low on memory? How is that pool balanced against other kernel >> memory users? >> >> -Andi >> >> -- >> a...@linux.intel.com -- Speaking for myself only. > >In general, > >When an internal pool is created/used, their SHOULD be a reason. >Maybe, to keep allocation latency to a min, OR ... > The internal pool here is a collection of user buffers submitted by guest virtio-net driver. Guest put buffers here and driver get buffers from it. If guest submit more buffers then driver needs, we need somewhere to put the buffers, it's the internal pool here to deal with. >Now IMO, > >internal pool objects should have a ref count and >if that count is 0, then under memory pressure and/or num >of objects are above a high water mark, then they are freed, > >OR if there is a last reference age field, then the object is to be >cleaned if dirty, then freed, > >Else, the pool is allowed to grow if the number of objects in the >pool is below a set max (max COULD equal Infinity). Thanks for the thoughts. Basically, the size of the internal pool is not decided by the pool itself, To add/delete the objects in the pool is not a task of the pool itself too. It's decided by guest virtio-net driver and vhost-net driver both, and decided by the guest receive speed and submit speed. The max size of the pool is limited by the virtqueue buffer numbers. Thanks Xiaohui > > > >Mitchell Erblich -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Tuesday, June 08, 2010 1:28 PM >To: Stephen Hemminger >Cc: Xin, Xiaohui; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >On Sun, Jun 06, 2010 at 04:13:48PM -0700, Stephen Hemminger wrote: >> Still not sure this is a good idea for a couple of reasons: >> >> 1. We already have lots of special cases with skb's (frags and fraglist), >>and skb's travel through a lot of different parts of the kernel. So any >>new change like this creates lots of exposed points for new bugs. Look >>at cases like MD5 TCP and netfilter, and forwarding these SKB's to ipsec >>and ppp and ... >> >> 2. SKB's can have infinite lifetime in the kernel. If these buffers come from >>a fixed size pool in an external device, they can easily all get tied up >>if you have a slow listener. What happens then? > >I agree with Stephen on this. > >FWIW I don't think we even need the external pages concept in >order to implement zero-copy receive (which I gather is the intent >here). > >Here is one way to do it, simply construct a completely non-linear >packet in the driver, as you would if you were using the GRO frags >interface (grep for napi_gro_frags under drivers/net for examples). > I'm not sure if I understand your way correctly: 1) Does the way only deal with driver with SG feature? Since packet is non-linear... 2) Is skb->data still pointing to guest user buffers? If yes, how to avoid the modifications to net core change to skb? 3) In our way only parts of drivers need be modified to support zero-copy. and here, need we modify all the drivers? If I missed your idea, may you explain it in more detail? >This way you can transfer the entire contents of the packet without >copying through to the other side, provided that the host stack does >not modify the packet. > >If the host side did modify the packet then we have to incur the >memory cost anyway. > >IOW I think the only feature provided by the external pages >construct is allowing the skb->head area to be shared without >copying. I'm claiming that this can be done by simply making >skb->head empty. > I think to make skb->head empty at first will cause more effort to pass the check with skb header. Have I missed something here? I really make the skb->head NULL just before kfree(skb) in skb_release_data(), it's done by callback we have made for skb. Thanks Xiaohui >Cheers, >-- >Visit Openswan at http://www.openswan.org/ >Email: Herbert Xu ~{PmV>HI~} >Home Page: http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Friday, June 11, 2010 1:21 PM >To: Xin, Xiaohui >Cc: Stephen Hemminger; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >On Wed, Jun 09, 2010 at 05:54:02PM +0800, Xin, Xiaohui wrote: >> >> I'm not sure if I understand your way correctly: >> 1) Does the way only deal with driver with SG feature? Since packet >> is non-linear... > >No the hardware doesn't have to support SG. You just need to >place the entire packet contents in a page instead of skb->head. > >> 2) Is skb->data still pointing to guest user buffers? >> If yes, how to avoid the modifications to net core change to skb? > >skb->data would not point to guest user buffers. In the common >case the packet is not modified on its way to the guest so this >is not an issue. > >In the rare case where it is modified, you only have to copy the >bits which are modified and the cost of that is inconsequential >since you have to write to that memory anyway. > >> 3) In our way only parts of drivers need be modified to support zero-copy. >> and here, need we modify all the drivers? > >If you're asking the portion of each driver supporting zero-copy >that needs to be modified, then AFAICS this doesn't change that >very much at all. > >> I think to make skb->head empty at first will cause more effort to pass the >> check with >> skb header. Have I missed something here? I really make the skb->head NULL >> just before kfree(skb) in skb_release_data(), it's done by callback we have >> made for skb. > >No I'm not suggesting you set it to NULL. It should have some >memory allocated, but skb_headlen(skb) should be zero. > >Please have a look at how the napi_gro_frags interface works (e.g., >in drivers/net/cxgb3/sge.c). This is exactly the model that I am >suggesting. > >Cheers, >-- >Visit Openswan at http://www.openswan.org/ >Email: Herbert Xu ~{PmV>HI~} >Home Page: http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt Herbert, I explained what I think the thought in your mind here, please clarify if something missed. 1) Modify driver from netdev_alloc_skb() to alloc user pages if dev is zero-copyed. If the driver support PS mode, then modify alloc_page() too. 2) Add napi_gro_frags() in driver to receive the user pages instead of driver's receiving function. 3) napi_gro_frags() will allocate small skb and pull the header data from the first page to skb->data. Is above the way what you have suggested? I have thought something in detail about the way. 1) The first page will have an offset after the header is copied into allocated kernel skb. The offset should be recalculated when the user page data is transferred to guest. This may modify some of the gro code. 2) napi_gro_frags() may remove a page when it's data is totally be pulled, but we cannot put a user page as normally. This may modify the gro code too. 3) When the user buffer returned to guest, some of them need to be appended a vnet header. That means for some pages, the vnet header room should be reserved when allocated. But we cannot know which one will be used as the first page when allocated. If we reserved vnet header for each page, since the set_skb_frag() in guest driver only use the offset 0 for second pages, then page data will be wrong. 4) Since the user buffer pages should be released, so we still need a dtor callback to do that, and then I still need a place to hold it. How do you think about to put it in skb_shinfo? Currently I can only think of this. How do you think about then? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On >Behalf Of >Xin, Xiaohui >Sent: Saturday, June 12, 2010 5:31 PM >To: Herbert Xu >Cc: Stephen Hemminger; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com >Subject: RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >>-Original Message- >>From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >>Sent: Friday, June 11, 2010 1:21 PM >>To: Xin, Xiaohui >>Cc: Stephen Hemminger; net...@vger.kernel.org; kvm@vger.kernel.org; >>linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >>da...@davemloft.net; >>jd...@linux.intel.com >>Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >>external. >> >>On Wed, Jun 09, 2010 at 05:54:02PM +0800, Xin, Xiaohui wrote: >>> >>> I'm not sure if I understand your way correctly: >>> 1) Does the way only deal with driver with SG feature? Since packet >>> is non-linear... >> >>No the hardware doesn't have to support SG. You just need to >>place the entire packet contents in a page instead of skb->head. >> >>> 2) Is skb->data still pointing to guest user buffers? >>> If yes, how to avoid the modifications to net core change to skb? >> >>skb->data would not point to guest user buffers. In the common >>case the packet is not modified on its way to the guest so this >>is not an issue. >> >>In the rare case where it is modified, you only have to copy the >>bits which are modified and the cost of that is inconsequential >>since you have to write to that memory anyway. >> >>> 3) In our way only parts of drivers need be modified to support zero-copy. >>> and here, need we modify all the drivers? >> >>If you're asking the portion of each driver supporting zero-copy >>that needs to be modified, then AFAICS this doesn't change that >>very much at all. >> >>> I think to make skb->head empty at first will cause more effort to pass the >>> check with >>> skb header. Have I missed something here? I really make the skb->head NULL >>> just before kfree(skb) in skb_release_data(), it's done by callback we have >>> made for skb. >> >>No I'm not suggesting you set it to NULL. It should have some >>memory allocated, but skb_headlen(skb) should be zero. >> >>Please have a look at how the napi_gro_frags interface works (e.g., >>in drivers/net/cxgb3/sge.c). This is exactly the model that I am >>suggesting. >> >>Cheers, >>-- >>Visit Openswan at http://www.openswan.org/ >>Email: Herbert Xu ~{PmV>HI~} >>Home Page: http://gondor.apana.org.au/~herbert/ >>PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt > >Herbert, >I explained what I think the thought in your mind here, please clarify if >something missed. > >1) Modify driver from netdev_alloc_skb() to alloc user pages if dev is >zero-copyed. > If the driver support PS mode, then modify alloc_page() too. >2) Add napi_gro_frags() in driver to receive the user pages instead of >driver's receiving >function. >3) napi_gro_frags() will allocate small skb and pull the header data from >the first page to skb->data. > >Is above the way what you have suggested? >I have thought something in detail about the way. > >1) The first page will have an offset after the header is copied into >allocated kernel skb. >The offset should be recalculated when the user page data is transferred to >guest. This >may modify some of the gro code. > >2) napi_gro_frags() may remove a page when it's data is totally be pulled, but >we cannot >put a user page as normally. This may modify the gro code too. > >3) When the user buffer returned to guest, some of them need to be appended a >vnet header. >That means for some pages, the vnet header room should be reserved when >allocated. >But we cannot know which one will be used as the first page when allocated. If >we reserved >vnet header for each page, since the set_skb_frag() in guest driver only use >the offset 0 for >second pages, then page data will be wrong. > >4) Since the user buffer pages should be released, so we still need a dtor >callback to do that, >and then I still need a place to hold it. How do you think about to put it in >skb_shinfo? > >Currently I can only think of this. >How do you think about then? > >Thanks >Xiaohu
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Thursday, June 17, 2010 7:21 PM >To: Xin, Xiaohui >Cc: Stephen Hemminger; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >On Sun, Jun 13, 2010 at 04:58:36PM +0800, Xin, Xiaohui wrote: >> >> Herbert, >> In this way, I think we should create 3 functions at least in drivers to >> allocate rx buffer, to >receive the rx buffers, and to clean the rx buffers. >> >> We can also have another way here. We can provide a function to only >> substitute >> alloc_page(), and a function to release the pages when cleaning the rx >> buffers. > >Yes that's exactly what I had in mind. > Herbert, I have questions about the idea above: 1) Since netdev_alloc_skb() is still there, and we only modify alloc_page(), then we don't need napi_gro_frags() any more, the driver's original receiving function is ok. Right? 2) Is napi_gro_frags() only suitable for TCP protocol packet? I have done a small test for ixgbe driver to let it only allocate paged buffers and found kernel hangs when napi_gro_frags() receives an ARP packet. 3) As I have mentioned above, with this idea, netdev_alloc_skb() will allocate as usual, the data pointed by skb->data will be copied into the first guest buffer. That means we should reserve sufficient room in guest buffer. For PS mode supported driver (for example ixgbe), the room will be more than 128. After 128bytes, we will put the first frag data. Look into virtio-net.c the function page_to_skb() and receive_mergeable(), that means we should modify guest virtio-net driver to compute the offset as the parameter for skb_set_frag(). How do you think about this? Attached is a patch to how to modify the guest driver. I reserve 512 bytes as an example, and transfer the header len of the skb in hdr->hdr_len. Thanks Xiaohui >Cheers, >-- >Visit Openswan at http://www.openswan.org/ >Email: Herbert Xu ~{PmV>HI~} >Home Page: http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt mod-guest.diff Description: mod-guest.diff
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Friday, June 18, 2010 1:59 PM >To: Xin, Xiaohui >Cc: Stephen Hemminger; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com; Rusty Russell >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >On Fri, Jun 18, 2010 at 01:26:49PM +0800, Xin, Xiaohui wrote: >> >> Herbert, >> I have questions about the idea above: >> 1) Since netdev_alloc_skb() is still there, and we only modify alloc_page(), >> then we don't need napi_gro_frags() any more, the driver's original receiving >> function is ok. Right? > >Well I was actually thinking about converting all drivers that >need this to napi_gro_frags. But now that you mention it, yes >we could still keep the old interface to minimise the work. > >> 2) Is napi_gro_frags() only suitable for TCP protocol packet? >> I have done a small test for ixgbe driver to let it only allocate paged >> buffers >> and found kernel hangs when napi_gro_frags() receives an ARP packet. > >It should work with any packet. In fact, I'm pretty sure the >other drivers (e.g., cxgb3) use that interface for all packets. > Thanks for the verification. By the way, does that mean that nearly all drivers can use the same napi_gro_frags() to receive buffers though currently each driver has it's own receiving function? >> 3) As I have mentioned above, with this idea, netdev_alloc_skb() will >> allocate >> as usual, the data pointed by skb->data will be copied into the first guest >> buffer. >> That means we should reserve sufficient room in guest buffer. For PS mode >> supported driver (for example ixgbe), the room will be more than 128. After >> 128bytes, >> we will put the first frag data. Look into virtio-net.c the function >> page_to_skb() >> and receive_mergeable(), that means we should modify guest virtio-net driver >> to >> compute the offset as the parameter for skb_set_frag(). >> >> How do you think about this? Attached is a patch to how to modify the guest >> driver. >> I reserve 512 bytes as an example, and transfer the header len of the skb in >> hdr->hdr_len. > >Expanding the buffer size to 512 bytes to accomodate PS mode >looks reasonable to me. > >However, I don't think we should increase the copy threshold to >512 bytes at the same time. I don't have any figures myself but >I think if we are to make such a change it should be a separate >one and come with supporting numbers. > Let me have a look to see if I can retain the copy threshold as 128 bytes and copy the header data safely. >Cheers, >-- >Visit Openswan at http://www.openswan.org/ >Email: Herbert Xu ~{PmV>HI~} >Home Page: http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
Herbert, That's why I have sent you the patch for guest virtio-net driver. I reserved 512 bytes in each page, then I can always have the space to copy and avoid the backend memory used up issue. Thanks Xiaohui >-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Thursday, June 24, 2010 6:09 PM >To: Dong, Eddie >Cc: Xin, Xiaohui; Stephen Hemminger; net...@vger.kernel.org; >kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >On Wed, Jun 23, 2010 at 06:05:41PM +0800, Dong, Eddie wrote: >> >> I mean once the frontend side driver post the buffers to the backend driver, >> the backend >driver will "immediately" use that buffers to compose skb or gro_frags and >post them to the >assigned host NIC driver as receive buffers. In that case, if the backend >driver recieves a >packet from the NIC that requires to do copy, it may be unable to find >additional free guest >buffer because all of them are already used by the NIC driver. We have to >reserve some guest >buffers for the possible copy even if the buffer address is not identified by >original skb :( > >OK I see what you mean. Can you tell me how does Xiaohui's >previous patch-set deal with this problem? > >Thanks, >-- >Visit Openswan at http://www.openswan.org/ >Email: Herbert Xu ~{PmV>HI~} >Home Page: http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Provide a zero-copy method on KVM virtio-net.
The idea is simple, just to pin the guest VM user space and then let host NIC driver has the chance to directly DMA to it. The patches are based on vhost-net backend driver. We add a device which provides proto_ops as sendmsg/recvmsg to vhost-net to send/recv directly to/from the NIC driver. KVM guest who use the vhost-net backend may bind any ethX interface in the host side to get copyless data transfer thru guest virtio-net frontend. We provide multiple submits and asynchronous notifiicaton to vhost-net too. Our goal is to improve the bandwidth and reduce the CPU usage. Exact performance data will be provided later. But for simple test with netperf, we found bindwidth up and CPU % up too, but the bindwidth up ratio is much more than CPU % up ratio. What we have not done yet: To support GRO Performance tuning -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] Let host NIC driver to DMA to guest user space.
The patch let host NIC driver to receive user space skb, then the driver has chance to directly DMA to guest user space buffers thru single ethX interface. Signed-off-by: Xin Xiaohui Signed-off-by: Zhao Yu Sigend-off-by: Jeff Dike --- include/linux/netdevice.h | 72 + include/linux/skbuff.h| 32 ++-- net/core/dev.c| 27 + net/core/skbuff.c | 62 +++ 4 files changed, 184 insertions(+), 9 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 94958c1..0de8688 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -486,6 +486,16 @@ struct netdev_queue { } cacheline_aligned_in_smp; +struct netdev_page_ctor{ + int hdr_len; + int data_len; + int npages; + unsignedflags; + struct socket *sock; + struct skb_user_page*(*ctor)(struct netdev_page_ctor *, + struct sk_buff *, int); +}; + /* * This structure defines the management hooks for network devices. * The following hooks can be defined; unless noted otherwise, they are @@ -636,6 +646,8 @@ struct net_device_ops { int (*ndo_fcoe_ddp_done)(struct net_device *dev, u16 xid); #endif + int (*ndo_page_ctor_prep)(struct net_device *dev, + struct netdev_page_ctor *ctor); }; /* @@ -916,6 +928,7 @@ struct net_device /* max exchange id for FCoE LRO by ddp */ unsigned intfcoe_ddp_xid; #endif + struct netdev_page_ctor *page_ctor; }; #define to_net_dev(d) container_of(d, struct net_device, dev) @@ -2013,6 +2026,65 @@ static inline u32 dev_ethtool_get_flags(struct net_device *dev) return 0; return dev->ethtool_ops->get_flags(dev); } + +static inline int netdev_page_ctor_prep(struct net_device *dev, + struct netdev_page_ctor *ctor) +{ + int rc; + int npages, data_len; + const struct net_device_ops *ops = dev->netdev_ops; + + /* needed by packet split */ + if (ops->ndo_page_ctor_prep) { + rc = ops->ndo_page_ctor_prep(dev, ctor); + if (rc) + return rc; + } else { /* should be temp */ + ctor->hdr_len = 128; + ctor->data_len = 2048; + ctor->npages = 1; + } + + if (ctor->hdr_len <= 0) + goto err; + + npages = ctor->npages; + data_len = ctor->data_len; + if (npages <= 0 || npages > MAX_SKB_FRAGS || + (data_len < PAGE_SIZE * (npages - 1) || +data_len > PAGE_SIZE * npages)) + goto err; + + return 0; +err: + dev_warn(&dev->dev, "invalid page constructor parameters\n"); + + return -EINVAL; +} + +static inline int netdev_page_ctor_attach(struct net_device *dev, + struct netdev_page_ctor *ctor) +{ + if (dev->flags & IFF_UP) + return -EBUSY; + + if (rcu_dereference(dev->page_ctor)) + return -EBUSY; + + rcu_assign_pointer(dev->page_ctor, ctor); + + return 0; +} + +static inline void netdev_page_ctor_detach(struct net_device *dev) +{ + if (!rcu_dereference(dev->page_ctor)) + return; + + rcu_assign_pointer(dev->page_ctor, NULL); + synchronize_rcu(); +} + #endif /* __KERNEL__ */ #endif /* _LINUX_NETDEVICE_H */ diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index df7b23a..c77837e 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -209,6 +209,13 @@ struct skb_shared_info { void * destructor_arg; }; +struct skb_user_page { + u8 *start; + int size; + struct skb_frag_struct *frags; + struct skb_shared_info *ushinfo; + void(*dtor)(struct skb_user_page *); +}; /* We divide dataref into two halves. The higher 16 bits hold references * to the payload part of skb->data. The lower 16 bits hold references to * the entire skb->data. A clone of a headerless skb holds the length of @@ -441,17 +448,18 @@ extern void kfree_skb(struct sk_buff *skb); extern void consume_skb(struct sk_buff *skb); extern void __kfree_skb(struct sk_buff *skb); extern struct sk_buff *__alloc_skb(unsigned int size, - gfp_t priority, int fclone, int node); + gfp_t priority, int fclone, + int node, struct net_device *dev); static inline struct sk_buff *alloc_skb(unsigned int size,
[PATCH 1/3] A device for zero-copy based on KVM virtio-net.
Add a device to utilize the vhost-net backend driver for copy-less data transfer between guest FE and host NIC. It pins the guest user space to the host memory and provides proto_ops as sendmsg/recvmsg to vhost-net. Signed-off-by: Xin Xiaohui Signed-off-by: Zhao Yu Sigend-off-by: Jeff Dike --- drivers/vhost/Kconfig |5 + drivers/vhost/Makefile |2 + drivers/vhost/mpassthru.c | 1178 include/linux/miscdevice.h |1 + include/linux/mpassthru.h | 17 + 5 files changed, 1203 insertions(+), 0 deletions(-) create mode 100644 drivers/vhost/mpassthru.c create mode 100644 include/linux/mpassthru.h diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig index 9f409f4..ee32a3b 100644 --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -9,3 +9,8 @@ config VHOST_NET To compile this driver as a module, choose M here: the module will be called vhost_net. +config VHOST_PASSTHRU + tristate "Zerocopy network driver (EXPERIMENTAL)" + depends on VHOST_NET + ---help--- + zerocopy network I/O support diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile index 72dd020..3f79c79 100644 --- a/drivers/vhost/Makefile +++ b/drivers/vhost/Makefile @@ -1,2 +1,4 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o vhost_net-y := vhost.o net.o + +obj-$(CONFIG_VHOST_PASSTHRU) += mpassthru.o diff --git a/drivers/vhost/mpassthru.c b/drivers/vhost/mpassthru.c new file mode 100644 index 000..d8d153f --- /dev/null +++ b/drivers/vhost/mpassthru.c @@ -0,0 +1,1178 @@ +/* + * MPASSTHRU - Mediate passthrough device. + * Copyright (C) 2009 ZhaoYu, XinXiaohui, Dike, Jeffery G + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#define DRV_NAME"mpassthru" +#define DRV_DESCRIPTION "Mediate passthru device driver" +#define DRV_COPYRIGHT "(C) 2009 ZhaoYu, XinXiaohui, Dike, Jeffery G" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "vhost.h" + +/* Uncomment to enable debugging */ +/* #define MPASSTHRU_DEBUG 1 */ + +#ifdef MPASSTHRU_DEBUG +static int debug; + +#define DBG if (mp->debug) printk +#define DBG1 if (debug == 2) printk +#else +#define DBG(a...) +#define DBG1(a...) +#endif + +#define COPY_THRESHOLD (L1_CACHE_BYTES * 4) +#define COPY_HDR_LEN (L1_CACHE_BYTES < 64 ? 64 : L1_CACHE_BYTES) + +struct frag { + u16 offset; + u16 size; +}; + +struct page_ctor { + struct list_headreadq; + int w_len; + int r_len; + spinlock_t read_lock; + atomic_trefcnt; + struct kmem_cache *cache; + struct net_device *dev; + struct netdev_page_ctor ctor; + void*sendctrl; + void*recvctrl; +}; + +struct page_info { + struct list_headlist; + int header; + /* indicate the actual length of bytes +* send/recv in the user space buffers +*/ + int total; + int offset; + struct page *pages[MAX_SKB_FRAGS+1]; + struct skb_frag_struct frag[MAX_SKB_FRAGS+1]; + struct sk_buff *skb; + struct page_ctor*ctor; + + /* The pointer relayed to skb, to indicate +* it's a user space allocated skb or kernel +*/ + struct skb_user_pageuser; + struct skb_shared_info ushinfo; + +#define INFO_READ 0 +#define INFO_WRITE 1 + unsignedflags; + unsignedpnum; + + /* It's meaningful for receive, means +* the max length allowed +*/ + size_t len; + + /* The fields after that is for backend +* driver, now for vhost-net. +*/ + struct vhost_notifier notifier; + unsigned intdesc_pos; + unsigned intlog; + struct iovechdr[VHOST_NET_MAX_SG]; + struct ioveciov[VHOST_NET_MAX_SG]; + void*ctl
[PATCH 2/3] Provides multiple submits and asynchronous notifications.
The vhost-net backend now only supports synchronous send/recv operations. The patch provides multiple submits and asynchronous notifications. This is needed for zero-copy case. Signed-off-by: Xin Xiaohui --- drivers/vhost/net.c | 145 +++-- drivers/vhost/vhost.h | 23 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 22d5fef..8a85227 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -91,6 +92,10 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock) net->tx_poll_state = VHOST_NET_POLL_STARTED; } +static void handle_async_rx_events_notify(struct vhost_net *net, + struct vhost_virtqueue *vq); +static void handle_async_tx_events_notify(struct vhost_net *net, + struct vhost_virtqueue *vq); /* Expects to be always run from workqueue - which acts as * read-size critical section for our kind of RCU. */ static void handle_tx(struct vhost_net *net) @@ -124,6 +129,8 @@ static void handle_tx(struct vhost_net *net) tx_poll_stop(net); hdr_size = vq->hdr_size; + handle_async_tx_events_notify(net, vq); + for (;;) { head = vhost_get_vq_desc(&net->dev, vq, vq->iov, ARRAY_SIZE(vq->iov), @@ -151,6 +158,12 @@ static void handle_tx(struct vhost_net *net) /* Skip header. TODO: support TSO. */ s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out); msg.msg_iovlen = out; + + if (vq->link_state == VHOST_VQ_LINK_ASYNC) { + vq->head = head; + msg.msg_control = (void *)vq; + } + len = iov_length(vq->iov, out); /* Sanity check */ if (!len) { @@ -166,6 +179,10 @@ static void handle_tx(struct vhost_net *net) tx_poll_start(net, sock); break; } + + if (vq->link_state == VHOST_VQ_LINK_ASYNC) + continue; + if (err != len) pr_err("Truncated TX packet: " " len %d != %zd\n", err, len); @@ -177,6 +194,8 @@ static void handle_tx(struct vhost_net *net) } } + handle_async_tx_events_notify(net, vq); + mutex_unlock(&vq->mutex); unuse_mm(net->dev.mm); } @@ -206,7 +225,8 @@ static void handle_rx(struct vhost_net *net) int err; size_t hdr_size; struct socket *sock = rcu_dereference(vq->private_data); - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue)) + if (!sock || (skb_queue_empty(&sock->sk->sk_receive_queue) && + vq->link_state == VHOST_VQ_LINK_SYNC)) return; use_mm(net->dev.mm); @@ -217,6 +237,8 @@ static void handle_rx(struct vhost_net *net) vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ? vq->log : NULL; + handle_async_rx_events_notify(net, vq); + for (;;) { head = vhost_get_vq_desc(&net->dev, vq, vq->iov, ARRAY_SIZE(vq->iov), @@ -245,6 +267,11 @@ static void handle_rx(struct vhost_net *net) s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in); msg.msg_iovlen = in; len = iov_length(vq->iov, in); + if (vq->link_state == VHOST_VQ_LINK_ASYNC) { + vq->head = head; + vq->_log = log; + msg.msg_control = (void *)vq; + } /* Sanity check */ if (!len) { vq_err(vq, "Unexpected header len for RX: " @@ -259,6 +286,10 @@ static void handle_rx(struct vhost_net *net) vhost_discard_vq_desc(vq); break; } + + if (vq->link_state == VHOST_VQ_LINK_ASYNC) + continue; + /* TODO: Should check and handle checksum. */ if (err > len) { pr_err("Discarded truncated rx packet: " @@ -284,10 +315,83 @@ static void handle_rx(struct vhost_net *net) } } + handle_async_rx_events_notify(net, vq); + mutex_unlock(&vq->mutex); unuse_mm(net->dev.mm); } +struct vhost_notifier *notify_dequeue(struct vhost_virtqueue *vq) +{ + struct vhost_notifier *vnotify = NULL; + unsigned long flags; + + s
RE: [PATCH 1/3] A device for zero-copy based on KVM virtio-net.
Eric, Thanks. I will look into that. But don't stop there. Please comments more. :-) Thanks Xiaohui -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] Sent: Wednesday, February 10, 2010 11:18 PM To: Xin, Xiaohui Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; m...@redhat.com; jd...@c2.user-mode-linux.org; Zhao Yu Subject: Re: [PATCH 1/3] A device for zero-copy based on KVM virtio-net. Le mercredi 10 février 2010 à 19:48 +0800, Xin Xiaohui a écrit : > Add a device to utilize the vhost-net backend driver for > copy-less data transfer between guest FE and host NIC. > It pins the guest user space to the host memory and > provides proto_ops as sendmsg/recvmsg to vhost-net. > > Signed-off-by: Xin Xiaohui > Signed-off-by: Zhao Yu > Sigend-off-by: Jeff Dike > +static int page_ctor_attach(struct mp_struct *mp) > +{ > + int rc; > + struct page_ctor *ctor; > + struct net_device *dev = mp->dev; > + > + rcu_read_lock(); > + if (rcu_dereference(mp->ctor)) { > + rcu_read_unlock(); > + return -EBUSY; > + } > + rcu_read_unlock(); Strange read locking here, for an obvious writer role. What do you really want to do ? If writer are serialized by mp_mutex, you dont need this recu_read_lock()/rcu_read_unlock() stuff. > + > + ctor = kzalloc(sizeof(*ctor), GFP_KERNEL); > + if (!ctor) > + return -ENOMEM; > + rc = netdev_page_ctor_prep(dev, &ctor->ctor); > + if (rc) > + goto fail; > + > + ctor->cache = kmem_cache_create("skb_page_info", > + sizeof(struct page_info), 0, > + SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL); SLAB_PANIC here means : crash whole system in case of error. This is not what you want in a driver. > + > + if (!ctor->cache) > + goto cache_fail; > + > + INIT_LIST_HEAD(&ctor->readq); > + spin_lock_init(&ctor->read_lock); > + > + ctor->w_len = 0; > + ctor->r_len = 0; > + > + dev_hold(dev); > + ctor->dev = dev; > + ctor->ctor.ctor = page_ctor; > + ctor->ctor.sock = &mp->socket; > + atomic_set(&ctor->refcnt, 1); > + > + rc = netdev_page_ctor_attach(dev, &ctor->ctor); > + if (rc) > + goto fail; > + > + /* locked by mp_mutex */ > + rcu_assign_pointer(mp->ctor, ctor); > + > + /* XXX:Need we do set_offload here ? */ > + > + return 0; > + > +fail: > + kmem_cache_destroy(ctor->cache); > +cache_fail: > + kfree(ctor); > + dev_put(dev); > + > + return rc; > +} > + > + > +static inline void get_page_ctor(struct page_ctor *ctor) > +{ > + atomic_inc(&ctor->refcnt); > +} > + > +static inline void put_page_ctor(struct page_ctor *ctor) > +{ > + if (atomic_dec_and_test(&ctor->refcnt)) > + kfree(ctor); Are you sure a RCU grace period is not needed before freeing ? > + > +static int page_ctor_detach(struct mp_struct *mp) > +{ > + struct page_ctor *ctor; > + struct page_info *info; > + int i; > + > + rcu_read_lock(); > + ctor = rcu_dereference(mp->ctor); > + rcu_read_unlock(); Strange locking again here > + > + if (!ctor) > + return -ENODEV; > + > + while ((info = info_dequeue(ctor))) { > + for (i = 0; i < info->pnum; i++) > + if (info->pages[i]) > + put_page(info->pages[i]); > + kmem_cache_free(ctor->cache, info); > + } > + kmem_cache_destroy(ctor->cache); > + netdev_page_ctor_detach(ctor->dev); > + dev_put(ctor->dev); > + > + /* locked by mp_mutex */ > + rcu_assign_pointer(mp->ctor, NULL); > + synchronize_rcu(); > + > + put_page_ctor(ctor); > + > + return 0; > +} > + > +/* For small user space buffers transmit, we don't need to call > + * get_user_pages(). > + */ > +static struct page_info *alloc_small_page_info(struct page_ctor *ctor, > + int total) > +{ > + struct page_info *info = kmem_cache_alloc(ctor->cache, GFP_KERNEL); kmem_cache_zalloc() ? > + > + if (!info) > + return NULL; > + memset(info, 0, sizeof(struct page_info)); > + memset(info->pages, 0, sizeof(info->pages)); redundant memset() whole structure already cleared one line above > + > + info->header = 0; already cleared > + info->total = total; > + info->skb = NULL; already cleared > > +
RE: [PATCH 0/3] Provide a zero-copy method on KVM virtio-net.
>>On Wednesday 10 February 2010, Xin Xiaohui wrote: > >The idea is simple, just to pin the guest VM user space and then > >let host NIC driver has the chance to directly DMA to it. > >The patches are based on vhost-net backend driver. We add a device > >which provides proto_ops as sendmsg/recvmsg to vhost-net to > >send/recv directly to/from the NIC driver. KVM guest who use the > >vhost-net backend may bind any ethX interface in the host side to > >get copyless data transfer thru guest virtio-net frontend. >> >> We provide multiple submits and asynchronous notifiicaton to > >vhost-net too. >This does a lot of things that I had planned for macvtap. It's >great to hear that you have made this much progress. > >However, I'd hope that we could combine this with the macvtap driver, >which would give us zero-copy transfer capability both with and >without vhost, as well as (tx at least) when using multiple guests >on a macvlan setup. You mean the zero-copy can work with macvtap driver without vhost. May you give me some detailed info about your macvtap driver and the relationship between vhost and macvtap to make me have a clear picture then? >For transmit, it should be fairly straightforward to hook up >your zero-copy method and the vhost-net interface into the >macvtap driver. > >You have simplified the receiv path significantly by assuming >that the entire netdev can receive into a single guest, right? Yes. >I'm assuming that the idea is to allow VMDq adapters to simply >show up as separate adapters and have the driver handle this >in a hardware specific way. Does the VMDq driver do so now? >My plan for this was to instead move support for VMDq into the >macvlan driver so we can transparently use VMDq on hardware where >available, including zero-copy receives, but fall back to software >operation on non-VMDq hardware. Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 0/3] Provide a zero-copy method on KVM virtio-net.
Will be in a vacation during 2/13~2/20, so email may be very slow or no replied for your comments. But please don't hesitate to comment more, and I will address them after the vacation. :-) Thanks Xiaohui -Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Xin Xiaohui Sent: Wednesday, February 10, 2010 7:49 PM To: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; mi...@elte.hu; m...@redhat.com; jd...@c2.user-mode-linux.org Subject: [PATCH 0/3] Provide a zero-copy method on KVM virtio-net. The idea is simple, just to pin the guest VM user space and then let host NIC driver has the chance to directly DMA to it. The patches are based on vhost-net backend driver. We add a device which provides proto_ops as sendmsg/recvmsg to vhost-net to send/recv directly to/from the NIC driver. KVM guest who use the vhost-net backend may bind any ethX interface in the host side to get copyless data transfer thru guest virtio-net frontend. We provide multiple submits and asynchronous notifiicaton to vhost-net too. Our goal is to improve the bandwidth and reduce the CPU usage. Exact performance data will be provided later. But for simple test with netperf, we found bindwidth up and CPU % up too, but the bindwidth up ratio is much more than CPU % up ratio. What we have not done yet: To support GRO Performance tuning -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 0/3] Provide a zero-copy method on KVM virtio-net.
>On Sat, Mar 06, 2010 at 05:38:35PM +0800, xiaohui@intel.com wrote: >> The idea is simple, just to pin the guest VM user space and then > >let host NIC driver has the chance to directly DMA to it. > >The patches are based on vhost-net backend driver. We add a device > >which provides proto_ops as sendmsg/recvmsg to vhost-net to > >send/recv directly to/from the NIC driver. KVM guest who use the > >vhost-net backend may bind any ethX interface in the host side to > >get copyless data transfer thru guest virtio-net frontend. > > > >We provide multiple submits and asynchronous notifiicaton to > >vhost-net too. > > > >Our goal is to improve the bandwidth and reduce the CPU usage. > >Exact performance data will be provided later. But for simple > >test with netperf, we found bindwidth up and CPU % up too, > >but the bindwidth up ratio is much more than CPU % up ratio. > > > >What we have not done yet: > > packet split support > > To support GRO > > Performance tuning > > >Am I right to say that nic driver needs changes for these patches >to work? If so, please publish nic driver patches as well. For drivers not support packet split mode, the NIC drivers don't need to change. For packet split support drivers, we plan to add the drivers API in updated versions. Now for PS support drivers, just disable the PS mode, it also works. > > what we have done in v1: > > polish the RCU usage > > deal with write logging in asynchroush mode in vhost > > add notifier block for mp device > > rename page_ctor to mp_port in netdevice.h to make it looks generic > > add mp_dev_change_flags() for mp device to change NIC state > > add CONIFG_VHOST_MPASSTHRU to limit the usage when module is not load > > a small fix for missing dev_put when fail > > using dynamic minor instead of static minor number > > a __KERNEL__ protect to mp_get_sock() > > > >performance: > > using netperf with GSO/TSO disabled, 10G NIC, > > disabled packet split mode, with raw socket case compared to vhost. > > > > bindwidth will be from 1.1Gbps to 1.7Gbps > > CPU % from 120%-140% to 140%-160% > That's pretty low for a 10Gb nic. Are you hitting some other bottleneck, > like high interrupt rate? Also, GSO support and performance tuning > for raw are incomplete. Try comparing with e.g. tap with GSO. I'm curious too. I have tested vhost-net without zero-copy patch at first in RAW socket case with ixgbe driver, with that driver GRO feature is enabled default, but netperf data is extremely low, after disabled GRO, then I can get more than 1Gbps. So I thought I have missed something there, but I had send 2 emails to you about this before and got no reply from you. Have you got some perf data in raw socket case with vhost-net? The data I have got from your web page is always tap with GSO case. If GSO is not supported, I think the data cannot compare with tap with GSO case in 1500 MTU. Maybe mergable buffers may help the performance in raw socket case? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
>> +/* The structure to notify the virtqueue for async socket */ >> +struct vhost_notifier { >> +struct list_head list; > >+struct vhost_virtqueue *vq; > >+int head; > >+int size; > >+int log; > >+void *ctrl; > >+void (*dtor)(struct vhost_notifier *); > >+}; > >+ >So IMO, this is not the best interface between vhost >and your driver, exposing them to each other unnecessarily. > >If you think about it, your driver should not care about this structure. >It could get e.g. a kiocb (sendmsg already gets one), and call ki_dtor >on completion. vhost could save it's state in ki_user_data. If your >driver needs to add more data to do more tracking, I think it can put >skb pointer in the private pointer. Then if I remove the struct vhost_notifier, and just use struct kiocb, but don't use the one got from sendmsg or recvmsg, but allocated within the page_info structure, and don't implement any aio logic related to it, is that ok? Sorry, I made a patch, but don't know how to reply mail with a good formatted patch here Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re:[PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
The vhost-net backend now only supports synchronous send/recv operations. The patch provides multiple submits and asynchronous notifications. This is needed for zero-copy case. Signed-off-by: Xin Xiaohui --- Michael, I don't use the kiocb comes from the sendmsg/recvmsg, since I have embeded the kiocb in page_info structure, and allocate it when page_info allocated. Please have a review and thanks for the instruction for replying email which helps me a lot. Thanks, Xiaohui drivers/vhost/net.c | 159 +++-- drivers/vhost/vhost.h | 12 2 files changed, 166 insertions(+), 5 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 22d5fef..5483848 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -17,11 +17,13 @@ #include #include #include +#include #include #include #include #include +#include #include @@ -91,6 +93,12 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock) net->tx_poll_state = VHOST_NET_POLL_STARTED; } +static void handle_async_rx_events_notify(struct vhost_net *net, + struct vhost_virtqueue *vq); + +static void handle_async_tx_events_notify(struct vhost_net *net, + struct vhost_virtqueue *vq); + /* Expects to be always run from workqueue - which acts as * read-size critical section for our kind of RCU. */ static void handle_tx(struct vhost_net *net) @@ -124,6 +132,8 @@ static void handle_tx(struct vhost_net *net) tx_poll_stop(net); hdr_size = vq->hdr_size; + handle_async_tx_events_notify(net, vq); + for (;;) { head = vhost_get_vq_desc(&net->dev, vq, vq->iov, ARRAY_SIZE(vq->iov), @@ -151,6 +161,12 @@ static void handle_tx(struct vhost_net *net) /* Skip header. TODO: support TSO. */ s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out); msg.msg_iovlen = out; + + if (vq->link_state == VHOST_VQ_LINK_ASYNC) { + vq->head = head; + msg.msg_control = (void *)vq; + } + len = iov_length(vq->iov, out); /* Sanity check */ if (!len) { @@ -166,6 +182,10 @@ static void handle_tx(struct vhost_net *net) tx_poll_start(net, sock); break; } + + if (vq->link_state == VHOST_VQ_LINK_ASYNC) + continue; + if (err != len) pr_err("Truncated TX packet: " " len %d != %zd\n", err, len); @@ -177,6 +197,8 @@ static void handle_tx(struct vhost_net *net) } } + handle_async_tx_events_notify(net, vq); + mutex_unlock(&vq->mutex); unuse_mm(net->dev.mm); } @@ -206,7 +228,8 @@ static void handle_rx(struct vhost_net *net) int err; size_t hdr_size; struct socket *sock = rcu_dereference(vq->private_data); - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue)) + if (!sock || (skb_queue_empty(&sock->sk->sk_receive_queue) && + vq->link_state == VHOST_VQ_LINK_SYNC)) return; use_mm(net->dev.mm); @@ -214,9 +237,18 @@ static void handle_rx(struct vhost_net *net) vhost_disable_notify(vq); hdr_size = vq->hdr_size; - vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ? + /* In async cases, for write logging, the simple way is to get +* the log info always, and really logging is decided later. +* Thus, when logging enabled, we can get log, and when logging +* disabled, we can get log disabled accordingly. +*/ + + vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) | + (vq->link_state == VHOST_VQ_LINK_ASYNC) ? vq->log : NULL; + handle_async_rx_events_notify(net, vq); + for (;;) { head = vhost_get_vq_desc(&net->dev, vq, vq->iov, ARRAY_SIZE(vq->iov), @@ -245,6 +277,11 @@ static void handle_rx(struct vhost_net *net) s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in); msg.msg_iovlen = in; len = iov_length(vq->iov, in); + if (vq->link_state == VHOST_VQ_LINK_ASYNC) { + vq->head = head; + vq->_log = log; + msg.msg_control = (void *)vq; + } /* Sanity check */ if (!len) { vq_err(vq, "Unexpected
RE: [PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
>> Michael, >> I don't use the kiocb comes from the sendmsg/recvmsg, > >since I have embeded the kiocb in page_info structure, > >and allocate it when page_info allocated. >So what I suggested was that vhost allocates and tracks the iocbs, and >passes them to your device with sendmsg/ recvmsg calls. This way your >device won't need to share structures and locking strategy with vhost: >you get an iocb, handle it, invoke a callback to notify vhost about >completion. >This also gets rid of the 'receiver' callback I'm not sure receiver callback can be removed here: The patch describes a work flow like this: netif_receive_skb() gets the packet, it does nothing but just queue the skb and wakeup the handle_rx() of vhost. handle_rx() then calls the receiver callback to deal with skb and and get the necessary notify info into a list, vhost owns the list and in the same handle_rx() context use it to complete. We use "receiver" callback here is because only handle_rx() is waked up from netif_receive_skb(), and we need mp device context to deal with the skb and notify info attached to it. We also have some lock in the callback function. If I remove the receiver callback, I can only deal with the skb and notify info in netif_receive_skb(), but this function is in an interrupt context, which I think lock is not allowed there. But I cannot remove the lock there. >> Please have a review and thanks for the instruction >> for replying email which helps me a lot. >> > >Thanks, > >Xiaohui > > > > drivers/vhost/net.c | 159 > > +++-- >> drivers/vhost/vhost.h | 12 >> 2 files changed, 166 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > >index 22d5fef..5483848 100644 > >--- a/drivers/vhost/net.c > >+++ b/drivers/vhost/net.c > >@@ -17,11 +17,13 @@ > > #include > > #include > > #include > >+#include > > > > #include > > #include > > #include > > #include > >+#include > > > > #include > > > >@@ -91,6 +93,12 @@ static void tx_poll_start(struct vhost_net *net, struct > >socket *sock) > > net->tx_poll_state = VHOST_NET_POLL_STARTED; > > } > > > >+static void handle_async_rx_events_notify(struct vhost_net *net, > >+struct vhost_virtqueue *vq); > >+ > >+static void handle_async_tx_events_notify(struct vhost_net *net, > >+struct vhost_virtqueue *vq); > >+ >A couple of style comments: > >- It's better to arrange functions in such order that forward declarations >aren't necessary. Since we don't have recursion, this should always be >possible. >- continuation lines should be idented at least at the position of '(' >on the previous line. Thanks. I'd correct that. >> /* Expects to be always run from workqueue - which acts as >> * read-size critical section for our kind of RCU. */ >> static void handle_tx(struct vhost_net *net) >> @@ -124,6 +132,8 @@ static void handle_tx(struct vhost_net *net) >> tx_poll_stop(net); >> hdr_size = vq->hdr_size; >> >> +handle_async_tx_events_notify(net, vq); > >+ >> for (;;) { >> head = vhost_get_vq_desc(&net->dev, vq, vq->iov, >> ARRAY_SIZE(vq->iov), > >@@ -151,6 +161,12 @@ static void handle_tx(struct vhost_net *net) >> /* Skip header. TODO: support TSO. */ >> s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out); >> msg.msg_iovlen = out; > >+ > >+if (vq->link_state == VHOST_VQ_LINK_ASYNC) { > >+vq->head = head; > >+msg.msg_control = (void *)vq; >So here a device gets a pointer to vhost_virtqueue structure. If it gets >an iocb and invokes a callback, it would not care about vhost internals. >> +} >> + >> len = iov_length(vq->iov, out); >> /* Sanity check */ >> if (!len) { >> @@ -166,6 +182,10 @@ static void handle_tx(struct vhost_net *net) >> tx_poll_start(net, sock); >> break; >> } >> + >> +if (vq->link_state == VHOST_VQ_LINK_ASYNC) >> +continue; >>+ >> if (err != len) >> pr_err("Truncated TX packet: " >> " len %d != %zd\n", err, len); >> @@ -177,6 +197,8 @@ static void handle_tx(struct vhost_net *net) >> } >> } >> >> +handle_async_tx_events_notify(net, vq); >> + >> mutex_unlock(&vq->mutex); >> unuse_mm(net->dev.mm); >> } >>@@ -206,7 +228,8 @@ static void handle_rx(struct vhost_net *net) >> int err; >> size_t hdr_size; >> struct socket *sock = rcu_dereference(vq->private_data); >> -if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue)) >> +if (!sock || (skb_queue_empty(&sock->sk->sk_receive_queue) && >> +vq->link_state == VHOST_VQ_LINK_SYNC)) >>
Re:[PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
The vhost-net backend now only supports synchronous send/recv operations. The patch provides multiple submits and asynchronous notifications. This is needed for zero-copy case. Signed-off-by: Xin Xiaohui --- Michael, Now, I made vhost to alloc/destroy the kiocb, and transfer it from sendmsg/recvmsg. I did not remove vq->receiver, since what the callback does is related to the structures owned by mp device, and I think isolation them to vhost is a good thing to us all. And it will not prevent mp device to be independent of vhost in future. Later, when mp device can be a real device which provides asynchronous read/write operations and not just report proto_ops, it will use another callback function which is not related to vhost at all. For the write logging, do you have a function in hand that we can recompute the log? If that, I think I can use it to recompute the log info when the logging is suddenly enabled. For the outstanding requests, do you mean all the user buffers have submitted before the logging ioctl changed? That may be a lot, and some of them are still in NIC ring descriptors. Waiting them to be finished may be need some time. I think when logging ioctl changed, then the logging is changed just after that is also reasonable. Thanks Xiaohui drivers/vhost/net.c | 189 +++-- drivers/vhost/vhost.h | 10 +++ 2 files changed, 192 insertions(+), 7 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 22d5fef..2aafd90 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -17,11 +17,13 @@ #include #include #include +#include #include #include #include #include +#include #include @@ -47,6 +49,7 @@ struct vhost_net { struct vhost_dev dev; struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX]; struct vhost_poll poll[VHOST_NET_VQ_MAX]; + struct kmem_cache *cache; /* Tells us whether we are polling a socket for TX. * We only do this when socket buffer fills up. * Protected by tx vq lock. */ @@ -91,11 +94,88 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock) net->tx_poll_state = VHOST_NET_POLL_STARTED; } +struct kiocb *notify_dequeue(struct vhost_virtqueue *vq) +{ + struct kiocb *iocb = NULL; + unsigned long flags; + + spin_lock_irqsave(&vq->notify_lock, flags); + if (!list_empty(&vq->notifier)) { + iocb = list_first_entry(&vq->notifier, + struct kiocb, ki_list); + list_del(&iocb->ki_list); + } + spin_unlock_irqrestore(&vq->notify_lock, flags); + return iocb; +} + +static void handle_async_rx_events_notify(struct vhost_net *net, + struct vhost_virtqueue *vq) +{ + struct kiocb *iocb = NULL; + struct vhost_log *vq_log = NULL; + int rx_total_len = 0; + int log, size; + + if (vq->link_state != VHOST_VQ_LINK_ASYNC) + return; + + if (vq->receiver) + vq->receiver(vq); + + vq_log = unlikely(vhost_has_feature( + &net->dev, VHOST_F_LOG_ALL)) ? vq->log : NULL; + while ((iocb = notify_dequeue(vq)) != NULL) { + vhost_add_used_and_signal(&net->dev, vq, + iocb->ki_pos, iocb->ki_nbytes); + log = (int)iocb->ki_user_data; + size = iocb->ki_nbytes; + rx_total_len += iocb->ki_nbytes; + + if (iocb->ki_dtor) + iocb->ki_dtor(iocb); + kmem_cache_free(net->cache, iocb); + + if (unlikely(vq_log)) + vhost_log_write(vq, vq_log, log, size); + if (unlikely(rx_total_len >= VHOST_NET_WEIGHT)) { + vhost_poll_queue(&vq->poll); + break; + } + } +} + +static void handle_async_tx_events_notify(struct vhost_net *net, + struct vhost_virtqueue *vq) +{ + struct kiocb *iocb = NULL; + int tx_total_len = 0; + + if (vq->link_state != VHOST_VQ_LINK_ASYNC) + return; + + while ((iocb = notify_dequeue(vq)) != NULL) { + vhost_add_used_and_signal(&net->dev, vq, + iocb->ki_pos, 0); + tx_total_len += iocb->ki_nbytes; + + if (iocb->ki_dtor) + iocb->ki_dtor(iocb); + + kmem_cache_free(net->cache, iocb); + if (unlikely(tx_total_len >= VHOST_NET_WEIGHT)) { + vhost_poll_queue(&vq->poll); + break; + } + } +} + /* Expects to be always run from workqueue - which acts as * read-size crit
Re:[PATCH 1/3] A device for zero-copy based on KVM virtio-net.
Add a device to utilize the vhost-net backend driver for copy-less data transfer between guest FE and host NIC. It pins the guest user space to the host memory and provides proto_ops as sendmsg/recvmsg to vhost-net. Signed-off-by: Xin Xiaohui Signed-off-by: Zhao Yu Sigend-off-by: Jeff Dike --- Micheal, Sorry, I did not resolve all your comments this time. I did not move the device out of vhost directory because I did not implement real asynchronous read/write operations to mp device for now, We wish we can do this after the network code checked in. For the DOS issue, I'm not sure how much the limit get_user_pages() can pin is reasonable, should we compute the bindwidth to make it? We use get_user_pages_fast() and use set_page_dirty_lock(). Remove read_rcu_lock()/unlock(), since the ctor pointer is only changed by BIND/UNBIND ioctl, and during that time, the NIC is always stoped, all outstanding requests are done, so the ctor pointer cannot be raced into wrong condition. Qemu needs a userspace write, is that a synchronous one or asynchronous one? Thanks Xiaohui drivers/vhost/Kconfig |5 + drivers/vhost/Makefile|2 + drivers/vhost/mpassthru.c | 1162 + include/linux/mpassthru.h | 29 ++ 4 files changed, 1198 insertions(+), 0 deletions(-) create mode 100644 drivers/vhost/mpassthru.c create mode 100644 include/linux/mpassthru.h diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig index 9f409f4..ee32a3b 100644 --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -9,3 +9,8 @@ config VHOST_NET To compile this driver as a module, choose M here: the module will be called vhost_net. +config VHOST_PASSTHRU + tristate "Zerocopy network driver (EXPERIMENTAL)" + depends on VHOST_NET + ---help--- + zerocopy network I/O support diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile index 72dd020..3f79c79 100644 --- a/drivers/vhost/Makefile +++ b/drivers/vhost/Makefile @@ -1,2 +1,4 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o vhost_net-y := vhost.o net.o + +obj-$(CONFIG_VHOST_PASSTHRU) += mpassthru.o diff --git a/drivers/vhost/mpassthru.c b/drivers/vhost/mpassthru.c new file mode 100644 index 000..6e8fc4d --- /dev/null +++ b/drivers/vhost/mpassthru.c @@ -0,0 +1,1162 @@ +/* + * MPASSTHRU - Mediate passthrough device. + * Copyright (C) 2009 ZhaoYu, XinXiaohui, Dike, Jeffery G + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#define DRV_NAME"mpassthru" +#define DRV_DESCRIPTION "Mediate passthru device driver" +#define DRV_COPYRIGHT "(C) 2009 ZhaoYu, XinXiaohui, Dike, Jeffery G" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "vhost.h" + +/* Uncomment to enable debugging */ +/* #define MPASSTHRU_DEBUG 1 */ + +#ifdef MPASSTHRU_DEBUG +static int debug; + +#define DBG if (mp->debug) printk +#define DBG1 if (debug == 2) printk +#else +#define DBG(a...) +#define DBG1(a...) +#endif + +#define COPY_THRESHOLD (L1_CACHE_BYTES * 4) +#define COPY_HDR_LEN (L1_CACHE_BYTES < 64 ? 64 : L1_CACHE_BYTES) + +struct frag { + u16 offset; + u16 size; +}; + +struct page_ctor { + struct list_headreadq; + int w_len; + int r_len; + spinlock_t read_lock; + struct kmem_cache *cache; + struct net_device *dev; + struct mpassthru_port port; +}; + +struct page_info { + void*ctrl; + struct list_headlist; + int header; + /* indicate the actual length of bytes +* send/recv in the user space buffers +*/ + int total; + int offset; + struct page *pages[MAX_SKB_FRAGS+1]; + struct skb_frag_struct frag[MAX_SKB_FRAGS+1]; + struct sk_buff *skb; + struct page_ctor*ctor; + + /* The pointer relayed to skb, to indicate +* it's a user space allocated skb or kernel +*/ + struct skb_user_pageuser; + struct skb_shared_info ushinf
RE: [PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
>> For the write logging, do you have a function in hand that we can >> recompute the log? If that, I think I can use it to recompute the >>log info when the logging is suddenly enabled. >> For the outstanding requests, do you mean all the user buffers have >>submitted before the logging ioctl changed? That may be a lot, and >> some of them are still in NIC ring descriptors. Waiting them to be >>finished may be need some time. I think when logging ioctl changed, >> then the logging is changed just after that is also reasonable. >The key point is that after loggin ioctl returns, any >subsequent change to memory must be logged. It does not >matter when was the request submitted, otherwise we will >get memory corruption on migration. The change to memory happens when vhost_add_used_and_signal(), right? So after ioctl returns, just recompute the log info to the events in the async queue, is ok. Since the ioctl and write log operations are all protected by vq->mutex. Thanks Xiaohui > Thanks > Xiaohui > > drivers/vhost/net.c | 189 > +++-- > drivers/vhost/vhost.h | 10 +++ > 2 files changed, 192 insertions(+), 7 deletions(-) > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > index 22d5fef..2aafd90 100644 > --- a/drivers/vhost/net.c > +++ b/drivers/vhost/net.c > @@ -17,11 +17,13 @@ > #include > #include > #include > +#include > > #include > #include > #include > #include > +#include > > #include > > @@ -47,6 +49,7 @@ struct vhost_net { > struct vhost_dev dev; > struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX]; > struct vhost_poll poll[VHOST_NET_VQ_MAX]; > + struct kmem_cache *cache; > /* Tells us whether we are polling a socket for TX. >* We only do this when socket buffer fills up. >* Protected by tx vq lock. */ > @@ -91,11 +94,88 @@ static void tx_poll_start(struct vhost_net *net, struct > socket *sock) > net->tx_poll_state = VHOST_NET_POLL_STARTED; > } > > +struct kiocb *notify_dequeue(struct vhost_virtqueue *vq) > +{ > + struct kiocb *iocb = NULL; > + unsigned long flags; > + > + spin_lock_irqsave(&vq->notify_lock, flags); > + if (!list_empty(&vq->notifier)) { > + iocb = list_first_entry(&vq->notifier, > + struct kiocb, ki_list); > + list_del(&iocb->ki_list); > + } > + spin_unlock_irqrestore(&vq->notify_lock, flags); > + return iocb; > +} > + > +static void handle_async_rx_events_notify(struct vhost_net *net, > + struct vhost_virtqueue *vq) > +{ > + struct kiocb *iocb = NULL; > + struct vhost_log *vq_log = NULL; > + int rx_total_len = 0; > + int log, size; > + > + if (vq->link_state != VHOST_VQ_LINK_ASYNC) > + return; > + > + if (vq->receiver) > + vq->receiver(vq); > + > + vq_log = unlikely(vhost_has_feature( > + &net->dev, VHOST_F_LOG_ALL)) ? vq->log : NULL; > + while ((iocb = notify_dequeue(vq)) != NULL) { > + vhost_add_used_and_signal(&net->dev, vq, > + iocb->ki_pos, iocb->ki_nbytes); > + log = (int)iocb->ki_user_data; > + size = iocb->ki_nbytes; > + rx_total_len += iocb->ki_nbytes; > + > + if (iocb->ki_dtor) > + iocb->ki_dtor(iocb); > + kmem_cache_free(net->cache, iocb); > + > + if (unlikely(vq_log)) > + vhost_log_write(vq, vq_log, log, size); > + if (unlikely(rx_total_len >= VHOST_NET_WEIGHT)) { > + vhost_poll_queue(&vq->poll); > + break; > + } > + } > +} > + > +static void handle_async_tx_events_notify(struct vhost_net *net, > + struct vhost_virtqueue *vq) > +{ > + struct kiocb *iocb = NULL; > + int tx_total_len = 0; > + > + if (vq->link_state != VHOST_VQ_LINK_ASYNC) > + return; > + > + while ((iocb = notify_dequeue(vq)) != NULL) { > + vhost_add_used_and_signal(&net->dev, vq, > + iocb->ki_pos, 0); > + tx_total_len += iocb->ki_nbytes; > + > + if (iocb->ki_dtor) > + iocb->ki_dtor(iocb); > + > + kmem_cache_free(net->cache, iocb); > + if (unlikely(tx_total_len >= VHOST_NET_WEIGHT)) { > + vhost_poll_queue(&vq->poll); > + break; > + } > + } > +} > + > /* Expects to be always run from workqueue - which acts as > * read-size critical section for our kind of RCU. */ > static void handle_tx(struct vhost_net *net) > { > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX]; > + struct kiocb *iocb = NULL; > unsigned head, out, in, s; > struct msghdr msg = { > .msg_name = NULL, > @@ -124,
RE: [PATCH 1/3] A device for zero-copy based on KVM virtio-net.
Michael, >> >>For the DOS issue, I'm not sure how much the limit get_user_pages() >> can pin is reasonable, should we compute the bindwidth to make it? >There's a ulimit for locked memory. Can we use this, decreasing >the value for rlimit array? We can do this when backend is >enabled and re-increment when backend is disabled. I have tried it with rlim[RLIMIT_MEMLOCK].rlim_cur, but I found the initial value for it is 0x1, after right shift PAGE_SHIFT, it's only 16 pages we can lock then, it seems too small, since the guest virito-net driver may submit a lot requests one time. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
Michael, > >>> For the write logging, do you have a function in hand that we can > >>> recompute the log? If that, I think I can use it to recompute the > >>>log info when the logging is suddenly enabled. > >>> For the outstanding requests, do you mean all the user buffers have > >>>submitted before the logging ioctl changed? That may be a lot, and > >> >some of them are still in NIC ring descriptors. Waiting them to be > >>>finished may be need some time. I think when logging ioctl changed, > >> >then the logging is changed just after that is also reasonable. > >>The key point is that after loggin ioctl returns, any > >>subsequent change to memory must be logged. It does not > >>matter when was the request submitted, otherwise we will > >>get memory corruption on migration. > >The change to memory happens when vhost_add_used_and_signal(), right? > >So after ioctl returns, just recompute the log info to the events in the > >async queue, > >is ok. Since the ioctl and write log operations are all protected by > >vq->mutex. >> Thanks >> Xiaohui >Yes, I think this will work. Thanks, so do you have the function to recompute the log info in your hand that I can use? I have weakly remembered that you have noticed it before some time. > > Thanks > > Xiaohui > > > > drivers/vhost/net.c | 189 > > +++-- > > drivers/vhost/vhost.h | 10 +++ > > 2 files changed, 192 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > > index 22d5fef..2aafd90 100644 > > --- a/drivers/vhost/net.c > > +++ b/drivers/vhost/net.c > > @@ -17,11 +17,13 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -47,6 +49,7 @@ struct vhost_net { > > struct vhost_dev dev; > > struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX]; > > struct vhost_poll poll[VHOST_NET_VQ_MAX]; > > + struct kmem_cache *cache; > > /* Tells us whether we are polling a socket for TX. > > * We only do this when socket buffer fills up. > > * Protected by tx vq lock. */ > > @@ -91,11 +94,88 @@ static void tx_poll_start(struct vhost_net *net, struct > > socket *sock) > > net->tx_poll_state = VHOST_NET_POLL_STARTED; > > } > > > > +struct kiocb *notify_dequeue(struct vhost_virtqueue *vq) > > +{ > > + struct kiocb *iocb = NULL; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&vq->notify_lock, flags); > > + if (!list_empty(&vq->notifier)) { > > + iocb = list_first_entry(&vq->notifier, > > + struct kiocb, ki_list); > > + list_del(&iocb->ki_list); > > + } > > + spin_unlock_irqrestore(&vq->notify_lock, flags); > > + return iocb; > > +} > > + > > +static void handle_async_rx_events_notify(struct vhost_net *net, > > + struct vhost_virtqueue *vq) > > +{ > > + struct kiocb *iocb = NULL; > > + struct vhost_log *vq_log = NULL; > > + int rx_total_len = 0; > > + int log, size; > > + > > + if (vq->link_state != VHOST_VQ_LINK_ASYNC) > > + return; > > + > > + if (vq->receiver) > > + vq->receiver(vq); > > + > > + vq_log = unlikely(vhost_has_feature( > > + &net->dev, VHOST_F_LOG_ALL)) ? vq->log : NULL; > > + while ((iocb = notify_dequeue(vq)) != NULL) { > > + vhost_add_used_and_signal(&net->dev, vq, > > + iocb->ki_pos, iocb->ki_nbytes); > > + log = (int)iocb->ki_user_data; > > + size = iocb->ki_nbytes; > > + rx_total_len += iocb->ki_nbytes; > > + > > + if (iocb->ki_dtor) > > + iocb->ki_dtor(iocb); > > + kmem_cache_free(net->cache, iocb); > > + > > + if (unlikely(vq_log)) > > + vhost_log_write(vq, vq_log, log, size); > > + if (unlikely(rx_total_len >= VHOST_NET_WEIGHT)) { > > + vhost_poll_queue(&vq->poll); > > + break; > > + } > > + } > > +} > > + > > +static void handle_async_tx_events_notify(struct vhost_net *net, > > + struct vhost_virtqueue *vq) > > +{ > > + struct kiocb *iocb = NULL; > > + int tx_total_len = 0; > > + > > + if (vq->link_state != VHOST_VQ_LINK_ASYNC) > > + return; > > + > > + while ((iocb = notify_dequeue(vq)) != NULL) { > > + vhost_add_used_and_signal(&net->dev, vq, > > + iocb->ki_pos, 0); > > + tx_total_len += iocb->ki_nbytes; > > + > > + if (iocb->ki_dtor) > > + iocb->ki_dtor(iocb); > > + > > + kmem_cache_free(net->cache, iocb); > > + if (unlikely(tx_total_len >= VHOST_NET_WEIGHT)) { > > + vhost_poll_queue(&vq->poll); > > + break; > > + } > > + } > > +} > > + > > /* Expects to
RE: [RFC][PATCH v2 0/3] Provide a zero-copy method on KVM virtio-net.
Sridhar, >> The idea is simple, just to pin the guest VM user space and then >> let host NIC driver has the chance to directly DMA to it. >> The patches are based on vhost-net backend driver. We add a device >> which provides proto_ops as sendmsg/recvmsg to vhost-net to >> send/recv directly to/from the NIC driver. KVM guest who use the >>vhost-net backend may bind any ethX interface in the host side to >> get copyless data transfer thru guest virtio-net frontend. >What is the advantage of this approach compared to PCI-passthrough >of the host NIC to the guest? PCI-passthrough needs hardware support, a kind of iommu engine will help to translate guest physical address to host physical address. And currently, a PCI-passthrough device cannot pass live migration. The zero-copy is a pure software solution. It doesn't need special hardware support. In theory, it can pass live migration. >Does this require pinning of the entire guest memory? Or only the >send/receive buffers? We need only to pin the send/receive buffers. Thanks Xiaohui >Thanks >Sridhar > > The scenario is like this: > > The guest virtio-net driver submits multiple requests thru vhost-net > backend driver to the kernel. And the requests are queued and then > completed after corresponding actions in h/w are done. > > For read, user space buffers are dispensed to NIC driver for rx when > a page constructor API is invoked. Means NICs can allocate user buffers > from a page constructor. We add a hook in netif_receive_skb() function > to intercept the incoming packets, and notify the zero-copy device. > > For write, the zero-copy deivce may allocates a new host skb and puts > payload on the skb_shinfo(skb)->frags, and copied the header to skb->data. > The request remains pending until the skb is transmitted by h/w. > > Here, we have ever considered 2 ways to utilize the page constructor > API to dispense the user buffers. > > One: Modify __alloc_skb() function a bit, it can only allocate a > structure of sk_buff, and the data pointer is pointing to a > user buffer which is coming from a page constructor API. > Then the shinfo of the skb is also from guest. > When packet is received from hardware, the skb->data is filled > directly by h/w. What we have done is in this way. > > Pros: We can avoid any copy here. > Cons: Guest virtio-net driver needs to allocate skb as almost > the same method with the host NIC drivers, say the size > of netdev_alloc_skb() and the same reserved space in the > head of skb. Many NIC drivers are the same with guest and > ok for this. But some lastest NIC drivers reserves special > room in skb head. To deal with it, we suggest to provide > a method in guest virtio-net driver to ask for parameter > we interest from the NIC driver when we know which device > we have bind to do zero-copy. Then we ask guest to do so. > Is that reasonable? > > Two: Modify driver to get user buffer allocated from a page constructor > API(to substitute alloc_page()), the user buffer are used as payload > buffers and filled by h/w directly when packet is received. Driver > should associate the pages with skb (skb_shinfo(skb)->frags). For > the head buffer side, let host allocates skb, and h/w fills it. > After that, the data filled in host skb header will be copied into > guest header buffer which is submitted together with the payload buffer. > > Pros: We could less care the way how guest or host allocates their > buffers. > Cons: We still need a bit copy here for the skb header. > > We are not sure which way is the better here. This is the first thing we want > to get comments from the community. We wish the modification to the network > part will be generic which not used by vhost-net backend only, but a user > application may use it as well when the zero-copy device may provides async > read/write operations later. > > Please give comments especially for the network part modifications. > > > We provide multiple submits and asynchronous notifiicaton to > vhost-net too. > > Our goal is to improve the bandwidth and reduce the CPU usage. > Exact performance data will be provided later. But for simple > test with netperf, we found bindwidth up and CPU % up too, > but the bindwidth up ratio is much more than CPU % up ratio. > > What we have not done yet: > packet split support > To support GRO > Performance tuning > > what we have done in v1: > polish the RCU usage > deal with write logging in asynchroush mode in vhost > add notifier block for mp device > rename page_ctor to mp_port in netdevice.h to make it looks generic > add mp_dev_change_flags() for mp device to change NIC state > add CONIFG_VHOST_MPASSTHRU to limit the usage w
RE: [RFC] [PATCH v2 3/3] Let host NIC driver to DMA to guest user space.
>> From: Xin Xiaohui >> >> The patch let host NIC driver to receive user space skb, >> then the driver has chance to directly DMA to guest user >> space buffers thru single ethX interface. >> We want it to be more generic as a zero copy framework. >> >> Signed-off-by: Xin Xiaohui >> Signed-off-by: Zhao Yu >> Sigend-off-by: Jeff Dike >> --- >> >> We consider 2 way to utilize the user buffres, but not sure which one >> is better. Please give any comments. >> >> One:Modify __alloc_skb() function a bit, it can only allocate a >> structure of sk_buff, and the data pointer is pointing to a >> user buffer which is coming from a page constructor API. >> Then the shinfo of the skb is also from guest. >> When packet is received from hardware, the skb->data is filled >> directly by h/w. What we have done is in this way. >> >> Pros: We can avoid any copy here. >> Cons: Guest virtio-net driver needs to allocate skb as almost >> the same method with the host NIC drivers, say the size >> of netdev_alloc_skb() and the same reserved space in the >> head of skb. Many NIC drivers are the same with guest and >> ok for this. But some lastest NIC drivers reserves special >> room in skb head. To deal with it, we suggest to provide >> a method in guest virtio-net driver to ask for parameter >> we interest from the NIC driver when we know which device >> we have bind to do zero-copy. Then we ask guest to do so. >> Is that reasonable? >> >> Two:Modify driver to get user buffer allocated from a page constructor >> API(to substitute alloc_page()), the user buffer are used as payload >> buffers and filled by h/w directly when packet is received. Driver >> should associate the pages with skb (skb_shinfo(skb)->frags). For >> the head buffer side, let host allocates skb, and h/w fills it. >> After that, the data filled in host skb header will be copied into >> guest header buffer which is submitted together with the payload >> buffer. >> >> Pros: We could less care the way how guest or host allocates their >> buffers. >> Cons: We still need a bit copy here for the skb header. >> >> We are not sure which way is the better here. This is the first thing we want >> to get comments from the community. We wish the modification to the network >> part will be generic which not used by vhost-net backend only, but a user >> application may use it as well when the zero-copy device may provides async >> read/write operations later. >> >> >> Thanks >> Xiaohui >How do you deal with the DoS problem of hostile user space app posting huge >number of receives and never getting anything. That's a problem we are trying to deal with. It's critical for long term. Currently, we tried to limit the pages it can pin, but not sure how much is reasonable. For now, the buffers submitted is from guest virtio-net driver, so it's safe in some extent just for now. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 2/3] Provides multiple submits and asynchronous notifications.
Michael, > > For the write logging, do you have a function in hand that we can > > recompute the log? If that, I think I can use it to recompute the > > log info when the logging is suddenly enabled. > > For the outstanding requests, do you mean all the user buffers have > > submitted before the logging ioctl changed? That may be a lot, and > > >> >>some of them are still in NIC ring descriptors. Waiting them to be > > finished may be need some time. I think when logging ioctl changed, > > >> >>then the logging is changed just after that is also reasonable. > > >>>The key point is that after loggin ioctl returns, any > > >>>subsequent change to memory must be logged. It does not > > >>>matter when was the request submitted, otherwise we will > > >>>get memory corruption on migration. > > >>The change to memory happens when vhost_add_used_and_signal(), right? > > >>So after ioctl returns, just recompute the log info to the events in the > > >>async queue, > > >>is ok. Since the ioctl and write log operations are all protected by > > >>vq->mutex. > >>> Thanks > >> >Xiaohui > >>Yes, I think this will work. >> Thanks, so do you have the function to recompute the log info in your hand >> that I can >>use? I have weakly remembered that you have noticed it before some time. >Doesn't just rerunning vhost_get_vq_desc work? Am I missing something here? The vhost_get_vq_desc() looks in vq, and finds the first available buffers, and converts it to an iovec. I think the first available buffer is not the buffers in the async queue, so I think rerunning vhost_get_vq_desc() cannot work. Thanks Xiaohui > > > Thanks > > > Xiaohui > > > > > > drivers/vhost/net.c | 189 > > > +++-- > > > drivers/vhost/vhost.h | 10 +++ > > > 2 files changed, 192 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > > > index 22d5fef..2aafd90 100644 > > > --- a/drivers/vhost/net.c > > > +++ b/drivers/vhost/net.c > > > @@ -17,11 +17,13 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include > > > #include > > > #include > > > #include > > > +#include > > > > > > #include > > > > > > @@ -47,6 +49,7 @@ struct vhost_net { > > > struct vhost_dev dev; > > > struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX]; > > > struct vhost_poll poll[VHOST_NET_VQ_MAX]; > > > + struct kmem_cache *cache; > > > /* Tells us whether we are polling a socket for TX. > > >* We only do this when socket buffer fills up. > > >* Protected by tx vq lock. */ > > > @@ -91,11 +94,88 @@ static void tx_poll_start(struct vhost_net *net, > > > struct socket *sock) > > > net->tx_poll_state = VHOST_NET_POLL_STARTED; > > > } > > > > > > +struct kiocb *notify_dequeue(struct vhost_virtqueue *vq) > > > +{ > > > + struct kiocb *iocb = NULL; > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&vq->notify_lock, flags); > > > + if (!list_empty(&vq->notifier)) { > > > + iocb = list_first_entry(&vq->notifier, > > > + struct kiocb, ki_list); > > > + list_del(&iocb->ki_list); > > > + } > > > + spin_unlock_irqrestore(&vq->notify_lock, flags); > > > + return iocb; > > > +} > > > + > > > +static void handle_async_rx_events_notify(struct vhost_net *net, > > > + struct vhost_virtqueue *vq) > > > +{ > > > + struct kiocb *iocb = NULL; > > > + struct vhost_log *vq_log = NULL; > > > + int rx_total_len = 0; > > > + int log, size; > > > + > > > + if (vq->link_state != VHOST_VQ_LINK_ASYNC) > > > + return; > > > + > > > + if (vq->receiver) > > > + vq->receiver(vq); > > > + > > > + vq_log = unlikely(vhost_has_feature( > > > + &net->dev, VHOST_F_LOG_ALL)) ? vq->log : NULL; > > > + while ((iocb = notify_dequeue(vq)) != NULL) { > > > + vhost_add_used_and_signal(&net->dev, vq, > > > + iocb->ki_pos, iocb->ki_nbytes); > > > + log = (int)iocb->ki_user_data; > > > + size = iocb->ki_nbytes; > > > + rx_total_len += iocb->ki_nbytes; > > > + > > > + if (iocb->ki_dtor) > > > + iocb->ki_dtor(iocb); > > > + kmem_cache_free(net->cache, iocb); > > > + > > > + if (unlikely(vq_log)) > > > + vhost_log_write(vq, vq_log, log, size); > > > + if (unlikely(rx_total_len >= VHOST_NET_WEIGHT)) { > > > + vhost_poll_queue(&vq->poll); > > > + break; > > > + } > > > + } > > > +} > > > + > > > +static void handle_async_tx_events_notify(struct vhost_net *net, > > > + struct vhost_virtqueue *vq) > > > +{ > > > + struct kiocb *iocb = NULL; > > > + int tx_total_len = 0; > > > + > > > + if (vq->link_state != VHOST_VQ_LINK_ASYNC) > > > + return; > > > + > > > + while ((iocb = notify_dequeue(vq)) != NULL) { > > > + vhost_add
RE: [PATCH 1/3] A device for zero-copy based on KVM virtio-net.
Michael, >> Qemu needs a userspace write, is that a synchronous one or >>asynchronous one? >It's a synchronous non-blocking write. Sorry, why the Qemu live migration needs the device have a userspace write? how does the write operation work? And why a read operation is not cared here? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH v3 1/3] A device for zero-copy based on KVM virtio-net.
Arnd, >> From: Xin Xiaohui >> >> Add a device to utilize the vhost-net backend driver for >> copy-less data transfer between guest FE and host NIC. >> It pins the guest user space to the host memory and >> provides proto_ops as sendmsg/recvmsg to vhost-net. >Sorry for taking so long before finding the time to look >at your code in more detail. >It seems that you are duplicating a lot of functionality that >is already in macvtap. I've asked about this before but then >didn't look at your newer versions. Can you explain the value >of introducing another interface to user land? >I'm still planning to add zero-copy support to macvtap, >hopefully reusing parts of your code, but do you think there >is value in having both? I have not looked into your macvtap code in detail before. Does the two interface exactly the same? We just want to create a simple way to do zero-copy. Now it can only support vhost, but in future we also want it to support directly read/write operations from user space too. Basically, compared to the interface, I'm more worried about the modification to net core we have made to implement zero-copy now. If this hardest part can be done, then any user space interface modifications or integrations are more easily to be done after that. >> diff --git a/drivers/vhost/mpassthru.c b/drivers/vhost/mpassthru.c >> new file mode 100644 >> index 000..86d2525 >> --- /dev/null >> +++ b/drivers/vhost/mpassthru.c > >@@ -0,0 +1,1264 @@ > >+ > >+#ifdef MPASSTHRU_DEBUG >> +static int debug; >> + >> +#define DBG if (mp->debug) printk > >+#define DBG1 if (debug == 2) printk > >+#else > >+#define DBG(a...) > >+#define DBG1(a...) > >+#endif >This should probably just use the existing dev_dbg/pr_debug infrastructure. Thanks. Will try that. > [... skipping buffer management code for now] > >+static int mp_sendmsg(struct kiocb *iocb, struct socket *sock, > >+struct msghdr *m, size_t total_len) > >+{ > >[...] >This function looks like we should be able to easily include it into >macvtap and get zero-copy transmits without introducing the new >user-level interface. >> +static int mp_recvmsg(struct kiocb *iocb, struct socket *sock, >> +struct msghdr *m, size_t total_len, >> +int flags) >> +{ >> +struct mp_struct *mp = container_of(sock->sk, struct mp_sock, sk)->mp; >> +struct page_ctor *ctor; >> +struct vhost_virtqueue *vq = (struct vhost_virtqueue *)(iocb->private); >It smells like a layering violation to look at the iocb->private field >from a lower-level driver. I would have hoped that it's possible to implement >this without having this driver know about the higher-level vhost driver >internals. Can you explain why this is needed? I don't like this too, but since the kiocb is maintained by vhost with a list_head. And mp device is responsible to collect the kiocb into the list_head, We need something known by vhost/mp both. >> +spin_lock_irqsave(&ctor->read_lock, flag); >> +list_add_tail(&info->list, &ctor->readq); > >+spin_unlock_irqrestore(&ctor->read_lock, flag); > >+ > >+if (!vq->receiver) { > >+vq->receiver = mp_recvmsg_notify; > >+set_memlock_rlimit(ctor, RLIMIT_MEMLOCK, > >+ vq->num * 4096, > >+ vq->num * 4096); > >+} > >+ > >+return 0; > >+} >Not sure what I'm missing, but who calls the vq->receiver? This seems >to be neither in the upstream version of vhost nor introduced by your >patch. See Patch v3 2/3 I have sent out, it is called by handle_rx() in vhost. >> +static void __mp_detach(struct mp_struct *mp) >> +{ > >+mp->mfile = NULL; > >+ > >+mp_dev_change_flags(mp->dev, mp->dev->flags & ~IFF_UP); > >+page_ctor_detach(mp); > >+mp_dev_change_flags(mp->dev, mp->dev->flags | IFF_UP); > >+ > >+/* Drop the extra count on the net device */ > >+dev_put(mp->dev); > >+} > >+ > >+static DEFINE_MUTEX(mp_mutex); > >+ > >+static void mp_detach(struct mp_struct *mp) > >+{ > >+mutex_lock(&mp_mutex); > >+__mp_detach(mp); > >+mutex_unlock(&mp_mutex); > >+} > >+ > >+static void mp_put(struct mp_file *mfile) > >+{ > >+if (atomic_dec_and_test(&mfile->count)) > >+mp_detach(mfile->mp); > >+} > >+ > >+static int mp_releas
RE: [RFC][PATCH v2 0/3] Provide a zero-copy method on KVM virtio-net.
Michael, >> The idea is simple, just to pin the guest VM user space and then >> let host NIC driver has the chance to directly DMA to it. >> The patches are based on vhost-net backend driver. We add a device >> which provides proto_ops as sendmsg/recvmsg to vhost-net to >> send/recv directly to/from the NIC driver. KVM guest who use the >> vhost-net backend may bind any ethX interface in the host side to >> get copyless data transfer thru guest virtio-net frontend. >> >> The scenario is like this: >> >> The guest virtio-net driver submits multiple requests thru vhost-net >> backend driver to the kernel. And the requests are queued and then >> completed after corresponding actions in h/w are done. >> >> For read, user space buffers are dispensed to NIC driver for rx when >> a page constructor API is invoked. Means NICs can allocate user buffers >> from a page constructor. We add a hook in netif_receive_skb() function >> to intercept the incoming packets, and notify the zero-copy device. >> >> For write, the zero-copy deivce may allocates a new host skb and puts >> payload on the skb_shinfo(skb)->frags, and copied the header to skb->data. >> The request remains pending until the skb is transmitted by h/w. >> >> Here, we have ever considered 2 ways to utilize the page constructor >> API to dispense the user buffers. >> >> One: Modify __alloc_skb() function a bit, it can only allocate a >> structure of sk_buff, and the data pointer is pointing to a >> user buffer which is coming from a page constructor API. >> Then the shinfo of the skb is also from guest. >> When packet is received from hardware, the skb->data is filled >> directly by h/w. What we have done is in this way. >> >> Pros: We can avoid any copy here. >> Cons: Guest virtio-net driver needs to allocate skb as almost >> the same method with the host NIC drivers, say the size >> of netdev_alloc_skb() and the same reserved space in the >> head of skb. Many NIC drivers are the same with guest and >> ok for this. But some lastest NIC drivers reserves special >> room in skb head. To deal with it, we suggest to provide >> a method in guest virtio-net driver to ask for parameter >> we interest from the NIC driver when we know which device >> we have bind to do zero-copy. Then we ask guest to do so. >> Is that reasonable? >Unfortunately, this would break compatibility with existing virtio. >This also complicates migration. You mean any modification to the guest virtio-net driver will break the compatibility? We tried to enlarge the virtio_net_config to contains the 2 parameter, and add one VIRTIO_NET_F_PASSTHRU flag, virtionet_probe() will check the feature flag, and get the parameters, then virtio-net driver use it to allocate buffers. How about this? >What is the room in skb head used for? I'm not sure, but the latest ixgbe driver does this, it reserves 32 bytes compared to NET_IP_ALIGN. >> Two: Modify driver to get user buffer allocated from a page constructor >> API(to substitute alloc_page()), the user buffer are used as payload >> buffers and filled by h/w directly when packet is received. Driver >> should associate the pages with skb (skb_shinfo(skb)->frags). For >> the head buffer side, let host allocates skb, and h/w fills it. >> After that, the data filled in host skb header will be copied into >> guest header buffer which is submitted together with the payload buffer. >> >> Pros: We could less care the way how guest or host allocates their >> buffers. >> Cons: We still need a bit copy here for the skb header. >> >> We are not sure which way is the better here. >The obvious question would be whether you see any speed difference >with the two approaches. If no, then the second approach would be >better. I remember the second approach is a bit slower in 1500MTU. But we did not tested too much. >> This is the first thing we want >> to get comments from the community. We wish the modification to the network >> part will be generic which not used by vhost-net backend only, but a user >> application may use it as well when the zero-copy device may provides async >> read/write operations later. >> >> Please give comments especially for the network part modifications. >> >> >> We provide multiple submits and asynchronous notifiicaton to >>vhost-net too. >> >> Our goal is to improve the bandwidth and reduce the CPU usage. >> Exact performance data will be provided later. But for simple >> test with netperf, we found bindwidth up and CPU % up too, >> but the bindwidth up ratio is much more than CPU % up ratio. >> >> What we have not done yet: >> packet split support >What does this mean, exactly? We can support 1500MTU, but for jumbo frame, since vhost driver before don't support mergeable buffer, we cannot try
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Monday, September 20, 2010 7:37 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. > >On Mon, Sep 20, 2010 at 04:08:48PM +0800, xiaohui@intel.com wrote: >> From: Xin Xiaohui >> >> --- >> Michael, >> I have move the ioctl to configure the locked memory to vhost > >It's ok to move this to vhost but vhost does not >know how much memory is needed by the backend. I think the backend here you mean is mp device. Actually, the memory needed is related to vq->num to run zero-copy smoothly. That means mp device did not know it but vhost did. And the rlimt stuff is per process, we use current pointer to set and check the rlimit, the operations should be in the same process. Now the check operations are in vhost process, as mp_recvmsg() or mp_sendmsg() are called by vhost. So set operations should be in vhost process too, it's natural. >So I think we'll need another ioctl in the backend >to tell userspace how much memory is needed? > Except vhost tells it to mp device, mp did not know how much memory is needed to run zero-copy smoothly. Is userspace interested about the memory mp is needed? >It seems a bit cleaner as a backend ioctl as vhost >does not lock memory itself, but I am not >principally opposed. > >> and >> check the limit with mm->locked_vm. please have a look. >> >> Thanks >> Xiaohui >> >> drivers/vhost/mpassthru.c | 74 +-- >> drivers/vhost/net.c | 78 >++-- >> include/linux/vhost.h |3 ++ >> 3 files changed, 85 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/vhost/mpassthru.c b/drivers/vhost/mpassthru.c >> index d86d94c..fd3827b 100644 >> --- a/drivers/vhost/mpassthru.c >> +++ b/drivers/vhost/mpassthru.c >> @@ -109,9 +109,6 @@ struct page_ctor { >> int wq_len; >> int rq_len; >> spinlock_t read_lock; >> -/* record the locked pages */ >> -int lock_pages; >> -struct rlimit o_rlim; >> struct net_device *dev; >> struct mpassthru_port port; >> struct page_info**hash_table; >> @@ -231,7 +228,6 @@ static int page_ctor_attach(struct mp_struct *mp) >> ctor->port.ctor = page_ctor; >> ctor->port.sock = &mp->socket; >> ctor->port.hash = mp_lookup; >> -ctor->lock_pages = 0; >> >> /* locked by mp_mutex */ >> dev->mp_port = &ctor->port; >> @@ -264,37 +260,6 @@ struct page_info *info_dequeue(struct page_ctor *ctor) >> return info; >> } >> >> -static int set_memlock_rlimit(struct page_ctor *ctor, int resource, >> - unsigned long cur, unsigned long max) >> -{ >> -struct rlimit new_rlim, *old_rlim; >> -int retval; >> - >> -if (resource != RLIMIT_MEMLOCK) >> -return -EINVAL; >> -new_rlim.rlim_cur = cur; >> -new_rlim.rlim_max = max; >> - >> -old_rlim = current->signal->rlim + resource; >> - >> -/* remember the old rlimit value when backend enabled */ >> -ctor->o_rlim.rlim_cur = old_rlim->rlim_cur; >> -ctor->o_rlim.rlim_max = old_rlim->rlim_max; >> - >> -if ((new_rlim.rlim_max > old_rlim->rlim_max) && >> -!capable(CAP_SYS_RESOURCE)) >> -return -EPERM; >> - >> -retval = security_task_setrlimit(resource, &new_rlim); >> -if (retval) >> -return retval; >> - >> -task_lock(current->group_leader); >> -*old_rlim = new_rlim; >> -task_unlock(current->group_leader); >> -return 0; >> -} >> - >> static void relinquish_resource(struct page_ctor *ctor) >> { >> if (!(ctor->dev->flags & IFF_UP) && >> @@ -322,8 +287,6 @@ static void mp_ki_dtor(struct kiocb *iocb) >> info->ctor->rq_len--; >> } else >> info->ctor->wq_len--; >> -/* Decrement the number of locked pages */ >> -info->ctor->lock_pages -= info->pnum; >> kmem_cache_free(ext_page_info_cache, info); >> relinquish_resource(info->ctor); >> >>
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>-Original Message- >From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Tuesday, September 21, 2010 9:14 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. > >On Tue, Sep 21, 2010 at 09:39:31AM +0800, Xin, Xiaohui wrote: >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >Sent: Monday, September 20, 2010 7:37 PM >> >To: Xin, Xiaohui >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >linux-ker...@vger.kernel.org; >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >jd...@linux.intel.com >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> > >> >On Mon, Sep 20, 2010 at 04:08:48PM +0800, xiaohui@intel.com wrote: >> >> From: Xin Xiaohui >> >> >> >> --- >> >> Michael, >> >> I have move the ioctl to configure the locked memory to vhost >> > >> >It's ok to move this to vhost but vhost does not >> >know how much memory is needed by the backend. >> >> I think the backend here you mean is mp device. >> Actually, the memory needed is related to vq->num to run zero-copy >> smoothly. >> That means mp device did not know it but vhost did. > >Well, this might be so if you insist on locking >all posted buffers immediately. However, let's assume I have a >very large ring and prepost a ton of RX buffers: >there's no need to lock all of them directly: > >if we have buffers A and B, we can lock A, pass it >to hardware, and when A is consumed unlock A, lock B >and pass it to hardware. > > >It's not really critical. But note we can always have userspace >tell MP device all it wants to know, after all. > Ok. Here are two values we have mentioned, one is how much memory user application wants to lock, and one is how much memory locked is needed to run smoothly. When net backend is setup, we first need an ioctl to get how much memory is needed to lock, and then we call another ioctl to set how much it want to lock. Is that what's in your mind? >> And the rlimt stuff is per process, we use current pointer to set >> and check the rlimit, the operations should be in the same process. > >Well no, the ring is handled from the kernel thread: we switch the mm to >point to the owner task so copy from/to user and friends work, but you >can't access the rlimit etc. > Yes, the userspace and vhost kernel is not the same process. But we can record the task pointer as mm. >> Now the check operations are in vhost process, as mp_recvmsg() or >> mp_sendmsg() are called by vhost. > >Hmm, what do you mean by the check operations? >send/recv are data path operations, they shouldn't >do any checks, should they? > As you mentioned what infiniband driver done: down_write(¤t->mm->mmap_sem); locked = npages + current->mm->locked_vm; lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) { ret = -ENOMEM; goto out; } cur_base = addr & PAGE_MASK; ret = 0; while (npages) { ret = get_user_pages(current, current->mm, cur_base, min_t(unsigned long, npages, PAGE_SIZE / sizeof (struct page *)), 1, !umem->writable, page_list, vma_list); I think it's a data path too. We do the check because get_user_pages() really pin and locked the memory. >> So set operations should be in >> vhost process too, it's natural. >> >> >So I think we'll need another ioctl in the backend >> >to tell userspace how much memory is needed? >> > >> Except vhost tells it to mp device, mp did not know >> how much memory is needed to run zero-copy smoothly. >> Is userspace interested about the memory mp is needed? > >Couldn't parse this last question. >I think userspace generally does want control over >how much memory we'll lock. We should not just lock >as much as we can. > >-- >MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>-Original Message- >From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Wednesday, September 22, 2010 7:55 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. > >On Wed, Sep 22, 2010 at 07:41:36PM +0800, Xin, Xiaohui wrote: >> >-Original Message- >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >Sent: Tuesday, September 21, 2010 9:14 PM >> >To: Xin, Xiaohui >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >linux-ker...@vger.kernel.org; >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >jd...@linux.intel.com >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> > >> >On Tue, Sep 21, 2010 at 09:39:31AM +0800, Xin, Xiaohui wrote: >> >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >> >Sent: Monday, September 20, 2010 7:37 PM >> >> >To: Xin, Xiaohui >> >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >> >linux-ker...@vger.kernel.org; >> >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >> >jd...@linux.intel.com >> >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> >> > >> >> >On Mon, Sep 20, 2010 at 04:08:48PM +0800, xiaohui@intel.com wrote: >> >> >> From: Xin Xiaohui >> >> >> >> >> >> --- >> >> >> Michael, >> >> >> I have move the ioctl to configure the locked memory to vhost >> >> > >> >> >It's ok to move this to vhost but vhost does not >> >> >know how much memory is needed by the backend. >> >> >> >> I think the backend here you mean is mp device. >> >> Actually, the memory needed is related to vq->num to run zero-copy >> >> smoothly. >> >> That means mp device did not know it but vhost did. >> > >> >Well, this might be so if you insist on locking >> >all posted buffers immediately. However, let's assume I have a >> >very large ring and prepost a ton of RX buffers: >> >there's no need to lock all of them directly: >> > >> >if we have buffers A and B, we can lock A, pass it >> >to hardware, and when A is consumed unlock A, lock B >> >and pass it to hardware. >> > >> > >> >It's not really critical. But note we can always have userspace >> >tell MP device all it wants to know, after all. >> > >> Ok. Here are two values we have mentioned, one is how much memory >> user application wants to lock, and one is how much memory locked >> is needed to run smoothly. When net backend is setup, we first need >> an ioctl to get how much memory is needed to lock, and then we call >> another ioctl to set how much it want to lock. Is that what's in your mind? > >That's fine. > >> >> And the rlimt stuff is per process, we use current pointer to set >> >> and check the rlimit, the operations should be in the same process. >> > >> >Well no, the ring is handled from the kernel thread: we switch the mm to >> >point to the owner task so copy from/to user and friends work, but you >> >can't access the rlimit etc. >> > >> Yes, the userspace and vhost kernel is not the same process. But we can >> record the task pointer as mm. > >So you will have to store mm and do device->mm, not current->mm. >Anyway, better not touch mm on data path. > >> >> Now the check operations are in vhost process, as mp_recvmsg() or >> >> mp_sendmsg() are called by vhost. >> > >> >Hmm, what do you mean by the check operations? >> >send/recv are data path operations, they shouldn't >> >do any checks, should they? >> > >> As you mentioned what infiniband driver done: >> down_write(¤t->mm->mmap_sem); >> >> locked = npages + current->mm->locked_vm; >> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >> >> if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) { >> ret = -ENOMEM; >> goto out; >> } >> >> cur_base = addr & PAGE_MASK; >> >> ret = 0; >> while
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Sunday, September 26, 2010 7:50 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. > >On Thu, Sep 23, 2010 at 08:56:33PM +0800, Xin, Xiaohui wrote: >> >-Original Message- >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >Sent: Wednesday, September 22, 2010 7:55 PM >> >To: Xin, Xiaohui >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >linux-ker...@vger.kernel.org; >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >jd...@linux.intel.com >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> > >> >On Wed, Sep 22, 2010 at 07:41:36PM +0800, Xin, Xiaohui wrote: >> >> >-----Original Message- >> >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >> >Sent: Tuesday, September 21, 2010 9:14 PM >> >> >To: Xin, Xiaohui >> >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >> >linux-ker...@vger.kernel.org; >> >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >> >jd...@linux.intel.com >> >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> >> > >> >> >On Tue, Sep 21, 2010 at 09:39:31AM +0800, Xin, Xiaohui wrote: >> >> >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >> >> >Sent: Monday, September 20, 2010 7:37 PM >> >> >> >To: Xin, Xiaohui >> >> >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >> >> >linux-ker...@vger.kernel.org; >> >> >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >> >> >jd...@linux.intel.com >> >> >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> >> >> > >> >> >> >On Mon, Sep 20, 2010 at 04:08:48PM +0800, xiaohui@intel.com wrote: >> >> >> >> From: Xin Xiaohui >> >> >> >> >> >> >> >> --- >> >> >> >> Michael, >> >> >> >> I have move the ioctl to configure the locked memory to vhost >> >> >> > >> >> >> >It's ok to move this to vhost but vhost does not >> >> >> >know how much memory is needed by the backend. >> >> >> >> >> >> I think the backend here you mean is mp device. >> >> >> Actually, the memory needed is related to vq->num to run zero-copy >> >> >> smoothly. >> >> >> That means mp device did not know it but vhost did. >> >> > >> >> >Well, this might be so if you insist on locking >> >> >all posted buffers immediately. However, let's assume I have a >> >> >very large ring and prepost a ton of RX buffers: >> >> >there's no need to lock all of them directly: >> >> > >> >> >if we have buffers A and B, we can lock A, pass it >> >> >to hardware, and when A is consumed unlock A, lock B >> >> >and pass it to hardware. >> >> > >> >> > >> >> >It's not really critical. But note we can always have userspace >> >> >tell MP device all it wants to know, after all. >> >> > >> >> Ok. Here are two values we have mentioned, one is how much memory >> >> user application wants to lock, and one is how much memory locked >> >> is needed to run smoothly. When net backend is setup, we first need >> >> an ioctl to get how much memory is needed to lock, and then we call >> >> another ioctl to set how much it want to lock. Is that what's in your >> >> mind? >> > >> >That's fine. >> > >> >> >> And the rlimt stuff is per process, we use current pointer to set >> >> >> and check the rlimit, the operations should be in the same process. >> >> > >> >> >Well no, the ring is handled from the kernel thread: we switch the mm to >> >> >point to the owner task so copy from/to user and friends work, but you >> >> >can't access the rlimit etc. >> >> > >> >
RE: [PATCH v11 00/17] Provide a zero-copy method on KVM virtio-net.
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Monday, September 27, 2010 1:02 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [PATCH v11 00/17] Provide a zero-copy method on KVM virtio-net. > >On Sat, Sep 25, 2010 at 12:27:18PM +0800, xiaohui@intel.com wrote: >> We provide an zero-copy method which driver side may get external >> buffers to DMA. Here external means driver don't use kernel space >> to allocate skb buffers. Currently the external buffer can be from >> guest virtio-net driver. >> >> The idea is simple, just to pin the guest VM user space and then >> let host NIC driver has the chance to directly DMA to it. >> The patches are based on vhost-net backend driver. We add a device >> which provides proto_ops as sendmsg/recvmsg to vhost-net to >> send/recv directly to/from the NIC driver. KVM guest who use the >> vhost-net backend may bind any ethX interface in the host side to >> get copyless data transfer thru guest virtio-net frontend. >> patch 01-10: net core and kernel changes. >> patch 11-13: new device as interface to mantpulate external buffers. >> patch 14:for vhost-net. >> patch 15:An example on modifying NIC driver to using napi_gro_frags(). >> patch 16:An example how to get guest buffers based on driver >> who using napi_gro_frags(). >> patch 17:It's a patch to address comments from Michael S. Thirkin >> to add 2 new ioctls in mp device. >> We split it out here to make easier reiewer. > > > >I commented on how to avoid mm semaphore on data path separately, and >since you didn't have time to review that yet, I won't repeat that here. > I think I did avoid that in data path to use mm semaphore. I stored the value in mp structures and check with that. >At this point what are the plans on macvtap integration? >You indicated this is the interface you intend to use longterm. I'm now working on that. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 00/17] Provide a zero-copy method on KVM virtio-net.
>-Original Message- >From: linux-kernel-ow...@vger.kernel.org >[mailto:linux-kernel-ow...@vger.kernel.org] On >Behalf Of Xin, Xiaohui >Sent: Monday, September 27, 2010 8:45 AM >To: Michael S. Tsirkin >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: RE: [PATCH v11 00/17] Provide a zero-copy method on KVM virtio-net. > >>From: Michael S. Tsirkin [mailto:m...@redhat.com] >>Sent: Monday, September 27, 2010 1:02 AM >>To: Xin, Xiaohui >>Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >>mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >>jd...@linux.intel.com >>Subject: Re: [PATCH v11 00/17] Provide a zero-copy method on KVM virtio-net. >> >>On Sat, Sep 25, 2010 at 12:27:18PM +0800, xiaohui@intel.com wrote: >>> We provide an zero-copy method which driver side may get external >>> buffers to DMA. Here external means driver don't use kernel space >>> to allocate skb buffers. Currently the external buffer can be from >>> guest virtio-net driver. >>> >>> The idea is simple, just to pin the guest VM user space and then >>> let host NIC driver has the chance to directly DMA to it. >>> The patches are based on vhost-net backend driver. We add a device >>> which provides proto_ops as sendmsg/recvmsg to vhost-net to >>> send/recv directly to/from the NIC driver. KVM guest who use the >>> vhost-net backend may bind any ethX interface in the host side to >>> get copyless data transfer thru guest virtio-net frontend. >>> patch 01-10:net core and kernel changes. >>> patch 11-13:new device as interface to mantpulate external buffers. >>> patch 14: for vhost-net. >>> patch 15: An example on modifying NIC driver to using napi_gro_frags(). >>> patch 16: An example how to get guest buffers based on driver >>> who using napi_gro_frags(). >>> patch 17: It's a patch to address comments from Michael S. Thirkin >>> to add 2 new ioctls in mp device. >>> We split it out here to make easier reiewer. >> >> >> >>I commented on how to avoid mm semaphore on data path separately, and >>since you didn't have time to review that yet, I won't repeat that here. >> > >I think I did avoid that in data path to use mm semaphore. I stored the value >in mp structures and check with that. > Hi Michael, Did you think I have addressed your comments about to avoid mm semaphore on data path or not? I stored the value in mp structures and don't use mmap_semphore. Or you still have some concerns with it? >>At this point what are the plans on macvtap integration? >>You indicated this is the interface you intend to use longterm. > >I'm now working on that. > I'm now trying to export some functions from mp device, and let macvtap to use them for rx zero-copy. > >Thanks >Xiaohui >-- >To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html >Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 13/17] Add mp(mediate passthru) device.
>-Original Message- >From: Ben Hutchings [mailto:bhutchi...@solarflare.com] >Sent: Tuesday, September 28, 2010 5:24 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [PATCH v11 13/17] Add mp(mediate passthru) device. > >On Sat, 2010-09-25 at 12:27 +0800, xiaohui....@intel.com wrote: >> From: Xin Xiaohui >> >> The patch add mp(mediate passthru) device, which now >> based on vhost-net backend driver and provides proto_ops >> to send/receive guest buffers data from/to guest vitio-net >> driver. >> >> Signed-off-by: Xin Xiaohui >> Signed-off-by: Zhao Yu >> Reviewed-by: Jeff Dike >> --- >> drivers/vhost/mpassthru.c | 1407 >+ >> 1 files changed, 1407 insertions(+), 0 deletions(-) >> create mode 100644 drivers/vhost/mpassthru.c >> >> diff --git a/drivers/vhost/mpassthru.c b/drivers/vhost/mpassthru.c >> new file mode 100644 >> index 000..d86d94c >> --- /dev/null >> +++ b/drivers/vhost/mpassthru.c >[...] >> +/* #define MPASSTHRU_DEBUG 1 */ >> + >> +#ifdef MPASSTHRU_DEBUG >> +static int debug; >> + >> +#define DBG if (mp->debug) printk >> +#define DBG1 if (debug == 2) printk > >This is unsafe; consider this usage: > > if (foo) > DBG("bar\n"); > else > baz(); > >You should use the standard pr_debug() or dev_dbg() instead. > Ok. Move to pr_debug(). >[...] >> +struct page_ctor { >> + struct list_headreadq; >> + int wq_len; >> + int rq_len; >> + spinlock_t read_lock; > >Only one queue?! > This readq is for mp device to store the rx guest buffers coming from vhost. For tx side, we don't need that, since tx buffers are sent out to device immediately. >I would have appreciated some introductory comments on these structures. >I still don't have any sort of clear picture of how this is all supposed >to work. > Basically, struct page_info{} is used to store each info of the guest buffers and some of vhost ring descriptor info according to this buffers. struct page_ctor{} is used to manipulate multiple struct page_info{}. Each device can do zero-copy is according one struct page_ctor{}. Anyway, I will comments on fields of these structures. >[...] >> +/* The main function to allocate external buffers */ >> +static struct skb_ext_page *page_ctor(struct mpassthru_port *port, >> +struct sk_buff *skb, int npages) >> +{ >> +int i; >> +unsigned long flags; >> +struct page_ctor *ctor; >> +struct page_info *info = NULL; >> + >> +ctor = container_of(port, struct page_ctor, port); >> + >> +spin_lock_irqsave(&ctor->read_lock, flags); >> +if (!list_empty(&ctor->readq)) { >> +info = list_first_entry(&ctor->readq, struct page_info, list); >> +list_del(&info->list); >> +ctor->rq_len--; >> +} >> +spin_unlock_irqrestore(&ctor->read_lock, flags); >> +if (!info) >> +return NULL; >> + >> +for (i = 0; i < info->pnum; i++) >> +get_page(info->pages[i]); >> +info->skb = skb; >> +return &info->ext_page; >> +} > >Why isn't the npages parameter used? > Sorry, somehow forgot that, currently it only allocates one page a time, we want to use npages to see if we break the limit. >[...] >> +static void relinquish_resource(struct page_ctor *ctor) >> +{ >> +if (!(ctor->dev->flags & IFF_UP) && >> +!(ctor->wq_len + ctor->rq_len)) >> +printk(KERN_INFO "relinquish_resource\n"); >> +} > >Looks like something's missing here. > This function is for debug, and so on the rq_len stuff, I'll remove them later. >> +static void mp_ki_dtor(struct kiocb *iocb) >> +{ >> +struct page_info *info = (struct page_info *)(iocb->private); >> +int i; >> + >> +if (info->flags == INFO_READ) { >> +for (i = 0; i < info->pnum; i++) { >> +if (info->pages[i]) { >> +set_page_dirty_lock(info->pages[i]); >> +put_page(info->pages[i]); >> +} >> +
RE: [PATCH v11 12/17] Add a kconfig entry and make entry for mp device.
Thanks, I will move this in order next version. Thanks Xiaohui >-Original Message- >From: Ben Hutchings [mailto:bhutchi...@solarflare.com] >Sent: Monday, September 27, 2010 9:57 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [PATCH v11 12/17] Add a kconfig entry and make entry for mp >device. > >This patch is in the wrong position in the sequence. It needs to be >applied after mpassthru.c is created, not before. > >Ben. > >-- >Ben Hutchings, Senior Software Engineer, Solarflare Communications >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 03/17] Add a ndo_mp_port_prep pointer to net_device_ops.
Ok, I will comment it above the structure. Thanks Xiaohui >-Original Message- >From: Ben Hutchings [mailto:bhutchi...@solarflare.com] >Sent: Monday, September 27, 2010 9:43 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [PATCH v11 03/17] Add a ndo_mp_port_prep pointer to >net_device_ops. > >On Sat, 2010-09-25 at 12:27 +0800, xiaohui....@intel.com wrote: >> From: Xin Xiaohui >> >> If the driver want to allocate external buffers, >> then it can export it's capability, as the skb >> buffer header length, the page length can be DMA, etc. >> The external buffers owner may utilize this. >[...] > >This information needs to be included in the comment above struct >net_device_ops, not just in the commit message. > >Ben. > >-- >Ben Hutchings, Senior Software Engineer, Solarflare Communications >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 00/17] Provide a zero-copy method on KVM virtio-net.
Will be on leave during 10/01 ~ 10/07, and slow or no response to the comments. Thanks Xiaohui >-Original Message- >From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On >Behalf Of >xiaohui@intel.com >Sent: Thursday, September 30, 2010 10:04 PM >To: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: [PATCH v12 00/17] Provide a zero-copy method on KVM virtio-net. > >We provide an zero-copy method which driver side may get external >buffers to DMA. Here external means driver don't use kernel space >to allocate skb buffers. Currently the external buffer can be from >guest virtio-net driver. > >The idea is simple, just to pin the guest VM user space and then >let host NIC driver has the chance to directly DMA to it. >The patches are based on vhost-net backend driver. We add a device >which provides proto_ops as sendmsg/recvmsg to vhost-net to >send/recv directly to/from the NIC driver. KVM guest who use the >vhost-net backend may bind any ethX interface in the host side to >get copyless data transfer thru guest virtio-net frontend. > >patch 01-10: net core and kernel changes. >patch 11-13: new device as interface to mantpulate external buffers. >patch 14: for vhost-net. >patch 15: An example on modifying NIC driver to using napi_gro_frags(). >patch 16: An example how to get guest buffers based on driver > who using napi_gro_frags(). >patch 17: It's a patch to address comments from Michael S. Thirkin > to add 2 new ioctls in mp device. > We split it out here to make easier reiewer. > Need to revise. > >The guest virtio-net driver submits multiple requests thru vhost-net >backend driver to the kernel. And the requests are queued and then >completed after corresponding actions in h/w are done. > >For read, user space buffers are dispensed to NIC driver for rx when >a page constructor API is invoked. Means NICs can allocate user buffers >from a page constructor. We add a hook in netif_receive_skb() function >to intercept the incoming packets, and notify the zero-copy device. > >For write, the zero-copy deivce may allocates a new host skb and puts >payload on the skb_shinfo(skb)->frags, and copied the header to skb->data. >The request remains pending until the skb is transmitted by h/w. > >We provide multiple submits and asynchronous notifiicaton to >vhost-net too. > >Our goal is to improve the bandwidth and reduce the CPU usage. >Exact performance data will be provided later. > >What we have not done yet: > Performance tuning > >what we have done in v1: > polish the RCU usage > deal with write logging in asynchroush mode in vhost > add notifier block for mp device > rename page_ctor to mp_port in netdevice.h to make it looks generic > add mp_dev_change_flags() for mp device to change NIC state > add CONIFG_VHOST_MPASSTHRU to limit the usage when module is not load > a small fix for missing dev_put when fail > using dynamic minor instead of static minor number > a __KERNEL__ protect to mp_get_sock() > >what we have done in v2: > > remove most of the RCU usage, since the ctor pointer is only > changed by BIND/UNBIND ioctl, and during that time, NIC will be > stopped to get good cleanup(all outstanding requests are finished), > so the ctor pointer cannot be raced into wrong situation. > > Remove the struct vhost_notifier with struct kiocb. > Let vhost-net backend to alloc/free the kiocb and transfer them > via sendmsg/recvmsg. > > use get_user_pages_fast() and set_page_dirty_lock() when read. > > Add some comments for netdev_mp_port_prep() and handle_mpassthru(). > >what we have done in v3: > the async write logging is rewritten > a drafted synchronous write function for qemu live migration > a limit for locked pages from get_user_pages_fast() to prevent Dos > by using RLIMIT_MEMLOCK > > >what we have done in v4: > add iocb completion callback from vhost-net to queue iocb in mp device > replace vq->receiver by mp_sock_data_ready() > remove stuff in mp device which access structures from vhost-net > modify skb_reserve() to ignore host NIC driver reserved space > rebase to the latest vhost tree > split large patches into small pieces, especially for net core part. > > >what we have done in v5: > address Arnd Bergmann's comments > -remove IFF_MPASSTHRU_EXCL flag in mp device > -Add CONFIG_COMPAT macro > -remove mp_release ops > move dev_is_mpassthru() as inline func > fix a bug in memory relinquish > Apply to current git (2.6.34-rc6) tree. > >what we have done in v6: > move create_iocb() out of page_dtor which may happen in
RE: [PATCH v12 06/17] Use callback to deal with skb_release_data() specially.
>-Original Message- >From: David Miller [mailto:da...@davemloft.net] >Sent: Friday, October 01, 2010 3:15 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [PATCH v12 06/17] Use callback to deal with skb_release_data() >specially. > >From: xiaohui@intel.com >Date: Thu, 30 Sep 2010 22:04:23 +0800 > >> @@ -197,10 +197,11 @@ struct skb_shared_info { >> union skb_shared_tx tx_flags; >> struct sk_buff *frag_list; >> struct skb_shared_hwtstamps hwtstamps; >> -skb_frag_t frags[MAX_SKB_FRAGS]; >> /* Intermediate layers must ensure that destructor_arg >> * remains valid until skb destructor */ >> void * destructor_arg; >> + >> +skb_frag_t frags[MAX_SKB_FRAGS]; >> }; >> >> /* The structure is for a skb which pages may point to > >Why are you moving frags[] to the end like this? That's to avoid the new cache miss caused by using destructor_arg in data path like skb_release_data(). That's based on the comment from Eric Dumazet on v7 patches. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 10/17] Add a hook to intercept external buffers from NIC driver.
>-Original Message- >From: Eric Dumazet [mailto:eric.duma...@gmail.com] >Sent: Thursday, September 30, 2010 10:22 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [PATCH v12 10/17] Add a hook to intercept external buffers from >NIC driver. > >Le jeudi 30 septembre 2010 à 22:04 +0800, xiaohui....@intel.com a >écrit : >> From: Xin Xiaohui >> >> The hook is called in netif_receive_skb(). >> Signed-off-by: Xin Xiaohui >> Signed-off-by: Zhao Yu >> Reviewed-by: Jeff Dike >> --- >> net/core/dev.c | 35 +++ >> 1 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index c11e32c..83172b8 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -2517,6 +2517,37 @@ err: >> EXPORT_SYMBOL(netdev_mp_port_prep); >> #endif >> >> +#if defined(CONFIG_MEDIATE_PASSTHRU) || >defined(CONFIG_MEDIATE_PASSTHRU_MODULE) >> +/* Add a hook to intercept mediate passthru(zero-copy) packets, >> + * and insert it to the socket queue owned by mp_port specially. >> + */ >> +static inline struct sk_buff *handle_mpassthru(struct sk_buff *skb, >> + struct packet_type **pt_prev, >> + int *ret, >> + struct net_device *orig_dev) >> +{ >> +struct mp_port *mp_port = NULL; >> +struct sock *sk = NULL; >> + >> +if (!dev_is_mpassthru(skb->dev)) >> +return skb; >> +mp_port = skb->dev->mp_port; >> + >> +if (*pt_prev) { >> +*ret = deliver_skb(skb, *pt_prev, orig_dev); >> +*pt_prev = NULL; >> +} >> + >> +sk = mp_port->sock->sk; >> +skb_queue_tail(&sk->sk_receive_queue, skb); >> +sk->sk_state_change(sk); >> + >> +return NULL; >> +} >> +#else >> +#define handle_mpassthru(skb, pt_prev, ret, orig_dev) (skb) >> +#endif >> + >> /** >> * netif_receive_skb - process receive buffer from network >> * @skb: buffer to process >> @@ -2598,6 +2629,10 @@ int netif_receive_skb(struct sk_buff *skb) >> ncls: >> #endif >> >> +/* To intercept mediate passthru(zero-copy) packets here */ >> +skb = handle_mpassthru(skb, &pt_prev, &ret, orig_dev); >> +if (!skb) >> +goto out; >> skb = handle_bridge(skb, &pt_prev, &ret, orig_dev); >> if (!skb) >> goto out; > >This does not apply to current net-next-2.6 > >We now have dev->rx_handler (currently for bridge or macvlan) > > Ok. Thanks, will rebase to fix that. Thanks Xiaohui >commit ab95bfe01f9872459c8678572ccadbf646badad0 >Author: Jiri Pirko >Date: Tue Jun 1 21:52:08 2010 + > >net: replace hooks in __netif_receive_skb V5 > >What this patch does is it removes two receive frame hooks (for bridge and > for >macvlan) from __netif_receive_skb. These are replaced them with a single >hook for both. It only supports one hook per device because it makes no >sense to do bridging and macvlan on the same device. > >Then a network driver (of virtual netdev like macvlan or bridge) can > register >an rx_handler for needed net device. > >Signed-off-by: Jiri Pirko >Signed-off-by: Stephen Hemminger >Signed-off-by: David S. Miller > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 06/17] Use callback to deal with skb_release_data() specially.
>-Original Message- >From: Eric Dumazet [mailto:eric.duma...@gmail.com] >Sent: Monday, October 11, 2010 11:42 PM >To: David Miller >Cc: Xin, Xiaohui; net...@vger.kernel.org; kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >herb...@gondor.apana.org.au; jd...@linux.intel.com >Subject: Re: [PATCH v12 06/17] Use callback to deal with skb_release_data() >specially. > >Le lundi 11 octobre 2010 à 08:27 -0700, David Miller a écrit : >> From: "Xin, Xiaohui" >> Date: Mon, 11 Oct 2010 15:06:05 +0800 >> >> > That's to avoid the new cache miss caused by using destructor_arg in data >> > path >> > like skb_release_data(). >> > That's based on the comment from Eric Dumazet on v7 patches. >> >> Thanks for the explanation. > >Anyway, frags[] must be the last field of "struct skb_shared_info" >since commit fed66381 (net: pskb_expand_head() optimization) > >It seems Xin worked on a quite old tree. > > I will rebase soon. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 12/17] Add mp(mediate passthru) device.
>-Original Message- >From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Sunday, October 03, 2010 9:13 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [PATCH v12 12/17] Add mp(mediate passthru) device. > >On Thu, Sep 30, 2010 at 10:04:30PM +0800, xiaohui....@intel.com wrote: >> From: Xin Xiaohui >> >> The patch add mp(mediate passthru) device, which now >> based on vhost-net backend driver and provides proto_ops >> to send/receive guest buffers data from/to guest vitio-net >> driver. >> >> Signed-off-by: Xin Xiaohui >> Signed-off-by: Zhao Yu >> Reviewed-by: Jeff Dike > >So you plan to rewrite all this to make this code part of macvtap? > V13 exports functions which can be reused by macvtap to get zero-copy. Currently, I'm adding code in macvtap and trying to make it work. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v13 10/16] Add a hook to intercept external buffers from NIC driver.
>-Original Message- >From: David Miller [mailto:da...@davemloft.net] >Sent: Tuesday, October 19, 2010 11:24 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [PATCH v13 10/16] Add a hook to intercept external buffers from >NIC driver. > >From: xiaohui@intel.com >Date: Fri, 15 Oct 2010 17:12:11 +0800 > >> @@ -2891,6 +2922,11 @@ static int __netif_receive_skb(struct sk_buff *skb) >> ncls: >> #endif >> >> +/* To intercept mediate passthru(zero-copy) packets here */ >> +skb = handle_mpassthru(skb, &pt_prev, &ret, orig_dev); >> +if (!skb) >> +goto out; >> + >> /* Handle special case of bridge or macvlan */ >> rx_handler = rcu_dereference(skb->dev->rx_handler); >> if (rx_handler) { > >If you consume the packet here, devices in passthru mode cannot >be use with bonding. > >But there is nothing that prevents a bond being created with such >a device. > >So we have to either prevent such configurations (bad) or make >it work somehow (good) :-) The big picture may like this: To prevent such configurations, we should add code to check in both mp and bonding driver. If a nic is in zero-copy mode , bonding can't be made with it, and if nic is in bonding mode, we can't bind the device to do zero-copy. If we want to support such configurations, it also has some constraints. If bonding is created first, we need code to check if all the slaves support zero-copy mode, and if yes, all the slaves should be assigned a same page_ctor(), all the packets received should be intercepted with master nic. If not, fails. If zero-copy is enabled first, bonding created with it should fail. Somehow, it seems not a trivial work to support it now. Can we support it later and as a todo with our current work? Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v13 10/16] Add a hook to intercept external buffers from NIC driver.
I have addressed this issue in v14 patch set. Thanks Xiaohui >-Original Message- >From: David Miller [mailto:da...@davemloft.net] >Sent: Saturday, October 30, 2010 4:29 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [PATCH v13 10/16] Add a hook to intercept external buffers from >NIC driver. > >From: "Xin, Xiaohui" >Date: Wed, 27 Oct 2010 09:33:12 +0800 > >> Somehow, it seems not a trivial work to support it now. Can we support it >> later and as a todo with our current work? > >I would prefer the feature work properly, rather than only in specific >cases, before being integated. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Re:[PATCH v14 06/17] Use callback to deal with skb_release_data() specially.
>-Original Message- >From: Eric Dumazet [mailto:eric.duma...@gmail.com] >Sent: Monday, November 08, 2010 4:25 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: Re:[PATCH v14 06/17] Use callback to deal with skb_release_data() >specially. > >Le lundi 08 novembre 2010 à 16:03 +0800, xiaohui....@intel.com a écrit : >> From: Xin Xiaohui >> >> >> Hmm, I suggest you read the comment two lines above. >> >> >> >> If destructor_arg is now cleared each time we allocate a new skb, then, >> >> please move it before dataref in shinfo structure, so that the following >> >> memset() does the job efficiently... >> > >> > >> >Something like : >> > >> >diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >> >index e6ba898..2dca504 100644 >> >--- a/include/linux/skbuff.h >> >+++ b/include/linux/skbuff.h >> >@@ -195,6 +195,9 @@ struct skb_shared_info { >> >__be32 ip6_frag_id; >> >__u8tx_flags; >> >struct sk_buff *frag_list; >> >+ /* Intermediate layers must ensure that destructor_arg >> >+* remains valid until skb destructor */ >> >+ void*destructor_arg; >> >struct skb_shared_hwtstamps hwtstamps; >> > >> >/* >> >@@ -202,9 +205,6 @@ struct skb_shared_info { >> > */ >> >atomic_tdataref; >> > >> >- /* Intermediate layers must ensure that destructor_arg >> >-* remains valid until skb destructor */ >> >- void * destructor_arg; >> >/* must be last field, see pskb_expand_head() */ >> >skb_frag_t frags[MAX_SKB_FRAGS]; >> > }; >> > >> > >> >> Will that affect the cache line? > >What do you mean ? > >> Or, we can move the line to clear destructor_arg to the end of __alloc_skb(). >> It looks like as the following, which one do you prefer? >> >> Thanks >> Xiaohui >> >> --- >> net/core/skbuff.c |8 >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index c83b421..df852f2 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -224,6 +224,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t >> gfp_mask, >> >> child->fclone = SKB_FCLONE_UNAVAILABLE; >> } >> +shinfo->destructor_arg = NULL; >> out: >> return skb; >> nodata: > >I dont understand why you want to do this. > >This adds an instruction, makes code bigger, and no obvious gain for me, >at memory transactions side. > >If integrated in the existing memset(), cost is an extra iteration to >perform the clear of this field. > Ok. Thanks for this explanation and will update with your solution. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v15 00/17] Provide a zero-copy method on KVM virtio-net.
>-Original Message- >From: David Miller [mailto:da...@davemloft.net] >Sent: Thursday, November 11, 2010 1:47 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [PATCH v15 00/17] Provide a zero-copy method on KVM virtio-net. > >From: xiaohui@intel.com >Date: Wed, 10 Nov 2010 17:23:28 +0800 > >> From: Xin Xiaohui >> >>>2) The idea to key off of skb->dev in skb_release_data() is >>> fundamentally flawed since many actions can change skb->dev on you, >>> which will end up causing a leak of your external data areas. >> >> How about this one? If the destructor_arg is not a good candidate, >> then I have to add an apparent field in shinfo. > >If destructor_arg is actually a net_device pointer or similar, >you will need to take a reference count on it or similar. > Do you mean destructor_arg will be consumed by other user? If that case, may I add a new structure member in shinfo? Thus only zero-copy will use it, and no need for the reference count. >Which means --> good bye performance especially on SMP. > >You're going to be adding new serialization points and at >least two new atomics per packet. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v15 00/17] Provide a zero-copy method on KVM virtio-net.
>-Original Message- >From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf >Of Xin, >Xiaohui >Sent: Thursday, November 11, 2010 4:28 PM >To: David Miller >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: RE: [PATCH v15 00/17] Provide a zero-copy method on KVM virtio-net. > >>-Original Message- >>From: David Miller [mailto:da...@davemloft.net] >>Sent: Thursday, November 11, 2010 1:47 AM >>To: Xin, Xiaohui >>Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >>m...@redhat.com; mi...@elte.hu; herb...@gondor.apana.org.au; >>jd...@linux.intel.com >>Subject: Re: [PATCH v15 00/17] Provide a zero-copy method on KVM virtio-net. >> >>From: xiaohui@intel.com >>Date: Wed, 10 Nov 2010 17:23:28 +0800 >> >>> From: Xin Xiaohui >>> >>>>2) The idea to key off of skb->dev in skb_release_data() is >>>> fundamentally flawed since many actions can change skb->dev on you, >>>> which will end up causing a leak of your external data areas. >>> >>> How about this one? If the destructor_arg is not a good candidate, >>> then I have to add an apparent field in shinfo. >> >>If destructor_arg is actually a net_device pointer or similar, >>you will need to take a reference count on it or similar. >> >Do you mean destructor_arg will be consumed by other user? >If that case, may I add a new structure member in shinfo? >Thus only zero-copy will use it, and no need for the reference count. > How about this? It really needs somewhere to track the external data area, and if something wrong with it, we can also release the data area. We think skb_release_data() is the right place to deal with it. If I understood right, that destructor_arg will be used by other else that why reference count is needed, then how about add a new structure member in shinfo? Thanks Xiaohui >>Which means --> good bye performance especially on SMP. >> >>You're going to be adding new serialization points and at >>least two new atomics per packet. >-- >To unsubscribe from this list: send the line "unsubscribe kvm" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v16 14/17]Add a kconfig entry and make entry for mp device.
>-Original Message- >From: Randy Dunlap [mailto:randy.dun...@oracle.com] >Sent: Thursday, December 02, 2010 1:54 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [PATCH v16 14/17]Add a kconfig entry and make entry for mp device. > >On Wed, 1 Dec 2010 16:08:25 +0800 xiaohui....@intel.com wrote: > >> From: Xin Xiaohui >> >> Signed-off-by: Xin Xiaohui >> Reviewed-by: Jeff Dike >> --- >> drivers/vhost/Kconfig | 10 ++ >> drivers/vhost/Makefile |2 ++ >> 2 files changed, 12 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig >> index e4e2fd1..a6b8cbf 100644 >> --- a/drivers/vhost/Kconfig >> +++ b/drivers/vhost/Kconfig >> @@ -9,3 +9,13 @@ config VHOST_NET >>To compile this driver as a module, choose M here: the module will >>be called vhost_net. >> >> +config MEDIATE_PASSTHRU >> +tristate "mediate passthru network driver (EXPERIMENTAL)" >> +depends on VHOST_NET >> +---help--- >> + zerocopy network I/O support, we call it as mediate passthru to > > support; we call it "mediate passthru" to > >> + be distiguish with hardare passthru. > > distinguish it from hardware passthru. > Thanks. I will modify that. Thanks Xiaohui >> + >> + To compile this driver as a module, choose M here: the module will >> + be called mpassthru. >> + > > >--- >~Randy >*** Remember to use Documentation/SubmitChecklist when testing your code *** -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: vhost-net patches
Hi, Michael, What's your deferring skb allocation patch mentioned here, may you elaborate it a little more detailed? Thanks Xiaohui -Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Michael S. Tsirkin Sent: Tuesday, November 03, 2009 3:47 AM To: Shirley Ma Cc: Sridhar Samudrala; Shirley Ma; David Stevens; kvm@vger.kernel.org; s...@linux.vnet.ibm.com; mashi...@linux.vnet.ibm.com Subject: Re: vhost-net patches On Thu, Oct 29, 2009 at 12:11:33AM -0700, Shirley Ma wrote: > Hello Michael, > > I am able to get 63xxMb/s throughput with 10% less cpu utilization when > I apply deferring skb patch on top of your most recent vhost patch. The > userspace TCP_STREAM BW used to be 3xxxMb/s from upper stream git tree. > > After applying your recent vhost patch, it goes up to 53xxMb/s. Thanks for the report! I have pushed this patch (after cleaning it up and fixing raw sockets), and updated the numbers in the wiki here http://www.linux-kvm.org/page/VhostNet -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ANNOUNCE] KVM developer conference call
Hi, Chris We are interested in joining the conference, since we are now working on the zero copy patch based on vhost-net. Thanks Xiaohui -Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Chris Wright Sent: Tuesday, January 12, 2010 10:16 AM To: kvm@vger.kernel.org Cc: qemu-de...@nongnu.org Subject: [ANNOUNCE] KVM developer conference call The KVM project is announcing a regular conference call focused on KVM development issues. The call will take place weekly on Tuesdays at 15:00 UTC for 1 hour. The purpose of the call is to discuss relevant development issues in a high bandwidth forum, certainly _not_ meant to supplant discussion on list. I will send out a request for agenda items each week the day before the call, and post minutes after the call concludes. If you are interested in joining please send me an email. thanks, -chris -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>-Original Message- >From: Herbert Xu [mailto:herb...@gondor.apana.org.au] >Sent: Sunday, June 27, 2010 2:15 PM >To: Dong, Eddie >Cc: Xin, Xiaohui; Stephen Hemminger; net...@vger.kernel.org; >kvm@vger.kernel.org; >linux-ker...@vger.kernel.org; m...@redhat.com; mi...@elte.hu; >da...@davemloft.net; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from >external. > >On Fri, Jun 25, 2010 at 09:03:46AM +0800, Dong, Eddie wrote: >> >> In current patch, each SKB for the assigned device (SRIOV VF or NIC or a >> complete >queue pairs) uses the buffer from guest, so it eliminates copy completely in >software and >requires hardware to do so. If we can have an additonal place to store the >buffer per skb (may >cause copy later on), we can do copy later on or re-post the buffer to >assigned NIC driver >later on. But that may be not very clean either :( > >OK, if I understand you correctly then I don't think have a >problem. With your current patch-set you have exactly the same >situation when the skb->data is reallocated as a kernel buffer. > When will skb->data to be reallocated? May you point me the code path? >This is OK because as you correctly argue, it is a rare situation. > >With my proposal you will need to get this extra external buffer >in even less cases, because you'd only need to do it if the skb >head grows, which only happens if it becomes encapsulated. >So let me explain it in a bit more detail: > >Our packet starts out as a purely non-linear skb, i.e., skb->head >contains nothing and all the page frags come from the guest. > >During host processing we may pull data into skb->head but the >first frag will remain unless we pull all of it. If we did do >that then you would have a free external buffer anyway. > >Now in the common case the header may be modified or pulled, but >it very rarely grows. So you can just copy the header back into >the first frag just before we give it to the guest. > Since the data is still there, so recompute the page offset and size is ok, right? >Only in the case where the packet header grows (e.g., encapsulation) >would you need to get an extra external buffer. > >Cheers, >-- >Email: Herbert Xu >Home Page: http://gondor.apana.org.au/~herbert/ >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v8 00/16] Provide a zero-copy method on KVM virtio-net.
>Hello Xiaohui, > >On Thu, 2010-07-29 at 19:14 +0800, xiaohui@intel.com wrote: >> The idea is simple, just to pin the guest VM user space and then >> let host NIC driver has the chance to directly DMA to it. >> The patches are based on vhost-net backend driver. We add a device >> which provides proto_ops as sendmsg/recvmsg to vhost-net to >> send/recv directly to/from the NIC driver. KVM guest who use the >> vhost-net backend may bind any ethX interface in the host side to >> get copyless data transfer thru guest virtio-net frontend. > >Since vhost-net already supports macvtap/tun backends, do you think >whether it's better to implement zero copy in macvtap/tun than inducing >a new media passthrough device here? > I'm not sure if there will be more duplicated code in the kernel. >> Our goal is to improve the bandwidth and reduce the CPU usage. >> Exact performance data will be provided later. > >I did some vhost performance measurement over 10Gb ixgbe, and found that >in order to get consistent BW results, netperf/netserver, qemu, vhost >threads smp affinities are required. > >Looking forward to these results for small message size comparison. For >large message size 10Gb ixgbe BW already reached by doing vhost smp >affinity w/i offloading support, we will see how much CPU utilization it >can be reduced. > >Please provide latency results as well. I did some experimental on >macvtap zero copy sendmsg, what I have found that get_user_pages latency >pretty high. > Ok, I will try that. >Thanks >Shirley > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v8 00/16] Provide a zero-copy method on KVM virtio-net.
>-Original Message- >From: Shirley Ma [mailto:mashi...@us.ibm.com] >Sent: Friday, July 30, 2010 6:31 AM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v8 00/16] Provide a zero-copy method on KVM virtio-net. > >Hello Xiaohui, > >On Thu, 2010-07-29 at 19:14 +0800, xiaohui@intel.com wrote: >> The idea is simple, just to pin the guest VM user space and then >> let host NIC driver has the chance to directly DMA to it. >> The patches are based on vhost-net backend driver. We add a device >> which provides proto_ops as sendmsg/recvmsg to vhost-net to >> send/recv directly to/from the NIC driver. KVM guest who use the >> vhost-net backend may bind any ethX interface in the host side to >> get copyless data transfer thru guest virtio-net frontend. > >Since vhost-net already supports macvtap/tun backends, do you think >whether it's better to implement zero copy in macvtap/tun than inducing >a new media passthrough device here? > >> Our goal is to improve the bandwidth and reduce the CPU usage. >> Exact performance data will be provided later. > >I did some vhost performance measurement over 10Gb ixgbe, and found that >in order to get consistent BW results, netperf/netserver, qemu, vhost >threads smp affinities are required. > >Looking forward to these results for small message size comparison. For >large message size 10Gb ixgbe BW already reached by doing vhost smp >affinity w/i offloading support, we will see how much CPU utilization it >can be reduced. > >Please provide latency results as well. I did some experimental on >macvtap zero copy sendmsg, what I have found that get_user_pages latency >pretty high. > May you share me with your performance results (including BW and latency)on vhost-net and how you get them(your configuration and especially with the affinity settings)? Thanks Xiaohui >Thanks >Shirley > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v8 00/16] Provide a zero-copy method on KVM virtio-net.
Herbert, The v8 patches are modified mostly based on your comments about napi_gro_frags interface. How do you think about the patches about net core system part? We know currently there are some comments about the mp device, such as to support zero-copy for tun/tap and macvtap. Since there isn't a decision yet about it. May you give comments about the net core system first, since this part is all the same for zero-copy. Thanks Xiaohui >-Original Message- >From: linux-kernel-ow...@vger.kernel.org >[mailto:linux-kernel-ow...@vger.kernel.org] On >Behalf Of xiaohui@intel.com >Sent: Thursday, July 29, 2010 7:15 PM >To: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: [RFC PATCH v8 00/16] Provide a zero-copy method on KVM virtio-net. > >We provide an zero-copy method which driver side may get external >buffers to DMA. Here external means driver don't use kernel space >to allocate skb buffers. Currently the external buffer can be from >guest virtio-net driver. > >The idea is simple, just to pin the guest VM user space and then >let host NIC driver has the chance to directly DMA to it. >The patches are based on vhost-net backend driver. We add a device >which provides proto_ops as sendmsg/recvmsg to vhost-net to >send/recv directly to/from the NIC driver. KVM guest who use the >vhost-net backend may bind any ethX interface in the host side to >get copyless data transfer thru guest virtio-net frontend. > >patch 01-10: net core and kernel changes. >patch 11-13: new device as interface to mantpulate external buffers. >patch 14: for vhost-net. >patch 15: An example on modifying NIC driver to using napi_gro_frags(). >patch 16: An example how to get guest buffers based on driver > who using napi_gro_frags(). > >The guest virtio-net driver submits multiple requests thru vhost-net >backend driver to the kernel. And the requests are queued and then >completed after corresponding actions in h/w are done. > >For read, user space buffers are dispensed to NIC driver for rx when >a page constructor API is invoked. Means NICs can allocate user buffers >from a page constructor. We add a hook in netif_receive_skb() function >to intercept the incoming packets, and notify the zero-copy device. > >For write, the zero-copy deivce may allocates a new host skb and puts >payload on the skb_shinfo(skb)->frags, and copied the header to skb->data. >The request remains pending until the skb is transmitted by h/w. > >We provide multiple submits and asynchronous notifiicaton to >vhost-net too. > >Our goal is to improve the bandwidth and reduce the CPU usage. >Exact performance data will be provided later. > >What we have not done yet: > Performance tuning > >what we have done in v1: > polish the RCU usage > deal with write logging in asynchroush mode in vhost > add notifier block for mp device > rename page_ctor to mp_port in netdevice.h to make it looks generic > add mp_dev_change_flags() for mp device to change NIC state > add CONIFG_VHOST_MPASSTHRU to limit the usage when module is not load > a small fix for missing dev_put when fail > using dynamic minor instead of static minor number > a __KERNEL__ protect to mp_get_sock() > >what we have done in v2: > > remove most of the RCU usage, since the ctor pointer is only > changed by BIND/UNBIND ioctl, and during that time, NIC will be > stopped to get good cleanup(all outstanding requests are finished), > so the ctor pointer cannot be raced into wrong situation. > > Remove the struct vhost_notifier with struct kiocb. > Let vhost-net backend to alloc/free the kiocb and transfer them > via sendmsg/recvmsg. > > use get_user_pages_fast() and set_page_dirty_lock() when read. > > Add some comments for netdev_mp_port_prep() and handle_mpassthru(). > >what we have done in v3: > the async write logging is rewritten > a drafted synchronous write function for qemu live migration > a limit for locked pages from get_user_pages_fast() to prevent Dos > by using RLIMIT_MEMLOCK > > >what we have done in v4: > add iocb completion callback from vhost-net to queue iocb in mp device > replace vq->receiver by mp_sock_data_ready() > remove stuff in mp device which access structures from vhost-net > modify skb_reserve() to ignore host NIC driver reserved space > rebase to the latest vhost tree > split large patches into small pieces, especially for net core part. > > >what we have done in v5: > address Arnd Bergmann's comments > -remove IFF_MPASSTHRU_EXCL flag in mp device > -Add CONFIG_COMPAT macro > -remove mp_release ops > move dev_is_mpassthru() as inline func > fix a bug in memory relinquish >
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
Michael, Sorry to reply the mail late. >So - does this driver help reduce service demand signifiantly? I'm looking at the performance now. >Some comments from looking at the code: > >On Fri, Aug 06, 2010 at 05:23:41PM +0800, xiaohui@intel.com wrote: >> +static struct page_info *alloc_page_info(struct page_ctor *ctor, >> +struct kiocb *iocb, struct iovec *iov, >> +int count, struct frag *frags, >> +int npages, int total) >> +{ >> +int rc; >> +int i, j, n = 0; >> +int len; >> +unsigned long base, lock_limit; >> +struct page_info *info = NULL; >> + >> +lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur; >> +lock_limit >>= PAGE_SHIFT; > >Playing with rlimit on data path, transparently to the application in this way >looks strange to me, I suspect this has unexpected security implications. >Further, applications may have other uses for locked memory >besides mpassthru - you should not just take it because it's there. > >Can we have an ioctl that lets userspace configure how much >memory to lock? This ioctl will decrement the rlimit and store >the data in the device structure so we can do accounting >internally. Put it back on close or on another ioctl. Yes, we can decrement the rlimit in ioctl in one time to avoid data path. >Need to be careful for when this operation gets called >again with 0 or another small value while we have locked memory - >maybe just fail with EBUSY? or wait until it gets unlocked? >Maybe 0 can be special-cased and deactivate zero-copy?. > In fact, if we choose RLIMIT_MEMLOCK to limit the lock memory, the default value is only 16 pages. It's too small to make the device to work. So we always to configure it with a large value. I think, if rlimit value after decrement is < 0, then deactivate zero-copy is better. 0 maybe ok. >> + >> +if (ctor->lock_pages + count > lock_limit && npages) { >> +printk(KERN_INFO "exceed the locked memory rlimit."); >> +return NULL; >> +} >> + >> +info = kmem_cache_zalloc(ext_page_info_cache, GFP_KERNEL); > >You seem to fill in all memory, why zalloc? this is data path ... Ok, Let me check this. > >> + >> +if (!info) >> +return NULL; >> + >> +for (i = j = 0; i < count; i++) { >> +base = (unsigned long)iov[i].iov_base; >> +len = iov[i].iov_len; >> + >> +if (!len) >> +continue; >> +n = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT; >> + >> +rc = get_user_pages_fast(base, n, npages ? 1 : 0, > >npages controls whether this is a write? Why? We use npages as a flag here. In mp_sendmsg(), we called alloc_page_info() with npages = 0. > >> +&info->pages[j]); >> +if (rc != n) >> +goto failed; >> + >> +while (n--) { >> +frags[j].offset = base & ~PAGE_MASK; >> +frags[j].size = min_t(int, len, >> +PAGE_SIZE - frags[j].offset); >> +len -= frags[j].size; >> +base += frags[j].size; >> +j++; >> +} >> +} >> + >> +#ifdef CONFIG_HIGHMEM >> +if (npages && !(dev->features & NETIF_F_HIGHDMA)) { >> +for (i = 0; i < j; i++) { >> +if (PageHighMem(info->pages[i])) >> +goto failed; >> +} >> +} >> +#endif > >Are non-highdma devices worth bothering with? If yes - >are there other limitations devices might have that we need to handle? >E.g. what about non-s/g devices or no checksum offloading?. Basically I think there is no limitations for both, but let me have a check. > >> +skb_push(skb, ETH_HLEN); >> + >> +if (skb_is_gso(skb)) { >> +hdr.hdr.hdr_len = skb_headlen(skb); >> +hdr.hdr.gso_size = shinfo->gso_size; >> +if (shinfo->gso_type & SKB_GSO_TCPV4) >> +hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4; >> +else if (shinfo->gso_type & SKB_GSO_TCPV6) >> +hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6; >> +else if (shinfo->gso_type & SKB_GSO_UDP) >> +hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP; >> +else >> +BUG(); >> +if (shinfo->gso_type & SKB_GSO_TCP_ECN) >> +hdr.hdr.gso_type |= VIRTIO_NET_HDR_GSO_ECN; >> + >> +} else >> +hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE; >> + >> +if (skb->ip_summed == CHECKSUM_PARTIAL) { >> +hdr.hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM; >> +hdr.hdr.csum_start = >> +skb->csum_start - skb_headroom(skb); >> +hdr.hdr
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>>Playing with rlimit on data path, transparently to the application in this way >>looks strange to me, I suspect this has unexpected security implications. >>Further, applications may have other uses for locked memory >>besides mpassthru - you should not just take it because it's there. >> >>Can we have an ioctl that lets userspace configure how much >>memory to lock? This ioctl will decrement the rlimit and store >>the data in the device structure so we can do accounting >>internally. Put it back on close or on another ioctl. >Yes, we can decrement the rlimit in ioctl in one time to avoid >data path. > >>Need to be careful for when this operation gets called >>again with 0 or another small value while we have locked memory - >>maybe just fail with EBUSY? or wait until it gets unlocked? >>Maybe 0 can be special-cased and deactivate zero-copy?. >> How about we don't use a new ioctl, but just check the rlimit in one MPASSTHRU_BINDDEV ioctl? If we find mp device break the rlimit, then we fail the bind ioctl, and thus can't do zero copy any more. >In fact, if we choose RLIMIT_MEMLOCK to limit the lock memory, >the default value is only 16 pages. It's too small to make the device to >work. So we always to configure it with a large value. >I think, if rlimit value after decrement is < 0, then deactivate zero-copy >is better. 0 maybe ok. > >>> + >>> + if (ctor->lock_pages + count > lock_limit && npages) { >>> + printk(KERN_INFO "exceed the locked memory rlimit."); >>> + return NULL; >>> + } >>> + >>> + info = kmem_cache_zalloc(ext_page_info_cache, GFP_KERNEL); >> >>You seem to fill in all memory, why zalloc? this is data path ... > >Ok, Let me check this. > >> >>> + >>> + if (!info) >>> + return NULL; >>> + >>> + for (i = j = 0; i < count; i++) { >>> + base = (unsigned long)iov[i].iov_base; >>> + len = iov[i].iov_len; >>> + >>> + if (!len) >>> + continue; >>> + n = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT; >>> + >>> + rc = get_user_pages_fast(base, n, npages ? 1 : 0, >> >>npages controls whether this is a write? Why? > >We use npages as a flag here. In mp_sendmsg(), we called alloc_page_info() >with npages = >0. > >> >>> + &info->pages[j]); >>> + if (rc != n) >>> + goto failed; >>> + >>> + while (n--) { >>> + frags[j].offset = base & ~PAGE_MASK; >>> + frags[j].size = min_t(int, len, >>> + PAGE_SIZE - frags[j].offset); >>> + len -= frags[j].size; >>> + base += frags[j].size; >>> + j++; >>> + } >>> + } >>> + >>> +#ifdef CONFIG_HIGHMEM >>> + if (npages && !(dev->features & NETIF_F_HIGHDMA)) { >>> + for (i = 0; i < j; i++) { >>> + if (PageHighMem(info->pages[i])) >>> + goto failed; >>> + } >>> + } >>> +#endif >> >>Are non-highdma devices worth bothering with? If yes - >>are there other limitations devices might have that we need to handle? >>E.g. what about non-s/g devices or no checksum offloading?. > >Basically I think there is no limitations for both, but let me have a check. > >> >>> + skb_push(skb, ETH_HLEN); >>> + >>> + if (skb_is_gso(skb)) { >>> + hdr.hdr.hdr_len = skb_headlen(skb); >>> + hdr.hdr.gso_size = shinfo->gso_size; >>> + if (shinfo->gso_type & SKB_GSO_TCPV4) >>> + hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4; >>> + else if (shinfo->gso_type & SKB_GSO_TCPV6) >>> + hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6; >>> + else if (shinfo->gso_type & SKB_GSO_UDP) >>> + hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP; >>> + else >>> + BUG(); >>> + if (shinfo->gso_type & SKB_GSO_TCP_ECN) >>> + hdr.hdr.gso_type |= VIRTIO_NET_HDR_GSO_ECN; >>> + >>> + } else >>> + hdr.hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE; >>> + >>> + if (skb->ip_summed == CHECKSUM_PARTIAL) { >>> + hdr.hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM; >>> + hdr.hdr.csum_start = >>> + skb->csum_start - skb_headroom(skb); >>> + hdr.hdr.csum_offset = skb->csum_offset; >>> + } >> >>We have this code in tun, macvtap and packet socket already. >>Could this be a good time to move these into networking core? >>I'm not asking you to do this right now, but could this generic >>virtio-net to skb stuff be encapsulated in functions? > >It seems reasonable. > >> >>-- >>MST >-- >To unsubscribe from this list: send the line "unsubscribe netdev" in >the body of a message to majord...@vger.kern
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>>> + >>> + if (ctor->lock_pages + count > lock_limit && npages) { >>> + printk(KERN_INFO "exceed the locked memory rlimit."); >>> + return NULL; >>> + } >>> + >>> + info = kmem_cache_zalloc(ext_page_info_cache, GFP_KERNEL); >> >>You seem to fill in all memory, why zalloc? this is data path ... > >Ok, Let me check this. It's mainly for info->next and info->prev, these two fields will be used in hash functions. But you are right, since most fields will be refilled. The new version includes the fix. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host kernel
>From: Shirley Ma [mailto:mashi...@us.ibm.com] >Sent: Tuesday, September 14, 2010 11:05 PM >To: Avi Kivity >Cc: David Miller; a...@arndb.de; m...@redhat.com; Xin, Xiaohui; >net...@vger.kernel.org; >kvm@vger.kernel.org; linux-ker...@vger.kernel.org >Subject: Re: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host >kernel > >On Tue, 2010-09-14 at 11:12 +0200, Avi Kivity wrote: >> >> +base = (unsigned long)from->iov_base + offset1; >> >> +size = ((base& ~PAGE_MASK) + len + ~PAGE_MASK)>> >> PAGE_SHIFT; >> >> +num_pages = get_user_pages_fast(base, size, >> 0,&page[i]); >> >> +if ((num_pages != size) || >> >> +(num_pages> MAX_SKB_FRAGS - >> skb_shinfo(skb)->nr_frags)) >> >> +/* put_page is in skb free */ >> >> +return -EFAULT; >> > What keeps the user from writing to these pages in it's address >> space >> > after the write call returns? >> > >> > A write() return of success means: >> > >> > "I wrote what you gave to me" >> > >> > not >> > >> > "I wrote what you gave to me, oh and BTW don't touch these >> > pages for a while." >> > >> > In fact "a while" isn't even defined in any way, as there is no way >> > for the write() invoker to know when the networking card is done >> with >> > those pages. >> >> That's what io_submit() is for. Then io_getevents() tells you what >> "a >> while" actually was. > >This macvtap zero copy uses iov buffers from vhost ring, which is >allocated from guest kernel. In host kernel, vhost calls macvtap >sendmsg. macvtap sendmsg calls get_user_pages_fast to pin these buffers' >pages for zero copy. > >The patch is relying on how vhost handle these buffers. I need to look >at vhost code (qemu) first for addressing the questions here. > >Thanks >Shirley I think what David said is what we have thought before in mp device. Since we are not sure the exact time the tx buffer was wrote though DMA operation. But the deadline is when the tx buffer was freed. So we only notify the vhost stuff about the write when tx buffer freed. But the deadline is maybe too late for performance. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host kernel
>From: Arnd Bergmann [mailto:a...@arndb.de] >Sent: Tuesday, September 14, 2010 11:21 PM >To: Shirley Ma >Cc: Avi Kivity; David Miller; m...@redhat.com; Xin, Xiaohui; >net...@vger.kernel.org; >kvm@vger.kernel.org; linux-ker...@vger.kernel.org >Subject: Re: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host >kernel > >On Tuesday 14 September 2010, Shirley Ma wrote: >> On Tue, 2010-09-14 at 11:12 +0200, Avi Kivity wrote: >> >> > That's what io_submit() is for. Then io_getevents() tells you what >> > "a >> > while" actually was. >> >> This macvtap zero copy uses iov buffers from vhost ring, which is >> allocated from guest kernel. In host kernel, vhost calls macvtap >> sendmsg. macvtap sendmsg calls get_user_pages_fast to pin these buffers' >> pages for zero copy. >> >> The patch is relying on how vhost handle these buffers. I need to look >> at vhost code (qemu) first for addressing the questions here. > >I guess the best solution would be to make macvtap_aio_write return >-EIOCBQUEUED when a packet gets passed down to the adapter, and >call aio_complete when the adapter is done with it. > >This would change the regular behavior of macvtap into a model where >every write on the file blocks until the packet has left the machine, >which gives us better flow control, but does slow down the traffic >when we only put one packet at a time into the queue. > >It also allows the user to call io_submit instead of write in order >to do an asynchronous submission as Avi was suggesting. > But currently, this patch is communicated with vhost-net, which is almost in the kernel side. If it uses aio stuff, it should be communicate with user space Backend. > Arnd -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host kernel
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Wednesday, September 15, 2010 12:30 AM >To: Shirley Ma >Cc: Arnd Bergmann; Avi Kivity; Xin, Xiaohui; David Miller; >net...@vger.kernel.org; >kvm@vger.kernel.org; linux-ker...@vger.kernel.org >Subject: Re: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host >kernel > >On Tue, Sep 14, 2010 at 09:00:25AM -0700, Shirley Ma wrote: >> On Tue, 2010-09-14 at 17:22 +0200, Michael S. Tsirkin wrote: >> > I would expect this to hurt performance significantly. >> > We could do this for asynchronous requests only to avoid the >> > slowdown. >> >> Is kiocb in sendmsg helpful here? It is not used now. >> >> Shirley > >Precisely. This is what the patch from Xin Xiaohui does. That code >already seems to do most of what you are trying to do, right? > >The main thing missing seems to be macvtap integration, so that we can fall >back >on data copy if zero copy is unavailable? >How hard would it be to basically link the mp and macvtap modules >together to get us this functionality? Anyone? > Michael, Is to support macvtap with zero-copy through mp device the functionality you mentioned above? Before Shirley Ma has suggested to move the zero-copy functionality into tun/tap device or macvtap device. How do you think about that? I suspect there will be a lot of duplicate code in that three drivers except we can extract code of zero-copy into kernel APIs and vhost APIs. Do you think that's worth to do and help current process which is blocked too long than I expected? > >-- >MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host kernel
>From: Shirley Ma [mailto:mashi...@us.ibm.com] >Sent: Wednesday, September 15, 2010 10:41 AM >To: Xin, Xiaohui >Cc: Avi Kivity; David Miller; a...@arndb.de; m...@redhat.com; >net...@vger.kernel.org; >kvm@vger.kernel.org; linux-ker...@vger.kernel.org >Subject: RE: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host >kernel > >On Wed, 2010-09-15 at 09:50 +0800, Xin, Xiaohui wrote: >> I think what David said is what we have thought before in mp device. >> Since we are not sure the exact time the tx buffer was wrote though >> DMA operation. >> But the deadline is when the tx buffer was freed. So we only notify >> the vhost stuff >> about the write when tx buffer freed. But the deadline is maybe too >> late for performance. > >Have you tried it? If so what's the performance penalty you have seen by >notifying vhost when tx buffer freed? > We did not try it before, as we cared RX side more. >I am thinking to have a callback in skb destructor, >vhost_add_used_and_signal gets updated when skb is actually freed, vhost >vq & head need to be passed to the callback. This might requires vhost >ring size is at least as big as the lower device driver. > That's almost the same what we have done except we use destructor_arg and another callback.. >Thanks >Shirley -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v10 00/16] Provide a zero-copy method on KVM virtio-net.
Herbert, Any comments on the modifications of the net core and driver side of this patch? Thanks Xiaohui >-Original Message- >From: linux-kernel-ow...@vger.kernel.org >[mailto:linux-kernel-ow...@vger.kernel.org] On >Behalf Of xiaohui@intel.com >Sent: Saturday, September 11, 2010 5:53 PM >To: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >m...@redhat.com; mi...@elte.hu; da...@davemloft.net; >herb...@gondor.apana.org.au; >jd...@linux.intel.com >Subject: [RFC PATCH v10 00/16] Provide a zero-copy method on KVM virtio-net. > >We provide an zero-copy method which driver side may get external >buffers to DMA. Here external means driver don't use kernel space >to allocate skb buffers. Currently the external buffer can be from >guest virtio-net driver. > >The idea is simple, just to pin the guest VM user space and then >let host NIC driver has the chance to directly DMA to it. >The patches are based on vhost-net backend driver. We add a device >which provides proto_ops as sendmsg/recvmsg to vhost-net to >send/recv directly to/from the NIC driver. KVM guest who use the >vhost-net backend may bind any ethX interface in the host side to >get copyless data transfer thru guest virtio-net frontend. > >patch 01-10: net core and kernel changes. >patch 11-13: new device as interface to mantpulate external buffers. >patch 14: for vhost-net. >patch 15: An example on modifying NIC driver to using napi_gro_frags(). >patch 16: An example how to get guest buffers based on driver > who using napi_gro_frags(). > >The guest virtio-net driver submits multiple requests thru vhost-net >backend driver to the kernel. And the requests are queued and then >completed after corresponding actions in h/w are done. > >For read, user space buffers are dispensed to NIC driver for rx when >a page constructor API is invoked. Means NICs can allocate user buffers >from a page constructor. We add a hook in netif_receive_skb() function >to intercept the incoming packets, and notify the zero-copy device. > >For write, the zero-copy deivce may allocates a new host skb and puts >payload on the skb_shinfo(skb)->frags, and copied the header to skb->data. >The request remains pending until the skb is transmitted by h/w. > >We provide multiple submits and asynchronous notifiicaton to >vhost-net too. > >Our goal is to improve the bandwidth and reduce the CPU usage. >Exact performance data will be provided later. > >What we have not done yet: > Performance tuning > >what we have done in v1: > polish the RCU usage > deal with write logging in asynchroush mode in vhost > add notifier block for mp device > rename page_ctor to mp_port in netdevice.h to make it looks generic > add mp_dev_change_flags() for mp device to change NIC state > add CONIFG_VHOST_MPASSTHRU to limit the usage when module is not load > a small fix for missing dev_put when fail > using dynamic minor instead of static minor number > a __KERNEL__ protect to mp_get_sock() > >what we have done in v2: > > remove most of the RCU usage, since the ctor pointer is only > changed by BIND/UNBIND ioctl, and during that time, NIC will be > stopped to get good cleanup(all outstanding requests are finished), > so the ctor pointer cannot be raced into wrong situation. > > Remove the struct vhost_notifier with struct kiocb. > Let vhost-net backend to alloc/free the kiocb and transfer them > via sendmsg/recvmsg. > > use get_user_pages_fast() and set_page_dirty_lock() when read. > > Add some comments for netdev_mp_port_prep() and handle_mpassthru(). > >what we have done in v3: > the async write logging is rewritten > a drafted synchronous write function for qemu live migration > a limit for locked pages from get_user_pages_fast() to prevent Dos > by using RLIMIT_MEMLOCK > > >what we have done in v4: > add iocb completion callback from vhost-net to queue iocb in mp device > replace vq->receiver by mp_sock_data_ready() > remove stuff in mp device which access structures from vhost-net > modify skb_reserve() to ignore host NIC driver reserved space > rebase to the latest vhost tree > split large patches into small pieces, especially for net core part. > > >what we have done in v5: > address Arnd Bergmann's comments > -remove IFF_MPASSTHRU_EXCL flag in mp device > -Add CONFIG_COMPAT macro > -remove mp_release ops > move dev_is_mpassthru() as inline func > fix a bug in memory relinquish > Apply to current git (2.6.34-rc6) tree. > >what we have done in v6: > move create_iocb() out of page_dtor which may happen in interrupt > context > -This remove the potential issues which lock called in interrupt context > make the cache used by mp, vhost as static, and created/destoryed during >
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Sunday, September 12, 2010 9:37 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. > >On Sat, Sep 11, 2010 at 03:41:14PM +0800, Xin, Xiaohui wrote: >> >>Playing with rlimit on data path, transparently to the application in this >> >>way >> >>looks strange to me, I suspect this has unexpected security implications. >> >>Further, applications may have other uses for locked memory >> >>besides mpassthru - you should not just take it because it's there. >> >> >> >>Can we have an ioctl that lets userspace configure how much >> >>memory to lock? This ioctl will decrement the rlimit and store >> >>the data in the device structure so we can do accounting >> >>internally. Put it back on close or on another ioctl. >> >Yes, we can decrement the rlimit in ioctl in one time to avoid >> >data path. >> > >> >>Need to be careful for when this operation gets called >> >>again with 0 or another small value while we have locked memory - >> >>maybe just fail with EBUSY? or wait until it gets unlocked? >> >>Maybe 0 can be special-cased and deactivate zero-copy?. >> >> >> >> How about we don't use a new ioctl, but just check the rlimit >> in one MPASSTHRU_BINDDEV ioctl? If we find mp device >> break the rlimit, then we fail the bind ioctl, and thus can't do >> zero copy any more. > >Yes, and not just check, but decrement as well. >I think we should give userspace control over >how much memory we can lock and subtract from the rlimit. >It's OK to add this as a parameter to MPASSTHRU_BINDDEV. >Then increment the rlimit back on unbind and on close? > >This opens up an interesting condition: process 1 >calls bind, process 2 calls unbind or close. >This will increment rlimit for process 2. >Not sure how to fix this properly. > I can't too, can we do any synchronous operations on rlimit stuff? I quite suspect in it. >-- >MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host kernel
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Wednesday, September 15, 2010 5:59 PM >To: Xin, Xiaohui >Cc: Shirley Ma; Arnd Bergmann; Avi Kivity; David Miller; >net...@vger.kernel.org; >kvm@vger.kernel.org; linux-ker...@vger.kernel.org >Subject: Re: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host >kernel > >On Wed, Sep 15, 2010 at 10:46:02AM +0800, Xin, Xiaohui wrote: >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >Sent: Wednesday, September 15, 2010 12:30 AM >> >To: Shirley Ma >> >Cc: Arnd Bergmann; Avi Kivity; Xin, Xiaohui; David Miller; >> >net...@vger.kernel.org; >> >kvm@vger.kernel.org; linux-ker...@vger.kernel.org >> >Subject: Re: [RFC PATCH 2/2] macvtap: TX zero copy between guest and host >> >kernel >> > >> >On Tue, Sep 14, 2010 at 09:00:25AM -0700, Shirley Ma wrote: >> >> On Tue, 2010-09-14 at 17:22 +0200, Michael S. Tsirkin wrote: >> >> > I would expect this to hurt performance significantly. >> >> > We could do this for asynchronous requests only to avoid the >> >> > slowdown. >> >> >> >> Is kiocb in sendmsg helpful here? It is not used now. >> >> >> >> Shirley >> > >> >Precisely. This is what the patch from Xin Xiaohui does. That code >> >already seems to do most of what you are trying to do, right? >> > >> >The main thing missing seems to be macvtap integration, so that we can fall >> >back >> >on data copy if zero copy is unavailable? >> >How hard would it be to basically link the mp and macvtap modules >> >together to get us this functionality? Anyone? >> > >> Michael, >> Is to support macvtap with zero-copy through mp device the functionality >> you mentioned above? > >I have trouble parsing the above question. At some point Arnd suggested >that the mp device functionality would fit nicely as part of the macvtap >driver. It seems to make sense superficially, the advantage if it >worked would be that we would get zero copy (mostly) transparently. > >Do you agree with this goal? > I would say yes. >> Before Shirley Ma has suggested to move the zero-copy functionality into >> tun/tap device or macvtap device. How do you think about that? >> I suspect >> there will be a lot of duplicate code in that three drivers except we can >> extract >> code of zero-copy into kernel APIs and vhost APIs. > > >tap would be very hard at this point as it does not bind to a device. >macvtap might work, we mainly need to figure out a way to detect that >device can do zero copy so the right mode is used. I think a first step >could be to simply link mp code into macvtap module, pass necessary >ioctls on, then move some code around as necessary. This might get rid >of code duplication nicely. I'll look into this to see how much effort would be. > > >> Do you think that's worth to do and help current process which is blocked too >> long than I expected? > >I think it's nice to have. > >And if done hopefully this will get the folk working on the macvtap >driver to review the code, which will help find all issues faster. > >I think if you post some performance numbers, >this will also help get people excited and looking at the code. > The performance data I have posted before is compared with raw socket on vhost-net. But currently, the raw socket backend is removed from the qemu side. So I can only compare with tap on vhost-net. But unfortunately, I missed something that I even can't bring it up. I was blocked by this for a time. >I also don't see the process as completely blocked, each review round points >out more issues: we aren't going back and forth changing >same lines again and again, are we? > >One thing that might help is increase the frequency of updates, >try sending them out sooner. >On the other hand 10 new patches each revision is a lot: >if there is a part of patchset that has stabilised you can split it out, >post once and keep posting the changing part separately. > >I hope these suggestions help. Thanks, Michael! > >> > >> >-- >> >MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH v9 12/16] Add mp(mediate passthru) device.
>From: Michael S. Tsirkin [mailto:m...@redhat.com] >Sent: Wednesday, September 15, 2010 7:28 PM >To: Xin, Xiaohui >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; linux-ker...@vger.kernel.org; >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >jd...@linux.intel.com >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. > >On Wed, Sep 15, 2010 at 11:13:44AM +0800, Xin, Xiaohui wrote: >> >From: Michael S. Tsirkin [mailto:m...@redhat.com] >> >Sent: Sunday, September 12, 2010 9:37 PM >> >To: Xin, Xiaohui >> >Cc: net...@vger.kernel.org; kvm@vger.kernel.org; >> >linux-ker...@vger.kernel.org; >> >mi...@elte.hu; da...@davemloft.net; herb...@gondor.hengli.com.au; >> >jd...@linux.intel.com >> >Subject: Re: [RFC PATCH v9 12/16] Add mp(mediate passthru) device. >> > >> >On Sat, Sep 11, 2010 at 03:41:14PM +0800, Xin, Xiaohui wrote: >> >> >>Playing with rlimit on data path, transparently to the application in >> >> >>this way >> >> >>looks strange to me, I suspect this has unexpected security >> >> >>implications. >> >> >>Further, applications may have other uses for locked memory >> >> >>besides mpassthru - you should not just take it because it's there. >> >> >> >> >> >>Can we have an ioctl that lets userspace configure how much >> >> >>memory to lock? This ioctl will decrement the rlimit and store >> >> >>the data in the device structure so we can do accounting >> >> >>internally. Put it back on close or on another ioctl. >> >> >Yes, we can decrement the rlimit in ioctl in one time to avoid >> >> >data path. >> >> > >> >> >>Need to be careful for when this operation gets called >> >> >>again with 0 or another small value while we have locked memory - >> >> >>maybe just fail with EBUSY? or wait until it gets unlocked? >> >> >>Maybe 0 can be special-cased and deactivate zero-copy?. >> >> >> >> >> >> >> How about we don't use a new ioctl, but just check the rlimit >> >> in one MPASSTHRU_BINDDEV ioctl? If we find mp device >> >> break the rlimit, then we fail the bind ioctl, and thus can't do >> >> zero copy any more. >> > >> >Yes, and not just check, but decrement as well. >> >I think we should give userspace control over >> >how much memory we can lock and subtract from the rlimit. >> >It's OK to add this as a parameter to MPASSTHRU_BINDDEV. >> >Then increment the rlimit back on unbind and on close? >> > >> >This opens up an interesting condition: process 1 >> >calls bind, process 2 calls unbind or close. >> >This will increment rlimit for process 2. >> >Not sure how to fix this properly. >> > >> I can't too, can we do any synchronous operations on rlimit stuff? >> I quite suspect in it. >> >> >-- >> >MST > >Here's what infiniband does: simply pass the amount of memory userspace >wants you to lock on an ioctl, and verify that either you have >CAP_IPC_LOCK or this number does not exceed the current rlimit. (must >be on ioctl, not on open, as we likely want the fd passed around between >processes), but do not decrement rlimit. Use this on following >operations. Be careful if this can be changed while operations are in >progress. > >This does mean that the effective amount of memory that userspace can >lock is doubled, but at least it is not unlimited, and we sidestep all >other issues such as userspace running out of lockable memory simply by >virtue of using the driver. > What I have done in mp device is almost the same as it. The difference is that I do not check the capability, and I use my own parameter ctor->pages instead of mm->locked_vm. So currently, 1) add the capability check 2) use mm->locked_vm 3) add an ioctl for userspace to configure how much memory can lock. >-- >MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: usb-linux.c - delay between USB URBs
Erik, Did you use uhci or ehci controller in qemu? If uhuci, then Don’t know we are met the same issue or not. May you try the patch to see if it has some effect or not? We observe it has effect on native qemu side. The patch is simple but experimental, you may try to modify the "count" number as you need to see a effect. Thanks Xiaohui -Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Erik Rull Sent: 2009年3月19日 5:42 To: kvm@vger.kernel.org Subject: usb-linux.c - delay between USB URBs Hello, my problem is still the same - the USB key has only a transfer rate of ~ 2 MB / min through the virtualization. So I did some debugging in usb-linux.c - first I switched on debugging. The debug output was way to much so I added a timestamp calculation between a) start and end of async transfer and b) end of one and start of the next block (each block is 64 bytes large). The transfer rate of the driver itself is ~ 200kByte / sec - quite fine and much faster than the complete chain (only ~32kByte / sec). The delay between one and the next packet is ~2msec (transfer of 2MByte from USB key to HDD measured)! that means, 64 bytes are transferred then a pause of nearly 2msec is placed and then the next 64 bytes are sent to the driver. This fits nearly exactly my measured transfer rate of 32kByte /sec (64 Bytes / 2msec => 32kByte / sec). Where do the 2 msec come from? I have no real other processes running that could interfere here. In the Windows guest there is nothing else but the windows explorer and some other standard processes running. Any Idea where to continue searching for these 2msecs?? Best regards, Erik -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html usb.diff Description: usb.diff