[ovs-dev,v4,1/2] handlers: Create additional handler threads when using CPU isolation

Additional threads are required to service upcalls when we have CPU
isolation (in per-cpu dispatch mode). The formula used to calculate
the number of handler threads to create is as follows

handlers_n = min(next_prime(active_cores+1), total_cores)

Where next_prime is a function that returns the next numeric prime
number that is strictly greater than active_cores

Fixes: b1e517bd2f81 ("dpif-netlink: Introduce per-cpu upcall dispatch.")
Signed-off-by: Michael Santana <msantana@redhat.com>
---
 lib/dpif-netlink.c | 32 ++++++++++++++++++++-
 lib/ovs-thread.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/ovs-thread.h   |  3 ++
 3 files changed, 103 insertions(+), 1 deletion(-)

Michael Santana <msantana@redhat.com> writes:

> Additional threads are required to service upcalls when we have CPU
> isolation (in per-cpu dispatch mode). The formula used to calculate
> the number of handler threads to create is as follows
>
> handlers_n = min(next_prime(active_cores+1), total_cores)
>
> Where next_prime is a function that returns the next numeric prime
> number that is strictly greater than active_cores
>
> Fixes: b1e517bd2f81 ("dpif-netlink: Introduce per-cpu upcall dispatch.")
> Signed-off-by: Michael Santana <msantana@redhat.com>
> ---
>  lib/dpif-netlink.c | 32 ++++++++++++++++++++-
>  lib/ovs-thread.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ovs-thread.h   |  3 ++
>  3 files changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 06e1e8ca0..53de16e12 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2506,6 +2506,36 @@ dpif_netlink_handler_uninit(struct dpif_handler *handler)
>  }
>  #endif
>  
> +/*
> + * Calcuales and returns the number of handler threads needed based
> + * the following formula:
> + *
> + * handlers_n = min(next_prime(active_cores+1), total_cores)
> + *
> + * Where next_prime is a function that returns the next numeric prime
> + * number that is strictly greater than active_cores
> + */
> +static uint32_t
> +dpif_netlink_calculate_handlers_num(void)
> +{
> +    uint32_t next_prime_num;
> +    uint32_t n_handlers = count_cpu_cores();
> +    uint32_t total_cores = count_total_cores();
> +
> +    /*
> +     * If we have isolated cores, add additional handler threads to
> +     * service inactive cores in the unlikely event that traffic goes
> +     * through inactive cores
> +     */
> +    if (n_handlers < total_cores) {
> +        next_prime_num = next_prime(n_handlers +1, 10000000);
> +        n_handlers = next_prime_num >= total_cores ?
> +                                            total_cores : next_prime_num;
> +    }
> +
> +    return n_handlers;
> +}
> +
>  static int
>  dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif)
>      OVS_REQ_WRLOCK(dpif->upcall_lock)
> @@ -2755,7 +2785,7 @@ dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers)
>      struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>  
>      if (dpif_netlink_upcall_per_cpu(dpif)) {
> -        *n_handlers = count_cpu_cores();
> +        *n_handlers = dpif_netlink_calculate_handlers_num();
>          return true;
>      }
>  
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index 805cba622..fc3080d7c 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -18,6 +18,7 @@
>  #include "ovs-thread.h"
>  #include <errno.h>
>  #include <poll.h>
> +#include <math.h>
>  #ifndef _WIN32
>  #include <signal.h>
>  #endif
> @@ -663,6 +664,74 @@ count_cpu_cores(void)
>      return n_cores > 0 ? n_cores : 0;
>  }
>  
> +/* Returns the total number of cores on the system, or 0 if the
> + * number cannot be determined. */
> +int
> +count_total_cores(void) {
> +    long int n_cores;
> +
> +#ifndef _WIN32
> +    n_cores = sysconf(_SC_NPROCESSORS_CONF);
> +#else
> +    n_cores = 0;
> +    errno = ENOTSUP;
> +#endif
> +
> +    return n_cores > 0 ? n_cores : 0;
> +}
> +
> +/*
> + * Returns 1 if num is a prime number,
> + * Otherwise return 0
> + */

Should this really be in ovs-thread?  Does it need to be in the
"exposed" API?  For example, we could just leave it out of the header
and make it static.  WDYT?

