Message-ID: <aa70346d6983a0146b2220e93dac001706723fe3.camel@infradead.org>
Date:   Wed, 30 Jun 2021 11:02:54 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Jason Wang <jasowang@...hat.com>, netdev@...r.kernel.org
Cc:     Eugenio Pérez <eperezma@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>,
        "Michael S.Tsirkin" <mst@...hat.com>
Subject: Re: [PATCH v3 3/5] vhost_net: remove virtio_net_hdr validation, let
 tun/tap do it themselves

On Wed, 2021-06-30 at 12:39 +0800, Jason Wang wrote:
> 在 2021/6/29 下午6:49, David Woodhouse 写道:
> > On Tue, 2021-06-29 at 11:43 +0800, Jason Wang wrote:
> > > > The kernel on a c5.metal can transmit (AES128-SHA1) ESP at about
> > > > 1.2Gb/s from iperf, as it seems to be doing it all from the iperf
> > > > thread.
> > > > 
> > > > Before I started messing with OpenConnect, it could transmit 1.6Gb/s.
> > > > 
> > > > When I pull in the 'stitched' AES+SHA code from OpenSSL instead of
> > > > doing the encryption and the HMAC in separate passes, I get to 2.1Gb/s.
> > > > 
> > > > Adding vhost support on top of that takes me to 2.46Gb/s, which is a
> > > > decent enough win.
> > > 
> > > Interesting, I think the latency should be improved as well in this
> > > case.
> > 
> > I tried using 'ping -i 0.1' to get an idea of latency for the
> > interesting VoIP-like case of packets where we have to wake up each
> > time.
> > 
> > For the *inbound* case, RX on the tun device followed by TX of the
> > replies, I see results like this:
> > 
> >       --- 172.16.0.2 ping statistics ---
> >       141 packets transmitted, 141 received, 0% packet loss, time 14557ms
> >       rtt min/avg/max/mdev = 0.380/0.419/0.461/0.024 ms
> > 
> > 
> > The opposite direction (tun TX then RX) is similar:
> > 
> >       --- 172.16.0.1 ping statistics ---
> >       295 packets transmitted, 295 received, 0% packet loss, time 30573ms
> >       rtt min/avg/max/mdev = 0.454/0.545/0.718/0.024 ms
> > 
> > 
> > Using vhost-net (and TUNSNDBUF of INT_MAX-1 just to avoid XDP), it
> > looks like this. Inbound:
> > 
> >       --- 172.16.0.2 ping statistics ---
> >       139 packets transmitted, 139 received, 0% packet loss, time 14350ms
> >       rtt min/avg/max/mdev = 0.432/0.578/0.658/0.058 ms
> > 
> > Outbound:
> > 
> >       --- 172.16.0.1 ping statistics ---
> >       149 packets transmitted, 149 received, 0% packet loss, time 15391ms
> >       rtt mn/avg/max/mdev = 0.496/0.682/0.935/0.036 ms
> > 
> > 
> > So as I expected, the throughput is better with vhost-net once I get to
> > the point of 100% CPU usage in my main thread, because it offloads the
> > kernel←→user copies. But latency is somewhat worse.
> > 
> > I'm still using select() instead of epoll() which would give me a
> > little back — but only a little, as I only poll on 3-4 fds, and more to
> > the point it'll give me just as much win in the non-vhost case too, so
> > it won't make much difference to the vhost vs. non-vhost comparison.
> > 
> > Perhaps I really should look into that trick of "if the vhost TX ring
> > is already stopped and would need a kick, and I only have a few packets
> > in the batch, just write them directly to /dev/net/tun".
> 
> 
> That should work on low throughput.

Indeed it works remarkably well, as I noted in my follow-up. I also
fixed a minor stupidity where I was reading from the 'call' eventfd
*before* doing the real work of moving packets around. And that gives
me a few tens of microseconds back too.

> > I'm wondering how that optimisation would translate to actual guests,
> > which presumably have the same problem. Perhaps it would be an
> > operation on the vhost fd, which ends up processing the ring right
> > there in the context of *that* process instead of doing a wakeup?
> 
> 
> It might improve the latency in an ideal case but several possible issues:
> 
> 1) this will blocks vCPU running until the sent is done
> 2) copy_from_user() may sleep which will block the vcpu thread further

Yes, it would block the vCPU for a short period of time, but we could
limit that. The real win is to improve latency of single, short packets
like a first SYN, or SYNACK. It should work fine even if it's limited
to *one* *short* packet which *is* resident in memory.

Although actually I'm not *overly* worried about the 'resident' part.
For a transmit packet, especially a short one not a sendpage(), it's
fairly likely the the guest has touched the buffer right before sending
it. And taken the hit of faulting it in then, if necessary. If the host
is paging out memory which is *active* use by a guest, that guest is
screwed anyway :)

I'm thinking of something like an ioctl on the vhost-net fd which *if*
the thread is currently sleeping and there's a single short packet,
processes it immediately. {Else,then} it wakes the thread just like the
eventfd would have done. (Perhaps just by signalling the kick eventfd,
but perhaps there's a more efficient way anyway).

> > My bandwidth tests go up from 2.46Gb/s with the workarounds, to
> > 2.50Gb/s once I enable XDP, and 2.52Gb/s when I drop the virtio-net
> > header. But there's no way for userspace to *detect* that those bugs
> > are fixed, which makes it hard to ship that version.

I'm up to 2.75Gb/s now with epoll and other fixes (including using
sendmmsg() on the other side). Since the kernel can only do *half*
that, I'm now wondering if I really want my data plane in the kernel at
all, which was my long-term plan :)

> Yes, that's sad. One possible way to advertise a VHOST_NET_TUN flag via 
> VHOST_GET_BACKEND_FEATURES?

Strictly it isn't VHOST_NET_TUN, as that *does* work today if you pick
the right (very non-intuitive) combination of features. The feature is
really "VHOST_NET_TUN_WITHOUT_TUNSNDBUF_OR_UNWANTED_VNET_HEADER" :)

But we don't need a feature specifically for that; I only need to check
for *any* feature that goes in after the fixes.

Maybe if we do add a new low-latency kick then I could key on *that*
feature to assume the bugs are fixed.

Alternatively, there's still the memory map thing I need to fix before
I can commit this in my application:

#ifdef __x86_64__
	vmem->regions[0].guest_phys_addr = 4096;
	vmem->regions[0].memory_size = 0x7fffffffe000;
	vmem->regions[0].userspace_addr = 4096;
#else
#error FIXME
#endif
	if (ioctl(vpninfo->vhost_fd, VHOST_SET_MEM_TABLE, vmem) < 0) {

Perhaps if we end up with a user-visible feature to deal with that,
then I could use the presence of *that* feature to infer that the tun
bugs are fixed.

Another random thought as I stare at this... can't we handle checksums
in tun_get_user() / tun_put_user()? We could always set NETIF_F_HW_CSUM
on the tun device, and just do it *while* we're copying the packet to
userspace, if userspace doesn't support it. That would be better than
having the kernel complete the checksum in a separate pass *before*
handing the skb to tun_net_xmit().

We could similarly do a partial checksum in tun_get_user() and hand it
off to the network stack with ->ip_summed == CHECKSUM_PARTIAL.



Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5174 bytes)

Powered by blists - more mailing lists