In D23971#532136, @jhb wrote:

Reading the spec, it doesn't say that you have to define MQ to use MTU. The language is a bit odd in that it says "only exists" when in practice it seems to mean "is only valid and certain to exist". The Linux driver doesn't mandate that MTU requires MQ, and it will just ignore the MQ field. I think we are probably fine with taking that approach. Neither NetBSD nor OpenBSD support the option yet, so Linux's behavior is the only one I've seen.

Just to recap:

1. This patch has no effect if the user does not specify the mtu parameter explicitly. That is no effect in the default case.
2. This patch is implementing a feature described by the virtio standard, and in the same way QEMU does.

As a result, I personally do not see any reason why we should not go ahead.

Also, if the bhyve user does not provide a value for mtu, this patch won't have any effect.

LGTM

Il giorno ven 13 mar 2020 alle ore 17:56 aleksandr.fedorov_itglobal.com
(Aleksandr Fedorov) <phabric-noreply@freebsd.org> ha scritto:

I assume you have done your tests adding mtu=9000 as a bhyve parameter.
Could you please check that you have no regression in case you don't add an mtu argument (and so with default MTU)?

Il giorno mer 11 mar 2020 alle ore 11:51 aleksandr.fedorov_itglobal.com
(Aleksandr Fedorov) <phabric-noreply@freebsd.org> ha scritto:

Il giorno mer 11 mar 2020 alle ore 03:11 rgrimes <
phabric-noreply@freebsd.org> ha scritto:

I actually see an additional issue. According to the standard, VIRTIO_NET_F_MQ depends on VIRTIO_NET_F_CTRL_VQ.
So it looks like we should add support for VIRTIO_NET_F_CTRL_VQ as a first step.

In D23472#522420, @aleksandr.fedorov_itglobal.com wrote:

I tested this patch with various OS’s(windows, Ubuntu, centos) and didn’t find any issues. So, I think it’s ready to go.

Ok, I'll just go ahead with the commit.

In any case I tested 10 different combinations:

In D23472#521553, @aleksandr.fedorov_itglobal.com wrote:

Sorry, I didn’t have time to test the patch. But as for the code, I have no questions.

Ping. Any comments on this change?

In D23342#519736, @grehan wrote:

Is this good to go?

I gave it a tick.

Yes, this has been merged already.

Is this good to go?

Fix: Keep into account virtio-net header when collecting rx buffers.

Has anybody tested this on e1000?

In D23546#516998, @bcr wrote:

That's it, ready to go!
Thanks for making this change.

In D23546#516883, @lwhsu wrote:

Please don't forget to bump .Dd (date) :-)

Update date

Added missing newline.

Is this good to go?

In D23342#514165, @aleksandr.fedorov_itglobal.com wrote:

Yes indeed. But also the Linux virtio-net driver should do the same when mergeable rx buffers are not allocated.... so it is weird that it does not work in that case!

This is a big virtio specification problem. Too many unspecified actions and interpretations. Two weeks ago, I observed at a Windows guest that descriptors stand out as follows: | 12 | 1536 | 1536 | .... Even if the mergable buffers are enabled.

In D23342#514141, @aleksandr.fedorov_itglobal.com wrote:

Have you set mtu 9000 on the vtnet0 in the guest?
My tests were VM-2-VM, both freebsd guests.

Yes.
ifconfig from the guest (Ubuntu 16.04):

root@ubuntu:~# ifconfig
enp0s2    Link encap:Ethernet  HWaddr 00:a0:98:cb:89:bb
          inet addr:192.168.1.4  Bcast:192.168.1.255  Mask:255.255.255.0
          inet6 addr: fe80::2a0:98ff:fecb:89bb/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST  MTU:9000  Metric:1
          RX packets:11 errors:0 dropped:0 overruns:0 frame:0
          TX packets:150 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000
          RX bytes:8066 (8.0 KB)  TX bytes:48636 (48.6 KB)

Ping from the guest:

root@ubuntu:~# ping 192.168.1.77
PING 192.168.1.77 (192.168.1.77) 56(84) bytes of data.
64 bytes from 192.168.1.77: icmp_seq=1 ttl=64 time=1.05 ms
64 bytes from 192.168.1.77: icmp_seq=2 ttl=64 time=0.197 ms
--- 192.168.1.77 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1005ms
rtt min/avg/max/mdev = 0.197/0.627/1.057/0.430 ms
^Croot@ubuntu:~#

^Croot@ubuntu:~# ping -s 8000 192.168.1.77
PING 192.168.1.77 (192.168.1.77) 8000(8028) bytes of data.
^C
--- 192.168.1.77 ping statistics ---
4 packets transmitted, 0 received, 100% packet loss, time 3019ms
root@ubuntu:~#

I tried to debug this situation, and saw that the guest descriptors are 1536 bytes in size. Therefore, I do not understand how it could work with MTU == 9000.
I think the previous version of the patch is more adequate. And there was no POLA violation because with tap backend and MTU > 1500, it didn't work.