> +uint32_t
> +is_prime(uint32_t num)
> +{

1 and 0 aren't prime numbers, but this will return '1' anyway.  In this
particular patch usage it isn't important.  In the future, maybe it
will be.  I guess this should be something like:

> +    if ((num % 2) == 0) {

+    if ((num < 2) || ((num % 2) == 0)) {

> +        return num == 2;
> +    }
> +
> +    uint32_t i;
> +    uint32_t limit = sqrt((float) num);
> +    for (i = 3; i <= limit; i += 2) {
> +        if (num % i == 0) {
> +            return 0;
> +        }
> +    }
> +
> +    return 1;
> +}
> +
> +/*
> + * Returns num if num is a prime number. Otherwise returns the next
> + * prime greater than num. Search is limited by the limit variable.
> + *
> + * Returns 0 if num >= limit, or if no prime has been found between
> + * num and limit
> + */
> +uint32_t
> +next_prime(uint32_t num, uint32_t limit)
> +{
> +    if (num < 2) {
> +        return 2;
> +    }
> +    if (num == 2) {
> +        return 3;
> +    }
> +    if (num % 2 == 0) {
> +        num++;
> +    }
> +
> +    uint32_t i;
> +    for (i = num; i < limit; i += 2) {
> +        if (is_prime(i)) {
> +            return i;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  /* Returns 'true' if current thread is PMD thread. */
>  bool
>  thread_is_pmd(void)
> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> index 3b444ccdc..6c1ebd098 100644
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -522,6 +522,9 @@ bool may_fork(void);
>  /* Useful functions related to threading. */
>  
>  int count_cpu_cores(void);
> +int count_total_cores(void);
> +uint32_t is_prime(uint32_t num);
> +uint32_t next_prime(uint32_t num, uint32_t limit);

Maybe this should be in a different location - I suggest the 'util'
files (util.{c,h}).

>  bool thread_is_pmd(void);
>  
>  #endif /* ovs-thread.h */

On 5/31/22 09:01, Aaron Conole wrote:
> Michael Santana <msantana@redhat.com> writes:
> 
>> Additional threads are required to service upcalls when we have CPU
>> isolation (in per-cpu dispatch mode). The formula used to calculate
>> the number of handler threads to create is as follows
>>
>> handlers_n = min(next_prime(active_cores+1), total_cores)
>>
>> Where next_prime is a function that returns the next numeric prime
>> number that is strictly greater than active_cores
>>
>> Fixes: b1e517bd2f81 ("dpif-netlink: Introduce per-cpu upcall dispatch.")
>> Signed-off-by: Michael Santana <msantana@redhat.com>
>> ---
>>   lib/dpif-netlink.c | 32 ++++++++++++++++++++-
>>   lib/ovs-thread.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/ovs-thread.h   |  3 ++
>>   3 files changed, 103 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 06e1e8ca0..53de16e12 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -2506,6 +2506,36 @@ dpif_netlink_handler_uninit(struct dpif_handler *handler)
>>   }
>>   #endif
>>   
>> +/*
>> + * Calcuales and returns the number of handler threads needed based
>> + * the following formula:
>> + *
>> + * handlers_n = min(next_prime(active_cores+1), total_cores)
>> + *
>> + * Where next_prime is a function that returns the next numeric prime
>> + * number that is strictly greater than active_cores
>> + */
>> +static uint32_t
>> +dpif_netlink_calculate_handlers_num(void)
>> +{
>> +    uint32_t next_prime_num;
>> +    uint32_t n_handlers = count_cpu_cores();
>> +    uint32_t total_cores = count_total_cores();
>> +
>> +    /*
>> +     * If we have isolated cores, add additional handler threads to
>> +     * service inactive cores in the unlikely event that traffic goes
>> +     * through inactive cores
>> +     */
>> +    if (n_handlers < total_cores) {
>> +        next_prime_num = next_prime(n_handlers +1, 10000000);
>> +        n_handlers = next_prime_num >= total_cores ?
>> +                                            total_cores : next_prime_num;
>> +    }
>> +
>> +    return n_handlers;
>> +}
>> +
>>   static int
>>   dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif)
>>       OVS_REQ_WRLOCK(dpif->upcall_lock)
>> @@ -2755,7 +2785,7 @@ dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers)
>>       struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>   
>>       if (dpif_netlink_upcall_per_cpu(dpif)) {
>> -        *n_handlers = count_cpu_cores();
>> +        *n_handlers = dpif_netlink_calculate_handlers_num();
>>           return true;
>>       }
>>   
>> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
>> index 805cba622..fc3080d7c 100644
>> --- a/lib/ovs-thread.c
>> +++ b/lib/ovs-thread.c
>> @@ -18,6 +18,7 @@
>>   #include "ovs-thread.h"
>>   #include <errno.h>
>>   #include <poll.h>
>> +#include <math.h>
>>   #ifndef _WIN32
>>   #include <signal.h>
>>   #endif
>> @@ -663,6 +664,74 @@ count_cpu_cores(void)
>>       return n_cores > 0 ? n_cores : 0;
>>   }
>>   
>> +/* Returns the total number of cores on the system, or 0 if the
>> + * number cannot be determined. */
>> +int
>> +count_total_cores(void) {
>> +    long int n_cores;
>> +
>> +#ifndef _WIN32
>> +    n_cores = sysconf(_SC_NPROCESSORS_CONF);
>> +#else
>> +    n_cores = 0;
>> +    errno = ENOTSUP;
>> +#endif
>> +
>> +    return n_cores > 0 ? n_cores : 0;
>> +}
>> +
>> +/*
>> + * Returns 1 if num is a prime number,
>> + * Otherwise return 0
>> + */
> 
> Should this really be in ovs-thread?  Does it need to be in the
> "exposed" API?  For example, we could just leave it out of the header
> and make it static.  WDYT?
This makes sense. The prime calculation stuff is not relevant for 
threads. the count_total_cores can stay, but the other two functions 
better to move.

