| Message ID | 1561628616-10218-1-git-send-email-david.marchand@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v4] netdev-dpdk: Reset queue number for vhost devices on vm shutdown. | expand |
On 6/27/2019 10:43 AM, David Marchand wrote: > Rather than poll all disabled queues and waste some memory for vms that > have been shutdown, we can reconfigure when receiving a destroy > connection notification from the vhost library. > > $ while true; do > ovs-appctl dpif-netdev/pmd-rxq-show |awk ' > /port: / { > tot++; > if ($5 == "(enabled)") { > en++; > } > } > END { > print "total: " tot ", enabled: " en > }' > sleep 1 > done > > total: 66, enabled: 66 > total: 6, enabled: 2 > > This change requires a fix in the DPDK vhost library, so bump the minimal > required version to 18.11.2. So in testing this I was seeing an error with the following 2019-06-27T10:00:08Z|00057|dpdk|INFO|VHOST_CONFIG: vhost peer closed 2019-06-27T10:00:08Z|00058|dpdk|ERR|VHOST_CONFIG: (0) device not found. 2019-06-27T10:00:08Z|00059|netdev_dpdk|INFO|vHost Device '' not found It wasn't until I realized I was testing with DPDK 18.11.2.rc1 that it dawned on me the required fix wasn't there...doh! So in short I can confirm the requirement for 18.11.2 :). Is there any requirement on the QEMU version side for this patch also? Or has the functionality been in place on that side for quite some time but was just mishandled in DPDK? One other minor query below but overall looks ok and has validated without issue. > > Co-authored-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Changes since v3: > - rebased on master following 18.11.2 support > - added a note in NEWS about minimal dpdk version > > Changes since v2: > - added authorship tags for Ilya > - added logs in destroy_connection > > --- > NEWS | 4 +++- > lib/netdev-dpdk.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/NEWS b/NEWS > index c03e287..2f8171f 100644 > --- a/NEWS > +++ b/NEWS > @@ -4,7 +4,9 @@ Post-v2.11.0 > * New option 'other_config:dpdk-socket-limit' to limit amount of > hugepage memory that can be used by DPDK. > * Add support for vHost Post-copy Live Migration (experimental). > - * OVS validated with DPDK 18.11.2 which is recommended to be used. > + * OVS validated with DPDK 18.11.2 which is the new minimal supported > + version. > + * DPDK 18.11.1 and lower is no longer supported. > - OpenFlow: > * All features required by OpenFlow 1.5 are now implemented, so > ovs-vswitchd now enables OpenFlow 1.5 by default (in addition to > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index b7f4543..0b9bea4 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -185,12 +185,15 @@ static const struct rte_eth_conf port_conf = { > static int new_device(int vid); > static void destroy_device(int vid); > static int vring_state_changed(int vid, uint16_t queue_id, int enable); > +static void destroy_connection(int vid); > static const struct vhost_device_ops virtio_net_device_ops = > { > .new_device = new_device, > .destroy_device = destroy_device, > .vring_state_changed = vring_state_changed, > - .features_changed = NULL > + .features_changed = NULL, > + .new_connection = NULL, I take it .new_connection is a requirement? It's just that it's added as NULL, but I assume it goes hand in hand with the introduction of .destroy connection? Ian > + .destroy_connection = destroy_connection, > }; > > enum { DPDK_RING_SIZE = 256 }; > @@ -3663,6 +3666,48 @@ vring_state_changed(int vid, uint16_t queue_id, int enable) > return 0; > } > > +static void > +destroy_connection(int vid) > +{ > + struct netdev_dpdk *dev; > + char ifname[IF_NAME_SZ]; > + bool exists = false; > + > + rte_vhost_get_ifname(vid, ifname, sizeof ifname); > + > + ovs_mutex_lock(&dpdk_mutex); > + LIST_FOR_EACH (dev, list_node, &dpdk_list) { > + ovs_mutex_lock(&dev->mutex); > + if (nullable_string_is_equal(ifname, dev->vhost_id)) { > + uint32_t qp_num = NR_QUEUE; > + > + if (netdev_dpdk_get_vid(dev) >= 0) { > + VLOG_ERR("Connection on socket '%s' destroyed while vhost " > + "device still attached.", dev->vhost_id); > + } > + > + /* Restore the number of queue pairs to default. */ > + if (dev->requested_n_txq != qp_num > + || dev->requested_n_rxq != qp_num) { > + dev->requested_n_rxq = qp_num; > + dev->requested_n_txq = qp_num; > + netdev_request_reconfigure(&dev->up); > + } > + ovs_mutex_unlock(&dev->mutex); > + exists = true; > + break; > + } > + ovs_mutex_unlock(&dev->mutex); > + } > + ovs_mutex_unlock(&dpdk_mutex); > + > + if (exists) { > + VLOG_INFO("vHost Device '%s' connection has been destroyed", ifname); > + } else { > + VLOG_INFO("vHost Device '%s' not found", ifname); > + } > +} > + > /* > * Retrieve the DPDK virtio device ID (vid) associated with a vhostuser > * or vhostuserclient netdev. >
On 27/06/2019 15:17, Ian Stokes wrote: > On 6/27/2019 10:43 AM, David Marchand wrote: >> Rather than poll all disabled queues and waste some memory for vms that >> have been shutdown, we can reconfigure when receiving a destroy >> connection notification from the vhost library. >> >> $ while true; do >> ovs-appctl dpif-netdev/pmd-rxq-show |awk ' >> /port: / { >> tot++; >> if ($5 == "(enabled)") { >> en++; >> } >> } >> END { >> print "total: " tot ", enabled: " en >> }' >> sleep 1 >> done >> >> total: 66, enabled: 66 >> total: 6, enabled: 2 >> >> This change requires a fix in the DPDK vhost library, so bump the minimal >> required version to 18.11.2. > > So in testing this I was seeing an error with the following > > 2019-06-27T10:00:08Z|00057|dpdk|INFO|VHOST_CONFIG: vhost peer closed > 2019-06-27T10:00:08Z|00058|dpdk|ERR|VHOST_CONFIG: (0) device not found. > 2019-06-27T10:00:08Z|00059|netdev_dpdk|INFO|vHost Device '' not found > > It wasn't until I realized I was testing with DPDK 18.11.2.rc1 that it > dawned on me the required fix wasn't there...doh! > Is that version a typo? There is only a couple of unrelated patches between between 18.11.2-rc1 and 18.11.2 version $ git log --oneline v18.11.2-rc1..v18.11.2 06c4b12a5 version: 18.11.2 4d1815fe6 Revert "app/testpmd: fix offload flags after port config" 832671a52 net/i40e: forbid two RSS flow rules > So in short I can confirm the requirement for 18.11.2 :). > > Is there any requirement on the QEMU version side for this patch also? > Or has the functionality been in place on that side for quite some time > but was just mishandled in DPDK? > > One other minor query below but overall looks ok and has validated > without issue. > >> >> Co-authored-by: Ilya Maximets <i.maximets@samsung.com> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> Signed-off-by: David Marchand <david.marchand@redhat.com> >> --- >> Changes since v3: >> - rebased on master following 18.11.2 support >> - added a note in NEWS about minimal dpdk version >> >> Changes since v2: >> - added authorship tags for Ilya >> - added logs in destroy_connection >> >> --- >> NEWS | 4 +++- >> lib/netdev-dpdk.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 49 insertions(+), 2 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index c03e287..2f8171f 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -4,7 +4,9 @@ Post-v2.11.0 >> * New option 'other_config:dpdk-socket-limit' to limit amount of >> hugepage memory that can be used by DPDK. >> * Add support for vHost Post-copy Live Migration (experimental). >> - * OVS validated with DPDK 18.11.2 which is recommended to be used. >> + * OVS validated with DPDK 18.11.2 which is the new minimal supported >> + version. >> + * DPDK 18.11.1 and lower is no longer supported. >> - OpenFlow: >> * All features required by OpenFlow 1.5 are now implemented, so >> ovs-vswitchd now enables OpenFlow 1.5 by default (in addition to >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index b7f4543..0b9bea4 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -185,12 +185,15 @@ static const struct rte_eth_conf port_conf = { >> static int new_device(int vid); >> static void destroy_device(int vid); >> static int vring_state_changed(int vid, uint16_t queue_id, int enable); >> +static void destroy_connection(int vid); >> static const struct vhost_device_ops virtio_net_device_ops = >> { >> .new_device = new_device, >> .destroy_device = destroy_device, >> .vring_state_changed = vring_state_changed, >> - .features_changed = NULL >> + .features_changed = NULL, >> + .new_connection = NULL, > > I take it .new_connection is a requirement? It's just that it's added as > NULL, but I assume it goes hand in hand with the introduction of > .destroy connection? > > Ian >> + .destroy_connection = destroy_connection, >> }; >> >> enum { DPDK_RING_SIZE = 256 }; >> @@ -3663,6 +3666,48 @@ vring_state_changed(int vid, uint16_t queue_id, int enable) >> return 0; >> } >> >> +static void >> +destroy_connection(int vid) >> +{ >> + struct netdev_dpdk *dev; >> + char ifname[IF_NAME_SZ]; >> + bool exists = false; >> + >> + rte_vhost_get_ifname(vid, ifname, sizeof ifname); >> + >> + ovs_mutex_lock(&dpdk_mutex); >> + LIST_FOR_EACH (dev, list_node, &dpdk_list) { >> + ovs_mutex_lock(&dev->mutex); >> + if (nullable_string_is_equal(ifname, dev->vhost_id)) { >> + uint32_t qp_num = NR_QUEUE; >> + >> + if (netdev_dpdk_get_vid(dev) >= 0) { >> + VLOG_ERR("Connection on socket '%s' destroyed while vhost " >> + "device still attached.", dev->vhost_id); >> + } >> + >> + /* Restore the number of queue pairs to default. */ >> + if (dev->requested_n_txq != qp_num >> + || dev->requested_n_rxq != qp_num) { >> + dev->requested_n_rxq = qp_num; >> + dev->requested_n_txq = qp_num; >> + netdev_request_reconfigure(&dev->up); >> + } >> + ovs_mutex_unlock(&dev->mutex); >> + exists = true; >> + break; >> + } >> + ovs_mutex_unlock(&dev->mutex); >> + } >> + ovs_mutex_unlock(&dpdk_mutex); >> + >> + if (exists) { >> + VLOG_INFO("vHost Device '%s' connection has been destroyed", ifname); >> + } else { >> + VLOG_INFO("vHost Device '%s' not found", ifname); >> + } >> +} >> + >> /* >> * Retrieve the DPDK virtio device ID (vid) associated with a vhostuser >> * or vhostuserclient netdev. >> >
> On 27/06/2019 15:17, Ian Stokes wrote: > > On 6/27/2019 10:43 AM, David Marchand wrote: > >> Rather than poll all disabled queues and waste some memory for vms that > >> have been shutdown, we can reconfigure when receiving a destroy > >> connection notification from the vhost library. > >> > >> $ while true; do > >> ovs-appctl dpif-netdev/pmd-rxq-show |awk ' > >> /port: / { > >> tot++; > >> if ($5 == "(enabled)") { > >> en++; > >> } > >> } > >> END { > >> print "total: " tot ", enabled: " en > >> }' > >> sleep 1 > >> done > >> > >> total: 66, enabled: 66 > >> total: 6, enabled: 2 > >> > >> This change requires a fix in the DPDK vhost library, so bump the > minimal > >> required version to 18.11.2. > > > > So in testing this I was seeing an error with the following > > > > 2019-06-27T10:00:08Z|00057|dpdk|INFO|VHOST_CONFIG: vhost peer closed > > 2019-06-27T10:00:08Z|00058|dpdk|ERR|VHOST_CONFIG: (0) device not found. > > 2019-06-27T10:00:08Z|00059|netdev_dpdk|INFO|vHost Device '' not found > > > > It wasn't until I realized I was testing with DPDK 18.11.2.rc1 that it > > dawned on me the required fix wasn't there...doh! > > > > Is that version a typo? There is only a couple of unrelated patches > between between 18.11.2-rc1 and 18.11.2 version You may be right, just looking at the package I had on the board was dpdk-stable-18.11.2-rc1, but it could have been the case I was pointing to 18.11.1 if the patch was part of 18.11.2.rc1. I've been chopping and changing so much this past week it was more likely a mistake on my part. Ian
On 27.06.2019 17:17, Ian Stokes wrote: > On 6/27/2019 10:43 AM, David Marchand wrote: >> Rather than poll all disabled queues and waste some memory for vms that >> have been shutdown, we can reconfigure when receiving a destroy >> connection notification from the vhost library. >> >> $ while true; do >> ovs-appctl dpif-netdev/pmd-rxq-show |awk ' >> /port: / { >> tot++; >> if ($5 == "(enabled)") { >> en++; >> } >> } >> END { >> print "total: " tot ", enabled: " en >> }' >> sleep 1 >> done >> >> total: 66, enabled: 66 >> total: 6, enabled: 2 >> >> This change requires a fix in the DPDK vhost library, so bump the minimal >> required version to 18.11.2. > > So in testing this I was seeing an error with the following > > 2019-06-27T10:00:08Z|00057|dpdk|INFO|VHOST_CONFIG: vhost peer closed > 2019-06-27T10:00:08Z|00058|dpdk|ERR|VHOST_CONFIG: (0) device not found. > 2019-06-27T10:00:08Z|00059|netdev_dpdk|INFO|vHost Device '' not found > > It wasn't until I realized I was testing with DPDK 18.11.2.rc1 that it dawned on me the required fix wasn't there...doh! > > So in short I can confirm the requirement for 18.11.2 :). > > Is there any requirement on the QEMU version side for this patch also? Or has the functionality been in place on that side for quite some time but was just mishandled in DPDK? 'destroy_connection' is just a callback that is called when socket closed by DPDK or QEMU. So, no dependency on QEMU version. The reason of above issue with 18.11.1 is that dpdk destroyed device before calling the 'destroy_connection' callback, so OVS wasn't able to find corresponding port, because 'rte_vhost_get_ifname' always returned an empty string. > > One other minor query below but overall looks ok and has validated without issue. > >> >> Co-authored-by: Ilya Maximets <i.maximets@samsung.com> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> Signed-off-by: David Marchand <david.marchand@redhat.com> >> --- >> Changes since v3: >> - rebased on master following 18.11.2 support >> - added a note in NEWS about minimal dpdk version >> >> Changes since v2: >> - added authorship tags for Ilya >> - added logs in destroy_connection >> >> --- >> NEWS | 4 +++- >> lib/netdev-dpdk.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 49 insertions(+), 2 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index c03e287..2f8171f 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -4,7 +4,9 @@ Post-v2.11.0 >> * New option 'other_config:dpdk-socket-limit' to limit amount of >> hugepage memory that can be used by DPDK. >> * Add support for vHost Post-copy Live Migration (experimental). >> - * OVS validated with DPDK 18.11.2 which is recommended to be used. >> + * OVS validated with DPDK 18.11.2 which is the new minimal supported >> + version. >> + * DPDK 18.11.1 and lower is no longer supported. >> - OpenFlow: >> * All features required by OpenFlow 1.5 are now implemented, so >> ovs-vswitchd now enables OpenFlow 1.5 by default (in addition to >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index b7f4543..0b9bea4 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -185,12 +185,15 @@ static const struct rte_eth_conf port_conf = { >> static int new_device(int vid); >> static void destroy_device(int vid); >> static int vring_state_changed(int vid, uint16_t queue_id, int enable); >> +static void destroy_connection(int vid); >> static const struct vhost_device_ops virtio_net_device_ops = >> { >> .new_device = new_device, >> .destroy_device = destroy_device, >> .vring_state_changed = vring_state_changed, >> - .features_changed = NULL >> + .features_changed = NULL, >> + .new_connection = NULL, > > I take it .new_connection is a requirement? It's just that it's added as NULL, but I assume it goes hand in hand with the introduction of .destroy connection? 'new_connection' was introduced in DPDK along with 'destroy_connection'. Setting it as NULL here just to have a full list of callbacks as we already have NULL 'features_changed'. To be consistent here we need to add 'new_connection' or remove 'features_changed'. Anyway compiler will clear all the non-initialized fields. > > Ian >> + .destroy_connection = destroy_connection, >> }; >> enum { DPDK_RING_SIZE = 256 }; >> @@ -3663,6 +3666,48 @@ vring_state_changed(int vid, uint16_t queue_id, int enable) >> return 0; >> } >> +static void >> +destroy_connection(int vid) >> +{ >> + struct netdev_dpdk *dev; >> + char ifname[IF_NAME_SZ]; >> + bool exists = false; >> + >> + rte_vhost_get_ifname(vid, ifname, sizeof ifname); >> + >> + ovs_mutex_lock(&dpdk_mutex); >> + LIST_FOR_EACH (dev, list_node, &dpdk_list) { >> + ovs_mutex_lock(&dev->mutex); >> + if (nullable_string_is_equal(ifname, dev->vhost_id)) { >> + uint32_t qp_num = NR_QUEUE; >> + >> + if (netdev_dpdk_get_vid(dev) >= 0) { >> + VLOG_ERR("Connection on socket '%s' destroyed while vhost " >> + "device still attached.", dev->vhost_id); >> + } >> + >> + /* Restore the number of queue pairs to default. */ >> + if (dev->requested_n_txq != qp_num >> + || dev->requested_n_rxq != qp_num) { >> + dev->requested_n_rxq = qp_num; >> + dev->requested_n_txq = qp_num; >> + netdev_request_reconfigure(&dev->up); >> + } >> + ovs_mutex_unlock(&dev->mutex); >> + exists = true; >> + break; >> + } >> + ovs_mutex_unlock(&dev->mutex); >> + } >> + ovs_mutex_unlock(&dpdk_mutex); >> + >> + if (exists) { >> + VLOG_INFO("vHost Device '%s' connection has been destroyed", ifname); >> + } else { >> + VLOG_INFO("vHost Device '%s' not found", ifname); >> + } >> +} >> + >> /* >> * Retrieve the DPDK virtio device ID (vid) associated with a vhostuser >> * or vhostuserclient netdev. >> > > >
On Thu, Jun 27, 2019 at 4:18 PM Ian Stokes <ian.stokes@intel.com> wrote: > On 6/27/2019 10:43 AM, David Marchand wrote: > > Rather than poll all disabled queues and waste some memory for vms that > > have been shutdown, we can reconfigure when receiving a destroy > > connection notification from the vhost library. > > > > $ while true; do > > ovs-appctl dpif-netdev/pmd-rxq-show |awk ' > > /port: / { > > tot++; > > if ($5 == "(enabled)") { > > en++; > > } > > } > > END { > > print "total: " tot ", enabled: " en > > }' > > sleep 1 > > done > > > > total: 66, enabled: 66 > > total: 6, enabled: 2 > > > > This change requires a fix in the DPDK vhost library, so bump the minimal > > required version to 18.11.2. > > So in testing this I was seeing an error with the following > > 2019-06-27T10:00:08Z|00057|dpdk|INFO|VHOST_CONFIG: vhost peer closed > 2019-06-27T10:00:08Z|00058|dpdk|ERR|VHOST_CONFIG: (0) device not found. > 2019-06-27T10:00:08Z|00059|netdev_dpdk|INFO|vHost Device '' not found > > It wasn't until I realized I was testing with DPDK 18.11.2.rc1 that it > dawned on me the required fix wasn't there...doh! > > So in short I can confirm the requirement for 18.11.2 :). > Yep, the logs match what I would expect without the patch. > Is there any requirement on the QEMU version side for this patch also? > Or has the functionality been in place on that side for quite some time > but was just mishandled in DPDK? > The issue was at the interface between dpdk and ovs. I can't think of a requirement from QEMU side. > One other minor query below but overall looks ok and has validated > without issue. > > > > > Co-authored-by: Ilya Maximets <i.maximets@samsung.com> > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > Changes since v3: > > - rebased on master following 18.11.2 support > > - added a note in NEWS about minimal dpdk version > > > > Changes since v2: > > - added authorship tags for Ilya > > - added logs in destroy_connection > > > > --- > > NEWS | 4 +++- > > lib/netdev-dpdk.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 49 insertions(+), 2 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index c03e287..2f8171f 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -4,7 +4,9 @@ Post-v2.11.0 > > * New option 'other_config:dpdk-socket-limit' to limit amount of > > hugepage memory that can be used by DPDK. > > * Add support for vHost Post-copy Live Migration (experimental). > > - * OVS validated with DPDK 18.11.2 which is recommended to be used. > > + * OVS validated with DPDK 18.11.2 which is the new minimal > supported > > + version. > > + * DPDK 18.11.1 and lower is no longer supported. > > - OpenFlow: > > * All features required by OpenFlow 1.5 are now implemented, so > > ovs-vswitchd now enables OpenFlow 1.5 by default (in addition to > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index b7f4543..0b9bea4 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -185,12 +185,15 @@ static const struct rte_eth_conf port_conf = { > > static int new_device(int vid); > > static void destroy_device(int vid); > > static int vring_state_changed(int vid, uint16_t queue_id, int enable); > > +static void destroy_connection(int vid); > > static const struct vhost_device_ops virtio_net_device_ops = > > { > > .new_device = new_device, > > .destroy_device = destroy_device, > > .vring_state_changed = vring_state_changed, > > - .features_changed = NULL > > + .features_changed = NULL, > > + .new_connection = NULL, > > I take it .new_connection is a requirement? It's just that it's added as > NULL, but I assume it goes hand in hand with the introduction of > .destroy connection? > Not a requirement, it documents that we don't care about new connection notifications.
On 6/27/2019 3:51 PM, Ilya Maximets wrote: > On 27.06.2019 17:17, Ian Stokes wrote: >> On 6/27/2019 10:43 AM, David Marchand wrote: >>> Rather than poll all disabled queues and waste some memory for vms that >>> have been shutdown, we can reconfigure when receiving a destroy >>> connection notification from the vhost library. >>> >>> $ while true; do >>> ovs-appctl dpif-netdev/pmd-rxq-show |awk ' >>> /port: / { >>> tot++; >>> if ($5 == "(enabled)") { >>> en++; >>> } >>> } >>> END { >>> print "total: " tot ", enabled: " en >>> }' >>> sleep 1 >>> done >>> >>> total: 66, enabled: 66 >>> total: 6, enabled: 2 >>> >>> This change requires a fix in the DPDK vhost library, so bump the minimal >>> required version to 18.11.2. >> >> So in testing this I was seeing an error with the following >> >> 2019-06-27T10:00:08Z|00057|dpdk|INFO|VHOST_CONFIG: vhost peer closed >> 2019-06-27T10:00:08Z|00058|dpdk|ERR|VHOST_CONFIG: (0) device not found. >> 2019-06-27T10:00:08Z|00059|netdev_dpdk|INFO|vHost Device '' not found >> >> It wasn't until I realized I was testing with DPDK 18.11.2.rc1 that it dawned on me the required fix wasn't there...doh! >> >> So in short I can confirm the requirement for 18.11.2 :). >> >> Is there any requirement on the QEMU version side for this patch also? Or has the functionality been in place on that side for quite some time but was just mishandled in DPDK? > > 'destroy_connection' is just a callback that is called when socket closed > by DPDK or QEMU. So, no dependency on QEMU version. > The reason of above issue with 18.11.1 is that dpdk destroyed device before > calling the 'destroy_connection' callback, so OVS wasn't able to find > corresponding port, because 'rte_vhost_get_ifname' always returned an empty > string. Sure, I saw your patch in 18.11.2 and hence the minimal version move now. > >> >> One other minor query below but overall looks ok and has validated without issue. >> >>> >>> Co-authored-by: Ilya Maximets <i.maximets@samsung.com> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>> --- >>> Changes since v3: >>> - rebased on master following 18.11.2 support >>> - added a note in NEWS about minimal dpdk version >>> >>> Changes since v2: >>> - added authorship tags for Ilya >>> - added logs in destroy_connection >>> >>> --- >>> NEWS | 4 +++- >>> lib/netdev-dpdk.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 49 insertions(+), 2 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index c03e287..2f8171f 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -4,7 +4,9 @@ Post-v2.11.0 >>> * New option 'other_config:dpdk-socket-limit' to limit amount of >>> hugepage memory that can be used by DPDK. >>> * Add support for vHost Post-copy Live Migration (experimental). >>> - * OVS validated with DPDK 18.11.2 which is recommended to be used. >>> + * OVS validated with DPDK 18.11.2 which is the new minimal supported >>> + version. >>> + * DPDK 18.11.1 and lower is no longer supported. >>> - OpenFlow: >>> * All features required by OpenFlow 1.5 are now implemented, so >>> ovs-vswitchd now enables OpenFlow 1.5 by default (in addition to >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index b7f4543..0b9bea4 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -185,12 +185,15 @@ static const struct rte_eth_conf port_conf = { >>> static int new_device(int vid); >>> static void destroy_device(int vid); >>> static int vring_state_changed(int vid, uint16_t queue_id, int enable); >>> +static void destroy_connection(int vid); >>> static const struct vhost_device_ops virtio_net_device_ops = >>> { >>> .new_device = new_device, >>> .destroy_device = destroy_device, >>> .vring_state_changed = vring_state_changed, >>> - .features_changed = NULL >>> + .features_changed = NULL, >>> + .new_connection = NULL, >> >> I take it .new_connection is a requirement? It's just that it's added as NULL, but I assume it goes hand in hand with the introduction of .destroy connection? > > 'new_connection' was introduced in DPDK along with 'destroy_connection'. > Setting it as NULL here just to have a full list of callbacks as we > already have NULL 'features_changed'. To be consistent here we need to > add 'new_connection' or remove 'features_changed'. > Anyway compiler will clear all the non-initialized fields. > Thanks for clarifying Ilya, that makes more sense. Thanks all for the work on this. I've now pushed to master. Ian
diff --git a/NEWS b/NEWS index c03e287..2f8171f 100644 --- a/NEWS +++ b/NEWS @@ -4,7 +4,9 @@ Post-v2.11.0 * New option 'other_config:dpdk-socket-limit' to limit amount of hugepage memory that can be used by DPDK. * Add support for vHost Post-copy Live Migration (experimental). - * OVS validated with DPDK 18.11.2 which is recommended to be used. + * OVS validated with DPDK 18.11.2 which is the new minimal supported + version. + * DPDK 18.11.1 and lower is no longer supported. - OpenFlow: * All features required by OpenFlow 1.5 are now implemented, so ovs-vswitchd now enables OpenFlow 1.5 by default (in addition to diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index b7f4543..0b9bea4 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -185,12 +185,15 @@ static const struct rte_eth_conf port_conf = { static int new_device(int vid); static void destroy_device(int vid); static int vring_state_changed(int vid, uint16_t queue_id, int enable); +static void destroy_connection(int vid); static const struct vhost_device_ops virtio_net_device_ops = { .new_device = new_device, .destroy_device = destroy_device, .vring_state_changed = vring_state_changed, - .features_changed = NULL + .features_changed = NULL, + .new_connection = NULL, + .destroy_connection = destroy_connection, }; enum { DPDK_RING_SIZE = 256 }; @@ -3663,6 +3666,48 @@ vring_state_changed(int vid, uint16_t queue_id, int enable) return 0; } +static void +destroy_connection(int vid) +{ + struct netdev_dpdk *dev; + char ifname[IF_NAME_SZ]; + bool exists = false; + + rte_vhost_get_ifname(vid, ifname, sizeof ifname); + + ovs_mutex_lock(&dpdk_mutex); + LIST_FOR_EACH (dev, list_node, &dpdk_list) { + ovs_mutex_lock(&dev->mutex); + if (nullable_string_is_equal(ifname, dev->vhost_id)) { + uint32_t qp_num = NR_QUEUE; + + if (netdev_dpdk_get_vid(dev) >= 0) { + VLOG_ERR("Connection on socket '%s' destroyed while vhost " + "device still attached.", dev->vhost_id); + } + + /* Restore the number of queue pairs to default. */ + if (dev->requested_n_txq != qp_num + || dev->requested_n_rxq != qp_num) { + dev->requested_n_rxq = qp_num; + dev->requested_n_txq = qp_num; + netdev_request_reconfigure(&dev->up); + } + ovs_mutex_unlock(&dev->mutex); + exists = true; + break; + } + ovs_mutex_unlock(&dev->mutex); + } + ovs_mutex_unlock(&dpdk_mutex); + + if (exists) { + VLOG_INFO("vHost Device '%s' connection has been destroyed", ifname); + } else { + VLOG_INFO("vHost Device '%s' not found", ifname); + } +} + /* * Retrieve the DPDK virtio device ID (vid) associated with a vhostuser * or vhostuserclient netdev.
Rather than poll all disabled queues and waste some memory for vms that have been shutdown, we can reconfigure when receiving a destroy connection notification from the vhost library. $ while true; do ovs-appctl dpif-netdev/pmd-rxq-show |awk ' /port: / { tot++; if ($5 == "(enabled)") { en++; } } END { print "total: " tot ", enabled: " en }' sleep 1 done total: 66, enabled: 66 total: 6, enabled: 2 This change requires a fix in the DPDK vhost library, so bump the minimal required version to 18.11.2. Co-authored-by: Ilya Maximets <i.maximets@samsung.com> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> Signed-off-by: David Marchand <david.marchand@redhat.com> --- Changes since v3: - rebased on master following 18.11.2 support - added a note in NEWS about minimal dpdk version Changes since v2: - added authorship tags for Ilya - added logs in destroy_connection --- NEWS | 4 +++- lib/netdev-dpdk.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-)