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

[Ravi Kerur <rkerur@gmail.com>]

On Mon, Jul 27, 2015 at 2:09 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> 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
>
>
That's correct on Linux and look like it's not defined for FreeBSD as
well.  FreeBSD man page doesn't mention anything about string length too.

>
>
> > >
> > > > +             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 *
>

That's correct, will remove it.