Virtio uses shiftleft/shiftright to tag packets #1034
Conversation
Instead of memmoving a packet's data around this patch instead moves around the packet pointer itself. The only performance testing I did was "snabb lwaftr bench", where it appears to be neutral-to-slight-win. I am hoping that the Hydra/R scientists can help me out on this one. This change to the packet structure does have a knock-on effect, in two significant ways. One is that the shiftleft, shiftright, and prepend functions now return a new packet pointer and silently corrupt the old one (because probably they shifted around the data but still had to update the new "length" pointer). That's somewhat OK though. The problem comes in the datagram library, which sometimes wants to work on a user-supplied packet. The sequence was: local dgram = datagram:new(pkt) dgram:push(....) dgram:free() ... foo(pkt) This pattern is no longer valid :( The reason is that the datagram methods that mutate a packet often do so by packet.prepend(). The datagram library updates its internal packet pointer, but the external one that was initially passed in is no longer valid. So much of this patch is visiting (hopefully) all of the uses/users and updating them.
Instead of defining duplicate versions of structs in virtq_driver.lua, just use the C headers.
Use direct descriptors, and use packet.shift to add and remove virtio headers.
|
stats will be here: https://hydra.snabb.co/build/659075 |
|
@lukego wdyt about this one? Only affects Snabb guests, not DPDK or other guests. There is no corresponding change for the host because the choice of descriptor layout is up to the guest, not the host. The PR is hard to read though because it includes earlier patches from #1032; perhaps just look at the last two commits. Nicola Larosa and @domenkozar are working on getting lwaftr tests into Hydra but I think we don't yet have them wired up. Still, obvious improvement I think, simplifying the code and giving us twice the descriptor count. |
|
ping :) |
|
It looks like a solid win on iperf results. On the three 'l2fwd by configuration' graphs, it looks like it may be slightly worse; I can't tell eyeballing it if that is significant. Interestingly, 2.1.0 noind is still bimodal. |
|
I'd be willing to upstream it, if @lukego is happy with an Igalian upstreaming for an Igalian. |
|
That linked perf analysis includes the result of #1032. Let me see if I find a new one. |
|
Oh, or. perhaps I have misunderstood this change. Is this changing the device side of virtio-net? I will look more closely now... |
|
This is changing the driver side of virtio-net. |
|
It changes src/lib/virtio/virtq_driver.lua |
|
OK. In that case I prefer not to be upstream as per ^^^ and I don't think this code has any coverage in existing Hydra tests (which currently always runs DPDK inside the guest and never Snabb). @kbara Sure but |
|
(The ideal would be to add a Hydra case that runs Snabb inside the VM in addition to DPDK. This should be fairly straightforward FWIW.) |
|
(i.e. it would be an extension of the existing |
|
I verrrrry much understand not wanting to land something without CI, but I think the right thing to do here is just merge this so we can merge the lwaftr. Merging it does not make the situation worse and this is the code that we are shipping and we are the only users of this code, and in parallel @dpino has #736 open and Domen and Nicola are working on getting hydra+lwaftr+qemu+... working too, so things are on the right track IMO. @kbara wdyt? |
|
Looks like @lukego has thumbed up it. I'm ok with that workflow, with the caveat that I think we really need to do our best to have test coverage of it in the next ~2 weeks, before the next release is cut. |
|
I've reviewed it. LGTM, @wingo . As per out of band discussion, please merge this to wingo-next, since you'll need it there for the lwaftr merge and there's no point putting it in two maintainer-next branches. :) |
|
Slightly off-topic...
There have been two instances of students asking for help with virtio_net on snabb-devel (latest instance: https://groups.google.com/forum/#!topic/snabb-devel/8eLfuiKksjY ). Sadly, I couldn’t help them because I have never used virtio_net and neither have a way to test it. Since there seems to heightened interest in this, I think it would be very valuable to make virtio_net a first class Snabb citizen like vhost_user, and integration with the l2fwd benchmark would go a long way! |
Load potentially 64-bit values using mov64
This builds on #1032 and uses shiftleft/shiftright in the virtio-net driver to tag and untag packets. It also switches guest snabb applications to use direct descriptors.