e1000 PXE breakage bisected down in kvm-userspace

e1000 PXE breakage bisected down in kvm-userspace

[Charles Duffy]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

Per subject. "git bisect run" script (and libvirt xml helper) attached.

> 87b5acf6125d205119e3194c02ed6e71715517dc is first bad commit
> commit 87b5acf6125d205119e3194c02ed6e71715517dc
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Wed May 7 13:09:40 2008 -0500
> 
>     kvm: qemu: stop dropping so many RX packets in tap
>     
>     Normally, tap always reads packets and simply lets the client drop them if it
>     cannot receive them.  For virtio-net, this results in massive packet loss and
>     about an 80% performance loss in TCP throughput.
>     
>     This patch modifies qemu_send_packet() to only deliver a packet to a VLAN
>     client if it doesn't have a fd_can_read method or the fd_can_read method
>     indicates that it can receive packets.  We also return a status of whether
>     any clients were able to receive the packet.
>     
>     If no clients were able to receive a packet, we buffer the packet until a
>     client indicates that it can receive packets again.
>     
>     This patch also modifies the tap code to only read from the tap fd if at least
>     one client on the VLAN is able to receive a packet.
>     
>     Finally, this patch changes the tap code to drain all possible packets from
>     the tap device when the tap fd is readable.
>     
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>     Signed-off-by: Avi Kivity <avi@qumranet.com>
> 
> :040000 040000 648a4d52612e0ce68167f279d30bd3f4ec0f22f4 85352e7b252abb1247af2f024eab10a3e8ec0084 M	qemu

