On Wed, Apr 22, 2020 at 05:54:20PM +0100, Stefan Hajnoczi wrote:
> On Tue, Apr 21, 2020 at 06:17:24PM +0200, Stefano Garzarella wrote:
> > On Tue, Apr 21, 2020 at 04:42:46PM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Apr 21, 2020 at 11:25:27AM +0200, Stefano Garzarella wrote:
> > > > We delivering packets to monitoring devices, before to check if
> > > > the virtqueue has enough space.
> > >
> > > "We [are] delivering packets" and "before to check" -> "before
> > > checking". Perhaps it can be rewritten as:
> > >
> > > Packets are delivered to monitoring devices before checking if the
> > > virtqueue has enough space.
> > >
> >
> > Yeah, it is better :-)
> >
> > > >
> > > > If the virtqueue is full, the transmitting packet is queued up
> > > > and it will be sent in the next iteration. This causes the same
> > > > packet to be delivered multiple times to monitoring devices.
> > > >
> > > > This patch fixes this issue, postponing the packet delivery
> > > > to monitoring devices, only when it is properly queued in the
> > >
> > > s/,//
> > >
> > > > virqueue.
> > >
> > > s/virqueue/virtqueue/
> > >
> >
> > Thanks, I'll fix in the v2!
> >
> > > > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work)
> > > > break;
> > > > }
> > > >
> > > > + /* Deliver to monitoring devices all correctly transmitted
> > > > + * packets.
> > > > + */
> > > > + virtio_transport_deliver_tap_pkt(pkt);
> > > > +
> > >
> > > The device may see the tx packet and therefore receive a reply to it
> > > before we can call virtio_transport_deliver_tap_pkt(). Does this mean
> > > that replies can now appear in the packet capture before the transmitted
> > > packet?
> >
> > hmm, you are right!
> >
> > And the same thing can already happen in vhost-vsock where we call
> > virtio_transport_deliver_tap_pkt() after the vhost_add_used(), right?
> >
> > The vhost-vsock case can be fixed in a simple way, but here do you think
> > we should serialize them? (e.g. mutex, spinlock)
> >
> > In this case I'm worried about performance.
> >
> > Or is there some virtqueue API to check availability?
>
> Let's stick to the same semantics as Ethernet netdevs. That way there
> are no surprises to anyone who is familiar with Linux packet captures.
> I don't know what those semantics are though, you'd need to check the
> code :).
IIUC, the packet is delivered to tap/monitoring devices before to call
the xmit() callback provided by the NIC driver.
At that point, if the packet is delayed/dropped/retransmitted by the driver
or the NIC, the monitoring application is not aware.
So, I think we can delivery it the first time that we see the packet,
before to queue it in the virtqueue (I should revert this change and fix
vhost-vsock), setting a flag in the 'struct virtio_vsock_pkt' to avoid
to delivery it multiple times.
I mean something like this:
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -157,7 +157,11 @@ static struct sk_buff *virtio_transport_build_skb(void *opaque)
void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
{
+ if (pkt->tap_delivered)
+ return;
+
vsock_deliver_tap(virtio_transport_build_skb, pkt);
+ pkt->tap_delivered = true;
}
EXPORT_SYMBOL_GPL(virtio_transport_deliver_tap_pkt);
Let me know if you think it is a bad idea.
I'll send a v2 whit these changes.
Thanks,
Stefano
