Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On 06/29/2015 06:39 PM, Stefan Hajnoczi wrote:
On Fri, Jun 26, 2015 at 05:06:01PM +0800, Jason Wang wrote:
On 06/25/2015 05:18 PM, Stefan Hajnoczi wrote:
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
This only solves the issue partially and just for e1000. After checking
all can_receive() functions, another card with similar behaviour is vmxnet3.
No, this bug is specific to e1000 (details on that below in case 2b).
There are more issues though, so additional patches are welcome.
Looking at commit a90a7425cf592a3afeff3eaf32f543b83050ee5c (tap: Drop
tap_can_send) again. The commit disable tap read poll when
qemu_net_queue_send() returns zero. Which is usually the following cases:
1) queue-delivering is 1
2) qemu_can_send_packet() returns zero, which is:
2a) vm_running is false
or
2b) can_receive() return zero
3) qemu_net_queue_deliver() returns zero, which is:
3a) nc-receive_disabled is true
or
3b) info-receive_raw() or nc-receive-receive() returns zero
This means we should enable tap read poll when one of those conditions
is not existed. This patch fixes 2b) only for e1000.
for 1, I'm not quite sure it's a real problem or how to fix.
This is not a problem. When the delivery code returns it flushes the
queue. This will invoke send callback functions, so tap will re-enable
read poll.
for 2a, we may probably need a vm state change handler to flush the
queue, otherwise network will stall after a stop and cont.
Yes, I'm surprised the code isn't doing this already.
for 2b, we can either fixes the card that check link status in its
can_receive() or just simply can qemu_flush_queued_packets() in set_link.
No, this doesn't work because the e1000 link status bit is *not*
equivalent to nc-link_down. The link auto-negotiation code changes the
e1000 bit independently from nc-link_down using a timer.
The net subsystem cannot know when this e1000-specific bit is supposed
to change. Therefore there is no general fix.
Also, qemu_send_packet() drops packets while nc-link_down is true.
We're not supposed to queue so it shouldn't be necessary to flush
packets in set_link at the net subsystem level.
But qemu_send_packet() only check sender's nc-link_down. So tap read
poll will be disabled and won't get enabled when its peers' link status
is recovered.
3a and 3b looks ok, since this happen when there's no space for rx,
after guest refill, qemu will call qemu_flush_queued_packets() to flush
and re-enable tap read poll.
2a and 2b does not exits before this commit, since tap_send check
qemu_can_send_packet() before.
The delivery and queuing mechanism in QEMU is overcomplicated. The
semantics of .can_receive() and .receive() need to be clarified before
we can do real cleanup though:
.receive() returning 0 is not quite the same as .can_receive() returning
false.
When .receive() returns 0 this means the peer should wait before sending
more. In most cases the net queue only needs to hold 1 packet. But
there are cases like slirp where QEMU code generates packets and may not
be able to continue without calling qemu_send_packet() on more packets
(I'm not sure but I suspect this is the case).
When .can_receive() returns false it also casues packets to be queued,
but NIC emulation code often includes more conditions in .can_receive()
than .receive() returning 0.
Some NICs do even more tricks. E.g when receiver is disabled, 8139's
can_receive() returns true and expect receive() to drop the packets.
Cleaning this up requires auditing all the NICs. Maybe we can get rid
of .can_receive() and just have a simplified .receive():
false - queue packet and tell peer to stop sending. Flush must be
called later to re-enable sending.
And call can_receive() in receive().
true - packet delivered or dropped
This looks good, but do you think we can do this in 2.4? Looks a little
bit risky.
Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On Fri, Jun 26, 2015 at 05:06:01PM +0800, Jason Wang wrote:
On 06/25/2015 05:18 PM, Stefan Hajnoczi wrote:
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
This only solves the issue partially and just for e1000. After checking
all can_receive() functions, another card with similar behaviour is vmxnet3.
No, this bug is specific to e1000 (details on that below in case 2b).
There are more issues though, so additional patches are welcome.
Looking at commit a90a7425cf592a3afeff3eaf32f543b83050ee5c (tap: Drop
tap_can_send) again. The commit disable tap read poll when
qemu_net_queue_send() returns zero. Which is usually the following cases:
1) queue-delivering is 1
2) qemu_can_send_packet() returns zero, which is:
2a) vm_running is false
or
2b) can_receive() return zero
3) qemu_net_queue_deliver() returns zero, which is:
3a) nc-receive_disabled is true
or
3b) info-receive_raw() or nc-receive-receive() returns zero
This means we should enable tap read poll when one of those conditions
is not existed. This patch fixes 2b) only for e1000.
for 1, I'm not quite sure it's a real problem or how to fix.
This is not a problem. When the delivery code returns it flushes the
queue. This will invoke send callback functions, so tap will re-enable
read poll.
for 2a, we may probably need a vm state change handler to flush the
queue, otherwise network will stall after a stop and cont.
Yes, I'm surprised the code isn't doing this already.
for 2b, we can either fixes the card that check link status in its
can_receive() or just simply can qemu_flush_queued_packets() in set_link.
No, this doesn't work because the e1000 link status bit is *not*
equivalent to nc-link_down. The link auto-negotiation code changes the
e1000 bit independently from nc-link_down using a timer.
The net subsystem cannot know when this e1000-specific bit is supposed
to change. Therefore there is no general fix.
Also, qemu_send_packet() drops packets while nc-link_down is true.
We're not supposed to queue so it shouldn't be necessary to flush
packets in set_link at the net subsystem level.
3a and 3b looks ok, since this happen when there's no space for rx,
after guest refill, qemu will call qemu_flush_queued_packets() to flush
and re-enable tap read poll.
2a and 2b does not exits before this commit, since tap_send check
qemu_can_send_packet() before.
The delivery and queuing mechanism in QEMU is overcomplicated. The
semantics of .can_receive() and .receive() need to be clarified before
we can do real cleanup though:
.receive() returning 0 is not quite the same as .can_receive() returning
false.
When .receive() returns 0 this means the peer should wait before sending
more. In most cases the net queue only needs to hold 1 packet. But
there are cases like slirp where QEMU code generates packets and may not
be able to continue without calling qemu_send_packet() on more packets
(I'm not sure but I suspect this is the case).
When .can_receive() returns false it also casues packets to be queued,
but NIC emulation code often includes more conditions in .can_receive()
than .receive() returning 0.
Cleaning this up requires auditing all the NICs. Maybe we can get rid
of .can_receive() and just have a simplified .receive():
false - queue packet and tell peer to stop sending. Flush must be
called later to re-enable sending.
true - packet delivered or dropped
pgpnSe8OknkSa.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On Mon, 06/29 11:39, Stefan Hajnoczi wrote:
On Fri, Jun 26, 2015 at 05:06:01PM +0800, Jason Wang wrote:
On 06/25/2015 05:18 PM, Stefan Hajnoczi wrote:
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
This only solves the issue partially and just for e1000. After checking
all can_receive() functions, another card with similar behaviour is vmxnet3.
No, this bug is specific to e1000 (details on that below in case 2b).
There are more issues though, so additional patches are welcome.
Looking at commit a90a7425cf592a3afeff3eaf32f543b83050ee5c (tap: Drop
tap_can_send) again. The commit disable tap read poll when
qemu_net_queue_send() returns zero. Which is usually the following cases:
1) queue-delivering is 1
2) qemu_can_send_packet() returns zero, which is:
2a) vm_running is false
or
2b) can_receive() return zero
3) qemu_net_queue_deliver() returns zero, which is:
3a) nc-receive_disabled is true
or
3b) info-receive_raw() or nc-receive-receive() returns zero
This means we should enable tap read poll when one of those conditions
is not existed. This patch fixes 2b) only for e1000.
for 1, I'm not quite sure it's a real problem or how to fix.
This is not a problem. When the delivery code returns it flushes the
queue. This will invoke send callback functions, so tap will re-enable
read poll.
for 2a, we may probably need a vm state change handler to flush the
queue, otherwise network will stall after a stop and cont.
Yes, I'm surprised the code isn't doing this already.
It's rare that a .can_receive checks vm_running. Why is it only necessary in
virtio-net, or is it that other NIC models are wrong?
for 2b, we can either fixes the card that check link status in its
can_receive() or just simply can qemu_flush_queued_packets() in set_link.
No, this doesn't work because the e1000 link status bit is *not*
equivalent to nc-link_down. The link auto-negotiation code changes the
e1000 bit independently from nc-link_down using a timer.
The net subsystem cannot know when this e1000-specific bit is supposed
to change. Therefore there is no general fix.
Also, qemu_send_packet() drops packets while nc-link_down is true.
We're not supposed to queue so it shouldn't be necessary to flush
packets in set_link at the net subsystem level.
3a and 3b looks ok, since this happen when there's no space for rx,
after guest refill, qemu will call qemu_flush_queued_packets() to flush
and re-enable tap read poll.
2a and 2b does not exits before this commit, since tap_send check
qemu_can_send_packet() before.
The delivery and queuing mechanism in QEMU is overcomplicated. The
semantics of .can_receive() and .receive() need to be clarified before
we can do real cleanup though:
.receive() returning 0 is not quite the same as .can_receive() returning
false.
When .receive() returns 0 this means the peer should wait before sending
more. In most cases the net queue only needs to hold 1 packet. But
there are cases like slirp where QEMU code generates packets and may not
be able to continue without calling qemu_send_packet() on more packets
(I'm not sure but I suspect this is the case).
When .can_receive() returns false it also casues packets to be queued,
but NIC emulation code often includes more conditions in .can_receive()
than .receive() returning 0.
Cleaning this up requires auditing all the NICs. Maybe we can get rid
of .can_receive() and just have a simplified .receive():
false - queue packet and tell peer to stop sending. Flush must be
called later to re-enable sending.
true - packet delivered or dropped
Yes, I find that some of the .can_receive callbacks look OK for a simple
dropping, for example net-hub and virtio-net. I can try this, or even better
if we can split the work.
As a starter maybe we can drop the .can_receive callback and move existing
implementations to the
Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On Fri, 06/26 17:06, Jason Wang wrote:
On 06/25/2015 05:18 PM, Stefan Hajnoczi wrote:
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
This only solves the issue partially and just for e1000. After checking
all can_receive() functions, another card with similar behaviour is vmxnet3.
Looking at commit a90a7425cf592a3afeff3eaf32f543b83050ee5c (tap: Drop
tap_can_send) again. The commit disable tap read poll when
qemu_net_queue_send() returns zero. Which is usually the following cases:
1) queue-delivering is 1
2) qemu_can_send_packet() returns zero, which is:
2a) vm_running is false
or
2b) can_receive() return zero
3) qemu_net_queue_deliver() returns zero, which is:
3a) nc-receive_disabled is true
or
3b) info-receive_raw() or nc-receive-receive() returns zero
This means we should enable tap read poll when one of those conditions
is not existed. This patch fixes 2b) only for e1000.
for 1, I'm not quite sure it's a real problem or how to fix.
for 2a, we may probably need a vm state change handler to flush the
queue, otherwise network will stall after a stop and cont.
for 2b, we can either fixes the card that check link status in its
can_receive() or just simply can qemu_flush_queued_packets() in set_link.
3a and 3b looks ok, since this happen when there's no space for rx,
after guest refill, qemu will call qemu_flush_queued_packets() to flush
and re-enable tap read poll.
2a and 2b does not exits before this commit, since tap_send check
qemu_can_send_packet() before.
Looks like netmap has the same issue.
Ouch! Good catch! I will take a look at the devices today and see if we can fix
the problem by adding qemu_flush_queued_packets() calls in the state transition
points (vm_running, can_receive).
The worst case is reverting the whole series (0bc12c4f7..f4d248bdc3) for 2.4,
but dropping can_read is a worthwhile step for optimizing our event loop.
Thanks,
Fam
Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On 06/25/2015 05:18 PM, Stefan Hajnoczi wrote:
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
This only solves the issue partially and just for e1000. After checking
all can_receive() functions, another card with similar behaviour is vmxnet3.
Looking at commit a90a7425cf592a3afeff3eaf32f543b83050ee5c (tap: Drop
tap_can_send) again. The commit disable tap read poll when
qemu_net_queue_send() returns zero. Which is usually the following cases:
1) queue-delivering is 1
2) qemu_can_send_packet() returns zero, which is:
2a) vm_running is false
or
2b) can_receive() return zero
3) qemu_net_queue_deliver() returns zero, which is:
3a) nc-receive_disabled is true
or
3b) info-receive_raw() or nc-receive-receive() returns zero
This means we should enable tap read poll when one of those conditions
is not existed. This patch fixes 2b) only for e1000.
for 1, I'm not quite sure it's a real problem or how to fix.
for 2a, we may probably need a vm state change handler to flush the
queue, otherwise network will stall after a stop and cont.
for 2b, we can either fixes the card that check link status in its
can_receive() or just simply can qemu_flush_queued_packets() in set_link.
3a and 3b looks ok, since this happen when there's no space for rx,
after guest refill, qemu will call qemu_flush_queued_packets() to flush
and re-enable tap read poll.
2a and 2b does not exits before this commit, since tap_send check
qemu_can_send_packet() before.
Looks like netmap has the same issue.
Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On Thu, 06/25 10:18, Stefan Hajnoczi wrote:
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Reviewed-by: Fam Zheng f...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
--
2.4.3
Re: [Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
On Thu, Jun 25, 2015 at 10:18:05AM +0100, Stefan Hajnoczi wrote: e1000_can_receive() checks the link up status register bit. If the bit is clear, packets will be queued and the peer may disable receive to avoid wasting CPU reading packets that cannot be delivered. The queue must be flushed once the link comes back up again. This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests and tap networking. Flushing the queue invokes the async send callback, which re-enables tap fd read. Reported-by: Jonathan Liu net...@gmail.com Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- hw/net/e1000.c | 3 +++ 1 file changed, 3 insertions(+) Thanks, applied to my net tree: https://github.com/stefanha/qemu/commits/net Stefan pgpinzV51XtRB.pgp Description: PGP signature
[Qemu-devel] [PATCH for-2.4] e1000: flush packets when link comes up
e1000_can_receive() checks the link up status register bit. If the bit
is clear, packets will be queued and the peer may disable receive to
avoid wasting CPU reading packets that cannot be delivered. The queue
must be flushed once the link comes back up again.
This patch fixes broken e1000 receive with Mac OS X Snow Leopard guests
and tap networking. Flushing the queue invokes the async send callback,
which re-enables tap fd read.
Reported-by: Jonathan Liu net...@gmail.com
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
hw/net/e1000.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index bab8e2a..5c6bcd0 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -185,6 +185,9 @@ e1000_link_up(E1000State *s)
{
s-mac_reg[STATUS] |= E1000_STATUS_LU;
s-phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
+
+/* E1000_STATUS_LU is tested by e1000_can_receive() */
+qemu_flush_queued_packets(qemu_get_queue(s-nic));
}
static bool
--
2.4.3
