Thank you very much for review. New version posted. Best regards, Ilya Maximets.
On 21.01.2016 02:26, Daniele Di Proietto wrote: > Hi Ilya, > > Thanks for the patch. I like the general approach and the implementation. > Couple of things: > > * This change should definitely be mentioned in NEWS, since it breaks > compatibility. > * I think INSTALL.DPDK.md should be updated too, since it contains > references to the old options. > * One more comment inline > > Other than these, I think this is ready to be merged. > > Thanks again for your contribution. > > On 11/01/2016 02:11, "Ilya Maximets" <[email protected]> wrote: > >> Currently, all of the PMD netdevs can only have the same number of >> rx queues, which is specified in other_config:n-dpdk-rxqs. >> >> Fix that by introducing of new option for PMD interfaces: 'n_rxq', which >> specifies the maximum number of rx queues to be created for this >> interface. >> >> Example: >> ovs-vsctl set Interface dpdk0 options:n_rxq=8 >> >> Old 'other_config:n-dpdk-rxqs' deleted. >> >> Signed-off-by: Ilya Maximets <[email protected]> >> Acked-by: Ben Pfaff <[email protected]> >> --- >> Version 2: >> * Changed wrong comment in struct netdev as suggested by Ben Pfaff. >> >> lib/dpif-netdev.c | 39 +++++++++++++++++++++++++-------------- >> lib/dpif-provider.h | 8 +++----- >> lib/dpif.c | 5 ++--- >> lib/dpif.h | 3 +-- >> lib/netdev-dpdk.c | 26 +++++++++++++++++++++----- >> lib/netdev-provider.h | 6 +++++- >> lib/netdev.c | 7 +++++++ >> lib/netdev.h | 1 + >> ofproto/ofproto-dpif.c | 2 +- >> ofproto/ofproto-provider.h | 3 --- >> ofproto/ofproto.c | 7 ------- >> ofproto/ofproto.h | 1 - >> vswitchd/bridge.c | 2 -- >> vswitchd/vswitch.xml | 24 +++++++++++++++--------- >> 14 files changed, 81 insertions(+), 53 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index cd72e62..fe2cd4b 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -221,9 +221,7 @@ struct dp_netdev { >> * 'struct dp_netdev_pmd_thread' in 'per_pmd_key'. */ >> ovsthread_key_t per_pmd_key; >> >> - /* Number of rx queues for each dpdk interface and the cpu mask >> - * for pin of pmd threads. */ >> - size_t n_dpdk_rxqs; >> + /* Cpu mask for pin of pmd threads. */ >> char *pmd_cmask; >> uint64_t last_tnl_conf_seq; >> }; >> @@ -847,7 +845,6 @@ create_dp_netdev(const char *name, const struct >> dpif_class *class, >> ovsthread_key_create(&dp->per_pmd_key, NULL); >> >> dp_netdev_set_nonpmd(dp); >> - dp->n_dpdk_rxqs = NR_QUEUE; >> >> ovs_mutex_lock(&dp->port_mutex); >> error = do_add_port(dp, name, "internal", ODPP_LOCAL); >> @@ -1086,7 +1083,8 @@ do_add_port(struct dp_netdev *dp, const char >> *devname, const char *type, >> /* There can only be ovs_numa_get_n_cores() pmd threads, >> * so creates a txq for each, and one extra for the non >> * pmd threads. */ >> - error = netdev_set_multiq(netdev, n_cores + 1, dp->n_dpdk_rxqs); >> + error = netdev_set_multiq(netdev, n_cores + 1, >> + netdev_requested_n_rxq(netdev)); >> if (error && (error != EOPNOTSUPP)) { >> VLOG_ERR("%s, cannot set multiq", devname); >> return errno; >> @@ -2360,9 +2358,21 @@ dpif_netdev_operate(struct dpif *dpif, struct >> dpif_op **ops, size_t n_ops) >> /* Returns true if the configuration for rx queues or cpu mask >> * is changed. */ >> static bool >> -pmd_config_changed(const struct dp_netdev *dp, size_t rxqs, const char >> *cmask) >> +pmd_config_changed(const struct dp_netdev *dp, const char *cmask) >> { >> - if (dp->n_dpdk_rxqs != rxqs) { >> + bool changed = false; >> + struct dp_netdev_port *port; >> + >> + CMAP_FOR_EACH (port, node, &dp->ports) { >> + struct netdev *netdev = port->netdev; >> + if (netdev_is_pmd(netdev) >> + && netdev_n_rxq(netdev) != netdev_requested_n_rxq(netdev)) { > > There's a problem here: netdev_n_rxq() might be different than > netdev_requested_n_rxq() even if the user hasn't asked for any change. > > Here's a relevant piece of comment in lib/netdev.c > > [...] > * > * 'n_rxq' specifies the maximum number of receive queues to create. > * The netdev provider might choose to create less (e.g. if the hardware > * supports only a smaller number). The caller can check how many have > been > * actually created by calling 'netdev_n_rxq()' > * > [...] > int > netdev_set_multiq(struct netdev *netdev, unsigned int n_txq, > unsigned int n_rxq) > { > > > One solution would be to store the result of netdev_requested_n_rxq() > in 'struct dp_netdev_port' and use that instead of netdev_n_rxq() to do > the comparison. > > >> + changed = true; >> + break; > > I think we can return right away. > >> + } >> + } >> + >> + if (changed) { >> return true; >> } else { >> if (dp->pmd_cmask != NULL && cmask != NULL) { >> @@ -2375,17 +2385,20 @@ pmd_config_changed(const struct dp_netdev *dp, >> size_t rxqs, const char *cmask) >> >> /* Resets pmd threads if the configuration for 'rxq's or cpu mask >> changes. */ >> static int >> -dpif_netdev_pmd_set(struct dpif *dpif, unsigned int n_rxqs, const char >> *cmask) >> +dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask) >> { >> struct dp_netdev *dp = get_dp_netdev(dpif); >> >> - if (pmd_config_changed(dp, n_rxqs, cmask)) { >> + if (pmd_config_changed(dp, cmask)) { >> struct dp_netdev_port *port; >> >> dp_netdev_destroy_all_pmds(dp); >> >> CMAP_FOR_EACH (port, node, &dp->ports) { >> - if (netdev_is_pmd(port->netdev)) { >> + struct netdev *netdev = port->netdev; >> + int requested_n_rxq = netdev_requested_n_rxq(netdev); >> + if (netdev_is_pmd(port->netdev) >> + && netdev_n_rxq(netdev) != requested_n_rxq) { > > Same as above: netdev_n_rxq() can return a smaller number for different > reasons. > >> int i, err; >> >> /* Closes the existing 'rxq's. */ >> @@ -2397,11 +2410,11 @@ dpif_netdev_pmd_set(struct dpif *dpif, unsigned >> int n_rxqs, const char *cmask) >> /* Sets the new rx queue config. */ >> err = netdev_set_multiq(port->netdev, >> ovs_numa_get_n_cores() + 1, >> - n_rxqs); >> + requested_n_rxq); >> if (err && (err != EOPNOTSUPP)) { >> VLOG_ERR("Failed to set dpdk interface %s rx_queue >> to:" >> " %u", netdev_get_name(port->netdev), >> - n_rxqs); >> + requested_n_rxq); >> return err; >> } >> >> @@ -2413,8 +2426,6 @@ dpif_netdev_pmd_set(struct dpif *dpif, unsigned int >> n_rxqs, const char *cmask) >> } >> } >> } >> - dp->n_dpdk_rxqs = n_rxqs; >> - >> /* Reconfigures the cpu mask. */ >> ovs_numa_set_cpu_mask(cmask); >> free(dp->pmd_cmask); > > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
