I see various errors in the man page:

Do you have any performance measurements?
Is it have advantages over injecting packets through ng_socket(4) or ng_device(4)?

Is this PR: 241133 ?

After looking at your abandoned review (D26420) where you add parsing of IPv6 addresses. I wondered if we could use the generic inet_aton(3) functions available from kernel space?

Sorry to start a discussion here, but we have a similar problem with bhyve. When it is necessary to deliver packets from VM with partial checksum and TSO to the host stack (inbound path).
For example, we need to solve the next path:
VM (virtio-net, TSO, partial checksum) -> if_bridge/ng_bridge -> if_tuntap/ng_eiface -> host stack.

I really like the changes related to option handling.

@vmaffione , do you have any objections?

• Rebase
• Use ',' as options separator.
• Move '#include <sys/sysctl.h>' to NETGRAPH section.

• Free optscopy early.
• While I'm here, fix a memory leak in e1000 frontend.

• Fix the indicated issues.

It looks good to me. Thank you.

@vmaffione , I left '/' as a separator, because ';' used by the shell (tcsh, sh), so there is a need to enclosure the option string.
I think the best solution is still to pass the full line of options to the backend. Then we can use ',' as the delimiter.
I have not completely understood how backward compatibility can be broken? May you clarify?

• Reuse tap backend functions.

@vmaffione do you have any objections?

• Revert: 'relpath' -> 'path' option.
• Correctly calculate the maximum available socket buffer size, as is done in the kernel.

Please keep it "path".
"relpath" is useless in this situation, because your node is not yet connected and can't use a relative path at all.

• Return -1 earlier if an error occurs.
• Change separator from ':' to '/' to allow different Netgraph address types (See man 4 netgraph 'Addressing').
• Fix resources leak through ctrl_sock.
• Change the "path" option to "relpath" to match the ngctl connect command.

ping

I will try to fix all the places.

ping?

In D24620#541979, @lutz_donnerhacke.de wrote:

If I understand correctly, you are adding code in the VM-setup (copied from ngctl) to create a ng_socket and connect it to a specified node (ng_bridge). Then you are using the data channel of the ng_socket to transmit Ethernet frames.

Are there any objections to not committing this?

• Allow entering MTU value in HEX.

I tested this patch in our test lab with a lot of netgraph nodes. And I find it very useful.

I look at this issue from network virtualization point of view.
I have a plan (and patches) for adding native Netgraph support to the Bhyve network backend through ng_socket(4).
For this case, it is very interesting to be able to create ng_bridge(4) with the following options:

• One "uplink" hook with learning turned off, but all unknown MAC's go through it.
• The rest of the hooks have learning enabled, but unknown MAC's are not sent to them.

Can we come to some kind of consensus?

• Add additional checks (see Test Plan).

I understand your opinion, but because we do not know what kind of OS will be used as a guest, I think additional checks will not be superfluous. Also, I think it's easier to look for errors if bhyve crashes earlier.

• Fix typo.
• Enable VIRTIO_NET_F_MTU only if user provide mtu argument.

Yes, I tested various MTU values ​​(including 9000) with different backends (if_tuntap(4), vale). I did not find any regressions without using the mtu argument.

• Fix typo.
• Disable VIRTIO_NET_F_MQ flag.
• Check the lower boundary of the MTU with ETHERMIN.

In D23971#528210, @vmaffione wrote:

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.

I agree that the lower bound looks weird. And it came from Linux:
https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L2978
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/if_ether.h#L40

• Move checks of the MTU value to pci_virtio_net.c and add the corresponding min/max definitions from the virtio specification.
• Fix default MTU value.
• Add VIRTIO_NET_F_MQ capability, and initialize max_virtqueue_pairs.

Oh, I'm glad to hear that. May you comment on support from Bhyve side?

Is it really useful to have multiple uplinks?

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.

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

divert sockets can be used with other software but present exactly same significant overhead.
Do you have an example when suggested change improves performance really?

Ping. Are there any issues that prevent committing these changes?

divert(4) sockets can be used not only with natd(8), so the changes looks reasonable for me.

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!

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

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

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

The changes looks good to me.

I think this patch is the right direction. Now it’s really very difficult to add a storage backend other than file-like operations.

ping

I catch this KASSERT (https://svnweb.freebsd.org/base/head/sys/amd64/vmm/vmm_dev.c?view=markup#l799) when trying to attach debugger to bhyve process.

We are also interested in this interface.
It looks good for use with vale (4) to connect a virtual network to the host stack: bhyve VM's - vale (4) - if_vether (4) - Host. I tested this and it works well. But now we use if_epair (4) for this configuration, because I didn't know about the existence of the port.

I welcome these changes. The current debug output looks like a mess.
Moreover, part of the files declares (W/D)PRINTF as

The race would not have been fatal (but is still good to fix) if the default been the more compatible behavior.

The changes look good to me. And I can not reproduce the bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241808

I think you need to add people with good knowledge of the netgraph to the reviewers: JulianElischer, Gleb Smirnoff and Alexander Motin. Because apparently not all are subscribed to the network mailing list.

In D20973#471364, @jhb wrote:

So I don't see any uses of mevent_add_disabled() in this change?