Yeah, but with mergeable buffers on, tap throughput dropped to 50%.
I'm not giving up on adding support for mergeable buffers. I just think it makes sense to split the patch into more changes.

Host:

root@mothership:/afedorov/vm # ifconfig tap1003
tap1003: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 9000
        options=80000<LINKSTATE>
        ether 58:9c:fc:10:cf:64
        inet 192.168.1.77 netmask 0xffffff00 broadcast 192.168.1.255
        groups: tap
        media: Ethernet autoselect
        status: active
        nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
        Opened by PID 89835
root@mothership:/afedorov/vm #

Ping from host:

root@mothership:/afedorov/vm # ping 192.168.1.4
PING 192.168.1.4 (192.168.1.4): 56 data bytes
64 bytes from 192.168.1.4: icmp_seq=0 ttl=64 time=0.156 ms
64 bytes from 192.168.1.4: icmp_seq=1 ttl=64 time=0.155 ms
^C
--- 192.168.1.4 ping statistics ---
2 packets transmitted, 2 packets received, 0.0% packet loss
round-trip min/avg/max/stddev = 0.155/0.156/0.156/0.001 ms
root@mothership:/afedorov/vm #

root@mothership:/afedorov/vm # ping -s 8000 192.168.1.4
PING 192.168.1.4 (192.168.1.4): 8000 data bytes
^C
--- 192.168.1.4 ping statistics ---
2 packets transmitted, 0 packets received, 100.0% packet loss
root@mothership:/afedorov/vm #

bhyve:

bhyve \
        -A -P -H \
        -s 0:0,hostbridge \
        -s 1:0,lpc \
        -s 2:0,virtio-net,tap1003 \
        -s 3:0,virtio-blk,./ubuntu-16.04.img \
        -l com1,stdio \
        -c 4\
        -m 2048M \
        ubuntu-16.04

Maybe of course I'm doing something wrong.

Yeah, but with mergeable buffers on, tap throughput dropped to 50%.
I'm not giving up on adding support for mergeable buffers. I just think it makes sense to split the patch into more changes.

Oh, then it's good. +1 for splitting patch.

In D23342#514144, @aleksandr.fedorov_itglobal.com wrote:

Hah, I checked fFreeBSD 12 guest and it works. It seems that, the FreeBSD guest driver allocates descriptors of size >= MTU.

In D23342#514125, @aleksandr.fedorov_itglobal.com wrote:

Thanks for the suggestion. I retried with -D (on head), and it still works. I cannot reproduce the broken jumbo-frames+tap...

It is very strange. In my tests, the version from the HEAD doesn't work:
Guest: Ubuntu 16.04 mtu 9000 192.168.1.4
Host: ifconfig tap1001 192.168.1.55/24 mtu 9000 up
Ping from host doesn't work.
ping -s 8000 -D 192.168.1.4

Removed the mergeable rx buffers changes. Keep the refactoring.

In D23342#513600, @darkfiberiru_gmail.com wrote:

@vmaffione Don't forget to add -D flag to your ping testing otherwise ping is fragmented and reassembled.

In D23342#513522, @grehan wrote:

The alternative solution (the same used by QEMU) is to read the packet in a local bounce buffer (64KB), so that we figure out its actual length and then we call vq_getchain() the corrent number of times (e.g. just once for 1500 bytes packets). The cost for this solution is an additional packet copy, which is not good imho.

Tap has enough overhead already that I suspect using a bounce-buffer would be noise (and I've done that in a proprietary implementation).
Tap needs to be fixed anyways e.g. by providing the length of the first available buffer in kqueue data, and providing the length of the following buffer if any in msgdata.

Is "mtu 9000" really not working? I tried on head (without this patch), by setting the mtu to 9000 for both if_vtnet guest interfaces and host interfaces, and I'm able to ping -s 8900 x.y.z.w. So what exactly is not working?

In D23342#512819, @aleksandr.fedorov_itglobal.com wrote:

The changes looks good to me.
Also, I tested Win10, CentOS7, Ubuntu 16 and18 in our test lab - everything works correctly.

Tested several combinations (vm-2-host, vm-2-vm, tap, vale, mergeable rx buffers on and off).
Everything seems to work correctly.

I refactored the code so that iov trimming is done by the virtio-net code, where this operation belongs.
As a nice result, now we can pass struct iovec arrays as const to the backend code.

• Aligned to current head.
• Fixed header in debug.h

Comments?

Thoughts about this change?

Removed "all rights reserved" line.
Fix multi-line debug logs in pci_xhci.c, splitting them into multiple PRINTLN() statements.

Fixed all the print statements.

Il giorno mar 19 nov 2019 alle ore 19:21 aleksandr.fedorov_itglobal.com
(Aleksandr Fedorov) <phabric-noreply@freebsd.org> ha scritto:

Init rx_merge to 0

I'll do it tonight.

Addressed comment.

Just one question: did this change come from some usage scenario? In theory you should not call nm_mmap() more than once.

Addressed more comments.

Addressed more comments.

Move check under the lock.
Minor refactor.

Comments addressed.

Addressed two comments.

Unfortunately there is a blocker for this change.

Thanks for testing.