[ovs-dev] [PATCH 2/5] dpif-netdev: Make PMD auto load balance use common rxq scheduling.

[Kevin Traynor]

On 23/06/2021 12:21, Pai G, Sunil wrote:
> Hey Kevin , 
> 
> Patch looks good to me.
> Builds fine , all test cases here http://patchwork.ozlabs.org/project/openvswitch/patch/20210316154532.127858-1-ktraynor@redhat.com/ pass as well.
> 
> Some minor nits inline :
> 
> <snipped>
> 
>> +static bool
>> +pmd_rebalance_dry_run(struct dp_netdev *dp)
>> +    OVS_REQUIRES(dp->port_mutex)
>> +{
>> +    struct sched_numa_list *numa_list_cur;
>> +    struct sched_numa_list *numa_list_est;
>> +    bool thresh_met = false;
>> +    uint64_t current, estimate;
> 
> current and estimate aren't specific, may be current_var and estimate_var ?
> 

changed to *_var.

>> +    uint64_t improvement = 0;
>> +
>> +    VLOG_DBG("PMD auto load balance performing dry run.");
>> +
>> +    /* Populate current assignments. */
>> +    numa_list_cur = xzalloc(sizeof *numa_list_cur);
>> +    sched_numa_list_populate(numa_list_cur, dp);
>> +    sched_numa_list_assignments(numa_list_cur, dp);
>> +
>> +    /* Populate estimated assignments. */
>> +    numa_list_est = xzalloc(sizeof *numa_list_est);
>> +    sched_numa_list_populate(numa_list_est, dp);
>> +    sched_numa_list_schedule(numa_list_est, dp,
>> +                             dp->pmd_rxq_assign_cyc, VLL_DBG);
>> +
>> +    /* Check if cross-numa polling, there is only one numa with PMDs. */
>> +    if (!sched_numa_list_cross_numa_polling(numa_list_est) ||
>> +            sched_numa_list_count(numa_list_est) == 1) {
>> +
>> +        /* Calculate variances. */
>> +        current = sched_numa_list_variance(numa_list_cur);
>> +        estimate = sched_numa_list_variance(numa_list_est);
>> +
>> +        if (estimate < current) {
>> +             improvement = ((current - estimate) * 100) / current;
>> +        }
>> +        VLOG_DBG("Current variance %"PRIu64" Estimated variance
>> %"PRIu64"",
>> +                 current, estimate);
> 
> space alignment issues. extra space is required at the start of second statement to align with the first one ?

yes, fixed.

> Also, comma or full stop after Current variance %"PRIu64" ?
> 

added

>> +        VLOG_DBG("Variance improvement %"PRIu64"%%", improvement);
>> +
>> +        if (improvement >= dp->pmd_alb.rebalance_improve_thresh) {
>> +            thresh_met = true;
>> +            VLOG_DBG("PMD load variance improvement threshold %u%% "
>> +                      "is met", dp->pmd_alb.rebalance_improve_thresh);
> 
> space alignment issue. Extra space added here before "is_met".
> 

fixed

> 
>> +        } else {
>> +            VLOG_DBG("PMD load variance improvement threshold %u%% is not
>> met",
>> +                      dp->pmd_alb.rebalance_improve_thresh);
>> +        }
>> +    } else {
>> +        VLOG_DBG("PMD auto load balance detected cross-numa polling with "
>> +                 "multiple numa nodes. Unable to accurately estimate.");
>> +    }
>> +
>> +    sched_numa_list_free_entries(numa_list_cur);
>> +    sched_numa_list_free_entries(numa_list_est);
>> +
>> +    free(numa_list_cur);
>> +    free(numa_list_est);
>> +
>> +    return thresh_met;
>> +}
>> +
>>  static void
>>  reload_affected_pmds(struct dp_netdev *dp) @@ -5897,215 +5925,4 @@
>> variance(uint64_t a[], int n)  }
> 
> <snipped>
>