Re: [dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support

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

On Tue, Oct 27, 2015 at 05:30:41PM +0800, Yuanhan Liu wrote:
> On Tue, Oct 27, 2015 at 11:17:16AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Oct 27, 2015 at 03:20:40PM +0900, Tetsuya Mukawa wrote:
> > > On 2015/10/26 14:42, Yuanhan Liu wrote:
> > > > On Mon, Oct 26, 2015 at 02:24:08PM +0900, Tetsuya Mukawa wrote:
> > > >> On 2015/10/22 21:35, Yuanhan Liu wrote:
> > > > ...
> > > >>> @@ -292,13 +300,13 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> > > >>>  	 * sent and only sent in vhost_vring_stop.
> > > >>>  	 * TODO: cleanup the vring, it isn't usable since here.
> > > >>>  	 */
> > > >>> -	if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> > > >>> -		close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> > > >>> -		dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> > > >>> +	if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > > >>> +		close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);
> > > >>> +		dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1;
> > > >>>  	}
> > > >> Hi Yuanhan,
> > > >>
> > > >> Please let me make sure whether below is correct.
> > > >>     if ((dev->virtqueue[state->index]->kickfd + VIRTIO_RXQ) >= 0) {
> > > >>
> > > >>> -	if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> > > >>> -		close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> > > >>> -		dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> > > >>> +	if ((dev->virtqueue[state->index]->kickfd + VIRTIO_TXQ) >= 0) {
> > > >>> +		close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);
> > > >>> +		dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1;
> > > >> Also, same question here.
> > > > Oops, silly typos... Thanks for catching it out!
> > > >
> > > > Here is an update patch (Thomas, please let me know if you prefer me
> > > > to send the whole patchset for you to apply):
> > > 
> > > Hi Yuanhan,
> > > 
> > > I may miss one more issue here.
> > > Could you please see below patch I've submitted today?
> > > (I may find a similar issue, so I've fixed it also in below patch.)
> > >  
> > > - http://dpdk.org/dev/patchwork/patch/8038/
> > >  
> > > Thanks,
> > > Tetsuya
> > 
> > Looking at that, at least when MQ is enabled, please don't key
> > stopping queues off GET_VRING_BASE.
> 
> Yes, that's only a workaround. I guess it has been there for quite a
> while, maybe at the time qemu doesn't send RESET_OWNER message.

RESET_OWNER was a bad idea since it basically closes
everything.

> > There are ENABLE/DISABLE messages for that.
> 
> That's something new,

That's part of multiqueue support. If you ignore them,
nothing works properly.

> though I have plan to use them instead, we still
> need to make sure our code work with old qemu, without ENABLE/DISABLE
> messages.

OK but don't rely on this for new code.

> And I will think more while enabling live migration: I should have
> more time to address issues like this at that time.
> 
> > Generally guys, don't take whatever QEMU happens to do for
> > granted! Look at the protocol spec under doc/specs directory,
> > if you are making more assumptions you must document them!
> 
> Indeed. And we will try to address them bit by bit in future.
> 
> 	--yliu

But don't pile up these workarounds meanwhile.  I'm very worried.  The
way you are carrying on, each new QEMU is likely to break your
assumptions.

-- 
MST