Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> > >
> > > 30/07/2021 23:44, Honnappa Nagarahalli:
> > > > The current expected behaviour of the function
> > > > rte_ctrl_thread_create is rigid which makes the implementation of the
> function complex.
> > > > Make the expected behaviour abstract to allow for simplified
> > > > implementation.
> > > >
> > > > With this change, the calls to pthread_setaffinity_np can be moved
> > > > to the control thread. This will avoid the use of
> > > > pthread_barrier_wait and simplify the synchronization mechanism
> > > > between rte_ctrl_thread_create and the calling thread.
> > > >
> > > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > ---
> > > > +* eal: The expected behaviour of the function
> > > > +``rte_ctrl_thread_create``
> > > > +  abstracted to allow for simplified implementation. The new
> > > > +behaviour is
> > > > +  as follows:
> > > > +  Creates a control thread with the given name. The affinity of
> > > > +the new
> > > > +  thread is based on the CPU affinity retrieved at the time
> > > > +rte_eal_init()
> > > > +  was called, the dataplane and service lcores are then excluded.
> > >
> > > I don't understand what is different of the current API:
> > >  * Wrapper to pthread_create(), pthread_setname_np() and
> > >  * pthread_setaffinity_np(). The affinity of the new thread is based
> > >  * on the CPU affinity retrieved at the time rte_eal_init() was
> > > called,
> > >  * the dataplane and service lcores are then excluded.
> > My concern is for the word "Wrapper". I am not sure how much we are
> bound by that to keep the code as a "wrapper".
> > The new patch does not change the high level behavior.
> 
> I am ok to remove the word "wrapper" from the description, and I agree it can
> be better described without quoting the pthread_* functions.
> 
> > Are you saying you are ok with the patch without the deprecation notice?
> 
> I don't think it requires a deprecation notice if the API and ABI is left
> unchanged. To be honnest, I find a bit hard to understand what is really
> changed by reading the deprecation notice:
Thanks Olivier. I agree, I was also not sure. The term "wrapper" made me feel that we are defining certain return codes to the application.

At the macro level, I think the expected behavior remains the same.

> 
> > +* eal: The expected behaviour of the function
> > +``rte_ctrl_thread_create``
> > +  abstracted to allow for simplified implementation. The new
> > +behaviour is
> > +  as follows:
> > +  Creates a control thread with the given name. The affinity of the
> > +new
> > +  thread is based on the CPU affinity retrieved at the time
> > +rte_eal_init()
> > +  was called, the dataplane and service lcores are then excluded.
> 
> I'll send my comments to your patch:
> http://patches.dpdk.org/project/dpdk/patch/20210802051652.3611-1-
> honnappa.nagarahalli@arm.com/
> 
> 
> Thanks,
> Olivier