diff mbox series

[ovs-dev,v2,3/3] dpdk: Use ovs-numa provided functions to manage thread affinity.

Message ID 20190902112711.2919-4-i.maximets@samsung.com
State Accepted
Headers show
Series Fix TSC frequency if DPDK is not available. | expand

Commit Message

Ilya Maximets Sept. 2, 2019, 11:27 a.m. UTC 
This allows to decrease code duplication and avoid using Linux-specific
functions (this might be useful in the future if we'll try to allow
running OvS+DPDK on FreeBSD).

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: William Tu <u9012063@gmail.com>
---
 lib/dpdk.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

Comments

David Marchand Sept. 2, 2019, 1:09 p.m. UTC | #1 
On Mon, Sep 2, 2019 at 1:27 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> This allows to decrease code duplication and avoid using Linux-specific
> functions (this might be useful in the future if we'll try to allow
> running OvS+DPDK on FreeBSD).
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> Acked-by: William Tu <u9012063@gmail.com>
> ---
>  lib/dpdk.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index fc58de55a..6f297d918 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -275,7 +275,7 @@ dpdk_init__(const struct smap *ovs_other_config)
>      int result;
>      bool auto_determine = true;
>      int err = 0;

Nit: err can be removed.


> -    cpu_set_t cpuset;
> +    struct ovs_numa_dump *affinity = NULL;
>      struct svec args = SVEC_EMPTY_INITIALIZER;
>
>      log_stream = fopencookie(NULL, "w+", dpdk_log_func);
> @@ -357,22 +357,22 @@ dpdk_init__(const struct smap *ovs_other_config)
>       * lcore for the DPDK Master.
>       */
>      if (auto_determine) {
> +        const struct ovs_numa_info_core *core;
>          int cpu = 0;
>
>          /* Get the main thread affinity */
> -        CPU_ZERO(&cpuset);
> -        err = pthread_getaffinity_np(pthread_self(), sizeof(cpu_set_t),
> -                                     &cpuset);
> -        if (!err) {
> -            for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
> -                if (CPU_ISSET(cpu, &cpuset)) {
> -                    break;
> +        affinity = ovs_numa_thread_getaffinity_dump();
> +        if (affinity) {
> +            cpu = INT_MAX;
> +            FOR_EACH_CORE_ON_DUMP (core, affinity) {
> +                if (cpu > core->core_id) {
> +                    cpu = core->core_id;
>                  }
>              }
>          } else {
>              /* User did not set dpdk-lcore-mask and unable to get current
>               * thread affintity - default to core #0 */
> -            VLOG_ERR("Thread getaffinity error %d. Using core #0", err);
> +            VLOG_ERR("Thread getaffinity failed. Using core #0");
>          }
>          svec_add(&args, "-l");
>          svec_add_nocopy(&args, xasprintf("%d", cpu));
> @@ -403,12 +403,9 @@ dpdk_init__(const struct smap *ovs_other_config)
>      svec_destroy(&args);
>
>      /* Set the main thread affinity back to pre rte_eal_init() value */
> -    if (auto_determine && !err) {
> -        err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t),
> -                                     &cpuset);
> -        if (err) {
> -            VLOG_ERR("Thread setaffinity error %d", err);
> -        }
> +    if (affinity) {
> +        ovs_numa_thread_setaffinity_dump(affinity);
> +        ovs_numa_dump_destroy(affinity);
>      }
>
>      if (result < 0) {
> --
> 2.17.1
>

Reviewed-by: David Marchand <david.marchand@redhat.com>


--
David Marchand
Ilya Maximets Sept. 2, 2019, 1:15 p.m. UTC | #2 
On 02.09.2019 16:09, David Marchand wrote:
> On Mon, Sep 2, 2019 at 1:27 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> This allows to decrease code duplication and avoid using Linux-specific
>> functions (this might be useful in the future if we'll try to allow
>> running OvS+DPDK on FreeBSD).
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> Acked-by: William Tu <u9012063@gmail.com>
>> ---
>>  lib/dpdk.c | 27 ++++++++++++---------------
>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>> index fc58de55a..6f297d918 100644
>> --- a/lib/dpdk.c
>> +++ b/lib/dpdk.c
>> @@ -275,7 +275,7 @@ dpdk_init__(const struct smap *ovs_other_config)
>>      int result;
>>      bool auto_determine = true;
>>      int err = 0;
> 
> Nit: err can be removed.

It's used 2 times in this function.
We could only avoid initializing it here.

> 
> 
>> -    cpu_set_t cpuset;
>> +    struct ovs_numa_dump *affinity = NULL;
>>      struct svec args = SVEC_EMPTY_INITIALIZER;
>>
>>      log_stream = fopencookie(NULL, "w+", dpdk_log_func);
>> @@ -357,22 +357,22 @@ dpdk_init__(const struct smap *ovs_other_config)
>>       * lcore for the DPDK Master.
>>       */
>>      if (auto_determine) {
>> +        const struct ovs_numa_info_core *core;
>>          int cpu = 0;
>>
>>          /* Get the main thread affinity */
>> -        CPU_ZERO(&cpuset);
>> -        err = pthread_getaffinity_np(pthread_self(), sizeof(cpu_set_t),
>> -                                     &cpuset);
>> -        if (!err) {
>> -            for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
>> -                if (CPU_ISSET(cpu, &cpuset)) {
>> -                    break;
>> +        affinity = ovs_numa_thread_getaffinity_dump();
>> +        if (affinity) {
>> +            cpu = INT_MAX;
>> +            FOR_EACH_CORE_ON_DUMP (core, affinity) {
>> +                if (cpu > core->core_id) {
>> +                    cpu = core->core_id;
>>                  }
>>              }
>>          } else {
>>              /* User did not set dpdk-lcore-mask and unable to get current
>>               * thread affintity - default to core #0 */
>> -            VLOG_ERR("Thread getaffinity error %d. Using core #0", err);
>> +            VLOG_ERR("Thread getaffinity failed. Using core #0");
>>          }
>>          svec_add(&args, "-l");
>>          svec_add_nocopy(&args, xasprintf("%d", cpu));
>> @@ -403,12 +403,9 @@ dpdk_init__(const struct smap *ovs_other_config)
>>      svec_destroy(&args);
>>
>>      /* Set the main thread affinity back to pre rte_eal_init() value */
>> -    if (auto_determine && !err) {
>> -        err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t),
>> -                                     &cpuset);
>> -        if (err) {
>> -            VLOG_ERR("Thread setaffinity error %d", err);
>> -        }
>> +    if (affinity) {
>> +        ovs_numa_thread_setaffinity_dump(affinity);
>> +        ovs_numa_dump_destroy(affinity);
>>      }
>>
>>      if (result < 0) {
>> --
>> 2.17.1
>>
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> 
> --
> David Marchand
> 
>
David Marchand Sept. 2, 2019, 1:17 p.m. UTC | #3 
On Mon, Sep 2, 2019 at 3:15 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> On 02.09.2019 16:09, David Marchand wrote:
> > On Mon, Sep 2, 2019 at 1:27 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>
> >> This allows to decrease code duplication and avoid using Linux-specific
> >> functions (this might be useful in the future if we'll try to allow
> >> running OvS+DPDK on FreeBSD).
> >>
> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >> Acked-by: William Tu <u9012063@gmail.com>
> >> ---
> >>  lib/dpdk.c | 27 ++++++++++++---------------
> >>  1 file changed, 12 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/lib/dpdk.c b/lib/dpdk.c
> >> index fc58de55a..6f297d918 100644
> >> --- a/lib/dpdk.c
> >> +++ b/lib/dpdk.c
> >> @@ -275,7 +275,7 @@ dpdk_init__(const struct smap *ovs_other_config)
> >>      int result;
> >>      bool auto_determine = true;
> >>      int err = 0;
> >
> > Nit: err can be removed.
>
> It's used 2 times in this function.
> We could only avoid initializing it here.

Yes, but we do not care about err itself, we are just checking
function return values.
Anyway, this is really minor.
Ilya Maximets Sept. 2, 2019, 1:38 p.m. UTC | #4 
On 02.09.2019 16:17, David Marchand wrote:
> On Mon, Sep 2, 2019 at 3:15 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> On 02.09.2019 16:09, David Marchand wrote:
>>> On Mon, Sep 2, 2019 at 1:27 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>
>>>> This allows to decrease code duplication and avoid using Linux-specific
>>>> functions (this might be useful in the future if we'll try to allow
>>>> running OvS+DPDK on FreeBSD).
>>>>
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> Acked-by: William Tu <u9012063@gmail.com>
>>>> ---
>>>>  lib/dpdk.c | 27 ++++++++++++---------------
>>>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>>>> index fc58de55a..6f297d918 100644
>>>> --- a/lib/dpdk.c
>>>> +++ b/lib/dpdk.c
>>>> @@ -275,7 +275,7 @@ dpdk_init__(const struct smap *ovs_other_config)
>>>>      int result;
>>>>      bool auto_determine = true;
>>>>      int err = 0;
>>>
>>> Nit: err can be removed.
>>
>> It's used 2 times in this function.
>> We could only avoid initializing it here.
> 
> Yes, but we do not care about err itself, we are just checking
> function return values.

I see. It seems for me more like a separate change. This could be
done, for example, along with improving stat/pdump error messages.

> Anyway, this is really minor.
David Marchand Sept. 2, 2019, 1:47 p.m. UTC | #5 
On Mon, Sep 2, 2019 at 3:38 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> On 02.09.2019 16:17, David Marchand wrote:
> > On Mon, Sep 2, 2019 at 3:15 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>
> >> On 02.09.2019 16:09, David Marchand wrote:
> >>> On Mon, Sep 2, 2019 at 1:27 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>>>
> >>>> This allows to decrease code duplication and avoid using Linux-specific
> >>>> functions (this might be useful in the future if we'll try to allow
> >>>> running OvS+DPDK on FreeBSD).
> >>>>
> >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >>>> Acked-by: William Tu <u9012063@gmail.com>
> >>>> ---
> >>>>  lib/dpdk.c | 27 ++++++++++++---------------
> >>>>  1 file changed, 12 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/lib/dpdk.c b/lib/dpdk.c
> >>>> index fc58de55a..6f297d918 100644
> >>>> --- a/lib/dpdk.c
> >>>> +++ b/lib/dpdk.c
> >>>> @@ -275,7 +275,7 @@ dpdk_init__(const struct smap *ovs_other_config)
> >>>>      int result;
> >>>>      bool auto_determine = true;
> >>>>      int err = 0;
> >>>
> >>> Nit: err can be removed.
> >>
> >> It's used 2 times in this function.
> >> We could only avoid initializing it here.
> >
> > Yes, but we do not care about err itself, we are just checking
> > function return values.
>
> I see. It seems for me more like a separate change. This could be
> done, for example, along with improving stat/pdump error messages.

This patch removed the only reason to keep this variable from my pov.
But yes, it can go in a cleanup later.
diff mbox series

Patch

diff --git a/lib/dpdk.c b/lib/dpdk.c
index fc58de55a..6f297d918 100644
--- a/lib/dpdk.c
+++ b/lib/dpdk.c
@@ -275,7 +275,7 @@  dpdk_init__(const struct smap *ovs_other_config)
     int result;
     bool auto_determine = true;
     int err = 0;
-    cpu_set_t cpuset;
+    struct ovs_numa_dump *affinity = NULL;
     struct svec args = SVEC_EMPTY_INITIALIZER;
 
     log_stream = fopencookie(NULL, "w+", dpdk_log_func);
@@ -357,22 +357,22 @@  dpdk_init__(const struct smap *ovs_other_config)
      * lcore for the DPDK Master.
      */
     if (auto_determine) {
+        const struct ovs_numa_info_core *core;
         int cpu = 0;
 
         /* Get the main thread affinity */
-        CPU_ZERO(&cpuset);
-        err = pthread_getaffinity_np(pthread_self(), sizeof(cpu_set_t),
-                                     &cpuset);
-        if (!err) {
-            for (cpu = 0; cpu < CPU_SETSIZE; cpu++) {
-                if (CPU_ISSET(cpu, &cpuset)) {
-                    break;
+        affinity = ovs_numa_thread_getaffinity_dump();
+        if (affinity) {
+            cpu = INT_MAX;
+            FOR_EACH_CORE_ON_DUMP (core, affinity) {
+                if (cpu > core->core_id) {
+                    cpu = core->core_id;
                 }
             }
         } else {
             /* User did not set dpdk-lcore-mask and unable to get current
              * thread affintity - default to core #0 */
-            VLOG_ERR("Thread getaffinity error %d. Using core #0", err);
+            VLOG_ERR("Thread getaffinity failed. Using core #0");
         }
         svec_add(&args, "-l");
         svec_add_nocopy(&args, xasprintf("%d", cpu));
@@ -403,12 +403,9 @@  dpdk_init__(const struct smap *ovs_other_config)
     svec_destroy(&args);
 
     /* Set the main thread affinity back to pre rte_eal_init() value */
-    if (auto_determine && !err) {
-        err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t),
-                                     &cpuset);
-        if (err) {
-            VLOG_ERR("Thread setaffinity error %d", err);
-        }
+    if (affinity) {
+        ovs_numa_thread_setaffinity_dump(affinity);
+        ovs_numa_dump_destroy(affinity);
     }
 
     if (result < 0) {