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.

Addressed reviewer comments.

Thanks for looking at this.
No, the patch is ready as is (I just retested everything again).

ping

No problems and thanks for the in-depth review.

Addressed more comments.
valectl.4 renamed to valectl.8

(removed Makefile.depend again)

Addressed reviewer comments.

Sounds good to me.
The valectl program already fails if netmap is not present, so no build-time check for MK_NETMAP or DEV_NETMAP is really needed. I was just thinking of people that want to shrink the image size as much as possible (and save a few KB in this case).

Removed Makefile.depend

Thanks for the review (and sorry for the trivial flaws).
Yes, I agree to stick to the existing convention for *ctl names.

Addressed reviewers' comments

Il giorno ven 18 ott 2019 alle ore 23:47 kevans (Kyle Evans) <
phabric-noreply@freebsd.org> ha scritto:

Thanks. Yes, I don't see either any reason why we want to reset device configuration (vnet hdr, mtu, etc.) on close.

Any reviews?
Thanks

Don't call tun_vnethdr_set() on pid change.

Addressed comments from kevans:

• style fixes
• reset vhdrlen to 0 when PID changes; this required the introduction of a new helper function (tun_vnethdr_set)

Note: this is the next patch to be reviewed. Thanks.

Does it look good now?

Follow-up change after updating D20973.
Define mevent_add_disabled here.

Regarding mevent_add_disabled, I can defer that to the next patch.

Addressed jhb's comments.

Thanks a lot!

ping

Minor changes.

The ptnet(4) case is a bit different, since the driver knows at attach time which if_capabilities are supported, and those are not going to change.
On the other hand, with these changes to tap(4) the user can change if_capabilities dynamically.

Added SIOCSIFCAP support. Addressed more comments.

The inline functions added to virtio_net.h were simply copy-pasted from if_vtnet.c to if_ptnet.c in the first place. And yes, I plan to make also if_vtnet.c use the same code in future. I haven't done it yet because if_vtnet.c increments device specific statistic counters.

Addressed suggestions.

Any opinions on this change?

Any additional opinions on this patch?

Any opinions on this patch?

Thanks a lot for your effort!
To be honest I'm not sure why the throughput increases so much, since TSO (64KB unchecksummed packets) is being used in both cases.
The main difference is that with mergeable rx buffers there is less pressure on the guest memory allocators, since the driver can allocate 2K clusters, rather than bigger packets.
Also, with mergeable rx buffer bhyve may do a little more work, because it needs to call vq_gechain() 33 times in order to receive each packet.
In any case, your results look very good to me, and they also agree with mine (taken with a smaller and less powerful machine).

Thanks. Did you notice any change in terms of performance?

Fix issue identified by @aleksandr.fedorov_itglobal.com

Removed traling space.

Upload the right patch

minimize patch by adding mevent_add_disabled().

Start the backends with receive enabled for the moment being.

Changed the patch to update the license identifiers only.

Looks good, thanks.
The netmap unit tests and integration tests still pass with these changes.

Thanks. This touches also TAP and setups, so we need tests with TAP.
Also, we would need to check that TAP+e1000 works.

Thanks for the review. I actually don't have yet a real testbed for this (will get one soon), and I can only do functional tests in a nested KVM VM. Any test is welcome.

Addressed coments from reviewers.

Thanks for the in depth review. Sorry about the style issues. Is there a FreeBSD clang-format file against which I can validate code?

Tried to address jhb's comments.