On 6/26/17, 6:52 AM, "O Mahony, Billy" <[email protected]> wrote:
Hi Darrell,
Thanks for reviewing.
> -----Original Message-----
> From: Darrell Ball [mailto:[email protected]]
> Sent: Monday, June 26, 2017 8:04 AM
> To: O Mahony, Billy <[email protected]>; [email protected]
> Subject: Re: [ovs-dev] [PATCH v6] dpif-netdev: Assign ports to pmds on
non-
> local numa node.
>
> I think this is helpful in some cases where lower performance is an
> acceptable tradeoff with more frugal and/or more flexible usage of cpu
> resources.
>
> I did not test it since Ian has already done that, but I reviewed the code
> change and other related code.
>
> One comment inline regarding the added documentation.
>
> Otherwise, Acked-by: Darrell Ball <[email protected]>
>
>
>
> On 5/10/17, 8:59 AM, "[email protected] on behalf of Billy
> O'Mahony" <[email protected] on behalf of
> [email protected]> wrote:
>
> From: billyom <[email protected]>
>
> Previously if there is no available (non-isolated) pmd on the numa
node
> for a port then the port is not polled at all. This can result in a
> non-operational system until such time as nics are physically
> repositioned. It is preferable to operate with a pmd on the 'wrong'
numa
> node albeit with lower performance. Local pmds are still chosen when
> available.
>
> Signed-off-by: Billy O'Mahony <[email protected]>
> ---
> v6: Change 'port' to 'queue' in a warning msg
> v5: Fix warning msg; Update same in docs
> v4: Fix a checkpatch error
> v3: Fix warning messages not appearing when using multiqueue
> v2: Add details of warning messages into docs
>
> Documentation/intro/install/dpdk.rst | 10 +++++++++
> lib/dpif-netdev.c | 43
+++++++++++++++++++++++++++++++---
> --
> 2 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/intro/install/dpdk.rst
> b/Documentation/intro/install/dpdk.rst
> index d1c0e65..7a66bff 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -460,6 +460,16 @@ affinitized accordingly.
> pmd thread on a NUMA node is only created if there is at least
one DPDK
> interface from that NUMA node added to OVS.
>
> + .. note::
> + On NUMA systems PCI devices are also local to a NUMA node. Rx
> queues for
> + PCI device will assigned to a pmd on it's local NUMA node if
pmd-cpu-
> mask
> + has created a pmd thread on that NUMA node. If not the queue
will be
> + assigned to a pmd on a remote NUMA node.
>
>
>
> I think the below text is a bit more accurate
> + .. note::
> + On NUMA systems PCI devices are also local to a NUMA node. Rx queues
> for
> + a PCI device will be assigned to a pmd on it's local NUMA node.
[[BO'M]]
Re suggested sentence: "Rx queues for a PCI device will be assigned to a
pmd on it's local NUMA node."
However, with this patch that is no longer always the case. Assignment to a
pmd on it's local NUMA node will be preferred but it that is not possible then
assignment will be done to a PMD on a non-local NUMA node.
Is the suggested change in order to emphasize that a PMD is created if
specified in pmd-cpu-mask but not actually scheduled by the OS unless at least
one rxq is assigned to it? That case is covered in the preceeding paragraph
(which is not modified by this patch) "A pmd thread on a NUMA node is only
created if there is at least one DPDK interface from that NUMA node added to
OVS." Which, now I read it should be modified to read "A PMD thread only
becomes runnable if there is at least one DPDK interface assigned to it."
[Darrell]
I unintentionally deleted the below line of text from the beginning of the
following paragraph – “my bad”.
“If not the queue will be assigned to a pmd on a remote NUMA node. “
The full text was intended to be
+ .. note::
+ On NUMA systems PCI devices are also local to a NUMA node. Rx queues for
+ a PCI device will be assigned to a pmd on it's local NUMA node. A pmd is
+ created if at least one dpdk interface is added to OVS on that NUMA node or
+ if the pmd-cpu-mask has created a pmd thread on that NUMA node. If not the
queue will be
+ assigned to a pmd on a remote NUMA node. This will result in reduced
+ maximum throughput on that device. In the case such a queue assignment
+ is made a warning message will be logged: "There's no available (non-
+ isolated) pmd thread on numa node N. Queue Q on port P will be assigned to
+ the pmd on core C (numa node N'). Expect reduced performance."
+
which is not totally accurate.
Also, as you pointed out, I also added a redundant line from the above paragraph
I expected one
.. note::
and you added a second.
This is getting confusing.
How about dividing the thread and queue discussions more cleanly
and adding more context.
.. note::
A pmd thread on a NUMA node is only created if there is at least one DPDK
interface from that NUMA node added to OVS. A pmd thread is created by
default on a core of a NUMA node or when a specified pmd-cpu-mask has
indicated so.
.. note::
On NUMA systems PCI devices are also local to a NUMA node. Unbound Rx
queues for a PCI device will assigned to a pmd on it's local NUMA node if a
non-isolated PMD exists on that NUMA node. If not, the queue will be
assigned to a non-isolated pmd on a remote NUMA node. This will result in
reduced maximum throughput on that device. In the case such, a queue
assignment is made a warning message will be logged: "There's no available
(non-isolated) pmd thread on numa node N. Queue Q on port P will
be assigned to the pmd on core C (numa node N'). Expect reduced performance."
A pmd is
> + created if at least one dpdk interface is added to OVS on that NUMA
node
> or
> + if the pmd-cpu-mask has created a pmd thread on that NUMA node.
[[BO'M]]
Is this part of the suggested change in order to emphasize that a PMD is
created if specified in pmd-cpu-mask but not actually scheduled by the OS
unless at least one rxq is assigned to it? (introduced 2788a1b). That detail is
covered somewhat in the preceding paragraph (which is not modified by this
patch) "A pmd thread on a NUMA node is only created if there is at least one
DPDK interface from that NUMA node added to OVS."
Which, now I read it should be modified to actually reflect that. What do
you think?
[Darrell] see above comment
>
> + This will result in reduced
> + maximum throughput on that device. In the case such a queue
> assignment
> + is made a warning message will be logged: "There's no available
(non-
> + isolated) pmd thread on numa node N. Queue Q on port P will be
> assigned to
> + the pmd on core C (numa node N'). Expect reduced performance."
> +
> - QEMU vCPU thread Affinity
>
> A VM performing simple packet forwarding or running complex packet
> pipelines
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index b3a0806..34f1963 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3149,10 +3149,13 @@ rr_numa_list_lookup(struct rr_numa_list *rr,
> int numa_id)
> }
>
> static void
> -rr_numa_list_populate(struct dp_netdev *dp, struct rr_numa_list *rr)
> +rr_numa_list_populate(struct dp_netdev *dp, struct rr_numa_list *rr,
> + int *all_numa_ids, unsigned all_numa_ids_sz,
> + int *num_ids_written)
> {
> struct dp_netdev_pmd_thread *pmd;
> struct rr_numa *numa;
> + unsigned idx = 0;
>
> hmap_init(&rr->numas);
>
> @@ -3170,7 +3173,11 @@ rr_numa_list_populate(struct dp_netdev *dp,
> struct rr_numa_list *rr)
> numa->n_pmds++;
> numa->pmds = xrealloc(numa->pmds, numa->n_pmds * sizeof
> *numa->pmds);
> numa->pmds[numa->n_pmds - 1] = pmd;
> +
> + all_numa_ids[idx % all_numa_ids_sz] = pmd->numa_id;
> + idx++;
> }
> + *num_ids_written = idx;
> }
>
> static struct dp_netdev_pmd_thread *
> @@ -3202,8 +3209,15 @@ rxq_scheduling(struct dp_netdev *dp, bool
> pinned) OVS_REQUIRES(dp->port_mutex)
> {
> struct dp_netdev_port *port;
> struct rr_numa_list rr;
> + int all_numa_ids [64];
> + int all_numa_ids_sz = sizeof all_numa_ids / sizeof
all_numa_ids[0];
> + unsigned all_numa_ids_idx = 0;
> + int all_numa_ids_max_idx = 0;
> + int num_numa_ids = 0;
>
> - rr_numa_list_populate(dp, &rr);
> + rr_numa_list_populate(dp, &rr, all_numa_ids, all_numa_ids_sz,
> + &num_numa_ids);
> + all_numa_ids_max_idx = MIN(num_numa_ids - 1, all_numa_ids_sz -
1);
>
> HMAP_FOR_EACH (port, node, &dp->ports) {
> struct rr_numa *numa;
> @@ -3234,10 +3248,29 @@ rxq_scheduling(struct dp_netdev *dp, bool
> pinned) OVS_REQUIRES(dp->port_mutex)
> }
> } else if (!pinned && q->core_id == OVS_CORE_UNSPEC) {
> if (!numa) {
> - VLOG_WARN("There's no available (non isolated)
pmd thread "
> + if (all_numa_ids_max_idx < 0) {
> + VLOG_ERR("There is no available
(non-isolated) pmd "
> + "thread for port \'%s\' queue %d.
This queue "
> + "will not be polled. Is
pmd-cpu-mask set to "
> + "zero? Or are all PMDs isolated to
other "
> + "queues?",
netdev_get_name(port->netdev),
> + qid);
> + continue;
> + }
> + int alt_numa_id = all_numa_ids[all_numa_ids_idx];
> + struct rr_numa *alt_numa;
> + alt_numa = rr_numa_list_lookup(&rr, alt_numa_id);
> + q->pmd = rr_numa_get_pmd(alt_numa);
> + VLOG_WARN("There's no available (non-isolated)
pmd thread "
> "on numa node %d. Queue %d on port
\'%s\' will "
> - "not be polled.",
> - numa_id, qid,
netdev_get_name(port->netdev));
> + "be assigned to the pmd on core %d "
> + "(numa node %d). Expect reduced
performance.",
> + numa_id, qid,
netdev_get_name(port->netdev),
> + q->pmd->core_id, q->pmd->numa_id);
> + all_numa_ids_idx++;
> + if (all_numa_ids_idx > all_numa_ids_max_idx) {
> + all_numa_ids_idx = 0;
> + }
> } else {
> q->pmd = rr_numa_get_pmd(numa);
> }
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> [email protected]
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=gjJwFxs-cPdXvKIlpDPy6Wk6vbKVeJuDjxVKrmLbI_E&s=iK8lUsArOqfRzfqIj6IYOGIAO03YY-a3eKUDZWt18Ig&e=
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
