Re: [Qemu-devel] [V2 PATCH] rtl8139: check the buffer availiability

[Jason Wang <jasowang@redhat.com>]

On 10/18/2011 06:09 PM, Mark Wu wrote:
> Hi Jason,
> Could you please elaborate what problem you try to resolve by this
> patch?  And do you think we need notify I/O thread re-polling tap fd
> when receive descriptor becomes available? Otherwise, tap read polling
> will be disabled until the I/O handlers are updated by other reasons. Am
> I right?

Hi Mark, sorry for the late reply.

I think this patch may help to reduce packets dropping at the head of
the queue ( tap or netqueue in qemu) which may lead out of order packet
receiving in guest stack.

For the iothread notification, it looks impossible for 8139 as the
ownership was tracked through descriptor it self instead of a dedicated
register and there's no register which could be used to 'kick' 8139 for
packet receiving. When guest have refilled rx buffer of 8139cp,
tap_can_send() would return true and qemu would re-pool it. This may
introduce some latency when qemu is doning a block select() in
main_loop_wait().

Thanks.
> 
> 
> On 10/17/2011 10:55 AM, Jason Wang wrote:
>> Reduce spurious packet drops on RX ring empty when in c+ mode by
>> verifying that
>> we have at least 1 buffer ahead of the time.
>>
>> Change from v1:
>> Fix style comments from Stefan.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>>   hw/rtl8139.c |   44 ++++++++++++++++++++++++++++++--------------
>>   1 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index 3753950..bcbc5e3 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -84,6 +84,19 @@
>>   #define VLAN_TCI_LEN 2
>>   #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
>>
>> +/* w0 ownership flag */
>> +#define CP_RX_OWN (1<<31)
>> +/* w0 end of ring flag */
>> +#define CP_RX_EOR (1<<30)
>> +/* w0 bits 0...12 : buffer size */
>> +#define CP_RX_BUFFER_SIZE_MASK ((1<<13) - 1)
>> +/* w1 tag available flag */
>> +#define CP_RX_TAVA (1<<16)
>> +/* w1 bits 0...15 : VLAN tag */
>> +#define CP_RX_VLAN_TAG_MASK ((1<<16) - 1)
>> +/* w2 low  32bit of Rx buffer ptr */
>> +/* w3 high 32bit of Rx buffer ptr */
>> +
>>   #if defined (DEBUG_RTL8139)
>>   #  define DPRINTF(fmt, ...) \
>>       do { fprintf(stderr, "RTL8139: " fmt, ## __VA_ARGS__); } while (0)
>> @@ -805,6 +818,22 @@ static inline target_phys_addr_t
>> rtl8139_addr64(uint32_t low, uint32_t high)
>>   #endif
>>   }
>>
>> +/* Verify that we have at least one available rx buffer */
>> +static int rtl8139_cp_has_rxbuf(RTL8139State *s)
>> +{
>> +    uint32_t val, rxdw0;
>> +    target_phys_addr_t cplus_rx_ring_desc =
>> rtl8139_addr64(s->RxRingAddrLO,
>> +                                                          
>> s->RxRingAddrHI);
>> +    cplus_rx_ring_desc += 16 * s->currCPlusRxDesc;
>> +    cpu_physical_memory_read(cplus_rx_ring_desc,&val, 4);
>> +    rxdw0 = le32_to_cpu(val);
>> +    if (rxdw0&  CP_RX_OWN) {
>> +        return 1;
>> +    } else {
>> +        return 0;
>> +    }
>> +}
>> +
>>   static int rtl8139_can_receive(VLANClientState *nc)
>>   {
>>       RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
>> @@ -819,7 +848,7 @@ static int rtl8139_can_receive(VLANClientState *nc)
>>       if (rtl8139_cp_receiver_enabled(s)) {
>>           /* ??? Flow control not implemented in c+ mode.
>>              This is a hack to work around slirp deficiencies anyway.  */
>> -        return 1;
>> +        return rtl8139_cp_has_rxbuf(s);
>>       } else {
>>           avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr,
>>                        s->RxBufferSize);
>> @@ -965,19 +994,6 @@ static ssize_t rtl8139_do_receive(VLANClientState
>> *nc, const uint8_t *buf, size_
>>
>>           /* begin C+ receiver mode */
>>
>> -/* w0 ownership flag */
>> -#define CP_RX_OWN (1<<31)
>> -/* w0 end of ring flag */
>> -#define CP_RX_EOR (1<<30)
>> -/* w0 bits 0...12 : buffer size */
>> -#define CP_RX_BUFFER_SIZE_MASK ((1<<13) - 1)
>> -/* w1 tag available flag */
>> -#define CP_RX_TAVA (1<<16)
>> -/* w1 bits 0...15 : VLAN tag */
>> -#define CP_RX_VLAN_TAG_MASK ((1<<16) - 1)
>> -/* w2 low  32bit of Rx buffer ptr */
>> -/* w3 high 32bit of Rx buffer ptr */
>> -
>>           int descriptor = s->currCPlusRxDesc;
>>           target_phys_addr_t cplus_rx_ring_desc;
>>
>>
>>
> 
>