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

[Bo Yang <boyang@suse.com>]

On 08/30/2012 03:17 AM, 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<boyang@suse.com>
>> ---
>>   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?
> Would the network show less performance? How much
> would the test scenario (Linux installation) take longer?

It is probably to show less performance. But I don't know how much it
will be.

> 
> What about the other nic emulations in QEMU?
> I observe hanging network rather often with the
> ARM versatilepb emulation.
> 
>>           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 am not sure..

> 
>>       }
>>       if (rfd_command&  COMMAND_S) {
>>           /* S bit is set. */
> 
> It's interesting to hear that your QA tests with eepro100.c.
> Which nic model do they use in their tests?

It is i82559er.

> 
> Regards,
> 
> Stefan W.
> 
>