[dpdk-dev,v4,06/17] eal: add eal_common_thread.c for common thread API
Commit Message
The API works for both EAL thread and none EAL thread.
When calling rte_thread_set_affinity, the *_socket_id* and
*_cpuset* of calling thread will be updated if the thread
successful set the cpu affinity.
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
lib/librte_eal/bsdapp/eal/Makefile | 1 +
lib/librte_eal/common/eal_common_thread.c | 142 ++++++++++++++++++++++++++++++
lib/librte_eal/linuxapp/eal/Makefile | 2 +
3 files changed, 145 insertions(+)
create mode 100644 lib/librte_eal/common/eal_common_thread.c
Comments
Hi,
On 02/02/2015 03:02 AM, Cunming Liang wrote:
> The API works for both EAL thread and none EAL thread.
> When calling rte_thread_set_affinity, the *_socket_id* and
> *_cpuset* of calling thread will be updated if the thread
> successful set the cpu affinity.
>
> [...]
> +int
> +rte_thread_set_affinity(rte_cpuset_t *cpusetp)
> +{
> + int s;
> + unsigned lcore_id;
> + pthread_t tid;
> +
> + if (!cpusetp)
> + return -1;
Is it really needed to test that cpusetp is not NULL?
> +
> + lcore_id = rte_lcore_id();
> + if (lcore_id != (unsigned)LCORE_ID_ANY) {
This is strange to see something that cannot happen:
lcore_id == LCORE_ID_ANY is only possible after your patch is 12/17
is added. Maybe it can be reordered to avoid this inconsistency?
> + /* EAL thread */
> + tid = lcore_config[lcore_id].thread_id;
> +
> + s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
> + if (s != 0) {
> + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> + return -1;
> + }
> +
> + /* store socket_id in TLS for quick access */
> + RTE_PER_LCORE(_socket_id) =
> + eal_cpuset_socket_id(cpusetp);
> +
> + /* store cpuset in TLS for quick access */
> + rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
> + sizeof(rte_cpuset_t));
> +
> + /* update lcore_config */
> + lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
> + rte_memcpy(&lcore_config[lcore_id].cpuset, cpusetp,
> + sizeof(rte_cpuset_t));
> + } else {
> + /* none EAL thread */
> + tid = pthread_self();
> +
> + s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
> + if (s != 0) {
> + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> + return -1;
> + }
> +
> + /* store cpuset in TLS for quick access */
> + rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
> + sizeof(rte_cpuset_t));
> +
> + /* store socket_id in TLS for quick access */
> + RTE_PER_LCORE(_socket_id) =
> + eal_cpuset_socket_id(cpusetp);
> + }
Why not always using pthread_self() to get the tid?
I think most of the code could be factorized here. The only difference
(which is hard to see as is as code is not exactly ordered in the same
manner) is that the config is updated in case it's an EAL thread.
> +
> + return 0;
> +}
> +
> +int
> +rte_thread_get_affinity(rte_cpuset_t *cpusetp)
> +{
> + if (!cpusetp)
> + return -1;
Same here. This is the only reason why rte_thread_get_affinity() could
fail. Removing this test would allow to change the API to return void
instead. It will avoid a useless test below in
eal_thread_dump_affinity().
> +
> + rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
> + sizeof(rte_cpuset_t));
> +
> + return 0;
> +}
> +
> +void
> +eal_thread_dump_affinity(char str[], unsigned size)
> +{
> + rte_cpuset_t cpuset;
> + unsigned cpu;
> + int ret;
> + unsigned int out = 0;
> +
> + if (rte_thread_get_affinity(&cpuset) < 0) {
> + str[0] = '\0';
> + return;
> + }
This one could be removed it the (== NULL) test is removed.
> +
> + for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
> + if (!CPU_ISSET(cpu, &cpuset))
> + continue;
> +
> + ret = snprintf(str + out,
> + size - out, "%u,", cpu);
> + if (ret < 0 || (unsigned)ret >= size - out)
> + break;
On the contrary, I think here returning an error to the user
would be useful so he can knows that the dump is not complete.
Regards,
Olivier
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 09, 2015 4:00 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for
> common thread API
>
> Hi,
>
> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> > The API works for both EAL thread and none EAL thread.
> > When calling rte_thread_set_affinity, the *_socket_id* and
> > *_cpuset* of calling thread will be updated if the thread
> > successful set the cpu affinity.
> >
> > [...]
> > +int
> > +rte_thread_set_affinity(rte_cpuset_t *cpusetp)
> > +{
> > + int s;
> > + unsigned lcore_id;
> > + pthread_t tid;
> > +
> > + if (!cpusetp)
> > + return -1;
>
> Is it really needed to test that cpusetp is not NULL?
[LCM] Accept, we can ignore it and depend on pthread_setaffinity_np() to return failure.
>
> > +
> > + lcore_id = rte_lcore_id();
> > + if (lcore_id != (unsigned)LCORE_ID_ANY) {
>
> This is strange to see something that cannot happen:
> lcore_id == LCORE_ID_ANY is only possible after your patch is 12/17
> is added. Maybe it can be reordered to avoid this inconsistency?
[LCM] You're right, here do some re-order.
The point is to make everything ready before switching the default value to -1.
And we can have the whole function implement in one patch.
It just won't take effect, but won't bring additional risk.
>
> > + /* EAL thread */
> > + tid = lcore_config[lcore_id].thread_id;
> > +
> > + s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
> > + if (s != 0) {
> > + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> > + return -1;
> > + }
> > +
> > + /* store socket_id in TLS for quick access */
> > + RTE_PER_LCORE(_socket_id) =
> > + eal_cpuset_socket_id(cpusetp);
> > +
> > + /* store cpuset in TLS for quick access */
> > + rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
> > + sizeof(rte_cpuset_t));
> > +
> > + /* update lcore_config */
> > + lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
> > + rte_memcpy(&lcore_config[lcore_id].cpuset, cpusetp,
> > + sizeof(rte_cpuset_t));
> > + } else {
> > + /* none EAL thread */
> > + tid = pthread_self();
> > +
> > + s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
> > + if (s != 0) {
> > + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> > + return -1;
> > + }
> > +
> > + /* store cpuset in TLS for quick access */
> > + rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
> > + sizeof(rte_cpuset_t));
> > +
> > + /* store socket_id in TLS for quick access */
> > + RTE_PER_LCORE(_socket_id) =
> > + eal_cpuset_socket_id(cpusetp);
> > + }
>
> Why not always using pthread_self() to get the tid?
[LCM] Good point, I haven't notice it.
>
> I think most of the code could be factorized here. The only difference
> (which is hard to see as is as code is not exactly ordered in the same
> manner) is that the config is updated in case it's an EAL thread.
[LCM] Accept.
>
>
>
> > +
> > + return 0;
> > +}
> > +
> > +int
> > +rte_thread_get_affinity(rte_cpuset_t *cpusetp)
> > +{
> > + if (!cpusetp)
> > + return -1;
>
> Same here. This is the only reason why rte_thread_get_affinity() could
> fail. Removing this test would allow to change the API to return void
> instead. It will avoid a useless test below in
> eal_thread_dump_affinity().
[LCM] The cpusetp is used as destination of memcpy and the function suppose an EAL API.
I don't think it's a good idea to remove the check, do you ?
>
> > +
> > + rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
> > + sizeof(rte_cpuset_t));
> > +
> > + return 0;
> > +}
> > +
> > +void
> > +eal_thread_dump_affinity(char str[], unsigned size)
> > +{
> > + rte_cpuset_t cpuset;
> > + unsigned cpu;
> > + int ret;
> > + unsigned int out = 0;
> > +
> > + if (rte_thread_get_affinity(&cpuset) < 0) {
> > + str[0] = '\0';
> > + return;
> > + }
>
> This one could be removed it the (== NULL) test is removed.
>
> > +
> > + for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
> > + if (!CPU_ISSET(cpu, &cpuset))
> > + continue;
> > +
> > + ret = snprintf(str + out,
> > + size - out, "%u,", cpu);
> > + if (ret < 0 || (unsigned)ret >= size - out)
> > + break;
>
> On the contrary, I think here returning an error to the user
> would be useful so he can knows that the dump is not complete.
[LCM] accept.
>
>
> Regards,
> Olivier
Hi,
On 02/09/2015 02:12 PM, Liang, Cunming wrote:
>>> +int
>>> +rte_thread_get_affinity(rte_cpuset_t *cpusetp)
>>> +{
>>> + if (!cpusetp)
>>> + return -1;
>>
>> Same here. This is the only reason why rte_thread_get_affinity() could
>> fail. Removing this test would allow to change the API to return void
>> instead. It will avoid a useless test below in
>> eal_thread_dump_affinity().
> [LCM] The cpusetp is used as destination of memcpy and the function suppose an EAL API.
> I don't think it's a good idea to remove the check, do you ?
I know we often have debate on this subject on the list. My personal
opinion is that checking a NULL pointer in these cases is useless
because the user is suppose to give a non-NULL pointer. Returning
an error will result in managing an error for something that cannot
happen.
On the other hand, adding an assert() (or the dpdk equivalent) would
be a good idea.
>>
>>> +
>>> + rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
>>> + sizeof(rte_cpuset_t));
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +void
>>> +eal_thread_dump_affinity(char str[], unsigned size)
>>> +{
>>> + rte_cpuset_t cpuset;
>>> + unsigned cpu;
>>> + int ret;
>>> + unsigned int out = 0;
>>> +
>>> + if (rte_thread_get_affinity(&cpuset) < 0) {
>>> + str[0] = '\0';
>>> + return;
>>> + }
>>
>> This one could be removed it the (== NULL) test is removed.
>>
>>> +
>>> + for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
>>> + if (!CPU_ISSET(cpu, &cpuset))
>>> + continue;
>>> +
>>> + ret = snprintf(str + out,
>>> + size - out, "%u,", cpu);
>>> + if (ret < 0 || (unsigned)ret >= size - out)
>>> + break;
>>
>> On the contrary, I think here returning an error to the user
>> would be useful so he can knows that the dump is not complete.
> [LCM] accept.
>>
>>
>> Regards,
>> Olivier
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, February 10, 2015 1:30 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for
> common thread API
>
> Hi,
>
> On 02/09/2015 02:12 PM, Liang, Cunming wrote:
> >>> +int
> >>> +rte_thread_get_affinity(rte_cpuset_t *cpusetp)
> >>> +{
> >>> + if (!cpusetp)
> >>> + return -1;
> >>
> >> Same here. This is the only reason why rte_thread_get_affinity() could
> >> fail. Removing this test would allow to change the API to return void
> >> instead. It will avoid a useless test below in
> >> eal_thread_dump_affinity().
> > [LCM] The cpusetp is used as destination of memcpy and the function suppose
> an EAL API.
> > I don't think it's a good idea to remove the check, do you ?
>
> I know we often have debate on this subject on the list. My personal
> opinion is that checking a NULL pointer in these cases is useless
> because the user is suppose to give a non-NULL pointer. Returning
> an error will result in managing an error for something that cannot
> happen.
>
> On the other hand, adding an assert() (or the dpdk equivalent) would
> be a good idea.
[LCM] Ok, I see. Will update it.
>
>
> >>
> >>> +
> >>> + rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
> >>> + sizeof(rte_cpuset_t));
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +void
> >>> +eal_thread_dump_affinity(char str[], unsigned size)
> >>> +{
> >>> + rte_cpuset_t cpuset;
> >>> + unsigned cpu;
> >>> + int ret;
> >>> + unsigned int out = 0;
> >>> +
> >>> + if (rte_thread_get_affinity(&cpuset) < 0) {
> >>> + str[0] = '\0';
> >>> + return;
> >>> + }
> >>
> >> This one could be removed it the (== NULL) test is removed.
> >>
> >>> +
> >>> + for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
> >>> + if (!CPU_ISSET(cpu, &cpuset))
> >>> + continue;
> >>> +
> >>> + ret = snprintf(str + out,
> >>> + size - out, "%u,", cpu);
> >>> + if (ret < 0 || (unsigned)ret >= size - out)
> >>> + break;
> >>
> >> On the contrary, I think here returning an error to the user
> >> would be useful so he can knows that the dump is not complete.
> > [LCM] accept.
> >>
> >>
> >> Regards,
> >> Olivier
@@ -73,6 +73,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_hexdump.c
SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_devargs.c
SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_dev.c
SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_options.c
+SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_thread.c
CFLAGS_eal.o := -D_GNU_SOURCE
#CFLAGS_eal_thread.o := -D_GNU_SOURCE
new file mode 100644
@@ -0,0 +1,142 @@
+/*-
+ * BSD LICENSE
+ *
+ * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <pthread.h>
+#include <sched.h>
+
+#include <rte_lcore.h>
+#include <rte_log.h>
+#include <rte_memcpy.h>
+
+#include "eal_thread.h"
+
+int
+rte_thread_set_affinity(rte_cpuset_t *cpusetp)
+{
+ int s;
+ unsigned lcore_id;
+ pthread_t tid;
+
+ if (!cpusetp)
+ return -1;
+
+ lcore_id = rte_lcore_id();
+ if (lcore_id != (unsigned)LCORE_ID_ANY) {
+ /* EAL thread */
+ tid = lcore_config[lcore_id].thread_id;
+
+ s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
+ if (s != 0) {
+ RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
+ return -1;
+ }
+
+ /* store socket_id in TLS for quick access */
+ RTE_PER_LCORE(_socket_id) =
+ eal_cpuset_socket_id(cpusetp);
+
+ /* store cpuset in TLS for quick access */
+ rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
+ sizeof(rte_cpuset_t));
+
+ /* update lcore_config */
+ lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
+ rte_memcpy(&lcore_config[lcore_id].cpuset, cpusetp,
+ sizeof(rte_cpuset_t));
+ } else {
+ /* none EAL thread */
+ tid = pthread_self();
+
+ s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
+ if (s != 0) {
+ RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
+ return -1;
+ }
+
+ /* store cpuset in TLS for quick access */
+ rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
+ sizeof(rte_cpuset_t));
+
+ /* store socket_id in TLS for quick access */
+ RTE_PER_LCORE(_socket_id) =
+ eal_cpuset_socket_id(cpusetp);
+ }
+
+ return 0;
+}
+
+int
+rte_thread_get_affinity(rte_cpuset_t *cpusetp)
+{
+ if (!cpusetp)
+ return -1;
+
+ rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
+ sizeof(rte_cpuset_t));
+
+ return 0;
+}
+
+void
+eal_thread_dump_affinity(char str[], unsigned size)
+{
+ rte_cpuset_t cpuset;
+ unsigned cpu;
+ int ret;
+ unsigned int out = 0;
+
+ if (rte_thread_get_affinity(&cpuset) < 0) {
+ str[0] = '\0';
+ return;
+ }
+
+ for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
+ if (!CPU_ISSET(cpu, &cpuset))
+ continue;
+
+ ret = snprintf(str + out,
+ size - out, "%u,", cpu);
+ if (ret < 0 || (unsigned)ret >= size - out)
+ break;
+
+ out += ret;
+ }
+
+ /* remove the last separator */
+ if (out > 0)
+ str[out - 1] = '\0';
+}
@@ -85,6 +85,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_hexdump.c
SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_devargs.c
SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_dev.c
SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_options.c
+SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_thread.c
CFLAGS_eal.o := -D_GNU_SOURCE
CFLAGS_eal_lcore.o := -D_GNU_SOURCE
@@ -96,6 +97,7 @@ CFLAGS_eal_pci.o := -D_GNU_SOURCE
CFLAGS_eal_pci_vfio.o := -D_GNU_SOURCE
CFLAGS_eal_common_whitelist.o := -D_GNU_SOURCE
CFLAGS_eal_common_options.o := -D_GNU_SOURCE
+CFLAGS_eal_common_thread.o := -D_GNU_SOURCE
# workaround for a gcc bug with noreturn attribute
# http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603