[0/2] virtio-pci: fix abort when fail to allocate ioeventfd

On 14/03/12 19:46, Stefan Hajnoczi wrote:
> On Wed, Mar 14, 2012 at 10:46 AM, Avi Kivity<avi@redhat.com>  wrote:
>> On 03/14/2012 12:39 PM, Stefan Hajnoczi wrote:
>>> On Wed, Mar 14, 2012 at 10:05 AM, Avi Kivity<avi@redhat.com>  wrote:
>>>> On 03/14/2012 11:59 AM, Stefan Hajnoczi wrote:
>>>>> On Wed, Mar 14, 2012 at 9:22 AM, Avi Kivity<avi@redhat.com>  wrote:
>>>>>> On 03/13/2012 12:42 PM, Amos Kong wrote:
>>>>>>> Boot up guest with 232 virtio-blk disk, qemu will abort for fail to
>>>>>>> allocate ioeventfd. This patchset changes kvm_has_many_ioeventfds(),
>>>>>>> and check if available ioeventfd exists. If not, virtio-pci will
>>>>>>> fallback to userspace, and don't use ioeventfd for io notification.
>>>>>>
>>>>>> How about an alternative way of solving this, within the memory core:
>>>>>> trap those writes in qemu and write to the ioeventfd yourself.  This way
>>>>>> ioeventfds work even without kvm:
>>>>>>
>>>>>>
>>>>>>   core: create eventfd
>>>>>>   core: install handler for memory address that writes to ioeventfd
>>>>>>   kvm (optional): install kernel handler for ioeventfd

Can you give some detail about this? I'm not familiar with Memory API.


btw, can we fix this problem by replacing abort() by a error note?
virtio-pci will auto fallback to userspace.

  }

@@ -775,7 +776,8 @@ static void kvm_io_ioeventfd_add(MemoryRegionSection 
*section,
      r = kvm_set_ioeventfd_pio_word(fd, 
section->offset_within_address_space,
                                     data, true);
      if (r < 0) {
-        abort();
+        fprintf(stderr, "%s: unable to map ioeventfd: %s.\nFallback to "
+                "userspace (slower).\n", __func__, strerror(-r));
      }
  }


>>>>>> even if the third step fails, the ioeventfd still works, it's just slower.
>>>>>
>>>>> That approach will penalize guests with large numbers of disks - they
>>>>> see an extra switch to vcpu thread instead of kvm.ko ->  iothread.
>>>>
>>>> It's only a failure path.  The normal path is expected to have a kvm
>>>> ioeventfd installed.
>>>
>>> It's the normal path when you attach>232 virtio-blk devices to a
>>> guest (or 300 in the future).
>>
>> Well, there's nothing we can do about it.
>>
>> We'll increase the limit of course, but old kernels will remain out
>> there.  The right fix is virtio-scsi anyway.
>>
>>>>>    It
>>>>> seems okay provided we can solve the limit in the kernel once and for
>>>>> all by introducing a more dynamic data structure for in-kernel
>>>>> devices.  That way future kernels will never hit an arbitrary limit
>>>>> below their file descriptor rlimit.
>>>>>
>>>>> Is there some reason why kvm.ko must use a fixed size array?  Would it
>>>>> be possible to use a tree (maybe with a cache for recent lookups)?
>>>>
>>>> It does use bsearch today IIRC.  We'll expand the limit, but there must
>>>> be a limit, and qemu must be prepared to deal with it.
>>>
>>> Shouldn't the limit be the file descriptor rlimit?  If userspace
>>> cannot create more eventfds then it cannot set up more ioeventfds.
>>
>> You can use the same eventfd for multiple ioeventfds.  If you mean to
>> slave kvm's ioeventfd limit to the number of files the process can have,
>> that's a good idea.  Surely an ioeventfd occupies less resources than an
>> open file.
>
> Yes.
>
> Ultimately I guess you're right in that we still need to have an error
> path and virtio-scsi will reduce the pressure on I/O eventfds for
> storage.
>
> Stefan

On Fri, Mar 16, 2012 at 04:59:35PM +0800, Amos Kong wrote:
> On 14/03/12 19:46, Stefan Hajnoczi wrote:
> >On Wed, Mar 14, 2012 at 10:46 AM, Avi Kivity<avi@redhat.com>  wrote:
> >>On 03/14/2012 12:39 PM, Stefan Hajnoczi wrote:
> >>>On Wed, Mar 14, 2012 at 10:05 AM, Avi Kivity<avi@redhat.com>  wrote:
> >>>>On 03/14/2012 11:59 AM, Stefan Hajnoczi wrote:
> >>>>>On Wed, Mar 14, 2012 at 9:22 AM, Avi Kivity<avi@redhat.com>  wrote:
> >>>>>>On 03/13/2012 12:42 PM, Amos Kong wrote:
> >>>>>>>Boot up guest with 232 virtio-blk disk, qemu will abort for fail to
> >>>>>>>allocate ioeventfd. This patchset changes kvm_has_many_ioeventfds(),
> >>>>>>>and check if available ioeventfd exists. If not, virtio-pci will
> >>>>>>>fallback to userspace, and don't use ioeventfd for io notification.
> >>>>>>
> >>>>>>How about an alternative way of solving this, within the memory core:
> >>>>>>trap those writes in qemu and write to the ioeventfd yourself.  This way
> >>>>>>ioeventfds work even without kvm:
> >>>>>>
> >>>>>>
> >>>>>>  core: create eventfd
> >>>>>>  core: install handler for memory address that writes to ioeventfd
> >>>>>>  kvm (optional): install kernel handler for ioeventfd
> 
> Can you give some detail about this? I'm not familiar with Memory API.
> 
> 
> btw, can we fix this problem by replacing abort() by a error note?
> virtio-pci will auto fallback to userspace.
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 3c6b4f0..cf23dbf 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -749,7 +749,8 @@ static void
> kvm_mem_ioeventfd_add(MemoryRegionSection *section,
>      r = kvm_set_ioeventfd_mmio_long(fd,
> section->offset_within_address_space,
>                                      data, true);
>      if (r < 0) {
> -        abort();
> +        fprintf(stderr, "%s: unable to map ioeventfd: %s.\nFallback to "
> +                "userspace (slower).\n", __func__, strerror(-r));

The challenge is propagating the error code.  If virtio-pci.c doesn't
know that ioeventfd has failed, then it's not possible to fall back to a
userspace handler.

I believe Avi's suggestion is to put the fallback code into the KVM
memory API implementation so that virtio-pci.c doesn't need to know that
ioeventfd failed at all.

Stefan

On 03/16/2012 10:59 AM, Amos Kong wrote:
>
> Can you give some detail about this? I'm not familiar with Memory API.

Well there's a huge amount of detail needed here.  The idea is that
memory_region_add_eventfd() will always work, with or without kvm, and
even if kvm is enabled but we run out of ioeventfds.

One way to do this is to implement core_eventfd_add() in exec.c.  This
is unlikely to be easy however.

>
> btw, can we fix this problem by replacing abort() by a error note?
> virtio-pci will auto fallback to userspace.

But other users will silently break, need to audit all other users of
ioeventfd, for example ivshmem.