| Message ID | 20211008101931.507718-1-ktraynor@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Sync PMD ALB state with user commands. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi Kevin, Patch LGTM in general, just a query below. <snipped> > + * This function checks that some basic conditions needed for a > +rebalance to be > + * effective are met. Such as Rxq scheduling assignment type, more than > +one > + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration > +change > + * since the last check, it reuses the last result. > + * > + * It is not intended to be an inclusive check of every condition that > +may make > + * a rebalance ineffective. It is done as a quick check so a full > + * pmd_rebalance_dry_run() can be avoided when it is not needed. > + */ > +static bool > +pmd_reblance_dry_run_needed(struct dp_netdev *dp) > + OVS_REQUIRES(dp->port_mutex) > +{ > + struct dp_netdev_pmd_thread *pmd; > + struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; > + unsigned int cnt = 0; > + bool multi_rxq = false; > + > + /* Check if there was no reconfiguration since last check. */ > + if (!pmd_alb->recheck_config) { > + if (!pmd_alb->do_dry_run) { > + VLOG_DBG("PMD auto load balance nothing to do, " > + "no configuration changes since last check."); > + return false; > + } > + return true; > + } > + pmd_alb->recheck_config = false; > + > + /* Check for incompatible assignment type. */ > + if (dp->pmd_rxq_assign_type == SCHED_ROUNDROBIN) { > + VLOG_DBG("PMD auto load balance nothing to do, " > + "pmd-rxq-assign=roundrobin assignment type configured."); > + return pmd_alb->do_dry_run = false; > + } > + > + /* Check that there is at least 2 non-isolated PMDs and > + * one of them is polling more than one rxq. */ > + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { > + if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) { > + continue; > + } > + > + if (hmap_count(&pmd->poll_list) > 1) { > + multi_rxq = true; > + } > + if (cnt && multi_rxq) { > + return pmd_alb->do_dry_run = true; > + } > + cnt++; > + } > + > + VLOG_DBG("PMD auto load balance nothing to do, " > + "not enough non-isolated PMDs or RxQs."); > + return pmd_alb->do_dry_run = false; } > + I wonder if the logs in the above function be INFO instead of DBG ? Don't have a strong preference TBH. But if they were info, we need not remove any test cases no ? <snipped> Thanks and regards, Sunil
On 18/10/2021 13:03, Pai G, Sunil wrote: > Hi Kevin, > Hi Sunil, Thanks for reviewing. > Patch LGTM in general, just a query below. > <snipped> > >> + * This function checks that some basic conditions needed for a >> +rebalance to be >> + * effective are met. Such as Rxq scheduling assignment type, more than >> +one >> + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration >> +change >> + * since the last check, it reuses the last result. >> + * >> + * It is not intended to be an inclusive check of every condition that >> +may make >> + * a rebalance ineffective. It is done as a quick check so a full >> + * pmd_rebalance_dry_run() can be avoided when it is not needed. >> + */ >> +static bool >> +pmd_reblance_dry_run_needed(struct dp_netdev *dp) >> + OVS_REQUIRES(dp->port_mutex) >> +{ >> + struct dp_netdev_pmd_thread *pmd; >> + struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; >> + unsigned int cnt = 0; >> + bool multi_rxq = false; >> + >> + /* Check if there was no reconfiguration since last check. */ >> + if (!pmd_alb->recheck_config) { >> + if (!pmd_alb->do_dry_run) { >> + VLOG_DBG("PMD auto load balance nothing to do, " >> + "no configuration changes since last check."); >> + return false; >> + } >> + return true; >> + } >> + pmd_alb->recheck_config = false; >> + >> + /* Check for incompatible assignment type. */ >> + if (dp->pmd_rxq_assign_type == SCHED_ROUNDROBIN) { >> + VLOG_DBG("PMD auto load balance nothing to do, " >> + "pmd-rxq-assign=roundrobin assignment type configured."); >> + return pmd_alb->do_dry_run = false; >> + } >> + >> + /* Check that there is at least 2 non-isolated PMDs and >> + * one of them is polling more than one rxq. */ >> + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { >> + if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) { >> + continue; >> + } >> + >> + if (hmap_count(&pmd->poll_list) > 1) { >> + multi_rxq = true; >> + } >> + if (cnt && multi_rxq) { >> + return pmd_alb->do_dry_run = true; >> + } >> + cnt++; >> + } >> + >> + VLOG_DBG("PMD auto load balance nothing to do, " >> + "not enough non-isolated PMDs or RxQs."); >> + return pmd_alb->do_dry_run = false; } >> + > > I wonder if the logs in the above function be INFO instead of DBG ? > Don't have a strong preference TBH. But if they were info, we need not remove any test cases no ? > The reason they are debug is so they won't pollute the logs when a rebalance cannot occur. For example, if user sets ALB enabled and there is only one PMD. If it is at 100% loaded, this message would be printed every minute to say that a ALB will do nothing. As nothing is changing and it can occur every minute I thought it was more appropriate to keep it at debug level for when you need to check/debug the operation of ALB. WDYT? Kevin. > <snipped> > > Thanks and regards, > Sunil >
Hi Kevin, > > > > I wonder if the logs in the above function be INFO instead of DBG ? > > Don't have a strong preference TBH. But if they were info, we need not > remove any test cases no ? > > > > The reason they are debug is so they won't pollute the logs when a rebalance > cannot occur Yup, agree, was just thinking if there is a way to not remove any testcases :) > > For example, if user sets ALB enabled and there is only one PMD. If it is at 100% > loaded, this message would be printed every minute to say that a ALB will do > nothing. > > As nothing is changing and it can occur every minute I thought it was more > appropriate to keep it at debug level for when you need to check/debug the > operation of ALB. WDYT? Agreed :) Thanks! Acked-by: Sunil Pai G <sunil.pai.g@intel.com>
On 18/10/2021 17:25, Pai G, Sunil wrote: > Hi Kevin, > >>> >>> I wonder if the logs in the above function be INFO instead of DBG ? >>> Don't have a strong preference TBH. But if they were info, we need not >> remove any test cases no ? >>> >> >> The reason they are debug is so they won't pollute the logs when a rebalance >> cannot occur > > Yup, agree, was just thinking if there is a way to not remove any testcases :) > At the moment, no, but it's more to do with not having cycle measurements and it is something to look into in general. info or debug won't impact this as debug could be enabled during the test if required. >> >> For example, if user sets ALB enabled and there is only one PMD. If it is at 100% >> loaded, this message would be printed every minute to say that a ALB will do >> nothing. >> >> As nothing is changing and it can occur every minute I thought it was more >> appropriate to keep it at debug level for when you need to check/debug the >> operation of ALB. WDYT? > > Agreed :) > Thanks! > > Acked-by: Sunil Pai G <sunil.pai.g@intel.com> > Thanks for reviewing.
On Fri, Oct 8, 2021 at 12:19 PM Kevin Traynor <ktraynor@redhat.com> wrote: > > Previously, when a user enabled PMD auto load balancer with > pmd-auto-lb="true", some conditions such as number of PMDs/RxQs > that were required for a rebalance to take place were checked. > > If the configuration meant that a rebalance would not take place > then PMD ALB was logged as 'disabled' and not run. > > Later, if the PMD/RxQ configuration changed whereby a rebalance > could be effective, PMD ALB was logged as 'enabled' and would run at > the appropriate time. > > This worked ok from a functional view but it is unintuitive for the > user reading the logs. > > e.g. with one PMD (PMD ALB would not be effective) > > User enables ALB, but logs say it is disabled because it won't run. > $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true" > |dpif_netdev|INFO|PMD auto load balance is disabled > > No dry run takes place. > > Add more PMDs (PMD ALB may be effective). > $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50 > |dpif_netdev|INFO|PMD auto load balance is enabled ... > > Dry run takes place. > |dpif_netdev|DBG|PMD auto load balance performing dry run. > > A better approach is to simply reflect back the user enable/disable > state in the logs and deal with whether the rebalance will be effective > when needed. That is the approach taken in this patch. > > To cut down on unneccessary work, some basic checks are also made before > starting a PMD ALB dry run and debug logs can indicate this to the user. > > e.g. with one PMD (PMD ALB would not be effective) > > User enables ALB, and logs confirm the user has enabled it. > $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true" > |dpif_netdev|INFO|PMD auto load balance is enabled... > > No dry run takes place. > |dpif_netdev|DBG|PMD auto load balance nothing to do, not enough non-isolated PMDs or RxQs. > > Add more PMDs (PMD ALB may be effective). > $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50 > > Dry run takes place. > |dpif_netdev|DBG|PMD auto load balance performing dry run. > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > Reviewed-by: David Marchand <david.marchand@redhat.com> Two nits below: [snip] > - if (pmd_alb->is_enabled != enable_alb || always_log) { > - pmd_alb->is_enabled = enable_alb; > + if (pmd_alb->is_enabled != state || always_log) { > + pmd_alb->is_enabled = state; > if (pmd_alb->is_enabled) { > + uint8_t rebalance_load_thresh; > + > atomic_read_relaxed(&pmd_alb->rebalance_load_thresh, > &rebalance_load_thresh); > - VLOG_INFO("PMD auto load balance is enabled " > + VLOG_INFO("PMD auto load balance is enabled. " Nit: I'd either let this log as is, or otherwise put a ',' here, and finish this log with a '.' after other infos. > "interval %"PRIu64" mins, " > "pmd load threshold %"PRIu8"%%, " [snip] > @@ -5482,4 +5453,62 @@ sched_numa_list_variance(struct sched_numa_list *numa_list) > } > > +/* > + * This function checks that some basic conditions needed for a rebalance to be > + * effective are met. Such as Rxq scheduling assignment type, more than one > + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration change > + * since the last check, it reuses the last result. > + * > + * It is not intended to be an inclusive check of every condition that may make > + * a rebalance ineffective. It is done as a quick check so a full > + * pmd_rebalance_dry_run() can be avoided when it is not needed. > + */ > +static bool > +pmd_reblance_dry_run_needed(struct dp_netdev *dp) > + OVS_REQUIRES(dp->port_mutex) > +{ > + struct dp_netdev_pmd_thread *pmd; > + struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; > + unsigned int cnt = 0; > + bool multi_rxq = false; > + > + /* Check if there was no reconfiguration since last check. */ > + if (!pmd_alb->recheck_config) { > + if (!pmd_alb->do_dry_run) { > + VLOG_DBG("PMD auto load balance nothing to do, " > + "no configuration changes since last check."); Nit: extra space as indent. > + return false; > + } > + return true; > + }
On 29/10/2021 14:20, David Marchand wrote: > On Fri, Oct 8, 2021 at 12:19 PM Kevin Traynor <ktraynor@redhat.com> wrote: >> >> Previously, when a user enabled PMD auto load balancer with >> pmd-auto-lb="true", some conditions such as number of PMDs/RxQs >> that were required for a rebalance to take place were checked. >> >> If the configuration meant that a rebalance would not take place >> then PMD ALB was logged as 'disabled' and not run. >> >> Later, if the PMD/RxQ configuration changed whereby a rebalance >> could be effective, PMD ALB was logged as 'enabled' and would run at >> the appropriate time. >> >> This worked ok from a functional view but it is unintuitive for the >> user reading the logs. >> >> e.g. with one PMD (PMD ALB would not be effective) >> >> User enables ALB, but logs say it is disabled because it won't run. >> $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true" >> |dpif_netdev|INFO|PMD auto load balance is disabled >> >> No dry run takes place. >> >> Add more PMDs (PMD ALB may be effective). >> $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50 >> |dpif_netdev|INFO|PMD auto load balance is enabled ... >> >> Dry run takes place. >> |dpif_netdev|DBG|PMD auto load balance performing dry run. >> >> A better approach is to simply reflect back the user enable/disable >> state in the logs and deal with whether the rebalance will be effective >> when needed. That is the approach taken in this patch. >> >> To cut down on unneccessary work, some basic checks are also made before >> starting a PMD ALB dry run and debug logs can indicate this to the user. >> >> e.g. with one PMD (PMD ALB would not be effective) >> >> User enables ALB, and logs confirm the user has enabled it. >> $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true" >> |dpif_netdev|INFO|PMD auto load balance is enabled... >> >> No dry run takes place. >> |dpif_netdev|DBG|PMD auto load balance nothing to do, not enough non-isolated PMDs or RxQs. >> >> Add more PMDs (PMD ALB may be effective). >> $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50 >> >> Dry run takes place. >> |dpif_netdev|DBG|PMD auto load balance performing dry run. >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> > > Reviewed-by: David Marchand <david.marchand@redhat.com> > Hi David, thanks for reviewing. I sent a v2 with the below items fixed and review/ack tags. > Two nits below: > > [snip] > >> - if (pmd_alb->is_enabled != enable_alb || always_log) { >> - pmd_alb->is_enabled = enable_alb; >> + if (pmd_alb->is_enabled != state || always_log) { >> + pmd_alb->is_enabled = state; >> if (pmd_alb->is_enabled) { >> + uint8_t rebalance_load_thresh; >> + >> atomic_read_relaxed(&pmd_alb->rebalance_load_thresh, >> &rebalance_load_thresh); >> - VLOG_INFO("PMD auto load balance is enabled " >> + VLOG_INFO("PMD auto load balance is enabled. " > > Nit: I'd either let this log as is, or otherwise put a ',' here, and > finish this log with a '.' after other infos. > Went with the latter option. > >> "interval %"PRIu64" mins, " >> "pmd load threshold %"PRIu8"%%, " > > [snip] > >> @@ -5482,4 +5453,62 @@ sched_numa_list_variance(struct sched_numa_list *numa_list) >> } >> >> +/* >> + * This function checks that some basic conditions needed for a rebalance to be >> + * effective are met. Such as Rxq scheduling assignment type, more than one >> + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration change >> + * since the last check, it reuses the last result. >> + * >> + * It is not intended to be an inclusive check of every condition that may make >> + * a rebalance ineffective. It is done as a quick check so a full >> + * pmd_rebalance_dry_run() can be avoided when it is not needed. >> + */ >> +static bool >> +pmd_reblance_dry_run_needed(struct dp_netdev *dp) >> + OVS_REQUIRES(dp->port_mutex) >> +{ >> + struct dp_netdev_pmd_thread *pmd; >> + struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; >> + unsigned int cnt = 0; >> + bool multi_rxq = false; >> + >> + /* Check if there was no reconfiguration since last check. */ >> + if (!pmd_alb->recheck_config) { >> + if (!pmd_alb->do_dry_run) { >> + VLOG_DBG("PMD auto load balance nothing to do, " >> + "no configuration changes since last check."); > > Nit: extra space as indent. > Fixed. > >> + return false; >> + } >> + return true; >> + } > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 75f381ec1..cf6753f7b 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -217,5 +217,6 @@ struct dp_meter { struct pmd_auto_lb { - bool auto_lb_requested; /* Auto load balancing requested by user. */ + bool do_dry_run; + bool recheck_config; bool is_enabled; /* Current status of Auto load balancing. */ uint64_t rebalance_intvl; @@ -4139,42 +4140,16 @@ dpif_netdev_operate(struct dpif *dpif, struct dpif_op **ops, size_t n_ops, /* Enable or Disable PMD auto load balancing. */ static void -set_pmd_auto_lb(struct dp_netdev *dp, bool always_log) +set_pmd_auto_lb(struct dp_netdev *dp, bool state, bool always_log) { - unsigned int cnt = 0; - struct dp_netdev_pmd_thread *pmd; struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; - uint8_t rebalance_load_thresh; - bool enable_alb = false; - bool multi_rxq = false; - enum sched_assignment_type pmd_rxq_assign_type = dp->pmd_rxq_assign_type; - - /* Ensure that there is at least 2 non-isolated PMDs and - * one of them is polling more than one rxq. */ - CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { - if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) { - continue; - } - - if (hmap_count(&pmd->poll_list) > 1) { - multi_rxq = true; - } - if (cnt && multi_rxq) { - enable_alb = true; - break; - } - cnt++; - } - - /* Enable auto LB if requested and not using roundrobin assignment. */ - enable_alb = enable_alb && pmd_rxq_assign_type != SCHED_ROUNDROBIN && - pmd_alb->auto_lb_requested; - - if (pmd_alb->is_enabled != enable_alb || always_log) { - pmd_alb->is_enabled = enable_alb; + if (pmd_alb->is_enabled != state || always_log) { + pmd_alb->is_enabled = state; if (pmd_alb->is_enabled) { + uint8_t rebalance_load_thresh; + atomic_read_relaxed(&pmd_alb->rebalance_load_thresh, &rebalance_load_thresh); - VLOG_INFO("PMD auto load balance is enabled " + VLOG_INFO("PMD auto load balance is enabled. " "interval %"PRIu64" mins, " "pmd load threshold %"PRIu8"%%, " @@ -4183,8 +4158,7 @@ set_pmd_auto_lb(struct dp_netdev *dp, bool always_log) rebalance_load_thresh, pmd_alb->rebalance_improve_thresh); - } else { pmd_alb->rebalance_poll_timer = 0; - VLOG_INFO("PMD auto load balance is disabled"); + VLOG_INFO("PMD auto load balance is disabled."); } } @@ -4307,10 +4281,4 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; - bool cur_rebalance_requested = pmd_alb->auto_lb_requested; - pmd_alb->auto_lb_requested = smap_get_bool(other_config, "pmd-auto-lb", - false); - if (cur_rebalance_requested != pmd_alb->auto_lb_requested) { - log_autolb = true; - } rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval", @@ -4354,5 +4322,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) log_autolb = true; } - set_pmd_auto_lb(dp, log_autolb); + + bool autolb_state = smap_get_bool(other_config, "pmd-auto-lb", false); + + set_pmd_auto_lb(dp, autolb_state, log_autolb); return 0; } @@ -5482,4 +5453,62 @@ sched_numa_list_variance(struct sched_numa_list *numa_list) } +/* + * This function checks that some basic conditions needed for a rebalance to be + * effective are met. Such as Rxq scheduling assignment type, more than one + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration change + * since the last check, it reuses the last result. + * + * It is not intended to be an inclusive check of every condition that may make + * a rebalance ineffective. It is done as a quick check so a full + * pmd_rebalance_dry_run() can be avoided when it is not needed. + */ +static bool +pmd_reblance_dry_run_needed(struct dp_netdev *dp) + OVS_REQUIRES(dp->port_mutex) +{ + struct dp_netdev_pmd_thread *pmd; + struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; + unsigned int cnt = 0; + bool multi_rxq = false; + + /* Check if there was no reconfiguration since last check. */ + if (!pmd_alb->recheck_config) { + if (!pmd_alb->do_dry_run) { + VLOG_DBG("PMD auto load balance nothing to do, " + "no configuration changes since last check."); + return false; + } + return true; + } + pmd_alb->recheck_config = false; + + /* Check for incompatible assignment type. */ + if (dp->pmd_rxq_assign_type == SCHED_ROUNDROBIN) { + VLOG_DBG("PMD auto load balance nothing to do, " + "pmd-rxq-assign=roundrobin assignment type configured."); + return pmd_alb->do_dry_run = false; + } + + /* Check that there is at least 2 non-isolated PMDs and + * one of them is polling more than one rxq. */ + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { + if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) { + continue; + } + + if (hmap_count(&pmd->poll_list) > 1) { + multi_rxq = true; + } + if (cnt && multi_rxq) { + return pmd_alb->do_dry_run = true; + } + cnt++; + } + + VLOG_DBG("PMD auto load balance nothing to do, " + "not enough non-isolated PMDs or RxQs."); + return pmd_alb->do_dry_run = false; +} + static bool pmd_rebalance_dry_run(struct dp_netdev *dp) @@ -5866,6 +5895,6 @@ reconfigure_datapath(struct dp_netdev *dp) reload_affected_pmds(dp); - /* Check if PMD Auto LB is to be enabled */ - set_pmd_auto_lb(dp, false); + /* PMD ALB will need to recheck if dry run needed. */ + dp->pmd_alb.recheck_config = true; } @@ -5995,4 +6024,5 @@ dpif_netdev_run(struct dpif *dpif) !dp_netdev_is_reconf_required(dp) && !ports_require_restart(dp) && + pmd_reblance_dry_run_needed(dp) && pmd_rebalance_dry_run(dp)) { VLOG_INFO("PMD auto load balance dry run. " diff --git a/tests/alb.at b/tests/alb.at index 903238fcb..444711129 100644 --- a/tests/alb.at +++ b/tests/alb.at @@ -37,50 +37,17 @@ AT_CLEANUP AT_SETUP([ALB - enable/disable]) OVS_VSWITCHD_START([add-port br0 p0 \ - -- set Interface p0 type=dummy-pmd options:n_rxq=3 \ - -- set Open_vSwitch . other_config:pmd-cpu-mask=3 \ - -- set open_vswitch . other_config:pmd-auto-lb="true"], - [], [], [DUMMY_NUMA]) -OVS_WAIT_UNTIL([grep "PMD auto load balance is enabled" ovs-vswitchd.log]) - -get_log_next_line_num -AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) - -get_log_next_line_num -AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) - -OVS_VSWITCHD_STOP -AT_CLEANUP - -AT_SETUP([ALB - min num PMD/RxQ]) -OVS_VSWITCHD_START([add-port br0 p0 \ - -- set Interface p0 type=dummy-pmd options:n_rxq=2 \ + -- set Interface p0 type=dummy-pmd options:n_rxq=1 \ -- set Open_vSwitch . other_config:pmd-cpu-mask=1 \ -- set open_vswitch . other_config:pmd-auto-lb="true"], [], [], [DUMMY_NUMA]) -OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log]) +OVS_WAIT_UNTIL([grep "PMD auto load balance is enabled" ovs-vswitchd.log]) -# Add more PMD -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x3]) -OVS_WAIT_UNTIL([grep "There are 2 pmd threads on numa node" ovs-vswitchd.log]) - -# Add one more rxq to have 2 rxq on a PMD -get_log_next_line_num -AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=3]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) - -# Reduce PMD -get_log_next_line_num -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x1]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) - -# Check logs when try to enable but min PMD/RxQ prevents get_log_next_line_num AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"]) OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) + get_log_next_line_num AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) OVS_VSWITCHD_STOP @@ -89,6 +56,6 @@ AT_CLEANUP AT_SETUP([ALB - PMD/RxQ assignment type]) OVS_VSWITCHD_START([add-port br0 p0 \ - -- set Interface p0 type=dummy-pmd options:n_rxq=3 \ - -- set Open_vSwitch . other_config:pmd-cpu-mask=3 \ + -- set Interface p0 type=dummy-pmd options:n_rxq=1 \ + -- set Open_vSwitch . other_config:pmd-cpu-mask=1 \ -- set open_vswitch . other_config:pmd-auto-lb="true"], [], [], [DUMMY_NUMA]) @@ -99,5 +66,11 @@ get_log_next_line_num AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-rxq-assign=roundrobin]) OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "mode changed to: 'roundrobin'"]) + +get_log_next_line_num +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"]) OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) +get_log_next_line_num +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"]) +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) # Change back assignment type @@ -105,16 +78,22 @@ get_log_next_line_num AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-rxq-assign=cycles]) OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "mode changed to: 'cycles'"]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) -# Check logs when try to enable but assignment prevents get_log_next_line_num AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"]) OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) get_log_next_line_num -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-rxq-assign=roundrobin]) -OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "mode changed to: 'roundrobin'"]) -get_log_next_line_num AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"]) +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) + +get_log_next_line_num +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-rxq-assign=group]) +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "mode changed to: 'group'"]) + +get_log_next_line_num +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"]) OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is disabled"]) +get_log_next_line_num +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"]) +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load balance is enabled"]) OVS_VSWITCHD_STOP
Previously, when a user enabled PMD auto load balancer with pmd-auto-lb="true", some conditions such as number of PMDs/RxQs that were required for a rebalance to take place were checked. If the configuration meant that a rebalance would not take place then PMD ALB was logged as 'disabled' and not run. Later, if the PMD/RxQ configuration changed whereby a rebalance could be effective, PMD ALB was logged as 'enabled' and would run at the appropriate time. This worked ok from a functional view but it is unintuitive for the user reading the logs. e.g. with one PMD (PMD ALB would not be effective) User enables ALB, but logs say it is disabled because it won't run. $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true" |dpif_netdev|INFO|PMD auto load balance is disabled No dry run takes place. Add more PMDs (PMD ALB may be effective). $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50 |dpif_netdev|INFO|PMD auto load balance is enabled ... Dry run takes place. |dpif_netdev|DBG|PMD auto load balance performing dry run. A better approach is to simply reflect back the user enable/disable state in the logs and deal with whether the rebalance will be effective when needed. That is the approach taken in this patch. To cut down on unneccessary work, some basic checks are also made before starting a PMD ALB dry run and debug logs can indicate this to the user. e.g. with one PMD (PMD ALB would not be effective) User enables ALB, and logs confirm the user has enabled it. $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true" |dpif_netdev|INFO|PMD auto load balance is enabled... No dry run takes place. |dpif_netdev|DBG|PMD auto load balance nothing to do, not enough non-isolated PMDs or RxQs. Add more PMDs (PMD ALB may be effective). $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50 Dry run takes place. |dpif_netdev|DBG|PMD auto load balance performing dry run. Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- GH actions: https://github.com/kevintraynor/ovs/actions/runs/1319783173 --- lib/dpif-netdev.c | 118 +++++++++++++++++++++++++++++----------------- tests/alb.at | 67 +++++++++----------------- 2 files changed, 97 insertions(+), 88 deletions(-)