[dpdk-dev] [PATCH v3 6/9] eal: register non-EAL threads as lcores

[Ananyev, Konstantin]

Hi David,

> > > diff --git a/lib/librte_eal/common/eal_common_lcore.c b/lib/librte_eal/common/eal_common_lcore.c
> > > index 86d32a3dd7..7db05428e7 100644
> > > --- a/lib/librte_eal/common/eal_common_lcore.c
> > > +++ b/lib/librte_eal/common/eal_common_lcore.c
> > > @@ -220,3 +221,38 @@ rte_socket_id_by_idx(unsigned int idx)
> > >       }
> > >       return config->numa_nodes[idx];
> > >  }
> > > +
> > > +static rte_spinlock_t lcore_lock = RTE_SPINLOCK_INITIALIZER;
> > > +
> > > +unsigned int
> > > +eal_lcore_non_eal_allocate(void)
> > > +{
> > > +     struct rte_config *cfg = rte_eal_get_configuration();
> > > +     unsigned int lcore_id;
> > > +
> > > +     rte_spinlock_lock(&lcore_lock);
> >
> > I think it will break current DPDK MP modes.
> > The problem here - rte_config (and lcore_role[]) is in shared memory,
> > while the lock is local.
> > Simplest way probably to move lcore_lock to rte_config.
> 
> Even before this series, MP has no protection on lcore placing between
> primary and secondary processes.

Agree, it is not a new problem, it has been there for a while.
Though making lcore assignment dynamic will make it more noticeable and harder to avoid.
With static only lcore distribution it is much easier to control things.
 
> Personally, I have no use for DPDK MP and marking MP as not supporting
> this new feature is tempting for a first phase.
> If this is a strong requirement, I can look at it in a second phase.
> What do you think?

In theory it is possible to mark this new API as not supported for MP.
At least for now. Though I think it is sort of temporal solution.
AFAIK, MP is used by customers, so sooner or later someone will hit that problem.
Let say, we do have pdump app/library in our mainline.
As I can see - it will be affected when users will start using this new dynamic lcore API
inside their apps.    

> 
> >
> > > +     for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> > > +             if (cfg->lcore_role[lcore_id] != ROLE_OFF)
> > > +                     continue;
> > > +             cfg->lcore_role[lcore_id] = ROLE_NON_EAL;
> > > +             cfg->lcore_count++;
> > > +             break;
> > > +     }
> > > +     if (lcore_id == RTE_MAX_LCORE)
> > > +             RTE_LOG(DEBUG, EAL, "No lcore available.\n");
> > > +     rte_spinlock_unlock(&lcore_lock);
> > > +     return lcore_id;
> > > +}
> > > +
> > > +void
> > > +eal_lcore_non_eal_release(unsigned int lcore_id)
> > > +{
> > > +     struct rte_config *cfg = rte_eal_get_configuration();
> > > +
> > > +     rte_spinlock_lock(&lcore_lock);
> > > +     if (cfg->lcore_role[lcore_id] == ROLE_NON_EAL) {
> > > +             cfg->lcore_role[lcore_id] = ROLE_OFF;
> > > +             cfg->lcore_count--;
> > > +     }
> > > +     rte_spinlock_unlock(&lcore_lock);
> > > +}
> > > diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
> > > index a7ae0691bf..1cbddc4b5b 100644
> > > --- a/lib/librte_eal/common/eal_common_thread.c
> > > +++ b/lib/librte_eal/common/eal_common_thread.c
> > > @@ -236,3 +236,36 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
> > >       pthread_join(*thread, NULL);
> > >       return -ret;
> > >  }
> > > +
> > > +void
> > > +rte_thread_register(void)
> > > +{
> > > +     unsigned int lcore_id;
> > > +     rte_cpuset_t cpuset;
> > > +
> > > +     /* EAL init flushes all lcores, we can't register before. */
> > > +     assert(internal_config.init_complete == 1);
> > > +     if (pthread_getaffinity_np(pthread_self(), sizeof(cpuset),
> > > +                     &cpuset) != 0)
> > > +             CPU_ZERO(&cpuset);
> > > +     lcore_id = eal_lcore_non_eal_allocate();
> > > +     if (lcore_id >= RTE_MAX_LCORE)
> > > +             lcore_id = LCORE_ID_ANY;
> > > +     rte_thread_init(lcore_id, &cpuset);
> >
> > So we just setting affinity to the same value, right?
> > Not a big deal, but might be easier to allow rte_thread_init()
> > to accept cpuset==NULL (and just don't change thread affinity in that case)
> 
> rte_thread_init does not change the thread cpu affinity, it handles
> per thread (TLS included) variables initialization.

Right, didn't read the code properly.
Please scratch that comment.

> 
> So do you mean accepting cpuset == NULL and do the getaffinity in this case?
> rte_thread_init is EAL private for now.
> That saves us some code in this function, but we will call with a !=
> NULL cpuset in all other EAL code.