On Mon, Apr 12, 2021 at 03:39:36PM -0700, Jiang Wang . wrote:
> On Mon, Apr 12, 2021 at 6:50 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Thu, Apr 01, 2021 at 04:36:02AM +0000, jiang.wang wrote:
> > > +For the rx side, dgram also uses the \field{buf_alloc}. If it is full, the packet
> > > +is dropped by the receiver.
> >
> > UDP is connectionless so any number of other sources can send messages
> > to the same destination, causing buf_alloc's value to be unpredictable.
> > Can you explain how buf_alloc works with datagram sockets in more
> > detail?
> 
> In the linux kernel in my prototype, datagram sockets also use
> virtio_transport_inc_rx_pkt() and virtio_transport_dec_rx_pkt() to update
> vvs->rx_bytes and compare it with vvs->buf_alloc. virtio_transport_inc_rx_pkt
> is called when enqueuing the datagram packets.
> virtio_transport_dec_rx_pkt is called when dequeuing those packets.
> 
> If multiple sources send messages to the same destination, they will share
> the same buf_alloc. Do you want something with more control?
> Maybe somehow allocate a buffer for each remote CID and port? But I
> feel that is a little bit overkill. Any other suggestions?

The opposite, I want less control :). It's not possible to track buffer
space accurately with connectionless sockets, so let's not try.

A real example is the iperf3 networking benchmark where you need to set
target bitrate for UDP sockets because there is no flow control (buffer
space) mechanism in UDP. That's how UDP applications work and for
similar reasons I don't think it's possible to achieve flow control with
vsock's buf_alloc.

> > >  \drivernormative{\paragraph}{Device Operation: Buffer Space Management}{Device Types / Socket Device / Device Operation / Buffer Space Management}
> > > -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > > -sufficient free buffer space for the payload.
> > > +For stream sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > > +sufficient free buffer space for the payload. For dgram sockets, VIRTIO_VSOCK_OP_RW data packets
> > > +MAY be transmitted when the peer buffer is full. Then the packet will be dropped by the receiver.
> > >
> > >  All packets associated with a stream flow MUST contain valid information in
> > >  \field{buf_alloc} and \field{fwd_cnt} fields.
> > >
> > >  \devicenormative{\paragraph}{Device Operation: Buffer Space Management}{Device Types / Socket Device / Device Operation / Buffer Space Management}
> > > -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > > -sufficient free buffer space for the payload.
> > > +For stream sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > > +sufficient free buffer space for the payload. For dgram sockets, VIRTIO_VSOCK_OP_RW data packets
> > > +MAY be transmitted when the peer buffer is full. Then the packet will be dropped by the receiver.
> > >
> > >  All packets associated with a stream flow MUST contain valid information in
> > >  \field{buf_alloc} and \field{fwd_cnt} fields.
> > > @@ -203,14 +234,14 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De
> > >  The \field{guest_cid} configuration field MUST be used as the source CID when
> > >  sending outgoing packets.
> > >
> > > -A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
> > > +For stream sockets, A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
> > >  unknown \field{type} value.
> >
> > What about datagram sockets? Please state what must happen and why.
> 
> I think datagram sockets should do the same thing as the stream to
> return a VIRTIO_VSOCK_OP_RST?
> Any other ideas?

Sounds good to me. I just wanted to check and request that you add that
to the spec.