- User Since
- Mar 10 2018, 1:54 AM (70 w, 5 d)
Tue, Jul 16
Upload the right patch
minimize patch by adding mevent_add_disabled().
Start the backends with receive enabled for the moment being.
Mon, Jul 8
Changed the patch to update the license identifiers only.
Sun, Jul 7
Thu, Jul 4
Mon, Jul 1
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.
Sun, Jun 30
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.
Thu, Jun 27
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.
Mon, Jun 24
Sat, Jun 22
Fri, Jun 21
Jun 16 2019
Jun 13 2019
Do you mean a common vq_notify callback for both transmit and receive queue? But pci_vtnet_ping_txq() and pci_vtnet_ping_rxq() perform different work ...
Removed "All rights reserved".
Jun 12 2019
Jun 11 2019
Jun 10 2019
Provide patch relative to /usr/src/.
Moved include <machine/atomic.h> to virtio.h.
This looks better, but the problem is that I was refactoring this file to separate virtio specific code from the backends (netmap, tap), and add
support for offloads (including TSO); this patch creates many conflicts with my pending work, and IMHO it should be rebased after my work to prevent a total mess.
Jun 9 2019
Jun 7 2019
Sorry for the diff context, I forgot about that.
Thanks for the suggestions, they look good to me, and I'll implement those in short.
Btw I would like to refactor/improve vtnet/tap/netmap support, so more patches will follow.
Jun 6 2019
Jun 4 2019
May 27 2019
Yes, but I think the story is more complex than that. Guests can publish single-descriptor chains or multi-descriptor chains, depending on feature negotiation and driver implementation. With mergeable buffers enabled, or TSO features disabled, guests will normally submit single-descriptor chains (because it makes sense), but this is not mandatory. With TSO enabled and mergeable buffers disabled, guests will normally pass in multi-descriptor chains, each one describing 64K or 32K buffers.
It makes sense to add more vq methods to handle the mergeable rx bufs case, but think they should be moved to usr.sbin/bhyve/virtio.c, so that they can be reused.
May 22 2019
Netmap usage by itself looks mostly ok, except where noted. We could improve the management of r->cur in certain cases, but it's not particularly important for now (for reference, please look at my QEMU implementation, for instance the transmit routine is here https://github.com/qemu/qemu/blob/master/net/netmap.c#L160-L225).
I still need to review your changes to the virtqueue processing. But why did you drop vq_getchain() and write a custom method? If another method is needed, it should be added to virtio.c IMHO.
May 17 2019
May 16 2019
Apr 9 2019
Mar 19 2019
Feb 18 2019
No worries, and thanks for testing!
Feb 14 2019
In my tests I can see that without the patch, two pkt-gen communicating over a VALE port will consume 9% CPU on kqueue processing (although kqueue is not used), on a separate CPU.
With the patch, the 9% goes away.
Feb 13 2019
We can't. We use a taskqueue there precisely because we cannot take the si->m lock (or we get LOR).
Feb 7 2019
It would probably help to have more lines of contexts for the patch.
If you used git, you should probably use something like:
git diff -U30 [...]
If you used svn
svn diff -x '-U30'
Jan 30 2019
Thanks for testing.
Jan 29 2019
It looks like reading the the HEAD of the first TX ring works, while the other TX rings don't...
knote() notification work deferred to a per-selinfo taskqueue
That's convincing, thanks. I will try to switch to a per-netmap-adapter private taskqueue, to avoid locking contention on the same taskqueue.
Jan 28 2019
Obsoleted by https://reviews.freebsd.org/D18956
I went for reusing a system-wide taskqueue because the work to be done is minimal (i.e. it is not something like processing a number of descriptors in a device queue, or a list of packets, etc.).
However, if you think it's better to use a netmap-wide taskqueue for this, we can go for it.
Are you worried about something in particular, w.r.t. the current approach?
Thanks for testing.
I think this is safe to commit.
Actually some people is experiencing problems when using netmap on ixl (see related PR), so I would like to understand what is going on...
Jan 26 2019
Sanitize the value of the hw head index read at the end of the TX ring.
Jan 25 2019
Thanks for testing. So this seems to work.
However, introducing a global lock is not the best approach for performance reasons. I tried to came up with a different approach that also allows us to get rid of the mtx_owned() call.
Jan 24 2019
I could not reproduce your issue, but this patch may be useful to match the actual locks involved against the corresponding netmap ports.
Thanks for reporting. Also the code sample can be useful to add some unit tests for kqueue.
Jan 23 2019
Uploading diff with some context (-U 100).
Fixed indentation as suggested.
Jan 21 2019
That's another interesting question.
It looks like it was added to support the case where multiple threads poll() on the same netmap port. In that case, one of the thread performs the actual TXSYNC or RXSYNC, and if some work was actually performed (e.g. ring indices were advanced) the thread will call nm_notify() to wake up all the other threads.
Thanks for the explanation. Yes, I understand that this use of f_event is unusual, and I see your point.
Jan 18 2019
Thanks a lot!
Added @markj to possibly answer the question about calling KNOTE() in f_event.
Thanks a lot for testing! Yes, a lot of comments looked really inconsistent with the code. I did some more consistent update.
It would be great if you could run your tests again, since I've done a small code change in netmap_knrw().
Cleanup comments related to kqueue.
Jan 17 2019
Dec 31 2018
Thank you very much for your thorough review!
I will try to convert that to a TAP test, and ask for help if I find obstacles.
Dec 24 2018
Formatted .dist file.
Dec 23 2018
Any more comments? Can I go ahead and commit this?
Dec 21 2018
Thanks for the quick reply!