[PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[Garrett Cooper]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2237 bytes --]

Hi,
 	When porting a newer version of OFED to FreeBSD and running the compile 
against clang with all of the warnings enabled, I ran into build failures 
because osm_log, etc assumes that pthread_t is an integral type (which is valid 
on Linux, but not necessarily correct on other platforms). The fix I 
implemented was attached, but that doesn't necessarily seem correct either, so 
I was wondering what was trying to be gained by printing out the thread ID 
(especially because syslog(9) isn't necessarily an asynch safe syscall [1]).
Thanks,
-Garrett

1. http://linux.die.net/man/7/signal

>From 55c3bafa1d866dc045e16fbf792dc64d03d25431 Mon Sep 17 00:00:00 2001
From: Garrett Cooper <garrett.cooper-nKaf5Oxk4VbQT0dZR+AlfA@public.gmane.org>
Date: Thu, 6 Dec 2012 21:41:17 -0800
Subject: [PATCH 2/2] Use %p when printing out pthread_t in the logging fns on FreeBSD

A [slightly] more portable way to do this would be to use
pthread_set(_)?name_np, but even that is not super well-defined across
BSD versions, Linux, and OSX. See
http://stackoverflow.com/questions/2369738/can-i-set-the-name-of-a-thread-in-pthreads-linux
for more details.
---
  contrib/ofed/management/opensm/opensm/osm_log.c | 9 +++++++++
  1 file changed, 9 insertions(+)

diff --git a/contrib/ofed/management/opensm/opensm/osm_log.c 
b/contrib/ofed/management/opensm/opensm/osm_log.c
index deadc57..f4ef394 100644
--- a/contrib/ofed/management/opensm/opensm/osm_log.c
+++ b/contrib/ofed/management/opensm/opensm/osm_log.c
@@ -189,7 +189,11 @@ _retry:
  _retry:
  	ret =
  	    fprintf(p_log->out_port,
+#if defined(__FreeBSD__)
+		    "%s %02d %02d:%02d:%02d %06d [%p] 0x%02x -> %s",
+#else
  		    "%s %02d %02d:%02d:%02d %06d [%04X] 0x%02x -> %s",
+#endif
  		    (result.tm_mon <
  		     12 ? month_str[result.tm_mon] : "???"),
  		    result.tm_mday, result.tm_hour, result.tm_min,
@@ -302,7 +306,12 @@ _retry:
  _retry:
  	ret =
  	    fprintf(p_log->out_port,
+#if defined(__FreeBSD__)
+		    "%s %02d %02d:%02d:%02d %06d [%p] 0x%02x -> %s",
+#else
  		    "%s %02d %02d:%02d:%02d %06d [%04X] 0x%02x -> %s",
+
+#endif
  		    (result.tm_mon <
  		     12 ? month_str[result.tm_mon] : "???"),
  		    result.tm_mday, result.tm_hour, result.tm_min,
-- 
1.8.0.1

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1646 bytes --]

From 55c3bafa1d866dc045e16fbf792dc64d03d25431 Mon Sep 17 00:00:00 2001
From: Garrett Cooper <garrett.cooper@isilon.com>
Date: Thu, 6 Dec 2012 21:41:17 -0800
Subject: [PATCH 2/2] Use %p when printing out pthread_t in the logging fns on
 FreeBSD

A [slightly] more portable way to do this would be to use
pthread_set(_)?name_np, but even that is not super well-defined across
BSD versions, Linux, and OSX. See
http://stackoverflow.com/questions/2369738/can-i-set-the-name-of-a-thread-in-pthreads-linux
for more details.
---
 contrib/ofed/management/opensm/opensm/osm_log.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/contrib/ofed/management/opensm/opensm/osm_log.c b/contrib/ofed/management/opensm/opensm/osm_log.c
index deadc57..f4ef394 100644
--- a/contrib/ofed/management/opensm/opensm/osm_log.c
+++ b/contrib/ofed/management/opensm/opensm/osm_log.c
@@ -189,7 +189,11 @@ _retry:
 _retry:
 	ret =
 	    fprintf(p_log->out_port,
+#if defined(__FreeBSD__)
+		    "%s %02d %02d:%02d:%02d %06d [%p] 0x%02x -> %s",
+#else
 		    "%s %02d %02d:%02d:%02d %06d [%04X] 0x%02x -> %s",
+#endif
 		    (result.tm_mon <
 		     12 ? month_str[result.tm_mon] : "???"),
 		    result.tm_mday, result.tm_hour, result.tm_min,
@@ -302,7 +306,12 @@ _retry:
 _retry:
 	ret =
 	    fprintf(p_log->out_port,
+#if defined(__FreeBSD__)
+		    "%s %02d %02d:%02d:%02d %06d [%p] 0x%02x -> %s",
+#else
 		    "%s %02d %02d:%02d:%02d %06d [%04X] 0x%02x -> %s",
+
+#endif
 		    (result.tm_mon <
 		     12 ? month_str[result.tm_mon] : "???"),
 		    result.tm_mday, result.tm_hour, result.tm_min,
-- 
1.8.0.1

Re: [PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[Bart Van Assche]

On 12/09/12 10:26, Garrett Cooper wrote:
> +#if defined(__FreeBSD__)
> +            "%s %02d %02d:%02d:%02d %06d [%p] 0x%02x -> %s",
> +#else
>               "%s %02d %02d:%02d:%02d %06d [%04X] 0x%02x -> %s",
> +#endif

Please cast the pthread_t value to an unsigned long long or another 
integral type. Such #ifdefs are a pain to maintain.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[Garrett Cooper]

On Dec 9, 2012, at 1:54 AM, Bart Van Assche wrote:

> On 12/09/12 10:26, Garrett Cooper wrote:
>> +#if defined(__FreeBSD__)
>> +            "%s %02d %02d:%02d:%02d %06d [%p] 0x%02x -> %s",
>> +#else
>>              "%s %02d %02d:%02d:%02d %06d [%04X] 0x%02x -> %s",
>> +#endif
> 
> Please cast the pthread_t value to an unsigned long long or another integral type. Such #ifdefs are a pain to maintain.

	I was fishing for an answer as to what the proper fix would be. I hate hacking in #ifdefs if there's a better way to do things (Net-SNMP/Perl are perfect examples of #ifdefs gone wrong).
	pthread_t is implemented as a pointer to a structure on FreeBSD, but the address is not even guaranteed to be unique (there's a less documented syscall for grabbing the thread id on FreeBSD, but I figured that people on the list didn't want me to go programming FreeBSDisms into opensm :)…). The fact of the matter is that POSIX says that the type should be assumed to be opaque and the problem with the examples I've provided in osm_log.c is that they don't honor the opaqueness.
Thanks,
-Garrett--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[Alex Netes]

Hi Garrett,

> Hi,
>  	When porting a newer version of OFED to FreeBSD and running the
> compile against clang with all of the warnings enabled, I ran into build
> failures because osm_log, etc assumes that pthread_t is an integral type
> (which is valid on Linux, but not necessarily correct on other platforms). The
> fix I implemented was attached, but that doesn't necessarily seem correct
> either, so I was wondering what was trying to be gained by printing out the
> thread ID (especially because syslog(9) isn't necessarily an asynch safe
> syscall [1]).

One reason to use "sort of thread id" would be to determine specific flows,
when looking through the log. Why it's important to use async-safe functions
in that case?

I think that instead of using pthread_self(), it's better to use gettid() in Linux
case. Is there any equivalent for this in BSD?


-- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[Garrett Cooper]

Hi Alex,

On Sun, Dec 9, 2012 at 9:43 AM, Alex Netes <alexne-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> Hi Garrett,
>
>> Hi,
>>       When porting a newer version of OFED to FreeBSD and running the
>> compile against clang with all of the warnings enabled, I ran into build
>> failures because osm_log, etc assumes that pthread_t is an integral type
>> (which is valid on Linux, but not necessarily correct on other platforms). The
>> fix I implemented was attached, but that doesn't necessarily seem correct
>> either, so I was wondering what was trying to be gained by printing out the
>> thread ID (especially because syslog(9) isn't necessarily an asynch safe
>> syscall [1]).
>
> One reason to use "sort of thread id" would be to determine specific flows,
> when looking through the log. Why it's important to use async-safe functions
> in that case?

Seems reasonable (and that was my guess, but I wanted to check :)..).

> I think that instead of using pthread_self(), it's better to use gettid() in Linux
> case. Is there any equivalent for this in BSD?

There isn't really, but there's an undocumented macro that gets to the
thread ID from a 'struct pthread' object (the "opaque type" for
pthread_t on FreeBSD). From lib/libthr/thread/thr_private.h:

336 /*
337  * lwpid_t is 32bit but kernel thr API exports tid as long type
338  * in very earily date.
339  */
340 #define TID(thread)     ((uint32_t) ((thread)->tid))

Need to check with the POSIX folks and freebsd-standards@ to see
whether or not we can get something properly standardized between
FreeBSD and Linux to handle this; being able to know the thread ID for
logging seems like a reasonable request that should be fulfilled in
multithreaded applications that the standards folks just seem to have
overlooked when creating the pthread APIs (it would also be
interesting to see whether or not C11 resolves this, but I hear it
introduces another can of worms with its half-baked threading
implementation).

Thanks,
-Garrett
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] osm_log printing incorrectly assumes that pthread_t is not opaque type

[Jason Gunthorpe]

On Sun, Dec 09, 2012 at 11:56:55AM -0800, Garrett Cooper wrote:

> > One reason to use "sort of thread id" would be to determine specific flows,
> > when looking through the log. Why it's important to use async-safe functions
> > in that case?
> 
> Seems reasonable (and that was my guess, but I wanted to check :)..).

There is no such thing as a 'thread id integer' in posix land. If you
want to number your threads then you need to use a thread local
variable to do it. That is probably alot cheaper than calling gettid
anyhow..

On Linux there is some value to using tid's for your ids because you
can strace that tid individually if you want, but I can't think of
many other uses..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html