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
