[Qemu-devel] [PATCH] fix the return value of rtl8139_can_receive()

[Qemu-devel] [PATCH] fix the return value of rtl8139_can_receive()

[Wen Congyang]

If rtl8139_can_receive() returns 1, it means that the nic can receive packet,
otherwise, it means the nic can not receive packet.

If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that the nic
can not receive packet. So the return value should be 0, not 1.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

---
 hw/rtl8139.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 2f8db58..9084678 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
 
     /* Receive (drop) packets if card is disabled.  */
     if (!s->clock_enabled)
-      return 1;
+      return 0;
     if (!rtl8139_receiver_enabled(s))
-      return 1;
+      return 0;
 
     if (rtl8139_cp_receiver_enabled(s)) {
         /* ??? Flow control not implemented in c+ mode.
-- 
1.7.1

Re: [Qemu-devel] [PATCH] fix the return value of rtl8139_can_receive()

[Kevin Wolf]

Am 16.06.2011 10:23, schrieb Wen Congyang:
> If rtl8139_can_receive() returns 1, it means that the nic can receive packet,
> otherwise, it means the nic can not receive packet.
> 
> If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that the nic
> can not receive packet. So the return value should be 0, not 1.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> 
> ---
>  hw/rtl8139.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 2f8db58..9084678 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
>  
>      /* Receive (drop) packets if card is disabled.  */
>      if (!s->clock_enabled)
> -      return 1;
> +      return 0;
>      if (!rtl8139_receiver_enabled(s))
> -      return 1;
> +      return 0;
>  
>      if (rtl8139_cp_receiver_enabled(s)) {
>          /* ??? Flow control not implemented in c+ mode.

NACK.

The old behaviour is clearly intentional. IIRC, can_receive() returning
0 means that the packet is kept in a queue and qemu tries to deliver it
later. For a disabled receiver, what I would expect is that it should
just drop the packets. This is what this code does by returning 1 in
can_receive() and then return -1 without processing the packet in receive().

That said, e1000 has a check for (s->mac_reg[RCTL] & E1000_RCTL_EN) in
can_receive. Should it be changed or is there a reason behind it? If
there is, we may as well change rtl8139, but it definitely needs a
better justification.

Kevin

Re: [Qemu-devel] [PATCH] fix the return value of rtl8139_can_receive()

[Michael S. Tsirkin]

On Thu, Jun 16, 2011 at 12:39:51PM +0200, Kevin Wolf wrote:
> Am 16.06.2011 10:23, schrieb Wen Congyang:
> > If rtl8139_can_receive() returns 1, it means that the nic can receive packet,
> > otherwise, it means the nic can not receive packet.
> > 
> > If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that the nic
> > can not receive packet. So the return value should be 0, not 1.
> > 
> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > 
> > ---
> >  hw/rtl8139.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> > index 2f8db58..9084678 100644
> > --- a/hw/rtl8139.c
> > +++ b/hw/rtl8139.c
> > @@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
> >  
> >      /* Receive (drop) packets if card is disabled.  */
> >      if (!s->clock_enabled)
> > -      return 1;
> > +      return 0;
> >      if (!rtl8139_receiver_enabled(s))
> > -      return 1;
> > +      return 0;
> >  
> >      if (rtl8139_cp_receiver_enabled(s)) {
> >          /* ??? Flow control not implemented in c+ mode.
> 
> NACK.
> 
> The old behaviour is clearly intentional. IIRC, can_receive() returning
> 0 means that the packet is kept in a queue and qemu tries to deliver it
> later. For a disabled receiver, what I would expect is that it should
> just drop the packets. This is what this code does by returning 1 in
> can_receive() and then return -1 without processing the packet in receive().
> 
> That said, e1000 has a check for (s->mac_reg[RCTL] & E1000_RCTL_EN) in
> can_receive. Should it be changed or is there a reason behind it? If
> there is, we may as well change rtl8139, but it definitely needs a
> better justification.
> 
> Kevin


I doubt it matters much. In practice returning 1 has the effect
that qemu will keep getting packets from host and wasting
CPU on dropping packets. This seems worse than packets
that should be dropped but aren't.

You do want to fix the comment above if you tweak this though.`

-- 
MST

[Qemu-devel] [PATCH] do not send packet to nic if the packet will be dropped by nic

[Wen Congyang]

If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that
the nic will drop all packets from host. So qemu will keep getting
packets from host and wasting CPU on dropping packets. This seems
worse than packets that should be dropped but aren't.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

---
 hw/rtl8139.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 2f8db58..9084678 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
 
     /* Receive (drop) packets if card is disabled.  */
     if (!s->clock_enabled)
-      return 1;
+      return 0;
     if (!rtl8139_receiver_enabled(s))
-      return 1;
+      return 0;
 
     if (rtl8139_cp_receiver_enabled(s)) {
         /* ??? Flow control not implemented in c+ mode.
-- 1.7.1 

Re: [Qemu-devel] [PATCH] do not send packet to nic if the packet will be dropped by nic

[Kevin Wolf]

Am 17.06.2011 03:33, schrieb Wen Congyang:
> If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that
> the nic will drop all packets from host. So qemu will keep getting
> packets from host and wasting CPU on dropping packets. This seems
> worse than packets that should be dropped but aren't.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Which bug does this change fix? I'm still not convinced that we should
do it.

> ---
>  hw/rtl8139.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 2f8db58..9084678 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
>  
>      /* Receive (drop) packets if card is disabled.  */

This comment isn't accurate any more after applying the patch.

>      if (!s->clock_enabled)
> -      return 1;
> +      return 0;
>      if (!rtl8139_receiver_enabled(s))
> -      return 1;
> +      return 0;
>  
>      if (rtl8139_cp_receiver_enabled(s)) {
>          /* ??? Flow control not implemented in c+ mode.
> -- 1.7.1 

Kevin

Re: [Qemu-devel] [PATCH] do not send packet to nic if the packet will be dropped by nic

[Wen Congyang]

At 06/20/2011 05:10 PM, Kevin Wolf Write:
> Am 17.06.2011 03:33, schrieb Wen Congyang:
>> If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that
>> the nic will drop all packets from host. So qemu will keep getting
>> packets from host and wasting CPU on dropping packets. This seems
>> worse than packets that should be dropped but aren't.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> 
> Which bug does this change fix? I'm still not convinced that we should
> do it.

Maybe not a bug fix now. As Michael S. Tsirkin said, if rtl8139_can_receive()
returns 1, qemu will keep getting packets from host and wasting CPU on
dropping packets. We can save CPU by return 0.

> 
>> ---
>>  hw/rtl8139.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index 2f8db58..9084678 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
>>  
>>      /* Receive (drop) packets if card is disabled.  */
> 
> This comment isn't accurate any more after applying the patch.
> 
>>      if (!s->clock_enabled)
>> -      return 1;
>> +      return 0;
>>      if (!rtl8139_receiver_enabled(s))
>> -      return 1;
>> +      return 0;
>>  
>>      if (rtl8139_cp_receiver_enabled(s)) {
>>          /* ??? Flow control not implemented in c+ mode.
>> -- 1.7.1 
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH] do not send packet to nic if the packet will be dropped by nic

[Kevin Wolf]

Am 20.06.2011 11:40, schrieb Wen Congyang:
> At 06/20/2011 05:10 PM, Kevin Wolf Write:
>> Am 17.06.2011 03:33, schrieb Wen Congyang:
>>> If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that
>>> the nic will drop all packets from host. So qemu will keep getting
>>> packets from host and wasting CPU on dropping packets. This seems
>>> worse than packets that should be dropped but aren't.
>>>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>
>> Which bug does this change fix? I'm still not convinced that we should
>> do it.
> 
> Maybe not a bug fix now. As Michael S. Tsirkin said, if rtl8139_can_receive()
> returns 1, qemu will keep getting packets from host and wasting CPU on
> dropping packets. We can save CPU by return 0.

Don't we waste memory instead then because we leave the packets queued
indefinitely?

Kevin

Re: [Qemu-devel] [PATCH] do not send packet to nic if the packet will be dropped by nic

[Michael S. Tsirkin]

On Mon, Jun 20, 2011 at 11:52:20AM +0200, Kevin Wolf wrote:
> Am 20.06.2011 11:40, schrieb Wen Congyang:
> > At 06/20/2011 05:10 PM, Kevin Wolf Write:
> >> Am 17.06.2011 03:33, schrieb Wen Congyang:
> >>> If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that
> >>> the nic will drop all packets from host. So qemu will keep getting
> >>> packets from host and wasting CPU on dropping packets. This seems
> >>> worse than packets that should be dropped but aren't.
> >>>
> >>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> >>
> >> Which bug does this change fix? I'm still not convinced that we should
> >> do it.
> > 
> > Maybe not a bug fix now. As Michael S. Tsirkin said, if rtl8139_can_receive()
> > returns 1, qemu will keep getting packets from host and wasting CPU on
> > dropping packets. We can save CPU by return 0.
> 
> Don't we waste memory instead then because we leave the packets queued
> indefinitely?
> 
> Kevin

Yes but the amount of wasted memory is bound from above
so this doesn't seem too bad to me ...

-- 
MST

Re: [Qemu-devel] [PATCH] fix the return value of rtl8139_can_receive()

[Wen Congyang]

At 06/16/2011 06:39 PM, Kevin Wolf Write:
> Am 16.06.2011 10:23, schrieb Wen Congyang:
>> If rtl8139_can_receive() returns 1, it means that the nic can receive packet,
>> otherwise, it means the nic can not receive packet.
>>
>> If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that the nic
>> can not receive packet. So the return value should be 0, not 1.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>
>> ---
>>  hw/rtl8139.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index 2f8db58..9084678 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
>>  
>>      /* Receive (drop) packets if card is disabled.  */
>>      if (!s->clock_enabled)
>> -      return 1;
>> +      return 0;
>>      if (!rtl8139_receiver_enabled(s))
>> -      return 1;
>> +      return 0;
>>  
>>      if (rtl8139_cp_receiver_enabled(s)) {
>>          /* ??? Flow control not implemented in c+ mode.
> 
> NACK.
> 
> The old behaviour is clearly intentional. IIRC, can_receive() returning
> 0 means that the packet is kept in a queue and qemu tries to deliver it
> later. For a disabled receiver, what I would expect is that it should
> just drop the packets. This is what this code does by returning 1 in
> can_receive() and then return -1 without processing the packet in receive().

Thanks for your detailed explanation.
I know why can_receive() returns 1 now.

> 
> That said, e1000 has a check for (s->mac_reg[RCTL] & E1000_RCTL_EN) in
> can_receive. Should it be changed or is there a reason behind it? If

This check is introduced in commit 4105de67 by Anthony Liguori.
He may know the reason.

> there is, we may as well change rtl8139, but it definitely needs a
> better justification.
> 
> Kevin
>