[-- Attachment #2: kvm-e1000-test --]
[-- Type: text/plain, Size: 1425 bytes --]

#!/bin/bash

LIBVIRT_VM_NAME=com.m1dev.syseng.cduffy.boottest
LIBVIRT_VM_XML=/local/src/kvm-e1000-test.xml
VM_CONSOLE_LOG=/local/src/kvm-e1000-test.out
SRC_TREE_USER=/local/src/kvm-userspace
SRC_TREE_KERN=/local/src/kvm

set -e

function try_patch() {
	local PATCH="$1"
	if patch -f -p0 --dry-run < "$PATCH" ; then
		patch -p0 < "$PATCH"
	fi
	return 0
}

function cleanup() {
	virsh destroy ${LIBVIRT_VM_NAME} || true
	rm -f ${VM_CONSOLE_LOG}
}

function test_failure() {
	exit 125
}
trap test_failure 0

cd ${SRC_TREE_USER}
git reset --hard
try_patch ../kvm-userspace.centos5_compat.kvm-69.patch
try_patch ../kvm-userspace.centos5_compat.kvm-70.patch
./configure \
	--prefix=/opt/kvm.git \
	--kerneldir=/usr/src/kernels/2.6.18-92.1.6.el5-x86_64 \
	--disable-gfx-check \
	--disable-sdl
make WANT_MODULE= KERNELDIR=${SRC_TREE_KERN} clean
make WANT_MODULE= KERNELDIR=${SRC_TREE_KERN}
make WANT_MODULE= KERNELDIR=${SRC_TREE_KERN} install
git reset --hard

cleanup
virsh create "${LIBVIRT_VM_XML}"
sleep 5
if [ ! -e "${VM_CONSOLE_LOG}" ] ; then
	echo 'ERR: No virtual console detected' >&2
	exit 125
fi

echo 'Waiting for kernel splash notice' >&2
trap - 0
trap cleanup 0
COUNTDOWN=60
while [ "$COUNTDOWN" -gt 0 ] ; do
	echo -n '.' >&2
	COUNTDOWN="$(($COUNTDOWN - 1))"
	sleep 1
	if grep -e 'Linux version' ${VM_CONSOLE_LOG} ; then
		echo 'OK: net boot succeeded' >&2
		exit 0
	fi
done
echo 'BAD: no net boot detected' >&2
exit 1

[-- Attachment #3: kvm-e1000-test.xml --]
[-- Type: text/xml, Size: 992 bytes --]

<domain type='kvm'>
  <name>com.m1dev.syseng.cduffy.boottest</name>
  <uuid>70d09285-f451-426d-8ab0-4c4810616ce6</uuid>
  <memory>768432</memory>
  <currentMemory>768432</currentMemory>
  <vcpu>1</vcpu>
  <os>
    <type arch='i686' machine='pc'>hvm</type>
    <boot dev='network'/>
  </os>
  <features>
    <acpi/>
  </features>
  <clock offset='utc'/>
  <on_poweroff>destroy</on_poweroff>
  <on_reboot>restart</on_reboot>
  <on_crash>destroy</on_crash>
  <devices>
    <emulator>/opt/kvm.git/bin/qemu-system-x86_64</emulator>
    <disk type='file' device='disk'>
      <source file='/local/VM/com.m1dev.syseng.cduffy.boottest.raw'/>
      <target dev='sda' bus='scsi'/>
    </disk>
    <interface type='network'>
      <mac address='00:16:3e:fe:cb:31'/>
      <source network='com.m1dev.syseng.cduffy'/>
      <model type='e1000'/>
    </interface>
    <console type='file'>
      <source path='/local/src/kvm-e1000-test.out'/>
      <target port='0'/>
    </console>
  </devices>
</domain>

Re: e1000 PXE breakage bisected down in kvm-userspace

[Anthony Liguori]

Charles Duffy wrote:
> Per subject. "git bisect run" script (and libvirt xml helper) attached.

Does the problem go away if you make the e1000_can_receive() function 
always return 1?

Regards,

Anthony Liguori

>> 87b5acf6125d205119e3194c02ed6e71715517dc is first bad commit
>> commit 87b5acf6125d205119e3194c02ed6e71715517dc
>> Author: Anthony Liguori <aliguori@us.ibm.com>
>> Date:   Wed May 7 13:09:40 2008 -0500
>>
>>     kvm: qemu: stop dropping so many RX packets in tap
>>         Normally, tap always reads packets and simply lets the client 
>> drop them if it
>>     cannot receive them.  For virtio-net, this results in massive 
>> packet loss and
>>     about an 80% performance loss in TCP throughput.
>>         This patch modifies qemu_send_packet() to only deliver a 
>> packet to a VLAN
>>     client if it doesn't have a fd_can_read method or the fd_can_read 
>> method
>>     indicates that it can receive packets.  We also return a status 
>> of whether
>>     any clients were able to receive the packet.
>>         If no clients were able to receive a packet, we buffer the 
>> packet until a
>>     client indicates that it can receive packets again.
>>         This patch also modifies the tap code to only read from the 
>> tap fd if at least
>>     one client on the VLAN is able to receive a packet.
>>         Finally, this patch changes the tap code to drain all 
>> possible packets from
>>     the tap device when the tap fd is readable.
>>         Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>     Signed-off-by: Avi Kivity <avi@qumranet.com>
>>
>> :040000 040000 648a4d52612e0ce68167f279d30bd3f4ec0f22f4 
>> 85352e7b252abb1247af2f024eab10a3e8ec0084 M    qemu

Re: e1000 PXE breakage bisected down in kvm-userspace

[Charles Duffy]

Anthony Liguori wrote:
> Charles Duffy wrote:
>> Per subject. "git bisect run" script (and libvirt xml helper) attached.
> 
> Does the problem go away if you make the e1000_can_receive() function 
> always return 1?

Yes.

Re: e1000 PXE breakage bisected down in kvm-userspace

[Anthony Liguori]

Charles Duffy wrote:
> Anthony Liguori wrote:
>> Charles Duffy wrote:
>>> Per subject. "git bisect run" script (and libvirt xml helper) attached.
>>
>> Does the problem go away if you make the e1000_can_receive() function 
>> always return 1?
>
> Yes.

Presumably, this is the proper fix.  Can you verify?

diff --git a/qemu/hw/e1000.c b/qemu/hw/e1000.c
index 1be69ec..6a07b0c 100644
--- a/qemu/hw/e1000.c
+++ b/qemu/hw/e1000.c
@@ -521,8 +521,8 @@ e1000_can_receive(void *opaque)
 {
     E1000State *s = opaque;
 
-    return ((s->mac_reg[RCTL] & E1000_RCTL_EN) &&
-           s->mac_reg[RDH] != s->mac_reg[RDT]);
+    return (!(s->mac_reg[RCTL] & E1000_RCTL_EN) ||
+            s->mac_reg[RDH] != s->mac_reg[RDT]);
 }
 
 static void

Regards,

Anthony Liguori

> -- 
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: e1000 PXE breakage bisected down in kvm-userspace

[Charles Duffy]

Anthony Liguori wrote:
> Charles Duffy wrote:
>> Anthony Liguori wrote:
>>> Charles Duffy wrote:
>>>> Per subject. "git bisect run" script (and libvirt xml helper) attached.
>>>
>>> Does the problem go away if you make the e1000_can_receive() function 
>>> always return 1?
>>
>> Yes.
> 
> Presumably, this is the proper fix.  Can you verify?

That patch is already applied in 87b5ac; reversing it there does not 
resolve the issue.

Applying the given patch on current HEAD (5c646) also does not resolve 
the issue.

Re: e1000 PXE breakage bisected down in kvm-userspace

[Charles Duffy]

Anthony Liguori wrote:
> diff --git a/qemu/hw/e1000.c b/qemu/hw/e1000.c
> index 1be69ec..6a07b0c 100644
> --- a/qemu/hw/e1000.c
> +++ b/qemu/hw/e1000.c
> @@ -521,8 +521,8 @@ e1000_can_receive(void *opaque)
> {
>     E1000State *s = opaque;
> 
> -    return ((s->mac_reg[RCTL] & E1000_RCTL_EN) &&
> -           s->mac_reg[RDH] != s->mac_reg[RDT]);
> +    return (!(s->mac_reg[RCTL] & E1000_RCTL_EN) ||
> +            s->mac_reg[RDH] != s->mac_reg[RDT]);
> }

Having e1000_can_receive return true only if s->mac_reg[RDH] != 
s->mac_reg[RDT] means that the logic in e1000_receive() which sets the 
receive overflow flag is never reached.

I think it may be more correct to check only for s->mac_reg[RCTL] & 
E1000_RCTL_EN.