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 <tho...@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 <tho...@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.
thanks
