[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] packed ring layout proposal
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 <firstname.lastname@example.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: commit b19d3763de3f6c96380b6decf51752acebd2acee Author: Pierre Pfister <email@example.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 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 : git://dpdk.org/next/dpdk-next-virtio : http://dpdk.org/dev/patchwork/patch/14797/