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