On 29/10/2021 14:20, David Marchand wrote:
On Fri, Oct 8, 2021 at 12:19 PM Kevin Traynor <[email protected]> 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 <[email protected]>


Reviewed-by: David Marchand <[email protected]>


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;
+    }



_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to