[ovs-dev] [PATCH] dpif-netdev: Sync PMD ALB state with user commands.

Kevin Traynor ktraynor at redhat.com
Mon Nov 1 16:50:27 UTC 2021


On 29/10/2021 14:20, David Marchand wrote:
> On Fri, Oct 8, 2021 at 12:19 PM Kevin Traynor <ktraynor at 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 at redhat.com>
>>
> 
> Reviewed-by: David Marchand <david.marchand at 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;
>> +    }
> 
> 



More information about the dev mailing list