Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Yuanhan Liu]

On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> > 
> > Patch 1 rename RESET_DEVICE back to RESET_OWNER
> > 
> > Patch 2 introduced a new function: vhost_net_reset(), which is invoked
> >         when reset happens, say, driver is unloaded (unbinded).
> > 
> >         Michael suggested to do that only when MQ is not negotiated.
> >         However, reset happens no matter MQ is enabled or negotiated
> >         or not, and we should give a sign to backend to reset some
> >         device to a proper state after it happens.
> 
> I don't think it's needed: we set all state at start anyway.

Agree with that.

> 
> >         Note that the message sent is still RESET_OWNER. It might not
> >         be a good idea, but we could not simply rename it to RESET_DEVICE,
> >         and maybe adding another RESET_DEVICE might be better.
> 
> So modern clients don't need this at all.  Old clients need something to
> stop device, but singling out reset is not a good idea: even if driver
> is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
> this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
> it stop all queues.
> 
> Does any dpdk version that was released respond to RESET_OWNER in some
> way? How exactly?

It just resets some states, such as closing call fd and kick fd,
unmapping buf from hugetlbfs from set_mem_table.

And, apparently we could do that on stop, too. So, from this pov, we
don't need RESET_OWNER.

> 
> > Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
> >         SET_FEATURES.
> > 
> > Patch 5 send SET_VRING_ENABLE at start/stop
> > 
> >         Michael, I intended to send it when MQ is negotiated as you 
> > suggested,
> >         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in 
> > vhost-user.c,
> >         which is not accessible to vhost.c.
> > 
> >         Exporting it to vhost.h will resolve that, however, it's not a good
> >         idea to move vhost user specific stuff to there. We could also 
> > introduce
> >         another vhost callback to test whether MQ is negotiated: I just 
> > don't
> >         think it's worthy.
> > 
> >         Hence, here I just used a simple test: invoke set_vring_enable() 
> > just
> >         when it is defined. Judging the callback self has another MQ check,
> >         I guess it's okay.
> > 
> > And sorry that it took so long to send this version.
> 
> Hmm, you are saying everyone needs SET_VRING_ENABLE?
> Maybe we should make SET_VRING_ENABLE depend on protocol features then,
> and not MQ?

I'm thinking something same. Otherwise, there is still no way to inform
the backend (or client) when a vhost dev is stopped when MQ is disabled
(which is the default state).

So, let's assume all clients have protocol features enabled, and send
SET_VRING_ENABLE at start/stop? And if it does not, it's just like we
are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE
messages are sent on stop: it worked before, and it should also work
now.

> 
> I applied patches 1 and 5 for now.

Do you have comment about patch 3 and 4? Should we set protocol features
just like we set features at start?

Thanks.

        --yliu