> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Monday, February 09, 2015 4:00 AM
> To: Liang, Cunming; dev at 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
