Linux Virtualization - Oct 2018 - [virtio-dev] Re: [PATCH net-next v2 0/5] virtio: support packed ring

On Thu, Sep 13, 2018 at 05:47:29PM +0800, Jason Wang
wrote:
> 
> 
> On 2018?09?13? 16:59, Tiwei Bie wrote:
> > > If what you say is true then we should take a careful look
> > > and not supporting these generic things with packed layout.
> > > Once we do support them it will be too late and we won't
> > > be able to get performance back.
> > I think it's a good point that we don't need to support
> > everything in packed ring (especially these which would
> > hurt the performance), as the packed ring aims at high
> > performance. I'm also wondering about the features. Is
> > there any possibility that we won't support the out of
> > order processing (at least not by default) in packed ring?
> > If I didn't miss anything, the need to support out of order
> > processing in packed ring will make the data structure
> > inside the driver not cache friendly which is similar to
> > the case of the descriptor table in the split ring (the
> > difference is that, it only happens in driver now).
> 
> Out of order is not the only user, DMA is another one. We don't have
used
> ring(len), so we need to maintain buffer length somewhere even for in order
> device.
For a bunch of systems dma unmap is a nop so we do not really
need to maintain it. It's a question of an API to detect that
and optimize for it. I posted a proposed patch for that -
want to try using that?
> But if it's not too late, I second for a OUT_OF_ORDER feature.
> Starting from in order can have much simpler code in driver.
> 
> Thanks
It's tricky to change the flag polarity because of compatibility
with legacy interfaces. Why is this such a big deal?

Let's teach drivers about IN_ORDER, then if devices
are in order it will get enabled by default.

-- 
MST

On Wed, Oct 10, 2018 at 10:36:26AM -0400, Michael S. Tsirkin
wrote:
> On Thu, Sep 13, 2018 at 05:47:29PM +0800, Jason Wang wrote:
> > On 2018?09?13? 16:59, Tiwei Bie wrote:
> > > > If what you say is true then we should take a careful look
> > > > and not supporting these generic things with packed layout.
> > > > Once we do support them it will be too late and we won't
> > > > be able to get performance back.
> > > I think it's a good point that we don't need to support
> > > everything in packed ring (especially these which would
> > > hurt the performance), as the packed ring aims at high
> > > performance. I'm also wondering about the features. Is
> > > there any possibility that we won't support the out of
> > > order processing (at least not by default) in packed ring?
> > > If I didn't miss anything, the need to support out of order
> > > processing in packed ring will make the data structure
> > > inside the driver not cache friendly which is similar to
> > > the case of the descriptor table in the split ring (the
> > > difference is that, it only happens in driver now).
> > 
> > Out of order is not the only user, DMA is another one. We don't
have used
> > ring(len), so we need to maintain buffer length somewhere even for in
order
> > device.
> 
> For a bunch of systems dma unmap is a nop so we do not really
> need to maintain it. It's a question of an API to detect that
> and optimize for it. I posted a proposed patch for that -
> want to try using that?
Yeah, definitely!
> 
> > But if it's not too late, I second for a OUT_OF_ORDER feature.
> > Starting from in order can have much simpler code in driver.
> > 
> > Thanks
> 
> It's tricky to change the flag polarity because of compatibility
> with legacy interfaces. Why is this such a big deal?
> 
> Let's teach drivers about IN_ORDER, then if devices
> are in order it will get enabled by default.
Yeah, make sense.

Besides, I have done some further profiling and debugging
both in kernel driver and DPDK vhost. Previously I was mislead
by a bug in vhost code. I will send a patch to fix that bug.
With that bug fixed, the performance of packed ring in the
test between kernel driver and DPDK vhost is better now.
I will send a new series soon. Thanks!
> 
> -- 
> MST

On Thu, Oct 11, 2018 at 08:12:21PM +0800, Tiwei Bie
wrote:
> > > But if it's not too late, I second for a OUT_OF_ORDER
feature.
> > > Starting from in order can have much simpler code in driver.
> > > 
> > > Thanks
> > 
> > It's tricky to change the flag polarity because of compatibility
> > with legacy interfaces. Why is this such a big deal?
> > 
> > Let's teach drivers about IN_ORDER, then if devices
> > are in order it will get enabled by default.
> 
> Yeah, make sense.
> 
> Besides, I have done some further profiling and debugging
> both in kernel driver and DPDK vhost. Previously I was mislead
> by a bug in vhost code. I will send a patch to fix that bug.
> With that bug fixed, the performance of packed ring in the
> test between kernel driver and DPDK vhost is better now.
OK, if we get a performance gain on the virtio side, we can finally
upstream it. If you see that please re-post ASAP so we can
put it in the next kernel release.

-- 
MST