| Message ID | 1456968802-4358-10-git-send-email-diproiettod@vmware.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
comment inline. Best regards, Ilya Maximets. On 03.03.2016 04:33, Daniele Di Proietto wrote: > This commit change reconfigure_pmd_threads() to interact with the ports > cmap using RCU semantics (the content of the port structure is not > altered while concurrent readers might access it) and to fail more > gracefully in case of a set_multiq fail (now we remove the port from the > datapath, instead of returning prematurely from the function without > restarting the pmd threads). > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > --- > lib/dpif-netdev.c | 79 ++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 55 insertions(+), 24 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 8ed6bcb..0c3673b 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -2591,6 +2591,11 @@ static void > reconfigure_pmd_threads(struct dp_netdev *dp) > { > struct dp_netdev_port *port; > + struct hmapx to_reconfigure = HMAPX_INITIALIZER(&to_reconfigure); > + struct hmapx_node *node; > + bool failed_config = false; > + > + ovs_mutex_lock(&dp->port_mutex); > > dp_netdev_destroy_all_pmds(dp); > > @@ -2599,33 +2604,59 @@ reconfigure_pmd_threads(struct dp_netdev *dp) > int requested_n_rxq = netdev_requested_n_rxq(netdev); > if (netdev_is_pmd(port->netdev) > && port->latest_requested_n_rxq != requested_n_rxq) { > - int i, err; > + cmap_remove(&dp->ports, &port->node, hash_odp_port(port->port_no)); > + hmapx_add(&to_reconfigure, port); > + } > + } > + ovs_mutex_unlock(&dp->port_mutex); > > - /* Closes the existing 'rxq's. */ > - for (i = 0; i < port->n_rxq; i++) { > - netdev_rxq_close(port->rxq[i]); > - port->rxq[i] = NULL; > - } > - port->n_rxq = 0; > - > - /* Sets the new rx queue config. */ > - err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1, > - requested_n_rxq); > - if (err && (err != EOPNOTSUPP)) { > - VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u", > - netdev_get_name(port->netdev), > - requested_n_rxq); > - return; > - } > - port->latest_requested_n_rxq = requested_n_rxq; > - /* If the set_multiq() above succeeds, reopens the 'rxq's. */ > - port->n_rxq = netdev_n_rxq(port->netdev); > - port->rxq = xrealloc(port->rxq, sizeof *port->rxq * port->n_rxq); > - for (i = 0; i < port->n_rxq; i++) { > - netdev_rxq_open(port->netdev, &port->rxq[i], i); > - } > + /* Waits for the other threads to see the ports removed from the cmap, > + * otherwise we are not allowed to alter them. */ > + ovsrcu_synchronize(); > + > + ovs_mutex_lock(&dp->port_mutex); > + HMAPX_FOR_EACH (node, &to_reconfigure) { > + int requested_n_rxq = netdev_requested_n_rxq(port->netdev); This will be removed lately but this is a bug in this patch. We should not call netdev_requested_n_rxq here. Also, this may lead to segmentation fault because of invalid 'port'. > + int i, err; > + > + port = node->data; > + requested_n_rxq = netdev_requested_n_rxq(port->netdev); > + /* Closes the existing 'rxq's. */ > + for (i = 0; i < port->n_rxq; i++) { > + netdev_rxq_close(port->rxq[i]); > + port->rxq[i] = NULL; > + } > + port->n_rxq = 0; > + > + /* Sets the new rx queue config. */ > + err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1, > + requested_n_rxq); > + if (err && (err != EOPNOTSUPP)) { > + VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u", > + netdev_get_name(port->netdev), > + requested_n_rxq); > + do_destroy_port(port); > + failed_config = true; > + continue; > } > + port->latest_requested_n_rxq = requested_n_rxq; > + /* If the netdev_reconfigure() above succeeds, reopens the 'rxq's and > + * inserts the port back in the cmap, to allow transmitting packets. */ > + port->n_rxq = netdev_n_rxq(port->netdev); > + port->rxq = xrealloc(port->rxq, sizeof *port->rxq * port->n_rxq); > + for (i = 0; i < port->n_rxq; i++) { > + netdev_rxq_open(port->netdev, &port->rxq[i], i); > + } > + cmap_insert(&dp->ports, &port->node, hash_port_no(port->port_no)); > + } > + ovs_mutex_unlock(&dp->port_mutex); > + > + hmapx_destroy(&to_reconfigure); > + > + if (failed_config) { > + seq_change(dp->port_seq); > } > + > /* Reconfigures the cpu mask. */ > ovs_numa_set_cpu_mask(dp->requested_pmd_cmask); > free(dp->pmd_cmask); >
Hi Ilya, On 15/03/2016 06:43, "Ilya Maximets" <i.maximets@samsung.com> wrote: >comment inline. > >Best regards, Ilya Maximets. > >On 03.03.2016 04:33, Daniele Di Proietto wrote: >> This commit change reconfigure_pmd_threads() to interact with the ports >> cmap using RCU semantics (the content of the port structure is not >> altered while concurrent readers might access it) and to fail more >> gracefully in case of a set_multiq fail (now we remove the port from the >> datapath, instead of returning prematurely from the function without >> restarting the pmd threads). >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >> --- >> lib/dpif-netdev.c | 79 >>++++++++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 55 insertions(+), 24 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 8ed6bcb..0c3673b 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -2591,6 +2591,11 @@ static void >> reconfigure_pmd_threads(struct dp_netdev *dp) >> { >> struct dp_netdev_port *port; >> + struct hmapx to_reconfigure = HMAPX_INITIALIZER(&to_reconfigure); >> + struct hmapx_node *node; >> + bool failed_config = false; >> + >> + ovs_mutex_lock(&dp->port_mutex); >> >> dp_netdev_destroy_all_pmds(dp); >> >> @@ -2599,33 +2604,59 @@ reconfigure_pmd_threads(struct dp_netdev *dp) >> int requested_n_rxq = netdev_requested_n_rxq(netdev); >> if (netdev_is_pmd(port->netdev) >> && port->latest_requested_n_rxq != requested_n_rxq) { >> - int i, err; >> + cmap_remove(&dp->ports, &port->node, >>hash_odp_port(port->port_no)); >> + hmapx_add(&to_reconfigure, port); >> + } >> + } >> + ovs_mutex_unlock(&dp->port_mutex); >> >> - /* Closes the existing 'rxq's. */ >> - for (i = 0; i < port->n_rxq; i++) { >> - netdev_rxq_close(port->rxq[i]); >> - port->rxq[i] = NULL; >> - } >> - port->n_rxq = 0; >> - >> - /* Sets the new rx queue config. */ >> - err = netdev_set_multiq(port->netdev, >>ovs_numa_get_n_cores() + 1, >> - requested_n_rxq); >> - if (err && (err != EOPNOTSUPP)) { >> - VLOG_ERR("Failed to set dpdk interface %s rx_queue to: >>%u", >> - netdev_get_name(port->netdev), >> - requested_n_rxq); >> - return; >> - } >> - port->latest_requested_n_rxq = requested_n_rxq; >> - /* If the set_multiq() above succeeds, reopens the 'rxq's. >>*/ >> - port->n_rxq = netdev_n_rxq(port->netdev); >> - port->rxq = xrealloc(port->rxq, sizeof *port->rxq * >>port->n_rxq); >> - for (i = 0; i < port->n_rxq; i++) { >> - netdev_rxq_open(port->netdev, &port->rxq[i], i); >> - } >> + /* Waits for the other threads to see the ports removed from the >>cmap, >> + * otherwise we are not allowed to alter them. */ >> + ovsrcu_synchronize(); >> + >> + ovs_mutex_lock(&dp->port_mutex); >> + HMAPX_FOR_EACH (node, &to_reconfigure) { >> + int requested_n_rxq = netdev_requested_n_rxq(port->netdev); > >This will be removed lately but this is a bug in this patch. We should not >call netdev_requested_n_rxq here. Also, this may lead to segmentation >fault >because of invalid 'port'. You're right, it is also unnecessary, since requested_n_rxq is properly set below. I've removed the initialization. Thanks for spotting this! > >> + int i, err; >> + >> + port = node->data; >> + requested_n_rxq = netdev_requested_n_rxq(port->netdev); >> + /* Closes the existing 'rxq's. */ >> + for (i = 0; i < port->n_rxq; i++) { >> + netdev_rxq_close(port->rxq[i]); >> + port->rxq[i] = NULL; >> + } >> + port->n_rxq = 0; >> + >> + /* Sets the new rx queue config. */ >> + err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + >>1, >> + requested_n_rxq); >> + if (err && (err != EOPNOTSUPP)) { >> + VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u", >> + netdev_get_name(port->netdev), >> + requested_n_rxq); >> + do_destroy_port(port); >> + failed_config = true; >> + continue; >> } >> + port->latest_requested_n_rxq = requested_n_rxq; >> + /* If the netdev_reconfigure() above succeeds, reopens the >>'rxq's and >> + * inserts the port back in the cmap, to allow transmitting >>packets. */ >> + port->n_rxq = netdev_n_rxq(port->netdev); >> + port->rxq = xrealloc(port->rxq, sizeof *port->rxq * >>port->n_rxq); >> + for (i = 0; i < port->n_rxq; i++) { >> + netdev_rxq_open(port->netdev, &port->rxq[i], i); >> + } >> + cmap_insert(&dp->ports, &port->node, >>hash_port_no(port->port_no)); >> + } >> + ovs_mutex_unlock(&dp->port_mutex); >> + >> + hmapx_destroy(&to_reconfigure); >> + >> + if (failed_config) { >> + seq_change(dp->port_seq); >> } >> + >> /* Reconfigures the cpu mask. */ >> ovs_numa_set_cpu_mask(dp->requested_pmd_cmask); >> free(dp->pmd_cmask); >>
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 8ed6bcb..0c3673b 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -2591,6 +2591,11 @@ static void reconfigure_pmd_threads(struct dp_netdev *dp) { struct dp_netdev_port *port; + struct hmapx to_reconfigure = HMAPX_INITIALIZER(&to_reconfigure); + struct hmapx_node *node; + bool failed_config = false; + + ovs_mutex_lock(&dp->port_mutex); dp_netdev_destroy_all_pmds(dp); @@ -2599,33 +2604,59 @@ reconfigure_pmd_threads(struct dp_netdev *dp) int requested_n_rxq = netdev_requested_n_rxq(netdev); if (netdev_is_pmd(port->netdev) && port->latest_requested_n_rxq != requested_n_rxq) { - int i, err; + cmap_remove(&dp->ports, &port->node, hash_odp_port(port->port_no)); + hmapx_add(&to_reconfigure, port); + } + } + ovs_mutex_unlock(&dp->port_mutex); - /* Closes the existing 'rxq's. */ - for (i = 0; i < port->n_rxq; i++) { - netdev_rxq_close(port->rxq[i]); - port->rxq[i] = NULL; - } - port->n_rxq = 0; - - /* Sets the new rx queue config. */ - err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1, - requested_n_rxq); - if (err && (err != EOPNOTSUPP)) { - VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u", - netdev_get_name(port->netdev), - requested_n_rxq); - return; - } - port->latest_requested_n_rxq = requested_n_rxq; - /* If the set_multiq() above succeeds, reopens the 'rxq's. */ - port->n_rxq = netdev_n_rxq(port->netdev); - port->rxq = xrealloc(port->rxq, sizeof *port->rxq * port->n_rxq); - for (i = 0; i < port->n_rxq; i++) { - netdev_rxq_open(port->netdev, &port->rxq[i], i); - } + /* Waits for the other threads to see the ports removed from the cmap, + * otherwise we are not allowed to alter them. */ + ovsrcu_synchronize(); + + ovs_mutex_lock(&dp->port_mutex); + HMAPX_FOR_EACH (node, &to_reconfigure) { + int requested_n_rxq = netdev_requested_n_rxq(port->netdev); + int i, err; + + port = node->data; + requested_n_rxq = netdev_requested_n_rxq(port->netdev); + /* Closes the existing 'rxq's. */ + for (i = 0; i < port->n_rxq; i++) { + netdev_rxq_close(port->rxq[i]); + port->rxq[i] = NULL; + } + port->n_rxq = 0; + + /* Sets the new rx queue config. */ + err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1, + requested_n_rxq); + if (err && (err != EOPNOTSUPP)) { + VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u", + netdev_get_name(port->netdev), + requested_n_rxq); + do_destroy_port(port); + failed_config = true; + continue; } + port->latest_requested_n_rxq = requested_n_rxq; + /* If the netdev_reconfigure() above succeeds, reopens the 'rxq's and + * inserts the port back in the cmap, to allow transmitting packets. */ + port->n_rxq = netdev_n_rxq(port->netdev); + port->rxq = xrealloc(port->rxq, sizeof *port->rxq * port->n_rxq); + for (i = 0; i < port->n_rxq; i++) { + netdev_rxq_open(port->netdev, &port->rxq[i], i); + } + cmap_insert(&dp->ports, &port->node, hash_port_no(port->port_no)); + } + ovs_mutex_unlock(&dp->port_mutex); + + hmapx_destroy(&to_reconfigure); + + if (failed_config) { + seq_change(dp->port_seq); } + /* Reconfigures the cpu mask. */ ovs_numa_set_cpu_mask(dp->requested_pmd_cmask); free(dp->pmd_cmask);
This commit change reconfigure_pmd_threads() to interact with the ports cmap using RCU semantics (the content of the port structure is not altered while concurrent readers might access it) and to fail more gracefully in case of a set_multiq fail (now we remove the port from the datapath, instead of returning prematurely from the function without restarting the pmd threads). Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- lib/dpif-netdev.c | 79 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 24 deletions(-)