In netdev_dpdk_send__() I folded in an OVS_UNLIKELY around the txq_needs_locking check as I'd like to optimize for the standard case where we don't. Otherwise looks good to me, I'll merge this series shortly.
Acked-by: Ethan Jackson <[email protected]> On Fri, May 22, 2015 at 9:14 AM, Daniele Di Proietto <[email protected]> wrote: > This commit changes the semantics of 'netdev_set_multiq()' to allow OVS > DPDK to run on device with limited multi queue support. > > * If a netdev doesn't have the requested number of rxqs it can simply > inform the datapath without failing. > * If a netdev doesn't have the requested number of txqs it should try > to create as many as possible and use locking. > > Signed-off-by: Daniele Di Proietto <[email protected]> > --- > lib/netdev-dpdk.c | 86 > +++++++++++++++++++++++++++++++++++++-------------- > lib/netdev-provider.h | 11 +++++++ > lib/netdev.c | 10 ++++++ > vswitchd/vswitch.xml | 2 +- > 4 files changed, 85 insertions(+), 24 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 975a842..44b6b5f 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -159,6 +159,10 @@ struct dpdk_tx_queue { > bool flush_tx; /* Set to true to flush queue everytime */ > /* pkts are queued. */ > int count; > + rte_spinlock_t tx_lock; /* Protects the members and the NIC queue > + * from concurrent access. It is used > only > + * if the queue is shared among different > + * pmd threads (see 'txq_needs_locking'). > */ > uint64_t tsc; > struct rte_mbuf *burst_pkts[MAX_TX_QUEUE_LEN]; > }; > @@ -203,12 +207,22 @@ struct netdev_dpdk { > struct rte_eth_link link; > int link_reset_cnt; > > + /* The user might request more txqs than the NIC has. We remap those > + * ('up.n_txq') on these ('real_n_txq'). > + * If the numbers match, 'txq_needs_locking' is false, otherwise it is > + * true and we will take a spinlock on transmission */ > + int real_n_txq; > + bool txq_needs_locking; > + > + /* Spinlock for vhost transmission. Other DPDK devices use spinlocks in > + * dpdk_tx_queue */ > + rte_spinlock_t vhost_tx_lock; > + > /* virtio-net structure for vhost device */ > OVSRCU_TYPE(struct virtio_net *) virtio_dev; > > /* In dpdk_list. */ > struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex); > - rte_spinlock_t txq_lock; > }; > > struct netdev_rxq_dpdk { > @@ -406,6 +420,7 @@ static int > dpdk_eth_dev_init(struct netdev_dpdk *dev) OVS_REQUIRES(dpdk_mutex) > { > struct rte_pktmbuf_pool_private *mbp_priv; > + struct rte_eth_dev_info info; > struct ether_addr eth_addr; > int diag; > int i; > @@ -414,14 +429,19 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > OVS_REQUIRES(dpdk_mutex) > return ENODEV; > } > > - diag = rte_eth_dev_configure(dev->port_id, dev->up.n_rxq, dev->up.n_txq, > + rte_eth_dev_info_get(dev->port_id, &info); > + dev->up.n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); > + dev->real_n_txq = MIN(info.max_tx_queues, dev->up.n_txq); > + > + diag = rte_eth_dev_configure(dev->port_id, dev->up.n_rxq, > dev->real_n_txq, > &port_conf); > if (diag) { > - VLOG_ERR("eth dev config error %d",diag); > + VLOG_ERR("eth dev config error %d. rxq:%d txq:%d", diag, > dev->up.n_rxq, > + dev->real_n_txq); > return -diag; > } > > - for (i = 0; i < dev->up.n_txq; i++) { > + for (i = 0; i < dev->real_n_txq; i++) { > diag = rte_eth_tx_queue_setup(dev->port_id, i, NIC_PORT_TX_Q_SIZE, > dev->socket_id, NULL); > if (diag) { > @@ -483,14 +503,20 @@ netdev_dpdk_alloc_txq(struct netdev_dpdk *netdev, > unsigned int n_txqs) > unsigned i; > > netdev->tx_q = dpdk_rte_mzalloc(n_txqs * sizeof *netdev->tx_q); > - /* Each index is considered as a cpu core id, since there should > - * be one tx queue for each cpu core. */ > for (i = 0; i < n_txqs; i++) { > int numa_id = ovs_numa_get_numa_id(i); > > - /* If the corresponding core is not on the same numa node > - * as 'netdev', flags the 'flush_tx'. */ > - netdev->tx_q[i].flush_tx = netdev->socket_id == numa_id; > + if (!netdev->txq_needs_locking) { > + /* Each index is considered as a cpu core id, since there should > + * be one tx queue for each cpu core. If the corresponding core > + * is not on the same numa node as 'netdev', flags the > + * 'flush_tx'. */ > + netdev->tx_q[i].flush_tx = netdev->socket_id == numa_id; > + } else { > + /* Queues are shared among CPUs. Always flush */ > + netdev->tx_q[i].flush_tx = true; > + } > + rte_spinlock_init(&netdev->tx_q[i].tx_lock); > } > } > > @@ -523,7 +549,6 @@ netdev_dpdk_init(struct netdev *netdev_, unsigned int > port_no, > netdev->flags = 0; > netdev->mtu = ETHER_MTU; > netdev->max_packet_len = MTU_TO_MAX_LEN(netdev->mtu); > - rte_spinlock_init(&netdev->txq_lock); > > netdev->dpdk_mp = dpdk_mp_get(netdev->socket_id, netdev->mtu); > if (!netdev->dpdk_mp) { > @@ -533,6 +558,7 @@ netdev_dpdk_init(struct netdev *netdev_, unsigned int > port_no, > > netdev_->n_txq = NR_QUEUE; > netdev_->n_rxq = NR_QUEUE; > + netdev->real_n_txq = NR_QUEUE; > > if (type == DPDK_DEV_ETH) { > netdev_dpdk_alloc_txq(netdev, NR_QUEUE); > @@ -570,6 +596,7 @@ dpdk_dev_parse_name(const char dev_name[], const char > prefix[], > static int > netdev_dpdk_vhost_construct(struct netdev *netdev_) > { > + struct netdev_dpdk *netdev = netdev_dpdk_cast(netdev_); > int err; > > if (rte_eal_init_ret) { > @@ -580,6 +607,8 @@ netdev_dpdk_vhost_construct(struct netdev *netdev_) > err = netdev_dpdk_init(netdev_, -1, DPDK_DEV_VHOST); > ovs_mutex_unlock(&dpdk_mutex); > > + rte_spinlock_init(&netdev->vhost_tx_lock); > + > return err; > } > > @@ -654,7 +683,8 @@ netdev_dpdk_get_config(const struct netdev *netdev_, > struct smap *args) > ovs_mutex_lock(&dev->mutex); > > smap_add_format(args, "configured_rx_queues", "%d", netdev_->n_rxq); > - smap_add_format(args, "configured_tx_queues", "%d", netdev_->n_txq); > + smap_add_format(args, "requested_tx_queues", "%d", netdev_->n_txq); > + smap_add_format(args, "configured_tx_queues", "%d", dev->real_n_txq); > ovs_mutex_unlock(&dev->mutex); > > return 0; > @@ -691,8 +721,10 @@ netdev_dpdk_set_multiq(struct netdev *netdev_, unsigned > int n_txq, > netdev->up.n_rxq = n_rxq; > > rte_free(netdev->tx_q); > - netdev_dpdk_alloc_txq(netdev, n_txq); > err = dpdk_eth_dev_init(netdev); > + netdev_dpdk_alloc_txq(netdev, netdev->real_n_txq); > + > + netdev->txq_needs_locking = netdev->real_n_txq != netdev->up.n_txq; > > ovs_mutex_unlock(&netdev->mutex); > ovs_mutex_unlock(&dpdk_mutex); > @@ -702,7 +734,7 @@ netdev_dpdk_set_multiq(struct netdev *netdev_, unsigned > int n_txq, > > static int > netdev_dpdk_vhost_set_multiq(struct netdev *netdev_, unsigned int n_txq, > - unsigned int n_rxq) > + unsigned int n_rxq) > { > struct netdev_dpdk *netdev = netdev_dpdk_cast(netdev_); > int err = 0; > @@ -715,7 +747,8 @@ netdev_dpdk_vhost_set_multiq(struct netdev *netdev_, > unsigned int n_txq, > ovs_mutex_lock(&netdev->mutex); > > netdev->up.n_txq = n_txq; > - netdev->up.n_rxq = n_rxq; > + netdev->real_n_txq = 1; > + netdev->up.n_rxq = 1; > > ovs_mutex_unlock(&netdev->mutex); > ovs_mutex_unlock(&dpdk_mutex); > @@ -894,7 +927,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, struct > dp_packet **pkts, > } > > /* There is vHost TX single queue, So we need to lock it for TX. */ > - rte_spinlock_lock(&vhost_dev->txq_lock); > + rte_spinlock_lock(&vhost_dev->vhost_tx_lock); > > do { > unsigned int tx_pkts; > @@ -930,12 +963,12 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, struct > dp_packet **pkts, > } > } > } while (cnt); > + rte_spinlock_unlock(&vhost_dev->vhost_tx_lock); > > rte_spinlock_lock(&vhost_dev->stats_lock); > vhost_dev->stats.tx_packets += (total_pkts - cnt); > vhost_dev->stats.tx_dropped += cnt; > rte_spinlock_unlock(&vhost_dev->stats_lock); > - rte_spinlock_unlock(&vhost_dev->txq_lock); > > out: > if (may_steal) { > @@ -1071,6 +1104,11 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid, > { > int i; > > + if (dev->txq_needs_locking) { > + qid = qid % dev->real_n_txq; > + rte_spinlock_lock(&dev->tx_q[qid].tx_lock); > + } > + > if (OVS_UNLIKELY(!may_steal || > pkts[0]->source != DPBUF_DPDK)) { > struct netdev *netdev = &dev->up; > @@ -1116,6 +1154,10 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid, > rte_spinlock_unlock(&dev->stats_lock); > } > } > + > + if (dev->txq_needs_locking) { > + rte_spinlock_unlock(&dev->tx_q[qid].tx_lock); > + } > } > > static int > @@ -1125,6 +1167,7 @@ netdev_dpdk_eth_send(struct netdev *netdev, int qid, > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > > netdev_dpdk_send__(dev, qid, pkts, cnt, may_steal); > + > return 0; > } > > @@ -1770,10 +1813,10 @@ dpdk_ring_open(const char dev_name[], unsigned int > *eth_port_id) OVS_REQUIRES(dp > } > > static int > -netdev_dpdk_ring_send(struct netdev *netdev, int qid OVS_UNUSED, > +netdev_dpdk_ring_send(struct netdev *netdev_, int qid, > struct dp_packet **pkts, int cnt, bool may_steal) > { > - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + struct netdev_dpdk *netdev = netdev_dpdk_cast(netdev_); > unsigned i; > > /* When using 'dpdkr' and sending to a DPDK ring, we want to ensure that > the > @@ -1784,10 +1827,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid > OVS_UNUSED, > dp_packet_set_rss_hash(pkts[i], 0); > } > > - /* DPDK Rings have a single TX queue, Therefore needs locking. */ > - rte_spinlock_lock(&dev->txq_lock); > - netdev_dpdk_send__(dev, 0, pkts, cnt, may_steal); > - rte_spinlock_unlock(&dev->txq_lock); > + netdev_dpdk_send__(netdev, qid, pkts, cnt, may_steal); > return 0; > } > > @@ -1965,7 +2005,7 @@ static const struct netdev_class dpdk_ring_class = > NULL, > netdev_dpdk_ring_construct, > netdev_dpdk_destruct, > - NULL, > + netdev_dpdk_set_multiq, > netdev_dpdk_ring_send, > netdev_dpdk_get_carrier, > netdev_dpdk_get_stats, > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h > index 734601d..eae1e64 100644 > --- a/lib/netdev-provider.h > +++ b/lib/netdev-provider.h > @@ -278,6 +278,17 @@ struct netdev_class { > /* Configures the number of tx queues and rx queues of 'netdev'. > * Return 0 if successful, otherwise a positive errno value. > * > + * '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 actual number of queues created > + * is stored in the 'netdev->n_rxq' field. > + * > + * 'n_txq' specifies the exact number of transmission queues to create. > + * The caller will call netdev_send() concurrently from 'n_txq' different > + * threads (with different qid). The netdev provider is responsible for > + * making sure that these concurrent calls do not create a race condition > + * by using multiple hw queues or locking. > + * > * On error, the tx queue and rx queue configuration is indeterminant. > * Caller should make decision on whether to restore the previous or > * the default configuration. Also, caller must make sure there is no > diff --git a/lib/netdev.c b/lib/netdev.c > index 45f7f29..03a7549 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -675,6 +675,16 @@ netdev_rxq_drain(struct netdev_rxq *rx) > /* Configures the number of tx queues and rx queues of 'netdev'. > * Return 0 if successful, otherwise a positive errno value. > * > + * '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()' > + * > + * 'n_txq' specifies the exact number of transmission queues to create. > + * If this function returns successfully, the caller can make 'n_txq' > + * concurrent calls to netdev_send() (each one with a different 'qid' in the > + * range [0..'n_txq'-1]). > + * > * On error, the tx queue and rx queue configuration is indeterminant. > * Caller should make decision on whether to restore the previous or > * the default configuration. Also, caller must make sure there is no > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 79b5606..8a60474 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -155,7 +155,7 @@ > <column name="other_config" key="n-dpdk-rxqs" > type='{"type": "integer", "minInteger": 1}'> > <p> > - Specifies the number of rx queues to be created for each dpdk > + Specifies the maximum number of rx queues to be created for each > dpdk > interface. If not specified or specified to 0, one rx queue will > be created for each dpdk interface by default. > </p> > -- > 2.1.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
