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

[Michael S. Tsirkin]

On Thu, Nov 12, 2015 at 10:08:15PM +0800, Yuanhan Liu wrote:
> 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 is this in some dpdk version that has been released?

> 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.

So maybe we should drop RESET_OWNER from stop then?

> > 
> > 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

We don't set the at start - we set them on connect.