qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Yuanhan Liu
Subject: Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
Date: Fri, 13 Nov 2015 10:03:29 +0800
User-agent: Mutt/1.5.21 (2010-09-15)

On Thu, Nov 12, 2015 at 04:44:19PM +0200, Michael S. Tsirkin wrote:
> 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?

Yes, it's been there long time ago, and you can find them in recent
releases (say, v2.1).

However, QEMU just added RESET_OWNER since v2.4.0, and it's likely that
we will remove it since this version, v2.5.0.

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

Yeah, agree with you. Sending reset meessage at stop doesn't make too
much sense.

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

Okay to me. I will drop them then.

        --yliu



reply via email to

[Prev in Thread] Current Thread [Next in Thread]