Re: help with pthread_t deprecation / api changes

[Thomas Monjalon <thomas@monjalon.net>]

13/12/2022 18:38, Tyler Retzlaff:
> On Tue, Dec 13, 2022 at 09:32:06AM +0100, Thomas Monjalon wrote:
> > 12/12/2022 18:45, Tyler Retzlaff:
> > > On Sun, Dec 11, 2022 at 08:50:48AM +0100, Thomas Monjalon wrote:
> > > > 10/12/2022 00:49, Tyler Retzlaff:
> > > > > On Fri, Dec 09, 2022 at 10:13:44PM +0100, Thomas Monjalon wrote:
> > > > > > 09/12/2022 21:06, Tyler Retzlaff:
> > > > > > > On Fri, Dec 09, 2022 at 08:48:14AM -0800, Stephen Hemminger wrote:
> > > > > > > > On Fri, 09 Dec 2022 08:53:57 +0100
> > > > > > > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > > > 
> > > > > > > > > > > If some execution environment doesn't support thread names, it could return a string that makes it possible for a human to identify the thread, e.g. the tread id. Again, this is assuming that it is only used for debugging, trace, and similar.  
> > > > > > > > > > 
> > > > > > > > > > i think this raises a good question. is the purpose of setting a thread name
> > > > > > > > > > meant to be something we can use from the application or is it something that
> > > > > > > > > > is for debugging diagnostics and may be a best effort?  
> > > > > > > > > 
> > > > > > > 
> > > > > > > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > > 
> > > > > > > > I think yes it is only for debugging.
> > > > > > > > So best effort looks to be a good approach.
> > > > > > > > I'm not sure you need to replace the functions.
> > > > > > > > Can you just complete the implementations?
> > > > > > > 
> > > > > > > the patch series put forward allows a set / get name per-lcore, where
> > > > > > > you get implicit (but not exposed via the eal api) call to underlying
> > > > > > > platform thread setname.
> > > > > > 
> > > > > > I don't understand how lcore ID and thread ID are connected.
> > > > > > You can run multiple control threads on a single lcore.
> > > > > > 
> > > > > 
> > > > > correct.
> > > > > 
> > > > > the new public api allows the set of a name on an lcore only. as a
> > > > > side-effect if the platform supports it the name is also set on the
> > > > > thread_id associated with the lcore (from lcore_config[].thread_id).
> > > > > 
> > > > > for control threads you just get the side-effect if the platform
> > > > > supports it, otherwise it is a noop.
> > > > 
> > > > What does it mean? which side effect? what must be supported?
> > > > 
> > > > > if we want set / get name at all this seemed the most usable balance
> > > > > between application consumption with debug use where available. if this
> > > > > isn't acceptable then i would suggest we simply remove both
> > > > > rte_thread_{get,set}name because as a debugging facility we cannot offer
> > > > > a consistent abstracted api which means it shouldn't be in the eal at
> > > > > all.
> > > > 
> > > > Why it cannot be consistent?
> > > > Please be more precise.
> > > > 
> > > 
> > > sorry i thought you had been looking at our implementation, let me
> > > summarize.
> > > 
> > > here are the differences between the underlying platform capabilities
> > > that prohibit both get and set. it's not a matter of just providing missing
> > > implementation for a specific platform.
> > > 
> > > set thread name:
> > >   freebsd
> > >     - set reports no failure, but may silently fail
> > >     - uncertain what name length limit is
> > >   linux
> > >     - set not available in older glibc
> > >     - current rte wrapper silently truncates name length
> > >   windows
> > >     - set always available
> > >     - uncertain what name length limit is
> > > 
> > > get thread name:
> > >   freebsd
> > >     - not available at all
> > >   linux
> > >     - get not available in older glibc
> > >     - get can fail
> > >   windows
> > >     - get always available
> > >     - get can fail
> > > 
> > > keep in mind the purpose of an abstraction is that the application *does
> > > not* have to do conditional evaluation on a per-platform basis around
> > > call sites. once you start putting #ifdef RTE_EXEC_ENV_XXX into code you
> > > failed and i presume we want none of the use of eal to be adorned with
> > > that.
> > > 
> > > at first glance you might think oh, well if get isn't supported then
> > > just return some default string or an empty string but even that is a
> > > violation of the abstraction that leaks implementation detail.
> > > 
> > > i.e. assuming success set() success  get(set()) should also succeed
> > > without conditional compilation of the code.
> > > 
> > > the common abstraction that can be reasonably provided explicitly
> > > operating a thread is something like.
> > > 
> > >   * for set you can provide a watered down version that doesn't report
> > >     failure and silently truncates and ignores errors within the
> > >     implementation. if it works it works if it doesn't you just don't
> > >     know i.e. best effort.
> > >   * for get it cannot be provided consistently, the platforms simply
> > >     aren't providing what is needed.
> > > 
> > > for background one of the request to expose the native platform thread
> > > id was to access the best effort behavior for the thread associated with
> > > an lcore. discussion on list highlighted the constraint that this should
> > > be done without exposing platform specific detail via the eal api.
> > > 
> > > http://mails.dpdk.org/archives/dev/2022-October/253411.html
> > > 
> > > as mentioned in a previous mail i provided a series that accomplishes
> > > this as a side effect of an api that can be consistently provided when
> > > available on the platform, it has 2 benefits.
> > >   * it does not expose the platform-specific native thread handle.
> > >   * for lcores it keeps the name in the application memory so it is
> > >     available in crash/coredumps.
> > > 
> > > so we have 2 options.
> > > 
> > > 1. a watered down set (no get) that is fire and forget and reports no
> > >    failure and maybe it works, maybe it doesn't depending on your platform.
> > > 2. the lcore set / get which is basically (1) for the threads associated
> > >    with lcores but provides some additional features that are supportable
> > >    in the api surface. (set/get, stored in application namespace).
> > 
> > Given thread name is not critical at all, I think best effort is OK.
> > We could make clear in the API documentation that it is not reliable.
> > 
> > I don't think implementing thread name in the specific case of
> > datapath lcore is a real improvement.
> 
> Okay, just one final confirmation. This is what we would like?
> 
> * completely remove the existing rte_thread_getname api.
>     - by implication this means remove the 1 use of it in eal in
>       logging.
> 
> * introduce a new void rte_thread_set_name(rte_thread_t, const char *name)
>   that:
>     - returns void (does not fail), but in cases it can be detected will
>       log a DEBUG level log message.
>     - quietly truncates the name (if longer) to RTE_MAX_THREAD_NAME_LEN on
>       all platforms.
>     - document that it is best effort and only works if the stars align
>       for the target platform.
> 
> * there will be no unit test, since the set doesn't fail and there is no
>   get to validate the set.
> 
> once i get confirmation i'll update the series.

Just my opinion: this proposal is my preference, yes.
What others think?