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

[Ravi Kerur <rkerur@gmail.com>]

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.
>

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?

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