Re: [PATCH] opensm: Add per module logging support
On 6/4/2012 8:28 PM, Jim Foraker wrote: > > On Wed, 2012-05-30 at 14:30 -0700, Hal Rosenstock wrote: >> Add _v2 APIs for osm_log, osm_log_is_active, and osm_log_msg_box >> Also, add these to libopensm.map >> Add _V2 forms of the various OSM_LOG macros > Why are the _V2 macros necessary? It appears that the calling > semantics/effect are the same, so why create new macros instead of > reusing the old ones? In particular, it seems like you could move the > FILE_ID defines above the #includes, and then in osm_log.h say things > like: > > #ifdef FILE_ID > #define OSM_LOG_ENTER( OSM_LOG_PTR ) \ > osm_log_v2( OSM_LOG_PTR, OSM_LOG_FUNCS, FILE_ID, \ > "%s: [\n", __func__); > #else > #define OSM_LOG_ENTER( OSM_LOG_PTR ) \ > osm_log( OSM_LOG_PTR, OSM_LOG_FUNCS, \ > "%s: [\n", __func__); > #endif Good idea; I'll rework the patch. > Or just define FILE_ID everywhere (it seems to be missing from many > of the libvendor files) Most of libvendor files are historical and on verge of deprecation so it wasn't worth the effort there. > and make OSM_LOG* always call the v2 functions. > It seems like this would drastically reduce the patch's footprint. If by footprint, you mean changed lines of code (rather than object code size), then yes, this makes sense. >> >> All of above pass additional parameter to identify module. >> Each module defines a unique FILE_ID from 0-254. >> 255 is reserved to indicate "name not found". >> FILE_ID must match index in osm_subnet.c:module_name_str >> table. Note also currently that subnet's >> per_mod_log_tbl is 128 entries (which is enough >> more than the number of modules). >> > Why are there so many magic numbers here? There are 2 magic numbers. One is to use 255 as not found rather than explicit status. 128 is to limit the array size but that can be eliminated at the cost of the array size being larger. > It seems like > per_mod_log_tbl's size could be defined in terms of the final value in > osm_file_ids_enum (ie, you could add a OSM_FILE_LAST_ENTRY to the enum). > If find_module_name() is changed to return an int (which is what > osm_log_v2() already uses as file_id's type, so consistency would be > improved), it could return -1 on failure, making the limit at 255 > irrelevant as well. Yes, that is an alternative to using 255 as indicating this. > Also, how is this expected to work with plugins? Will plugins be > expected to use their own logging? Plugins still use the original/coarse logging or their own logging. In order to use the per module logging, they'd need to reserve file IDs. > Would it be desirable to put FILE_ID (or better yet, module name) > into the log messages as well? This would make it a bit easier to find > where the relevant code is. We could then phase out the "ERR XXYY" > scheme, particularly since the XX numbers do not seem to correlate at > all with FILE_ID. If we decide to change the ERR logs to include name all we need is __FILE__. If we want to keep current ERR XXYY scheme, for modules with FILE_ID that could be used as the XX in ERR XXYY. The issues with doing this are with original logging uses where that's not the case and any conflict between FILE_ID as the XX in the ERR and the current ERR XXs used now. Anyhow, this seems like a follow on patch and needs more discussion/thought. -- Hal > Jim > >> >> Use version 2 API (osm_log_v2) and macros (OSM_LOG_V2_) >> in OpenSM client code >> >> Added options to enable/disable per module logging and >> it's configuration file >> >> Config file format >> Set of lines with module name and logging level as follows: >> >> where: >> module name is file name including .c >> separator is either = , space, or tab >> logging level is the same levels as used in the coarse/overall logging >> as follows: >> >> BITLOG LEVEL ENABLED >> - >> 0x01 - ERROR (error messages) >> 0x02 - INFO (basic messages, low volume) >> 0x04 - VERBOSE (interesting stuff, moderate volume) >> 0x08 - DEBUG (diagnostic, high volume) >> 0x10 - FUNCS (function entry/exit, very high volume) >> 0x20 - FRAMES (dumps all SMP and GMP frames) >> 0x40 - ROUTING (dump FDB routing information) >> 0x80 - currently unused >> >> Add parsing of the configuration file into table of log levels >> indexed by FILE_ID >> >> Added osm_get_log_per_module routine to obtain the configured info >> for a supplied module name. This is a new library function. >> >> Signed-off-by: Hal Rosenstock >> --- >> As this patch is too large for email, please find this @ >> https://www.openfabrics.org/~halr/0001-opensm-Add-per-module-logging-support.patch >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
Re: [PATCH] opensm: Add per module logging support
On Wed, 2012-05-30 at 14:30 -0700, Hal Rosenstock wrote: > Add _v2 APIs for osm_log, osm_log_is_active, and osm_log_msg_box > Also, add these to libopensm.map > Add _V2 forms of the various OSM_LOG macros Why are the _V2 macros necessary? It appears that the calling semantics/effect are the same, so why create new macros instead of reusing the old ones? In particular, it seems like you could move the FILE_ID defines above the #includes, and then in osm_log.h say things like: #ifdef FILE_ID #define OSM_LOG_ENTER( OSM_LOG_PTR ) \ osm_log_v2( OSM_LOG_PTR, OSM_LOG_FUNCS, FILE_ID, \ "%s: [\n", __func__); #else #define OSM_LOG_ENTER( OSM_LOG_PTR ) \ osm_log( OSM_LOG_PTR, OSM_LOG_FUNCS, \ "%s: [\n", __func__); #endif Or just define FILE_ID everywhere (it seems to be missing from many of the libvendor files) and make OSM_LOG* always call the v2 functions. It seems like this would drastically reduce the patch's footprint. > > All of above pass additional parameter to identify module. > Each module defines a unique FILE_ID from 0-254. > 255 is reserved to indicate "name not found". > FILE_ID must match index in osm_subnet.c:module_name_str > table. Note also currently that subnet's > per_mod_log_tbl is 128 entries (which is enough > more than the number of modules). > Why are there so many magic numbers here? It seems like per_mod_log_tbl's size could be defined in terms of the final value in osm_file_ids_enum (ie, you could add a OSM_FILE_LAST_ENTRY to the enum). If find_module_name() is changed to return an int (which is what osm_log_v2() already uses as file_id's type, so consistency would be improved), it could return -1 on failure, making the limit at 255 irrelevant as well. Also, how is this expected to work with plugins? Will plugins be expected to use their own logging? Would it be desirable to put FILE_ID (or better yet, module name) into the log messages as well? This would make it a bit easier to find where the relevant code is. We could then phase out the "ERR XXYY" scheme, particularly since the XX numbers do not seem to correlate at all with FILE_ID. Jim > > Use version 2 API (osm_log_v2) and macros (OSM_LOG_V2_) > in OpenSM client code > > Added options to enable/disable per module logging and > it's configuration file > > Config file format > Set of lines with module name and logging level as follows: > > where: > module name is file name including .c > separator is either = , space, or tab > logging level is the same levels as used in the coarse/overall logging > as follows: > > BITLOG LEVEL ENABLED > - > 0x01 - ERROR (error messages) > 0x02 - INFO (basic messages, low volume) > 0x04 - VERBOSE (interesting stuff, moderate volume) > 0x08 - DEBUG (diagnostic, high volume) > 0x10 - FUNCS (function entry/exit, very high volume) > 0x20 - FRAMES (dumps all SMP and GMP frames) > 0x40 - ROUTING (dump FDB routing information) > 0x80 - currently unused > > Add parsing of the configuration file into table of log levels > indexed by FILE_ID > > Added osm_get_log_per_module routine to obtain the configured info > for a supplied module name. This is a new library function. > > Signed-off-by: Hal Rosenstock > --- > As this patch is too large for email, please find this @ > https://www.openfabrics.org/~halr/0001-opensm-Add-per-module-logging-support.patch > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] opensm: Add per module logging support
On 5/30/2012 7:03 PM, Ira Weiny wrote: > On Wed, 30 May 2012 17:30:50 -0400 > Hal Rosenstock wrote: > >> >> Add _v2 APIs for osm_log, osm_log_is_active, and osm_log_msg_box >> Also, add these to libopensm.map >> Add _V2 forms of the various OSM_LOG macros >> >> All of above pass additional parameter to identify module. >> Each module defines a unique FILE_ID from 0-254. >> 255 is reserved to indicate "name not found". >> FILE_ID must match index in osm_subnet.c:module_name_str >> table. Note also currently that subnet's >> per_mod_log_tbl is 128 entries (which is enough >> more than the number of modules). > > This is a great idea for debugging but do you really need to define the > "FILE_ID's"? Could you use the __FILE__ macro to do a lookup? Or do you > think that would affect performance? That was the first implementation and the performance impact was too much. > Also do you really think that 255 files is enough id's to reserve? Yes but it's hard to predict the future. We're only at 88 now. We've only added a few per year and this leaves >160 more. -- Hal > Ira > >> >> Use version 2 API (osm_log_v2) and macros (OSM_LOG_V2_) >> in OpenSM client code >> >> Added options to enable/disable per module logging and >> it's configuration file >> >> Config file format >> Set of lines with module name and logging level as follows: >> >> where: >> module name is file name including .c >> separator is either = , space, or tab >> logging level is the same levels as used in the coarse/overall logging >> as follows: >> >> BITLOG LEVEL ENABLED >> - >> 0x01 - ERROR (error messages) >> 0x02 - INFO (basic messages, low volume) >> 0x04 - VERBOSE (interesting stuff, moderate volume) >> 0x08 - DEBUG (diagnostic, high volume) >> 0x10 - FUNCS (function entry/exit, very high volume) >> 0x20 - FRAMES (dumps all SMP and GMP frames) >> 0x40 - ROUTING (dump FDB routing information) >> 0x80 - currently unused >> >> Add parsing of the configuration file into table of log levels >> indexed by FILE_ID >> >> Added osm_get_log_per_module routine to obtain the configured info >> for a supplied module name. This is a new library function. >> >> Signed-off-by: Hal Rosenstock >> --- >> As this patch is too large for email, please find this @ >> https://www.openfabrics.org/~halr/0001-opensm-Add-per-module-logging-support.patch >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] opensm: Add per module logging support
On Wed, 30 May 2012 17:30:50 -0400 Hal Rosenstock wrote: > > Add _v2 APIs for osm_log, osm_log_is_active, and osm_log_msg_box > Also, add these to libopensm.map > Add _V2 forms of the various OSM_LOG macros > > All of above pass additional parameter to identify module. > Each module defines a unique FILE_ID from 0-254. > 255 is reserved to indicate "name not found". > FILE_ID must match index in osm_subnet.c:module_name_str > table. Note also currently that subnet's > per_mod_log_tbl is 128 entries (which is enough > more than the number of modules). This is a great idea for debugging but do you really need to define the "FILE_ID's"? Could you use the __FILE__ macro to do a lookup? Or do you think that would affect performance? Also do you really think that 255 files is enough id's to reserve? Ira > > Use version 2 API (osm_log_v2) and macros (OSM_LOG_V2_) > in OpenSM client code > > Added options to enable/disable per module logging and > it's configuration file > > Config file format > Set of lines with module name and logging level as follows: > > where: > module name is file name including .c > separator is either = , space, or tab > logging level is the same levels as used in the coarse/overall logging > as follows: > > BITLOG LEVEL ENABLED > - > 0x01 - ERROR (error messages) > 0x02 - INFO (basic messages, low volume) > 0x04 - VERBOSE (interesting stuff, moderate volume) > 0x08 - DEBUG (diagnostic, high volume) > 0x10 - FUNCS (function entry/exit, very high volume) > 0x20 - FRAMES (dumps all SMP and GMP frames) > 0x40 - ROUTING (dump FDB routing information) > 0x80 - currently unused > > Add parsing of the configuration file into table of log levels > indexed by FILE_ID > > Added osm_get_log_per_module routine to obtain the configured info > for a supplied module name. This is a new library function. > > Signed-off-by: Hal Rosenstock > --- > As this patch is too large for email, please find this @ > https://www.openfabrics.org/~halr/0001-opensm-Add-per-module-logging-support.patch > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Ira Weiny Member of Technical Staff Lawrence Livermore National Lab 925-423-8008 wei...@llnl.gov -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
