Re: [ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

2023-04-06 Thread Songtao Zhan
On Thu, Apr 06, 2023 at 10:37:50PM +0200, Ilya Maximets wrote:
> On 4/6/23 13:08, zhanst1 wrote:
> > On Wed, Apr 05, 2023 at 03:13:47PM +0200, Eelco Chaudron wrote:
> >>
> >>
> >> On 31 Mar 2023, at 10:11, Songtao Zhan wrote:
> >>
> >>> To: d...@openvswitch.org,
> >>> i.maxim...@ovn.org
> >>>
> >>> The name of the current thread consists of a name with a maximum
> >>> length of 16 bytes and a thread ID. The final name may be longer
> >>> than 16 bytes. If the name is longer than 16 bytes, the thread
> >>> name will fail to be set
> >>
> >> I replied to the v1, but did not see any response, so I repeat this on v2:
> >>
> >> “
> >> Thanks for the patch, do you have examples of when this happens? Maybe we 
> >> should also change the thread naming to avoid this?
> >>
> >> See one additional item below.
> >> “
> >>
> >>>
> >>> Signed-off-by: Songtao Zhan 
> >>> ---
> >>>
> >>> Notes:
> >>> v2:
> >>>  - modify code formatting and comments
> >>>
> >>>  lib/util.c | 4 
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/lib/util.c b/lib/util.c
> >>> index 96a71550d..b0eb9f343 100644
> >>> --- a/lib/util.c
> >>> +++ b/lib/util.c
> >>> @@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
> >>>  free(subprogram_name_set(pname));
> >>>
> >>>  #if HAVE_GLIBC_PTHREAD_SETNAME_NP
> >>> +/* The maximum thead name including '\0' supported is 16. */
> >>> +if (strlen(pname) > 15) {
> >>> +pname[15] = '\0';
> >>
> >> Not sure what is better, but would it make sense to use the last upper 
> >> characters? This way if we do truncate we know the internal thread id, as 
> >> naming in OVS, in general, is “xasprintf("%s%u", aux.name, id)”.
> >> If we do this we could add some indication is was truncated, like 
> >> “LONGTHREAD123456” would become “>NGTHREAD123456”.
> > 
> > Thanks for your review, Assume that there are two threads, 
> > disableXXthreadidAA and enableXXthreadidBB. If we keep the last 
> > character, like XXthreadAA and XXthreadBB, we cannot get the thread 
> > fuction by thread name. The thread id is not that important because we 
> > can know it by 'ps'
> 
> Yeah, truncating from the end seems more reasonable to me
> as we will likely truncate less meaningful parts.  However,
> it might make sense to add something like '>' as at 14th
> position to highlight that the name was truncated.  This
> will eat yet another symbol, but it will be harder to
> mistake one thread for another.
> 
> What do you think?
> 
> BTW, do you see threads with too long names in the current OVS?

When I developed a new feature, we created a new thread. I found that
the thread name failed to be created, so I submitted this path
> Best regards, Ilya Maximets
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

2023-04-06 Thread Ilya Maximets
On 4/6/23 13:08, zhanst1 wrote:
> On Wed, Apr 05, 2023 at 03:13:47PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 31 Mar 2023, at 10:11, Songtao Zhan wrote:
>>
>>> To: d...@openvswitch.org,
>>> i.maxim...@ovn.org
>>>
>>> The name of the current thread consists of a name with a maximum
>>> length of 16 bytes and a thread ID. The final name may be longer
>>> than 16 bytes. If the name is longer than 16 bytes, the thread
>>> name will fail to be set
>>
>> I replied to the v1, but did not see any response, so I repeat this on v2:
>>
>> “
>> Thanks for the patch, do you have examples of when this happens? Maybe we 
>> should also change the thread naming to avoid this?
>>
>> See one additional item below.
>> “
>>
>>>
>>> Signed-off-by: Songtao Zhan 
>>> ---
>>>
>>> Notes:
>>> v2:
>>>  - modify code formatting and comments
>>>
>>>  lib/util.c | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/util.c b/lib/util.c
>>> index 96a71550d..b0eb9f343 100644
>>> --- a/lib/util.c
>>> +++ b/lib/util.c
>>> @@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
>>>  free(subprogram_name_set(pname));
>>>
>>>  #if HAVE_GLIBC_PTHREAD_SETNAME_NP
>>> +/* The maximum thead name including '\0' supported is 16. */
>>> +if (strlen(pname) > 15) {
>>> +pname[15] = '\0';
>>
>> Not sure what is better, but would it make sense to use the last upper 
>> characters? This way if we do truncate we know the internal thread id, as 
>> naming in OVS, in general, is “xasprintf("%s%u", aux.name, id)”.
>> If we do this we could add some indication is was truncated, like 
>> “LONGTHREAD123456” would become “>NGTHREAD123456”.
> 
> Thanks for your review, Assume that there are two threads, 
> disableXXthreadidAA and enableXXthreadidBB. If we keep the last 
> character, like XXthreadAA and XXthreadBB, we cannot get the thread 
> fuction by thread name. The thread id is not that important because we 
> can know it by 'ps'

Yeah, truncating from the end seems more reasonable to me
as we will likely truncate less meaningful parts.  However,
it might make sense to add something like '>' as at 14th
position to highlight that the name was truncated.  This
will eat yet another symbol, but it will be harder to
mistake one thread for another.

What do you think?

BTW, do you see threads with too long names in the current OVS?

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

2023-04-06 Thread zhanst1
On Wed, Apr 05, 2023 at 03:13:47PM +0200, Eelco Chaudron wrote:
> 
> 
> On 31 Mar 2023, at 10:11, Songtao Zhan wrote:
> 
> > To: d...@openvswitch.org,
> > i.maxim...@ovn.org
> >
> > The name of the current thread consists of a name with a maximum
> > length of 16 bytes and a thread ID. The final name may be longer
> > than 16 bytes. If the name is longer than 16 bytes, the thread
> > name will fail to be set
> 
> I replied to the v1, but did not see any response, so I repeat this on v2:
> 
> “
> Thanks for the patch, do you have examples of when this happens? Maybe we 
> should also change the thread naming to avoid this?
> 
> See one additional item below.
> “
> 
> >
> > Signed-off-by: Songtao Zhan 
> > ---
> >
> > Notes:
> > v2:
> >  - modify code formatting and comments
> >
> >  lib/util.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/util.c b/lib/util.c
> > index 96a71550d..b0eb9f343 100644
> > --- a/lib/util.c
> > +++ b/lib/util.c
> > @@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
> >  free(subprogram_name_set(pname));
> >
> >  #if HAVE_GLIBC_PTHREAD_SETNAME_NP
> > +/* The maximum thead name including '\0' supported is 16. */
> > +if (strlen(pname) > 15) {
> > +pname[15] = '\0';
> 
> Not sure what is better, but would it make sense to use the last upper 
> characters? This way if we do truncate we know the internal thread id, as 
> naming in OVS, in general, is “xasprintf("%s%u", aux.name, id)”.
> If we do this we could add some indication is was truncated, like 
> “LONGTHREAD123456” would become “>NGTHREAD123456”.

Thanks for your review, Assume that there are two threads, 
disableXXthreadidAA and enableXXthreadidBB. If we keep the last 
character, like XXthreadAA and XXthreadBB, we cannot get the thread 
fuction by thread name. The thread id is not that important because we 
can know it by 'ps'

> > +}
> >  pthread_setname_np(pthread_self(), pname);
> >  #elif HAVE_NETBSD_PTHREAD_SETNAME_NP
> >  pthread_setname_np(pthread_self(), "%s", pname);
> > -- 
> > 2.31.1
> >
> >
> >
> >
> > zhan...@chinatelecom.cn
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

2023-04-05 Thread Eelco Chaudron


On 31 Mar 2023, at 10:11, Songtao Zhan wrote:

> To: d...@openvswitch.org,
> i.maxim...@ovn.org
>
> The name of the current thread consists of a name with a maximum
> length of 16 bytes and a thread ID. The final name may be longer
> than 16 bytes. If the name is longer than 16 bytes, the thread
> name will fail to be set

I replied to the v1, but did not see any response, so I repeat this on v2:

“
Thanks for the patch, do you have examples of when this happens? Maybe we 
should also change the thread naming to avoid this?

See one additional item below.
“

>
> Signed-off-by: Songtao Zhan 
> ---
>
> Notes:
> v2:
>  - modify code formatting and comments
>
>  lib/util.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/lib/util.c b/lib/util.c
> index 96a71550d..b0eb9f343 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
>  free(subprogram_name_set(pname));
>
>  #if HAVE_GLIBC_PTHREAD_SETNAME_NP
> +/* The maximum thead name including '\0' supported is 16. */
> +if (strlen(pname) > 15) {
> +pname[15] = '\0';

Not sure what is better, but would it make sense to use the last upper 
characters? This way if we do truncate we know the internal thread id, as 
naming in OVS, in general, is “xasprintf("%s%u", aux.name, id)”.
If we do this we could add some indication is was truncated, like 
“LONGTHREAD123456” would become “>NGTHREAD123456”.

> +}
>  pthread_setname_np(pthread_self(), pname);
>  #elif HAVE_NETBSD_PTHREAD_SETNAME_NP
>  pthread_setname_np(pthread_self(), "%s", pname);
> -- 
> 2.31.1
>
>
>
>
> zhan...@chinatelecom.cn
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

2023-04-04 Thread Simon Horman
On Fri, Mar 31, 2023 at 04:11:07PM +0800, Songtao Zhan wrote:
> To: d...@openvswitch.org,
> i.maxim...@ovn.org
> 
> The name of the current thread consists of a name with a maximum
> length of 16 bytes and a thread ID. The final name may be longer
> than 16 bytes. If the name is longer than 16 bytes, the thread
> name will fail to be set
> 
> Signed-off-by: Songtao Zhan 

Reviewed-by: Simon Horman 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

2023-03-31 Thread Songtao Zhan
To: d...@openvswitch.org,
i.maxim...@ovn.org

The name of the current thread consists of a name with a maximum
length of 16 bytes and a thread ID. The final name may be longer
than 16 bytes. If the name is longer than 16 bytes, the thread
name will fail to be set

Signed-off-by: Songtao Zhan 
---

Notes:
v2:
 - modify code formatting and comments

 lib/util.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/util.c b/lib/util.c
index 96a71550d..b0eb9f343 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
 free(subprogram_name_set(pname));

 #if HAVE_GLIBC_PTHREAD_SETNAME_NP
+/* The maximum thead name including '\0' supported is 16. */
+if (strlen(pname) > 15) {
+pname[15] = '\0';
+}
 pthread_setname_np(pthread_self(), pname);
 #elif HAVE_NETBSD_PTHREAD_SETNAME_NP
 pthread_setname_np(pthread_self(), "%s", pname);
-- 
2.31.1




zhan...@chinatelecom.cn
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev