Re: [PATCH v8 7/8] vhost-user: add vhost-user

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 14 Nov 2024 16:16:48 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 14/11/2024 15:23, Stefano Brivio wrote:
> > On Thu, 14 Nov 2024 11:20:09 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 17/10/2024 02:10, Stefano Brivio wrote:  
> >>>> +		if (frame_size == 0)
> >>>> +			first = &iov_vu[i + 1];
> >>>> +
> >>>> +		if (iov_vu[i + 1].iov_len > (size_t)len)
> >>>> +			iov_vu[i + 1].iov_len = len;
> >>>> +
> >>>> +		len -= iov_vu[i + 1].iov_len;
> >>>> +		iov_used++;
> >>>> +
> >>>> +		frame_size += iov_vu[i + 1].iov_len;
> >>>> +		num_buffers++;
> >>>> +
> >>>> +		if (frame_size >= mss || len == 0 ||
> >>>> +		    i + 1 == iov_cnt || !vu_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) {
> >>>> +			if (i + 1 == iov_cnt)
> >>>> +				check = NULL;
> >>>> +
> >>>> +			/* restore first iovec base: point to vnet header */
> >>>> +			vu_set_vnethdr(vdev, first, num_buffers, l2_hdrlen);
> >>>> +
> >>>> +			tcp_vu_prepare(c, conn, first, frame_size, &check);
> >>>> +			if (*c->pcap)  {
> >>>> +				tcp_vu_update_check(tapside, first, num_buffers);
> >>>> +				pcap_iov(first, num_buffers,
> >>>> +					 sizeof(struct virtio_net_hdr_mrg_rxbuf));
> >>>> +			}
> >>>> +
> >>>> +			conn->seq_to_tap += frame_size;  
> >>> We always increase this, even if, later...
> >>>      
> >>>> +
> >>>> +			frame_size = 0;
> >>>> +			num_buffers = 0;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	/* release unused buffers */
> >>>> +	vu_queue_rewind(vq, iov_cnt - iov_used);
> >>>> +
> >>>> +	/* send packets */
> >>>> +	vu_flush(vdev, vq, elem, iov_used);  
> >>> we fail to send packets, that is, even if vu_queue_fill_by_index()
> >>> returns early because (!vq->vring.avail).  
> >>
> >> vring.avail is a pointer to a structure. vring.avail is NULL if there is something wrong
> >> during the initialization. It's imported code, I think it's only a sanity check.
> >> So in theory vu_flush() cannot fail.  
> > 
> > Oh, I see now. I actually think it's preferable to crash in that
> > (theoretically impossible) case, without even an ASSERT() (we would
> > dereference a NULL pointer, eventually, even if not here).
> 
> So what you propose is to remove the "if (!vq->vring.avail) return;"?

Yes, all of them, actually. It also avoids confusion I think.

-- 
Stefano