Just one clarification if we make those functions static in the same 
ovs-thread.c file they wont be visible to dpif-netlink.c which is where 
we make use of them. We probably want to move those functions to this 
file and make them static?
> 
>> +uint32_t
>> +is_prime(uint32_t num)
>> +{
> 
> 1 and 0 aren't prime numbers, but this will return '1' anyway.  In this
> particular patch usage it isn't important.  In the future, maybe it
> will be.  I guess this should be something like:
Thanks for checking my math. Your suggestion does cover those edge 
cases. I will add them on next revision
> 
>> +    if ((num % 2) == 0) {
> 
> +    if ((num < 2) || ((num % 2) == 0)) {
> 
>> +        return num == 2;
>> +    }
>> +
>> +    uint32_t i;
>> +    uint32_t limit = sqrt((float) num);
>> +    for (i = 3; i <= limit; i += 2) {
>> +        if (num % i == 0) {
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +/*
>> + * Returns num if num is a prime number. Otherwise returns the next
>> + * prime greater than num. Search is limited by the limit variable.
>> + *
>> + * Returns 0 if num >= limit, or if no prime has been found between
>> + * num and limit
>> + */
>> +uint32_t
>> +next_prime(uint32_t num, uint32_t limit)
>> +{
>> +    if (num < 2) {
>> +        return 2;
>> +    }
>> +    if (num == 2) {
>> +        return 3;
>> +    }
>> +    if (num % 2 == 0) {
>> +        num++;
>> +    }
>> +
>> +    uint32_t i;
>> +    for (i = num; i < limit; i += 2) {
>> +        if (is_prime(i)) {
>> +            return i;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /* Returns 'true' if current thread is PMD thread. */
>>   bool
>>   thread_is_pmd(void)
>> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
>> index 3b444ccdc..6c1ebd098 100644
>> --- a/lib/ovs-thread.h
>> +++ b/lib/ovs-thread.h
>> @@ -522,6 +522,9 @@ bool may_fork(void);
>>   /* Useful functions related to threading. */
>>   
>>   int count_cpu_cores(void);
>> +int count_total_cores(void);
>> +uint32_t is_prime(uint32_t num);
>> +uint32_t next_prime(uint32_t num, uint32_t limit);
> 
> Maybe this should be in a different location - I suggest the 'util'
> files (util.{c,h}).
This wont be necessary if we make them static as mentioned above?
> 
>>   bool thread_is_pmd(void);
>>   
>>   #endif /* ovs-thread.h */
>