Re: [PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

just following up.

On Wed, Dec 07, 2022 at 08:38:39AM -0800, Tyler Retzlaff wrote:
> On Wed, Dec 07, 2022 at 10:13:39AM +0100, Mattias Rönnblom wrote:
> > 
> > To be consistent with function naming scheme, where "ctrl" is the
> > old API, and "control" the new, you could call the start functions
> > something with "ctrl" and "control" as well.
> 
> i'll make this change in v3.
> 
> > 
> > Maybe it's worth mentioning that fact somewhere in the beginning of
> > the file, as a comment (i.e., that "ctrl" denotes the old API).
> 
> i'll make this change in v3.

didn't end up doing this, didn't think it was necessary once i looked at
it the commit history and the rename you suggested above should be
satisfactory.

> 
> > 
> > >+	} u;
> > >  	void *arg;
> > >  	int ret;
> > 
> > Why is 'ret' needed? (This question is unrelated to your patch.)
> 
> i'm not the original author so difficult to answer authoritatively. but
> if i have to speculate i'd say it might be to work around the windows
> pthread_join stub being implemented as a noop. specifically it didn't
> communicate the return value from the start_routine.
> 
> the recently added rte_thread_join addresses this (which
> rte_control_thread_create uses) and could remove ret parameter and to
> avoid touching the new function implementation in the future it can not
> use ret now.
> 
> i'll make this change in v3.

also did not do this, once looked at i was concerned about inter-mixing
too much semantic change in the implementation of a couple of the
functions to accomplish it. it is best revisited when the old api is
removed.

> > >+ * the error is ignored and a debug message is logged.
> > >+ *
> > >+ * @param thread
> > >+ *   Filled with the thread id of the new created thread.
> > >+ * @param name
> > >+ *   The name of the control thread (max 16 characters including '\0').
> > 
> > Is there a constant for this limit?
> 
> i have a series introducing rte_lcore_{set,get}_name api that introduces
> a constant (probably i'll post it today). as of this series there is no
> constant.

change in v3, i forgot we were talking about the thread length limit and
not the lcore name length limit and there was already a preprocessor
definition for the limit. api documentation comment was updated in v3.

thanks!