From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 2068558E8 for ; Sun, 8 Feb 2015 21:00:30 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YKY5O-0005PQ-Dx; Sun, 08 Feb 2015 21:04:17 +0100 Message-ID: <54D7C04C.70308@6wind.com> Date: Sun, 08 Feb 2015 21:00:12 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.3.0 MIME-Version: 1.0 To: Cunming Liang , dev@dpdk.org References: <1422491072-5114-1-git-send-email-cunming.liang@intel.com> <1422842559-13617-1-git-send-email-cunming.liang@intel.com> <1422842559-13617-7-git-send-email-cunming.liang@intel.com> In-Reply-To: <1422842559-13617-7-git-send-email-cunming.liang@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for common thread API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Feb 2015 20:00:30 -0000 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