Patchwork do not send packet to nic if the packet will be dropped by nic

login
register
mail settings
Submitter Wen Congyang
Date June 17, 2011, 1:33 a.m.
Message ID <4DFAAF00.900@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/100751/
State New
Headers show

Comments

Wen Congyang - June 17, 2011, 1:33 a.m.
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(-)

-- 1.7.1
Kevin Wolf - June 20, 2011, 9:10 a.m.
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
Wen Congyang - June 20, 2011, 9:40 a.m.
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
>
Kevin Wolf - June 20, 2011, 9:52 a.m.
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
Michael S. Tsirkin - June 20, 2011, 11:13 a.m.
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 ...

Patch

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.