[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] Fix buffer run out in eepro100.

From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [PATCH v2] Fix buffer run out in eepro100.
Date: Fri, 31 Aug 2012 06:30:15 +0100

On Fri, Aug 31, 2012 at 4:40 AM, Bo Yang <address@hidden> wrote:
>>>> Stefan Hajnoczi <address@hidden> 08/30/12 7:42 PM >>>
> On Thu, Aug 30, 2012 at 9:38 AM, Bo Yang <address@hidden> wrote:
>> On 08/30/2012 04:04 PM, Stefan Hajnoczi wrote:
>>> On Wed, Aug 29, 2012 at 09:17:43PM +0200, Stefan Weil wrote:
>>>> Am 29.08.2012 13:26, schrieb Bo Yang:
>>>>> This is reported by QA. When installing os with pxe, after the initial
>>>>> kernel and initrd are loaded, the procedure tries to copy files from 
>>>>> install
>>>>> server to local harddisk, the network becomes stall because of running 
>>>>> out of
>>>>> receive descriptor.
>>>>> Signed-off-by: Bo Yang<address@hidden>
>>>>> ---
>>>>>  hw/eepro100.c |    5 ++++-
>>>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>>> index 50d117e..52a18ad 100644
>>>>> --- a/hw/eepro100.c
>>>>> +++ b/hw/eepro100.c
>>>>> @@ -1036,6 +1036,8 @@ static void eepro100_ru_command(EEPRO100State * s, 
>>>>> uint8_t val)
>>>>>          }
>>>>>          set_ru_state(s, ru_ready);
>>>>>          s->ru_offset = e100_read_reg4(s, SCBPointer);
>>>>> +           qemu_flush_queued_packets(&s->nic->nc);
>>>>> +   qemu_notify_event();
>>>> What would happen if the above changes were omitted?
>>> In the worst case the guest code would be unable to make progress since
>>> packet reception is disabled.
>>> The QEMU net subsystem needs to be kicked when rx buffers become
>>> available again so that any queued packets can be delivered and we can
>>> restart the event loop.
>>> The event loop needs to be restarted because net clients (like tap) use
>>> qemu_set_fd_handler2() with a read_poll() handler that returns false
>>> when the NIC is unable to receive.  Imagine this scenario:
>>> 1. NIC runs out of rx buffers.
>>> 2. Event loop iteration starts and calls tap's read_poll() handler,
>>>    which sees the NIC cannot receive and therefore does not add the tap
>>>    file descriptor to select(2).
>>> 3. NIC gets new rx buffers but does not kick net subsystem/event loop.
>>> 4. Event loop still sitting in select(2) without the tap file
>>>    descriptor.  Therefore incoming packets are not picked up by QEMU!
>>> In practice the event loop tends to iterate due to timers, etc.  But in
>>> the worst case we can go completely starved here.
>> Yes. The fd will be added to read set in the next iteration. The delay
>> depends on the select timeout. it is possible to go starved here.
>>>> Would the network show less performance? How much
>>>> would the test scenario (Linux installation) take longer?
>>> Yes, the lack of kicks causes reduced network performance.  This is
>>> especially true with -netdev tap and a guest driver that runs out of rx
>>> buffers.  If you're lucky you might not hit this depending on the
>>> -netdev and availability of rx buffers.
>>>> What about the other nic emulations in QEMU?
>>>> I observe hanging network rather often with the
>>>> ARM versatilepb emulation.
>>> virtio-net has been correct for some time.
>>> e1000, xen, usb, and eepro100 are now fixed in the net tree:
>>> http://github.com/stefanha/qemu/commits/net
>>> Other NICs may or may not be okay.  Really all of them need to be
>>> audited.
>>>>>          TRACE(OTHER, logout("val=0x%02x (rx start)\n", val));
>>>>>          break;
>>>>>      case RX_RESUME:
>>>>> @@ -1770,7 +1772,8 @@ static ssize_t nic_receive(NetClientState *nc, 
>>>>> const uint8_t * buf, size_t size)
>>>>>      if (rfd_command&  COMMAND_EL) {
>>>>>          /* EL bit is set, so this was the last frame. */
>>>>>          logout("receive: Running out of frames\n");
>>>>> -        set_ru_state(s, ru_suspended);
>>>>> +        set_ru_state(s, ru_no_resources);
>>>>> +   eepro100_rnr_interrupt(s);
>>>> Adding the interrupt here is correct (I have similar code in
>>>> http://repo.or.cz/w/qemu/ar7.git/blob/HEAD:/hw/eepro100.c
>>>> which is an improved version of hw/eepro100.c).
>>>> Setting ru_no_resources looks also good, but I am not
>>>> sure whether removing ru_suspended is ok. Maybe it should
>>>> be ru_no_resources | ru_suspended.
>>> I think the datasheet talks about setting the RU to no resources and the
>>> CU to suspended.  So there are two state machines and we only track one
>>> here.
>> I don't think I understand this. If we run out of rx descriptor, why do
>> we suspend tx unit too? maybe there are reasons I am unaware of.. I
>> don't know.
> I was wrong.  The datasheet "Table 55. CU Activities Performed at the
> End of Execution" shows that the EL and S bit cause the CU to enter
> the Idle State.
> In terms of hw/eepro100.c I don't think we care about the CU state.
> RU state No Resources is correct.
> I think we've done here, do we? sorry for the format of the mail, it is
> sent from web.. Thanks for reviewing and suggestions.

I'm happy with this patch.  I have merged it into the net tree:



reply via email to

[Prev in Thread] Current Thread [Next in Thread]