Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
On Fri, Nov 13, 2015 at 12:24:52PM +0200, Michael S. Tsirkin wrote: > On Fri, Nov 13, 2015 at 10:03:29AM +0800, Yuanhan Liu wrote: > > 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. > > > I see. So I think that if MQ is negotiated, we should not send it. > VRING_ENABLE(0) is enough. But what if MQ is not negotiated? > In any case, it must be done after all rings are stopped in vhost_dev_stop. > Otherwise we can not retrieve the ring index so we can't resume. > > I'm not sure it's worth the hassle. > To work correctly, when MQ is not negotiated then clients > need to stop ring on GET_VRING_BASE, and that seems enough > to make everything work correctly across reboot. > If MQ is negotiated then there's VRING_ENABLE so hacks > are not needed anymore. I'm okay with that. And if so, we may need put those kind of information to vhost-user spec, to make it a standard. Or, I'm wondering could we make it be consistent: sending GET_VRING_BASE as the only sign of vhost dev stop, no matter MQ is negotiated or not? And I'm re-think what's the necessary of sending VRING_ENABLE at start/stop? Just because it's a better name than GET_VRING_BASE? With current code, VRING_ENABLE will be sent in following several places: - virtio_net_set_queues, which will be invoked in the virtio net initiation stage. And it also will be invoked after user executing "ethtool -L" command. - vhost_dev_start - vhost_dev_stop We might need take different actions for each above cases, say we need release some resources (such as closing the kick fd) on vhost_dev_stop. But we definitely could not do that for VRING_ENABLE messages from ethtool as user can re-run the ethtool command to bring those vrings back to functional: doing those kind of resource release would not be able to bring it back. So, it might be difficult (or more accurate, messy) for the client to figure out which case it is: at which time we should release some resources, and at which time we should not. --yliu > > Cc Luke for an opinion on whether it's too late for > snabbswitch to adopt this approach. > > > > > > > > > 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 > > > > > >
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
On Fri, Nov 13, 2015 at 10:03:29AM +0800, Yuanhan Liu wrote: > 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. I see. So I think that if MQ is negotiated, we should not send it. VRING_ENABLE(0) is enough. But what if MQ is not negotiated? In any case, it must be done after all rings are stopped in vhost_dev_stop. Otherwise we can not retrieve the ring index so we can't resume. I'm not sure it's worth the hassle. To work correctly, when MQ is not negotiated then clients need to stop ring on GET_VRING_BASE, and that seems enough to make everything work correctly across reboot. If MQ is negotiated then there's VRING_ENABLE so hacks are not needed anymore. Cc Luke for an opinion on whether it's too late for snabbswitch to adopt this approach. > > > > > 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
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
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. > 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? > 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 applied patches 1 and 5 for now. Thanks! > --- > Yuanhan Liu (5): > vhost: rename RESET_DEVICE backto RESET_OWNER > vhost: reset vhost net when virtio_net_reset happens > vhost: introduce vhost_set/get_protocol_features callbacks > vhost: send SET_PROTOCOL_FEATURES at start > vhost: send SET_VRING_ENABLE at start/stop > > docs/specs/vhost-user.txt | 4 ++-- > hw/net/vhost_net.c| 20 ++-- > hw/net/virtio-net.c | 14 ++ > hw/virtio/vhost-backend.c | 2 +- > hw/virtio/vhost-user.c| 13 ++--- > hw/virtio/vhost.c | 17 + > include/hw/virtio/vhost-backend.h | 6 ++ > include/net/vhost_net.h | 1 + > linux-headers/linux/vhost.h | 2 +- > tests/vhost-user-bridge.c | 6 +++--- > tests/vhost-user-test.c | 4 ++-- > 11 files changed, 71 insertions(+), 18 deletions(-) > > -- > 1.9.0
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
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
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
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.
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
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
