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.