Re: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
On 14/07/2021 10:05, Jan Scheurich wrote: >>> In our patch series we decided to skip the check on cross-numa polling >>> during >> auto-load balancing. The rationale is as follows: >>> >>> If the estimated PMD-rxq distribution includes cross-NUMA rxq assignments, >> the same must apply for the current distribution, as none of the scheduling >> algorithms would voluntarily assign rxqs across NUMA nodes. So, current and >> estimated rxq assignments are comparable and it makes sense to consider >> rebalancing when the variance improves. >>> >>> Please consider removing this check. >>> >> >> The first thing is that this patch is not changing any behaviour, just re- >> implementing to reuse the common code, so it would not be the place to >> change this functionality. > > Fair enough. We should address this in a separate patch. > >> About the proposed change itself, just to be clear what is allowed >> currently. It >> will allow rebalance when there are local pmds, OR there are no local pmds >> and there is one other NUMA node with pmds available for cross-numa polling. >> >> The rationale of not doing a rebalance when there are no local pmds but >> multiple other NUMAs available for cross-NUMA polling is that the estimate >> may be incorrect due a different cross-NUMA being choosen for an Rxq than is >> currently used. >> >> I thought about some things like making an Rxq sticky with a particular >> cross- >> NUMA etc for this case but that brings a whole new set of problems, e.g. what >> happens if that NUMA gets overloaded, reduced cores, how can it ever be reset >> etc. so I decided not to pursue it as I think it is probably a corner case >> (at least >> for now). > > We currently don't see any scenarios with more than two NUMA nodes, but > different CPU/server architectures may perhaps have more NUMA nodes than CPU > sockets. > >> I know the case of no local pmd and one NUMA with pmds is not a corner case >> as I'm aware of users doing that. > > Agree such configurations are a must to support with auto-lb. > >> We can discuss further about the multiple non-local NUMA case and maybe >> there's some improvements we can think of, or maybe I've made some wrong >> assumptions but it would be a follow on from the current patchset. > > Our main use case for cross-NUMA balancing comes with the additional freedom > to allow cross-NUMA polling for selected ports that we introduce with fourth > patch: > > dpif-netdev: Allow cross-NUMA polling on selected ports > > Today dpif-netdev considers PMD threads on a non-local NUMA node for > automatic assignment of the rxqs of a port only if there are no local, > non-isolated PMDs. > > On typical servers with both physical ports on one NUMA node, this often > leaves the PMDs on the other NUMA node under-utilized, wasting CPU > resources. The alternative, to manually pin the rxqs to PMDs on remote > NUMA nodes, also has drawbacks as it limits OVS' ability to auto > load-balance the rxqs. > > This patch introduces a new interface configuration option to allow > ports to be automatically polled by PMDs on any NUMA node: > > ovs-vsctl set interface other_config:cross-numa-polling=true > > If this option is not present or set to false, legacy behaviour applies. > > We indeed use this for our physical ports to be polled by non-isolated PMDs > on both NUMAs. The observed capacity improvement is very substantial, so we > plan to port this feature on top of your patches once they are merged. > > This can only fly if the auto-load balancing is allowed to activate rxq > assignments with cross-numa polling also in the case there are local > non-isolated PMDs. > > Anyway, we can take this up later in our upcoming patch that introduces this > option. > Sure. I agree there might be an opportunity for using some unused resources there. The challenge is to have somewhat predictable estimates etc. but yes, let's discuss later with that patch. thanks, Kevin. > BR, Jan > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
> > In our patch series we decided to skip the check on cross-numa polling > > during > auto-load balancing. The rationale is as follows: > > > > If the estimated PMD-rxq distribution includes cross-NUMA rxq assignments, > the same must apply for the current distribution, as none of the scheduling > algorithms would voluntarily assign rxqs across NUMA nodes. So, current and > estimated rxq assignments are comparable and it makes sense to consider > rebalancing when the variance improves. > > > > Please consider removing this check. > > > > The first thing is that this patch is not changing any behaviour, just re- > implementing to reuse the common code, so it would not be the place to > change this functionality. Fair enough. We should address this in a separate patch. > About the proposed change itself, just to be clear what is allowed currently. > It > will allow rebalance when there are local pmds, OR there are no local pmds > and there is one other NUMA node with pmds available for cross-numa polling. > > The rationale of not doing a rebalance when there are no local pmds but > multiple other NUMAs available for cross-NUMA polling is that the estimate > may be incorrect due a different cross-NUMA being choosen for an Rxq than is > currently used. > > I thought about some things like making an Rxq sticky with a particular cross- > NUMA etc for this case but that brings a whole new set of problems, e.g. what > happens if that NUMA gets overloaded, reduced cores, how can it ever be reset > etc. so I decided not to pursue it as I think it is probably a corner case > (at least > for now). We currently don't see any scenarios with more than two NUMA nodes, but different CPU/server architectures may perhaps have more NUMA nodes than CPU sockets. > I know the case of no local pmd and one NUMA with pmds is not a corner case > as I'm aware of users doing that. Agree such configurations are a must to support with auto-lb. > We can discuss further about the multiple non-local NUMA case and maybe > there's some improvements we can think of, or maybe I've made some wrong > assumptions but it would be a follow on from the current patchset. Our main use case for cross-NUMA balancing comes with the additional freedom to allow cross-NUMA polling for selected ports that we introduce with fourth patch: dpif-netdev: Allow cross-NUMA polling on selected ports Today dpif-netdev considers PMD threads on a non-local NUMA node for automatic assignment of the rxqs of a port only if there are no local, non-isolated PMDs. On typical servers with both physical ports on one NUMA node, this often leaves the PMDs on the other NUMA node under-utilized, wasting CPU resources. The alternative, to manually pin the rxqs to PMDs on remote NUMA nodes, also has drawbacks as it limits OVS' ability to auto load-balance the rxqs. This patch introduces a new interface configuration option to allow ports to be automatically polled by PMDs on any NUMA node: ovs-vsctl set interface other_config:cross-numa-polling=true If this option is not present or set to false, legacy behaviour applies. We indeed use this for our physical ports to be polled by non-isolated PMDs on both NUMAs. The observed capacity improvement is very substantial, so we plan to port this feature on top of your patches once they are merged. This can only fly if the auto-load balancing is allowed to activate rxq assignments with cross-numa polling also in the case there are local non-isolated PMDs. Anyway, we can take this up later in our upcoming patch that introduces this option. BR, Jan ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
Hi Jan,
On 13/07/2021 09:38, Jan Scheurich wrote:
>> -Original Message-
>> From: dev On Behalf Of Kevin Traynor
>> Sent: Thursday, 8 July, 2021 15:54
>> To: d...@openvswitch.org
>> Cc: david.march...@redhat.com
>> Subject: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance
>> use common rxq scheduling.
>>
>> PMD auto load balance had its own separate implementation of the rxq
>> scheduling that it used for dry runs. This was done because previously the
>> rxq
>> scheduling was not made reusable for a dry run.
>>
>> Apart from the code duplication (which is a good enough reason to replace it
>> alone) this meant that if any further rxq scheduling changes or assignment
>> types were added they would also have to be duplicated in the auto load
>> balance code too.
>>
>> This patch replaces the current PMD auto load balance rxq scheduling code to
>> reuse the common rxq scheduling code.
>>
>> The behaviour does not change from a user perspective, except the logs are
>> updated to be more consistent.
>>
>> As the dry run will compare the pmd load variances for current and estimated
>> assignments, new functions are added to populate the current assignments and
>> use the rxq scheduling data structs for variance calculations.
>>
>> Now that the new rxq scheduling data structures are being used in PMD auto
>> load balance, the older rr_* data structs and associated functions can be
>> removed.
>>
>> Signed-off-by: Kevin Traynor
>> ---
>> lib/dpif-netdev.c | 508 +++---
>> 1 file changed, 161 insertions(+), 347 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index beafa00a0..338ffd971
>> 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -4903,138 +4903,4 @@ port_reconfigure(struct dp_netdev_port *port) }
>>
>> -struct rr_numa_list {
>> -struct hmap numas; /* Contains 'struct rr_numa' */
>> -};
>> -
>> -struct rr_numa {
>> -struct hmap_node node;
>> -
>> -int numa_id;
>> -
>> -/* Non isolated pmds on numa node 'numa_id' */
>> -struct dp_netdev_pmd_thread **pmds;
>> -int n_pmds;
>> -
>> -int cur_index;
>> -bool idx_inc;
>> -};
>> -
>> -static size_t
>> -rr_numa_list_count(struct rr_numa_list *rr) -{
>> -return hmap_count(>numas);
>> -}
>> -
>> -static struct rr_numa *
>> -rr_numa_list_lookup(struct rr_numa_list *rr, int numa_id) -{
>> -struct rr_numa *numa;
>> -
>> -HMAP_FOR_EACH_WITH_HASH (numa, node, hash_int(numa_id, 0),
>>> numas) {
>> -if (numa->numa_id == numa_id) {
>> -return numa;
>> -}
>> -}
>> -
>> -return NULL;
>> -}
>> -
>> -/* Returns the next node in numa list following 'numa' in round-robin
>> fashion.
>> - * Returns first node if 'numa' is a null pointer or the last node in 'rr'.
>> - * Returns NULL if 'rr' numa list is empty. */ -static struct rr_numa * -
>> rr_numa_list_next(struct rr_numa_list *rr, const struct rr_numa *numa) -{
>> -struct hmap_node *node = NULL;
>> -
>> -if (numa) {
>> -node = hmap_next(>numas, >node);
>> -}
>> -if (!node) {
>> -node = hmap_first(>numas);
>> -}
>> -
>> -return (node) ? CONTAINER_OF(node, struct rr_numa, node) : NULL;
>> -}
>> -
>> -static void
>> -rr_numa_list_populate(struct dp_netdev *dp, struct rr_numa_list *rr) -{
>> -struct dp_netdev_pmd_thread *pmd;
>> -struct rr_numa *numa;
>> -
>> -hmap_init(>numas);
>> -
>> -CMAP_FOR_EACH (pmd, node, >poll_threads) {
>> -if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) {
>> -continue;
>> -}
>> -
>> -numa = rr_numa_list_lookup(rr, pmd->numa_id);
>> -if (!numa) {
>> -numa = xzalloc(sizeof *numa);
>> -numa->numa_id = pmd->numa_id;
>> -hmap_insert(>numas, >node, hash_int(pmd->numa_id, 0));
>> -}
>> -numa->n_pmds++;
>> -numa->pmds = xrealloc(numa->pmds, numa->n_pmds * sizeof *numa-
>>> pmds);
>> -numa->pmds[numa->n_pmds - 1] = pmd;
>> -/* At least one pmd so initialise curr_idx and idx_inc. */
>> -numa->cur_index = 0;
>> -numa->idx_inc = true;
&g
Re: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
> -Original Message-
> From: dev On Behalf Of Kevin Traynor
> Sent: Thursday, 8 July, 2021 15:54
> To: d...@openvswitch.org
> Cc: david.march...@redhat.com
> Subject: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance
> use common rxq scheduling.
>
> PMD auto load balance had its own separate implementation of the rxq
> scheduling that it used for dry runs. This was done because previously the rxq
> scheduling was not made reusable for a dry run.
>
> Apart from the code duplication (which is a good enough reason to replace it
> alone) this meant that if any further rxq scheduling changes or assignment
> types were added they would also have to be duplicated in the auto load
> balance code too.
>
> This patch replaces the current PMD auto load balance rxq scheduling code to
> reuse the common rxq scheduling code.
>
> The behaviour does not change from a user perspective, except the logs are
> updated to be more consistent.
>
> As the dry run will compare the pmd load variances for current and estimated
> assignments, new functions are added to populate the current assignments and
> use the rxq scheduling data structs for variance calculations.
>
> Now that the new rxq scheduling data structures are being used in PMD auto
> load balance, the older rr_* data structs and associated functions can be
> removed.
>
> Signed-off-by: Kevin Traynor
> ---
> lib/dpif-netdev.c | 508 +++---
> 1 file changed, 161 insertions(+), 347 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index beafa00a0..338ffd971
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4903,138 +4903,4 @@ port_reconfigure(struct dp_netdev_port *port) }
>
> -struct rr_numa_list {
> -struct hmap numas; /* Contains 'struct rr_numa' */
> -};
> -
> -struct rr_numa {
> -struct hmap_node node;
> -
> -int numa_id;
> -
> -/* Non isolated pmds on numa node 'numa_id' */
> -struct dp_netdev_pmd_thread **pmds;
> -int n_pmds;
> -
> -int cur_index;
> -bool idx_inc;
> -};
> -
> -static size_t
> -rr_numa_list_count(struct rr_numa_list *rr) -{
> -return hmap_count(>numas);
> -}
> -
> -static struct rr_numa *
> -rr_numa_list_lookup(struct rr_numa_list *rr, int numa_id) -{
> -struct rr_numa *numa;
> -
> -HMAP_FOR_EACH_WITH_HASH (numa, node, hash_int(numa_id, 0),
> >numas) {
> -if (numa->numa_id == numa_id) {
> -return numa;
> -}
> -}
> -
> -return NULL;
> -}
> -
> -/* Returns the next node in numa list following 'numa' in round-robin
> fashion.
> - * Returns first node if 'numa' is a null pointer or the last node in 'rr'.
> - * Returns NULL if 'rr' numa list is empty. */ -static struct rr_numa * -
> rr_numa_list_next(struct rr_numa_list *rr, const struct rr_numa *numa) -{
> -struct hmap_node *node = NULL;
> -
> -if (numa) {
> -node = hmap_next(>numas, >node);
> -}
> -if (!node) {
> -node = hmap_first(>numas);
> -}
> -
> -return (node) ? CONTAINER_OF(node, struct rr_numa, node) : NULL;
> -}
> -
> -static void
> -rr_numa_list_populate(struct dp_netdev *dp, struct rr_numa_list *rr) -{
> -struct dp_netdev_pmd_thread *pmd;
> -struct rr_numa *numa;
> -
> -hmap_init(>numas);
> -
> -CMAP_FOR_EACH (pmd, node, >poll_threads) {
> -if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) {
> -continue;
> -}
> -
> -numa = rr_numa_list_lookup(rr, pmd->numa_id);
> -if (!numa) {
> -numa = xzalloc(sizeof *numa);
> -numa->numa_id = pmd->numa_id;
> -hmap_insert(>numas, >node, hash_int(pmd->numa_id, 0));
> -}
> -numa->n_pmds++;
> -numa->pmds = xrealloc(numa->pmds, numa->n_pmds * sizeof *numa-
> >pmds);
> -numa->pmds[numa->n_pmds - 1] = pmd;
> -/* At least one pmd so initialise curr_idx and idx_inc. */
> -numa->cur_index = 0;
> -numa->idx_inc = true;
> -}
> -}
> -
> -/*
> - * Returns the next pmd from the numa node.
> - *
> - * If 'updown' is 'true' it will alternate between selecting the next pmd in
> - * either an up or down walk, switching between up/down when the first or
> last
> - * core is reached. e.g. 1,2,3,3,2,1,1,2...
> - *
> - * If 'updown' is 'false' it will select the next pmd wrapping around when
> last
> - * core reached. e.g. 1,2,3,1,2,3,1,2...
> - */
> -static struct dp_netdev_pmd_thread *
&g
Re: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
On Thu, Jul 8, 2021 at 3:53 PM Kevin Traynor wrote: > > PMD auto load balance had its own separate implementation of the > rxq scheduling that it used for dry runs. This was done because > previously the rxq scheduling was not made reusable for a dry run. > > Apart from the code duplication (which is a good enough reason > to replace it alone) this meant that if any further rxq scheduling > changes or assignment types were added they would also have to be > duplicated in the auto load balance code too. > > This patch replaces the current PMD auto load balance rxq scheduling > code to reuse the common rxq scheduling code. > > The behaviour does not change from a user perspective, except the logs > are updated to be more consistent. > > As the dry run will compare the pmd load variances for current and > estimated assignments, new functions are added to populate the current > assignments and use the rxq scheduling data structs for variance > calculations. > > Now that the new rxq scheduling data structures are being used in > PMD auto load balance, the older rr_* data structs and associated > functions can be removed. > > Signed-off-by: Kevin Traynor Acked-by: David Marchand -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
Hi Kevin, > Subject: [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use > common rxq scheduling. > > PMD auto load balance had its own separate implementation of the rxq > scheduling that it used for dry runs. This was done because previously the > rxq scheduling was not made reusable for a dry run. > > Apart from the code duplication (which is a good enough reason to replace it > alone) this meant that if any further rxq scheduling changes or assignment > types were added they would also have to be duplicated in the auto load > balance code too. > > This patch replaces the current PMD auto load balance rxq scheduling code to > reuse the common rxq scheduling code. > > The behaviour does not change from a user perspective, except the logs are > updated to be more consistent. > > As the dry run will compare the pmd load variances for current and estimated > assignments, new functions are added to populate the current assignments > and use the rxq scheduling data structs for variance calculations. > > Now that the new rxq scheduling data structures are being used in PMD auto > load balance, the older rr_* data structs and associated functions can be > removed. > > Signed-off-by: Kevin Traynor > --- > lib/dpif-netdev.c | 508 +++--- > 1 file changed, 161 insertions(+), 347 deletions(-) LGTM. Acked-by: Sunil Pai G ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 2/7] dpif-netdev: Make PMD auto load balance use common rxq scheduling.
PMD auto load balance had its own separate implementation of the
rxq scheduling that it used for dry runs. This was done because
previously the rxq scheduling was not made reusable for a dry run.
Apart from the code duplication (which is a good enough reason
to replace it alone) this meant that if any further rxq scheduling
changes or assignment types were added they would also have to be
duplicated in the auto load balance code too.
This patch replaces the current PMD auto load balance rxq scheduling
code to reuse the common rxq scheduling code.
The behaviour does not change from a user perspective, except the logs
are updated to be more consistent.
As the dry run will compare the pmd load variances for current and
estimated assignments, new functions are added to populate the current
assignments and use the rxq scheduling data structs for variance
calculations.
Now that the new rxq scheduling data structures are being used in
PMD auto load balance, the older rr_* data structs and associated
functions can be removed.
Signed-off-by: Kevin Traynor
---
lib/dpif-netdev.c | 508 +++---
1 file changed, 161 insertions(+), 347 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index beafa00a0..338ffd971 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4903,138 +4903,4 @@ port_reconfigure(struct dp_netdev_port *port)
}
-struct rr_numa_list {
-struct hmap numas; /* Contains 'struct rr_numa' */
-};
-
-struct rr_numa {
-struct hmap_node node;
-
-int numa_id;
-
-/* Non isolated pmds on numa node 'numa_id' */
-struct dp_netdev_pmd_thread **pmds;
-int n_pmds;
-
-int cur_index;
-bool idx_inc;
-};
-
-static size_t
-rr_numa_list_count(struct rr_numa_list *rr)
-{
-return hmap_count(>numas);
-}
-
-static struct rr_numa *
-rr_numa_list_lookup(struct rr_numa_list *rr, int numa_id)
-{
-struct rr_numa *numa;
-
-HMAP_FOR_EACH_WITH_HASH (numa, node, hash_int(numa_id, 0), >numas) {
-if (numa->numa_id == numa_id) {
-return numa;
-}
-}
-
-return NULL;
-}
-
-/* Returns the next node in numa list following 'numa' in round-robin fashion.
- * Returns first node if 'numa' is a null pointer or the last node in 'rr'.
- * Returns NULL if 'rr' numa list is empty. */
-static struct rr_numa *
-rr_numa_list_next(struct rr_numa_list *rr, const struct rr_numa *numa)
-{
-struct hmap_node *node = NULL;
-
-if (numa) {
-node = hmap_next(>numas, >node);
-}
-if (!node) {
-node = hmap_first(>numas);
-}
-
-return (node) ? CONTAINER_OF(node, struct rr_numa, node) : NULL;
-}
-
-static void
-rr_numa_list_populate(struct dp_netdev *dp, struct rr_numa_list *rr)
-{
-struct dp_netdev_pmd_thread *pmd;
-struct rr_numa *numa;
-
-hmap_init(>numas);
-
-CMAP_FOR_EACH (pmd, node, >poll_threads) {
-if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) {
-continue;
-}
-
-numa = rr_numa_list_lookup(rr, pmd->numa_id);
-if (!numa) {
-numa = xzalloc(sizeof *numa);
-numa->numa_id = pmd->numa_id;
-hmap_insert(>numas, >node, hash_int(pmd->numa_id, 0));
-}
-numa->n_pmds++;
-numa->pmds = xrealloc(numa->pmds, numa->n_pmds * sizeof *numa->pmds);
-numa->pmds[numa->n_pmds - 1] = pmd;
-/* At least one pmd so initialise curr_idx and idx_inc. */
-numa->cur_index = 0;
-numa->idx_inc = true;
-}
-}
-
-/*
- * Returns the next pmd from the numa node.
- *
- * If 'updown' is 'true' it will alternate between selecting the next pmd in
- * either an up or down walk, switching between up/down when the first or last
- * core is reached. e.g. 1,2,3,3,2,1,1,2...
- *
- * If 'updown' is 'false' it will select the next pmd wrapping around when last
- * core reached. e.g. 1,2,3,1,2,3,1,2...
- */
-static struct dp_netdev_pmd_thread *
-rr_numa_get_pmd(struct rr_numa *numa, bool updown)
-{
-int numa_idx = numa->cur_index;
-
-if (numa->idx_inc == true) {
-/* Incrementing through list of pmds. */
-if (numa->cur_index == numa->n_pmds-1) {
-/* Reached the last pmd. */
-if (updown) {
-numa->idx_inc = false;
-} else {
-numa->cur_index = 0;
-}
-} else {
-numa->cur_index++;
-}
-} else {
-/* Decrementing through list of pmds. */
-if (numa->cur_index == 0) {
-/* Reached the first pmd. */
-numa->idx_inc = true;
-} else {
-numa->cur_index--;
-}
-}
-return numa->pmds[numa_idx];
-}
-
-static void
-rr_numa_list_destroy(struct rr_numa_list *rr)
-{
-struct rr_numa *numa;
-
-HMAP_FOR_EACH_POP (numa, node, >numas) {
-free(numa->pmds);
-free(numa);
-}
-hmap_destroy(>numas);
-}
-
struct sched_numa_list {
struct hmap numas; /* Contains
