fix the return value of rtl8139_can_receive()

PATCH NAK Wen Congyang 3 participants 9 messages Jun 16, 2011 – Jun 20, 2011

Share & tools

Wen Congyang

author

2011-06-16 09:50 UTC #0 lore raw

PATCH 1/1 fix the return value of rtl8139_can_receive()

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 <[email protected]>

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

hw/rtl8139.c +2 -2

index 2f8db58..9084678 100644
@@ -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.

Signature · 1 line

1.7.1

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

Michael S. Tsirkin

2011-06-16 13:31 UTC #2 lore raw

Quoted 38 lines · Kevin Wolf

Am 16.06.2011 10:23, schrieb Wen Congyang:
Quoted 18 lines
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 <[email protected]>
---
 hw/rtl8139.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
hw/rtl8139.c +0 -0
index 2f8db58..9084678 100644
@@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
 
hw/rtl8139.c static int rtl8139_can_receive(VLANClientState *nc) · line 818
Quoted 10 lines
hw/rtl8139.c +2 -2
     /* 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().

Quoted 6 lines

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.`

Signature · 1 line

MST

Wen Congyang

author

2011-06-17 01:36 UTC #3 lore raw

PATCH 1/1 do not send packet to nic if the packet will be dropped by nic

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 <[email protected]>

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

hw/rtl8139.c +2 -2

index 2f8db58..9084678 100644
@@ -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

Kevin Wolf

2011-06-20 09:08 UTC #4 lore raw

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

Am 17.06.2011 03:33, schrieb Wen Congyang:

Quoted 6 lines

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 <[email protected]>

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

hw/rtl8139.c static int rtl8139_can_receive(VLANClientState *nc) · line 811

Quoted 11 lines

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

hw/rtl8139.c +0 -0

index 2f8db58..9084678 100644
@@ -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.

hw/rtl8139.c static int rtl8139_can_receive(VLANClientState *nc)

Quoted 10 lines

hw/rtl8139.c +2 -3

     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

author

2011-06-20 09:43 UTC #5 lore raw

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

At 06/20/2011 05:10 PM, Kevin Wolf Write:

Quoted 10 lines

Am 17.06.2011 03:33, schrieb Wen Congyang:
Quoted 6 lines
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 <[email protected]>
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.

Quoted 28 lines

hw/rtl8139.c static int rtl8139_can_receive(VLANClientState *nc) · line 811

Quoted 12 lines

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

hw/rtl8139.c +0 -0

index 2f8db58..9084678 100644
@@ -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.

hw/rtl8139.c static int rtl8139_can_receive(VLANClientState *nc)

Quoted 10 lines

hw/rtl8139.c +2 -3

     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

2011-06-20 09:49 UTC #6 lore raw

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

Am 20.06.2011 11:40, schrieb Wen Congyang:

Quoted 7 lines

At 06/20/2011 05:10 PM, Kevin Wolf Write:
Quoted 6 lines
Am 17.06.2011 03:33, schrieb Wen Congyang:
Quoted 5 lines
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.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Wen Congyang <[email protected]>
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

2011-06-20 11:13 UTC #7 lore raw

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

Quoted 13 lines · Kevin Wolf

Am 20.06.2011 11:40, schrieb Wen Congyang:
Quoted 12 lines
At 06/20/2011 05:10 PM, Kevin Wolf Write:
Quoted 10 lines
Am 17.06.2011 03:33, schrieb Wen Congyang:
Quoted 6 lines
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 <[email protected]>
Which bug does this change fix? I'm still not convinced that we should do it.

Quoted 8 lines

Quoted 3 lines
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 ...

Signature · 1 line

MST

Wen Congyang

author

2011-06-17 01:52 UTC #8 lore raw

In reply to Kevin Wolf

At 06/16/2011 06:39 PM, Kevin Wolf Write:

Quoted 32 lines

Am 16.06.2011 10:23, schrieb Wen Congyang:
Quoted 18 lines
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 <[email protected]>
---
 hw/rtl8139.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
hw/rtl8139.c +0 -0
index 2f8db58..9084678 100644
@@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
 
hw/rtl8139.c static int rtl8139_can_receive(VLANClientState *nc) · line 818
Quoted 10 lines
hw/rtl8139.c +2 -2
     /* 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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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