> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, February 10, 2015 1:37 AM
> To: Liang, Cunming; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL thread by
> assigned cpuset
>
> Hi,
>
> On 02/09/2015 02:48 PM, Liang, Cunming wrote:
> >> -----Original Message-----
> >> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> >> Sent: Monday, February 09, 2015 4:01 AM
> >> To: Liang, Cunming; dev at dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL thread
> >> by
> >> assigned cpuset
> >>
> >> Hi,
> >>
> >> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> >>> EAL threads use assigned cpuset to set core affinity during startup.
> >>> It keeps 1:1 mapping, if no '--lcores' option is used.
> >>>
> >>> [...]
> >>>
> >>> lib/librte_eal/bsdapp/eal/eal.c | 13 ++++---
> >>> lib/librte_eal/bsdapp/eal/eal_thread.c | 63
> >>> +++++++++---------------------
> >>> lib/librte_eal/linuxapp/eal/eal.c | 7 +++-
> >>> lib/librte_eal/linuxapp/eal/eal_thread.c | 67
> >>> +++++++++++---------------------
> >>> 4 files changed, 54 insertions(+), 96 deletions(-)
> >>>
> >>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
> >>> b/lib/librte_eal/bsdapp/eal/eal.c
> >>> index 69f3c03..98c5a83 100644
> >>> --- a/lib/librte_eal/bsdapp/eal/eal.c
> >>> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> >>> @@ -432,6 +432,7 @@ rte_eal_init(int argc, char **argv)
> >>> int i, fctret, ret;
> >>> pthread_t thread_id;
> >>> static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
> >>> + char cpuset[CPU_STR_LEN];
> >>>
> >>> if (!rte_atomic32_test_and_set(&run_once))
> >>> return -1;
> >>> @@ -502,13 +503,17 @@ rte_eal_init(int argc, char **argv)
> >>> if (rte_eal_pci_init() < 0)
> >>> rte_panic("Cannot init PCI\n");
> >>>
> >>> - RTE_LOG(DEBUG, EAL, "Master core %u is ready (tid=%p)\n",
> >>> - rte_config.master_lcore, thread_id);
> >>> -
> >>> eal_check_mem_on_local_socket();
> >>>
> >>> rte_eal_mcfg_complete();
> >>>
> >>> + eal_thread_init_master(rte_config.master_lcore);
> >>> +
> >>> + eal_thread_dump_affinity(cpuset, CPU_STR_LEN);
> >>> +
> >>> + RTE_LOG(DEBUG, EAL, "Master lcore %u is ready
> (tid=%p;cpuset=[%s])\n",
> >>> + rte_config.master_lcore, thread_id, cpuset);
> >>> +
> >>> if (rte_eal_dev_init() < 0)
> >>> rte_panic("Cannot init pmd devices\n");
> >>>
> >>> @@ -532,8 +537,6 @@ rte_eal_init(int argc, char **argv)
> >>> rte_panic("Cannot create thread\n");
> >>> }
> >>>
> >>> - eal_thread_init_master(rte_config.master_lcore);
> >>> -
> >>> /*
> >>> * Launch a dummy function on all slave lcores, so that master lcore
> >>> * knows they are all ready when this function returns.
> >>
> >> I wonder if changing this may have an impact on third-party drivers
> >> that already use a management thread. Before the patch, the init()
> >> function of the external library was called with default affinities,
> >> and now it's called with the affinity from master lcore.
> >>
> >> I think it should at least be noticed in the commit log.
> >>
> >> Why are you doing this change? (I don't say it's a bad change, but
> >> I don't understand why you are doing it here)
> > [LCM] To be honest, the main purpose is I don't found any reason to have
> linuxapp and freebsdapp in different init sequence.
> > I means in linux it init_master before dev_init(), but in freebsd it
> > reverse.
>
>
> I agree that's something we should fix.
>
>
> > And as the default value of TLS already changes, if dev_init() first and
> > using
> those TLS, the result will be not in an EAL thread.
> > But actually they're in the EAL master thread. So I prefer to do the change
> follows linuxapp sequence.
>
> That makes sense. Is it possible to have this reordering in a separate
> patch? The title could be
> "eal: standardize init sequence between linux and bsd"
[LCM] Agree.
>
>
>
> >>
> >>
> >>> diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c
> >> b/lib/librte_eal/bsdapp/eal/eal_thread.c
> >>> index d0c077b..5b16302 100644
> >>> --- a/lib/librte_eal/bsdapp/eal/eal_thread.c
> >>> +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c
> >>> @@ -103,55 +103,27 @@ eal_thread_set_affinity(void)
> >>> {
> >>> int s;
> >>> pthread_t thread;
> >>> -
> >>> -/*
> >>> - * According to the section VERSIONS of the CPU_ALLOC man page:
> >>> - *
> >>> - * The CPU_ZERO(), CPU_SET(), CPU_CLR(), and CPU_ISSET() macros were
> >> added
> >>> - * in glibc 2.3.3.
> >>> - *
> >>> - * CPU_COUNT() first appeared in glibc 2.6.
> >>> - *
> >>> - * CPU_AND(), CPU_OR(), CPU_XOR(), CPU_EQUAL(),
> >> CPU_ALLOC(),
> >>> - * CPU_ALLOC_SIZE(), CPU_FREE(), CPU_ZERO_S(), CPU_SET_S(),
> >> CPU_CLR_S(),
> >>> - * CPU_ISSET_S(), CPU_AND_S(), CPU_OR_S(), CPU_XOR_S(), and
> >> CPU_EQUAL_S()
> >>> - * first appeared in glibc 2.7.
> >>> - */
> >>> -#if defined(CPU_ALLOC)
> >>> - size_t size;
> >>> - cpu_set_t *cpusetp;
> >>> -
> >>> - cpusetp = CPU_ALLOC(RTE_MAX_LCORE);
> >>> - if (cpusetp == NULL) {
> >>> - RTE_LOG(ERR, EAL, "CPU_ALLOC failed\n");
> >>> - return -1;
> >>> - }
> >>> -
> >>> - size = CPU_ALLOC_SIZE(RTE_MAX_LCORE);
> >>> - CPU_ZERO_S(size, cpusetp);
> >>> - CPU_SET_S(rte_lcore_id(), size, cpusetp);
> >>> + unsigned lcore_id = rte_lcore_id();
> >>>
> >>> thread = pthread_self();
> >>> - s = pthread_setaffinity_np(thread, size, cpusetp);
> >>> + s = pthread_setaffinity_np(thread, sizeof(cpuset_t),
> >>> + &lcore_config[lcore_id].cpuset);
> >>> if (s != 0) {
> >>> RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> >>> - CPU_FREE(cpusetp);
> >>> return -1;
> >>> }
> >>>
> >>> - CPU_FREE(cpusetp);
> >>> -#else /* CPU_ALLOC */
> >>> - cpuset_t cpuset;
> >>> - CPU_ZERO( &cpuset );
> >>> - CPU_SET( rte_lcore_id(), &cpuset );
> >>> + /* acquire system unique id */
> >>> + rte_gettid();
> >>
> >> As suggested in the previous patch, I think having rte_init_tid() would
> >> be clearer here.
> > [LCM] Sorry, I didn't get your [PATCH v4 07/17] comments, probably the
> mailbox issue.
> > Do you suggest to have a rte_init_tid() but not do syscall on the first
> > time ?
> > Any benefit, rte_gettid() looks like more simple and straight forward.
>
> I think the mail was properly sent, you can see it here:
> http://dpdk.org/ml/archives/dev/2015-February/012556.html
>
> Usually, "get" functions return a value and have no side effects.
> "init" functions return nothing (or an error code) but have a
> side effect which is to initialize an internal state.
[LCM] Thanks, agree your point, will update on v5.
>
>
> >>> +
> >>> + /* store socket_id in TLS for quick access */
> >>> + RTE_PER_LCORE(_socket_id) =
> >>> + eal_cpuset_socket_id(&lcore_config[lcore_id].cpuset);
> >>> +
> >>> + CPU_COPY(&lcore_config[lcore_id].cpuset, &RTE_PER_LCORE(_cpuset));
> >>> +
> >>> + lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
> >>>
> >>> - thread = pthread_self();
> >>> - s = pthread_setaffinity_np(thread, sizeof( cpuset ), &cpuset);
> >>> - if (s != 0) {
> >>> - RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> >>> - return -1;
> >>> - }
> >>> -#endif
> >>
> >> You are removing a lot of code that was using CPU_ALLOC().
> >> Are we sure that the cpuset_t type is large enough to store all the
> >> CPUs?
> >>
> >> It looks the current value of CPU_SETSIZE is 1024 now, but I wonder
> >> if this code was written when this value was lower. Could you check if
> >> it can happen today (maybe with an old libc)? A problem can occur if
> >> the size of cpuset_t is lower that the size of RTE_MAX_LCORE.
> > [LCM] I found actually the MACRO is not just for support CPU_ALLOC(), but
> > for
> linux or freebsd.
> > In freebsdapp, there's no CPU_ALLOC defined, it use fixed width *cpuset_t*.
> > In linuxapp, there's CPU_ALLOC defined, it use cpu_set_t* and dynamic
> CPU_ALLOC(RTE_MAX_LCORE).
> > But actually RTE_MAX_LCORE < 1024(sizeof(cpu_set_t)).
> > After using rte_cpuset_t, there's no additional reason to use CPU_ALLOC only
> for linuxapp and choose a small but dynamic width.
>
> I did a quick search on google, and it seems CPU_SETSIZE is 1024
> for a long time. So you are right, there is probably no reason to
> keep CPU_ALLOC(). As I said in a previous mail, it could be useful
> in the future when the number of CPUs will reach 1024, but we have
> some time to handle this.
[LCM] Ok, thanks.
>
>
>
