Thanks for the patch, I applied this to master and branch-2.5 with
a minor change: since tx_qid is accessed concurrently by multiple
threads, it seems that it should be atomic.
This is just a formality: I've used relaxed semantics so there should
be no change to the compiled code.
Here's the incremental:
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 24946b7..ad6b202 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -434,7 +434,7 @@ struct dp_netdev_pmd_thread {
/* threads on same numa node. */
unsigned core_id; /* CPU core id of this pmd thread. */
int numa_id; /* numa node id of this pmd thread. */
- int tx_qid; /* Queue id used by this pmd thread to
+ atomic_int tx_qid; /* Queue id used by this pmd thread to
* send packets on all netdevs */
struct ovs_mutex poll_mutex; /* Mutex for poll_list. */
@@ -2838,8 +2838,11 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread
*pmd, struct dp_netdev *dp,
pmd->numa_id = numa_id;
pmd->poll_cnt = 0;
- pmd->tx_qid = (core_id == NON_PMD_CORE_ID) ? ovs_numa_get_n_cores()
- : get_n_pmd_threads(dp);
+ atomic_init(&pmd->tx_qid,
+ (core_id == NON_PMD_CORE_ID)
+ ? ovs_numa_get_n_cores()
+ : get_n_pmd_threads(dp));
+
ovs_refcount_init(&pmd->ref_cnt);
latch_init(&pmd->exit_latch);
atomic_init(&pmd->change_seq, PMD_INITIAL_SEQ);
@@ -2931,15 +2934,22 @@ dp_netdev_del_pmds_on_numa(struct dp_netdev *dp,
int numa_id)
CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
if (pmd->numa_id == numa_id) {
- free_idx[k++] = pmd->tx_qid;
+ atomic_read_relaxed(&pmd->tx_qid, &free_idx[k]);
+ k++;
dp_netdev_del_pmd(dp, pmd);
}
}
n_pmds = get_n_pmd_threads(dp);
CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
- if (pmd->tx_qid >= n_pmds) {
- pmd->tx_qid = free_idx[--k];
+ int old_tx_qid;
+
+ atomic_read_relaxed(&pmd->tx_qid, &old_tx_qid);
+
+ if (old_tx_qid >= n_pmds) {
+ int new_tx_qid = free_idx[--k];
+
+ atomic_store_relaxed(&pmd->tx_qid, new_tx_qid);
}
}
@@ -3585,7 +3595,11 @@ dp_execute_cb(void *aux_, struct dp_packet
**packets, int cnt,
case OVS_ACTION_ATTR_OUTPUT:
p = dp_netdev_lookup_port(dp, u32_to_odp(nl_attr_get_u32(a)));
if (OVS_LIKELY(p)) {
- netdev_send(p->netdev, pmd->tx_qid, packets, cnt, may_steal);
+ int tx_qid;
+
+ atomic_read_relaxed(&pmd->tx_qid, &tx_qid);
+
+ netdev_send(p->netdev, tx_qid, packets, cnt, may_steal);
return;
}
break;
On 25/01/2016 22:12, "Ilya Maximets" <[email protected]> wrote:
>Currently tx_qid is equal to pmd->core_id. This leads to unexpected
>behavior if pmd-cpu-mask different from '/(0*)(1|3|7)?(f*)/',
>e.g. if core_ids are not sequential, or doesn't start from 0, or both.
>
>Example:
> starting 2 pmd threads with 1 port, 2 rxqs per port,
> pmd-cpu-mask = 00000014 and let dev->real_n_txq = 2
>
> It that case pmd_1->tx_qid = 2, pmd_2->tx_qid = 4 and
> txq_needs_locking = true (if device hasn't ovs_numa_get_n_cores()+1
> queues).
>
> In that case, after truncating in netdev_dpdk_send__():
> 'qid = qid % dev->real_n_txq;'
> pmd_1: qid = 2 % 2 = 0
> pmd_2: qid = 4 % 2 = 0
>
> So, both threads will call dpdk_queue_pkts() with same qid = 0.
> This is unexpected behavior if there is 2 tx queues in device.
> Queue #1 will not be used and both threads will lock queue #0
> on each send.
>
>Fix that by using sequential tx_qids.
>
>Signed-off-by: Ilya Maximets <[email protected]>
>---
> lib/dpif-netdev.c | 38 ++++++++++++++++++++++++++------------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index 8c87c05..24946b7 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -1285,6 +1285,13 @@ get_port_by_name(struct dp_netdev *dp,
> }
>
> static int
>+get_n_pmd_threads(struct dp_netdev *dp)
>+{
>+ /* There is one non pmd thread in dp->poll_threads */
>+ return cmap_count(&dp->poll_threads) - 1;
>+}
>+
>+static int
> get_n_pmd_threads_on_numa(struct dp_netdev *dp, int numa_id)
> {
> struct dp_netdev_pmd_thread *pmd;
>@@ -2820,16 +2827,6 @@ dp_netdev_pmd_get_next(struct dp_netdev *dp,
>struct cmap_position *pos)
> return next;
> }
>
>-static int
>-core_id_to_qid(unsigned core_id)
>-{
>- if (core_id != NON_PMD_CORE_ID) {
>- return core_id;
>- } else {
>- return ovs_numa_get_n_cores();
>- }
>-}
>-
> /* Configures the 'pmd' based on the input argument. */
> static void
> dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct
>dp_netdev *dp,
>@@ -2838,10 +2835,11 @@ dp_netdev_configure_pmd(struct
>dp_netdev_pmd_thread *pmd, struct dp_netdev *dp,
> pmd->dp = dp;
> pmd->index = index;
> pmd->core_id = core_id;
>- pmd->tx_qid = core_id_to_qid(core_id);
> pmd->numa_id = numa_id;
> pmd->poll_cnt = 0;
>
>+ pmd->tx_qid = (core_id == NON_PMD_CORE_ID) ? ovs_numa_get_n_cores()
>+ : get_n_pmd_threads(dp);
> ovs_refcount_init(&pmd->ref_cnt);
> latch_init(&pmd->exit_latch);
> atomic_init(&pmd->change_seq, PMD_INITIAL_SEQ);
>@@ -2919,17 +2917,33 @@ dp_netdev_destroy_all_pmds(struct dp_netdev *dp)
> }
> }
>
>-/* Deletes all pmd threads on numa node 'numa_id'. */
>+/* Deletes all pmd threads on numa node 'numa_id' and
>+ * fixes tx_qids of other threads to keep them sequential. */
> static void
> dp_netdev_del_pmds_on_numa(struct dp_netdev *dp, int numa_id)
> {
> struct dp_netdev_pmd_thread *pmd;
>+ int n_pmds_on_numa, n_pmds;
>+ int *free_idx, k = 0;
>+
>+ n_pmds_on_numa = get_n_pmd_threads_on_numa(dp, numa_id);
>+ free_idx = xmalloc(n_pmds_on_numa * sizeof *free_idx);
>
> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> if (pmd->numa_id == numa_id) {
>+ free_idx[k++] = pmd->tx_qid;
> dp_netdev_del_pmd(dp, pmd);
> }
> }
>+
>+ n_pmds = get_n_pmd_threads(dp);
>+ CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>+ if (pmd->tx_qid >= n_pmds) {
>+ pmd->tx_qid = free_idx[--k];
>+ }
>+ }
>+
>+ free(free_idx);
> }
>
> /* Returns PMD thread from this numa node with fewer rx queues to poll.
>--
>2.5.0
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
