Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 27 Jul 2015 13:40:08 -0700
Ravi Kerur <rkerur@gmail.com> wrote:

> On Sun, Jul 26, 2015 at 2:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > Hi Ravi,
> > It seems to be a nice improvement but it needs some cleanup.
> >
> > Checkpatch returns some errors.
> >
> > 2015-04-22 14:06, Ravi Kerur:
> > > use pthread_setname_np and pthread_set_name_np for Linux and
> > > FreeBSD respectively.
> > > Restrict pthread name len to 16 via config for both Linux and FreeBSD.
> >
> > One of the most important part of the commit message is to answer why.
> > Here you probably should explain that the goal is to help debugging.
> >
> 
> Sure will do it and will run checkpatch before next version.
> 
> >
> > >  #
> > > +# Max pthread name len
> > > +#
> > > +CONFIG_RTE_MAX_THREAD_NAME_LEN=16
> >
> > It doesn't have to be configurable. A define would be sufficient.
> >

Definitely should not be in the DPDK config.
It is property of system, which change some config parameter doesn't change.

Manpage implies it is fixed in glibc.

       The pthread_setname_np() function can be used to  set  a
       unique  name  for  a  thread,  which can be useful for debugging multi‐
       threaded applications.  The thread name  is  a  meaningful  C  language
       string, whose length is restricted to 16 characters, including the ter‐
       minating null byte ('\0').  

> Had run into issues(reported by Bruce) as name_len = 32 worked fine for
> FreeBSD but not for Linux, hence thought of making it configurable for
> Linux/FreeBSD and be able to have different name len's. Found out that
> there is also a definition PTHREAD_MAX_NAMELEN_NP, if it's available on
> both Linux and FreeBSD  I will use this, else should I restrict name_len =
> 16?

Not that I see on Linux
$ find /usr/include -type f | xargs grep PTHREAD_MAX_NAMELEN_NP



> >
> > > +             snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> > "print-stats");
> >
> > Why not put this line just before use of thread_name?
> >
> 
> Will do it.
> 
> >
> > > +
> > > +             ret = pthread_create(&tid, NULL, (void*)print_stats, NULL
> > );
> > > +
> > > +             if (ret != 0)
> > > +                     rte_exit(EXIT_FAILURE,
> > > +                             "Cannot create print-stats thread\n");
> > > +
> > > +             ret = pthread_setname_np(tid, thread_name);
> > > +
> > > +             if (ret != 0)
> > > +                     RTE_LOG(ERR, VHOST_CONFIG,
> > > +                             "Cannot set print-stats name\n");
> > [...]
> >
> > > +             pthread_set_name_np(lcore_config[i].thread_id,
> > > +                                     (const char *)thread_name);
> >
> > Is const casting really needed?
> >
> 
> Function signature has a (const char *) for second parameter, I will double
> check and remove if not needed.

You can always pass a char * when function takes const char *