Re: [virtio-dev] packed ring layout proposal

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Sep 17, 2016 at 08:54:06AM +0200, Maxime Coquelin wrote:
> 
> 
> On 09/16/2016 10:03 PM, Michael S. Tsirkin wrote:
> > On Fri, Sep 16, 2016 at 12:11:59PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 16/09/2016 11:46, Stefan Hajnoczi wrote:
> > > > > > The idea is to have a r/w descriptor in a ring structure,
> > > > > > replacing the used and available ring, index and descriptor
> > > > > > buffer.
> > > > This is probably obvious but:
> > > > 
> > > > Why is the desc.index field needed?
> > > > 
> > > > The index field would be needed if the driver had to jump over a DESC_HW
> > > > descriptor when adding descriptors, but that doesn't seem necessary
> > > > since the device is supposed to overwrite descriptors in ascending ring
> > > > order when they complete out-of-order.
> > > 
> > > AIUI, when reading completed descriptors, the driver uses the index in
> > > order to map the descriptor to the original request.
> > > 
> > > It's more of an id; it's certainly not a linked list as in virtio 1.0 rings.
> > > 
> > > I think we can refine the description of the index, saying that only the
> > > first descriptor in a request needs an index; descriptors reached via
> > > VRING_DESC_F_NEXT do not need an index.  The advantage is that the
> > > device only needs to remember the descriptor of the first descriptor.
> > > 
> > > However...
> > > 
> > > - is there any reason for the device or driver to even look at DESC_HW
> > > if VRING_DESC_F_NEXT is set?  DESC_HW only matters on the first buffer.
> > > 
> > > - why not drop VRING_DESC_F_NEXT altogether?
> > > 
> > > Because only two cases are strictly necessary:
> > > 
> > > - single-buffer requests.  These have some very important cases:
> > > virtio-rng, virtio-serial, virtio-net with merged rxbufs, virtio-scsi
> > > and virtio-vsock events, virtio-balloon, virtio-input all use
> > > single-buffer requests exclusively.
> > > 
> > > - indirect requests, where the length of the indirect descriptor can be
> > > used to figure out the number of buffers
> > > 
> > > Let's just drop the added complication of multi-buffer direct requests.
> > > Yes, this can cause one extra cache miss but 1) as shown above a lot of
> > > performance-critical cases use single-buffer requests 2) no one has ever
> > > bothered to use multi-buffer direct requests in Linux drivers, at least
> > > when indirect descriptors are available.
> > 
> > True. I note that dpdk used direct descriptors exclusively until
> > very recently.
> >     commit 6dc5de3a6aefba3946fe04368d93994db3f7a5fd
> >     Author: Stephen Hemminger <stephen@networkplumber.org>
> >     Date:   Fri Mar 4 10:19:19 2016 -0800
> > 
> >         virtio: use indirect ring elements
> 
> Indeed, and I'd like to add that this patch doesn't enable
> VIRTIO_RING_F_INDIRECT_DESC, so code was here but not used in mainline.
> 
> It has been fixed few weeks ago, but the patch didn't land yet into
> master, only in virtio-next[0]:
> 
> commit b19d3763de3f6c96380b6decf51752acebd2acee
> Author: Pierre Pfister <ppfister@cisco.com>
> Date:   Wed Sep 7 10:46:18 2016 +0800
> 
>     net/virtio: enable indirect descriptors feature
> 
> 
> > I'd like to make sure removing these doesn't cause regressions.
> > Stephen, could you please confirm that
> > 1. with your patch, each packet takes at most 1 slot
> > 2. your patch was a win for most workloads
> >    (as opposed to e.g. enabling/disabling this depending on workload)
> 
> > 
> > Also, it does not look like vhost in dpdk supports
> > indirect descriptors.
> 
> I have done the patch for vhost in dpdk[1] two months ago.
> It's not merged because I didn't show noticeable gain using it. Note
> that I didn't show noticeable loss either.
> I'll try to re-run some benchmarks next week.
> 
> Maxime

You will likely see a gain if you run linux with a ton of tcp sockets
within guest.


> [0]: git://dpdk.org/next/dpdk-next-virtio
> [1]: http://dpdk.org/dev/patchwork/patch/14797/