| Message ID | 1471107782.3467.28.camel@perches.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Joe Perches (2): be2iscsi: Coalesce split strings and formats be2iscsi: Use a standard logging style drivers/scsi/be2iscsi/be_cmds.c | 61 +++--- drivers/scsi/be2iscsi/be_iscsi.c | 115 ++++++----- drivers/scsi/be2iscsi/be_main.c | 398 +++++++++++++++++---------------------- drivers/scsi/be2iscsi/be_main.h | 19 +- drivers/scsi/be2iscsi/be_mgmt.c | 99 +++++----- 5 files changed, 314 insertions(+), 378 deletions(-)
On 08/13/16 13:42, Joe Perches wrote: > Joe Perches (2): > be2iscsi: Coalesce split strings and formats > be2iscsi: Use a standard logging style Hello Joe, As one can see in be_main.h the "level" argument of macro beiscsi_log() is ignored for log levels KERN_EMERG, KERN_ALERT, KERN_CRIT and KERN_ERR. So for these log levels beiscsi_log() is a synonym of shost_printk(). Have you considered to replace beiscsi_log() with shost_printk() for these log levels and additionally to change beiscsi_log() for the other log levels into pr_debug()? pr_debug() statements namely already can be enabled and disabled at runtime. If the BEISCSI_LOG_* log category would be embedded in the log text that would allow to eliminate the phba->attr_log_enable structure member. Additionally, pr_debug() has a facility for displaying the source file name and the line number. That would allow to leave out __LINE__ from be2iscsi log statements. I don't think it is useful to have that line number in non-debug be2iscsi log statements. Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2016-08-14 at 14:34 +0000, Bart Van Assche wrote: > On 08/13/16 13:42, Joe Perches wrote: > > Joe Perches (2): > > be2iscsi: Coalesce split strings and formats > > be2iscsi: Use a standard logging style > Hello Joe, Hello Bart. > As one can see in be_main.h the "level" argument of macro beiscsi_log() > is ignored for log levels KERN_EMERG, KERN_ALERT, KERN_CRIT and > KERN_ERR. So for these log levels beiscsi_log() is a synonym of > shost_printk(). Have you considered to replace beiscsi_log() with > shost_printk() for these log levels and additionally to change > beiscsi_log() for the other log levels into pr_debug()? pr_debug() > statements namely already can be enabled and disabled at runtime. If the > BEISCSI_LOG_* log category would be embedded in the log text that would > allow to eliminate the phba->attr_log_enable structure member. > Additionally, pr_debug() has a facility for displaying the source file > name and the line number. That would allow to leave out __LINE__ from > be2iscsi log statements. I don't think it is useful to have that line > number in non-debug be2iscsi log statements. My main consideration for submitting a patch at all was removing the apparent format/argument mismatches. As far as I can grep, only KERN_ERR, KERN_WARNING and KERN_INFO are actually used by be2iscsi today. I agree with the removal of __LINE__ from the macros as its utility is generally pretty low. Besides, using stringify(__LINE__) is almost always smaller object code than a format with "%d", __LINE__. Prefixes like "BC" and "BS" are __FILE__ equivalents, and could be removed as well with something like "%s, kbasename(__FILE__)" used if _really_ desired. I have no issue with defining and using beiscsi_<level> equivalents to shost_printks. I think the test inside beiscsi_log is better removed with multiple specific beiscsi_<level> calls used. I don't know why any KERN_ERR should ever be masked, but perhaps something like: #define beiscsi_printk(level, phba, mask, fmt, ...) \ do { \ if ((mask) & (phba)->attr_log_enable) \ shost_printk(level, phba->shost, fmt, ##__VA_ARGS__); \ } while (0) #define beiscsi_err(phba, mask, fmt, ...) \ beiscsi_printk(KERN_ERR, phba, mask, fmt, ##__VA_ARGS__) #define beiscsi_warn(phba, mask, fmt, ...) \ beiscsi_printk(KERN_WARNING, phba, mask, fmt, ##__VA_ARGS__) #define beiscsi_info(phba, mask, fmt, ...) \ beiscsi_printk(KERN_INFO, phba, mask, fmt, ##__VA_ARGS__) with a sed of the .c files: $ sed -i 's/beiscsi_log(phba, KERN_ERR/beiscsi_err(phba/g' drivers/scsi/be2iscsi/*.c $ sed -i 's/beiscsi_log(phba, KERN_WARNING/beiscsi_warn(phba/g' drivers/scsi/be2iscsi/*.c $ sed -i 's/beiscsi_log(phba, KERN_INFO/beiscsi_info(phba/g' drivers/scsi/be2iscsi/*.c with argument realignment of those lines. All of these are of course up to the actual maintainers of be2iscsi. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/14/16 09:24, Joe Perches wrote: > On Sun, 2016-08-14 at 14:34 +0000, Bart Van Assche wrote: >> As one can see in be_main.h the "level" argument of macro beiscsi_log() >> is ignored for log levels KERN_EMERG, KERN_ALERT, KERN_CRIT and >> KERN_ERR. So for these log levels beiscsi_log() is a synonym of >> shost_printk(). Have you considered to replace beiscsi_log() with >> shost_printk() for these log levels and additionally to change >> beiscsi_log() for the other log levels into pr_debug()? pr_debug() >> statements namely already can be enabled and disabled at runtime. If the >> BEISCSI_LOG_* log category would be embedded in the log text that would >> allow to eliminate the phba->attr_log_enable structure member. >> Additionally, pr_debug() has a facility for displaying the source file >> name and the line number. That would allow to leave out __LINE__ from >> be2iscsi log statements. I don't think it is useful to have that line >> number in non-debug be2iscsi log statements. > > My main consideration for submitting a patch at all > was removing the apparent format/argument mismatches. > > As far as I can grep, only KERN_ERR, KERN_WARNING and > KERN_INFO are actually used by be2iscsi today. > > I agree with the removal of __LINE__ from the macros > as its utility is generally pretty low. > > Besides, using stringify(__LINE__) is almost always > smaller object code than a format with "%d", __LINE__. > > Prefixes like "BC" and "BS" are __FILE__ equivalents, > and could be removed as well with something like > "%s, kbasename(__FILE__)" used if _really_ desired. > > I have no issue with defining and using beiscsi_<level> > equivalents to shost_printks. > > I think the test inside beiscsi_log is better removed > with multiple specific beiscsi_<level> calls used. > > I don't know why any KERN_ERR should ever be masked, > but perhaps something like: > > #define beiscsi_printk(level, phba, mask, fmt, ...) \ > do { \ > if ((mask) & (phba)->attr_log_enable) \ > shost_printk(level, phba->shost, fmt, ##__VA_ARGS__); \ > } while (0) > > #define beiscsi_err(phba, mask, fmt, ...) \ > beiscsi_printk(KERN_ERR, phba, mask, fmt, ##__VA_ARGS__) > #define beiscsi_warn(phba, mask, fmt, ...) \ > beiscsi_printk(KERN_WARNING, phba, mask, fmt, ##__VA_ARGS__) > #define beiscsi_info(phba, mask, fmt, ...) \ > beiscsi_printk(KERN_INFO, phba, mask, fmt, ##__VA_ARGS__) > > with a sed of the .c files: > > $ sed -i 's/beiscsi_log(phba, KERN_ERR/beiscsi_err(phba/g' drivers/scsi/be2iscsi/*.c > $ sed -i 's/beiscsi_log(phba, KERN_WARNING/beiscsi_warn(phba/g' drivers/scsi/be2iscsi/*.c > $ sed -i 's/beiscsi_log(phba, KERN_INFO/beiscsi_info(phba/g' drivers/scsi/be2iscsi/*.c > > with argument realignment of those lines. > > All of these are of course up to the actual maintainers of be2iscsi. Hello Joe, My primary concern is how to enable and disable log messages from user space. Many drivers define their own logging macros and export a bitmask that allows to enable and disable logging messages per category. These bitmask control mechanisms are annoying because figuring out what bit controls which message category requires a search through the driver source code. I'd like to see all these custom logging macros disappear and being replaced by a single mechanism. The "dynamic debug" mechanism e.g. is in my opinion much easier to use than the different custom logging mechanisms. Bart. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2016-08-14 at 17:09 +0000, Bart Van Assche wrote: > My primary concern is how to enable and disable log messages from user > space. Many drivers define their own logging macros and export a bitmask > that allows to enable and disable logging messages per category. These > bitmask control mechanisms are annoying because figuring out what bit > controls which message category requires a search through the driver > source code. I'd like to see all these custom logging macros disappear > and being replaced by a single mechanism. The "dynamic debug" mechanism > e.g. is in my opinion much easier to use than the different custom > logging mechanisms. Dynamic debug doesn't have a bitmask function and still requires looking through the code for lines and format strings. I think you are looking for a system wide equivalent for the ethtool/netif_<level> mechanism. Nothing like that exists currently. Some code uses a bitmask/and, other code uses a level/comparison. Care to propose something? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/14/16 10:29, Joe Perches wrote: > On Sun, 2016-08-14 at 17:09 +0000, Bart Van Assche wrote: >> My primary concern is how to enable and disable log messages from user >> space. Many drivers define their own logging macros and export a bitmask >> that allows to enable and disable logging messages per category. These >> bitmask control mechanisms are annoying because figuring out what bit >> controls which message category requires a search through the driver >> source code. I'd like to see all these custom logging macros disappear >> and being replaced by a single mechanism. The "dynamic debug" mechanism >> e.g. is in my opinion much easier to use than the different custom >> logging mechanisms. > > Dynamic debug doesn't have a bitmask function and > still requires looking through the code for lines > and format strings. > > I think you are looking for a system wide equivalent > for the ethtool/netif_<level> mechanism. > > Nothing like that exists currently. > > Some code uses a bitmask/and, other code uses a > level/comparison. > > Care to propose something? Hello Joe, As far as I can see all that the ethtool msglevel API implements is a mechanism to query and set the log level from user space. What various SCSI drivers implement is not a log level but a log mask mechanism. How about the following approach to associate a name with each bit in a log mask, to export these names to user space and to make it possible to enable/disable messages per log category: * Introduce a variant of pr_debug() that allows to specify a textual representation of the log category (a short string without spaces). * Make the log category names available in /sys/kernel/debug/dynamic_debug/... * Today dynamic debug allows to enable/disable log messages by specifying the source file name, function name, line number, module name and/or format string. My proposal is to make it also possible to enable/disable log messages based on the log category name. Anyway, this is just a proposal. Anyone is welcome to come up with an alternative proposal. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-08-17 at 01:19 +0000, Bart Van Assche wrote: > On 08/14/16 10:29, Joe Perches wrote: > > On Sun, 2016-08-14 at 17:09 +0000, Bart Van Assche wrote: > > > My primary concern is how to enable and disable log messages from user > > > space. [] > > I think you are looking for a system wide equivalent > > for the ethtool/netif_ mechanism. > > > > Nothing like that exists currently. > > > > Some code uses a bitmask/and, other code uses a > > level/comparison. [] > As far as I can see all that the ethtool msglevel API implements is a > mechanism to query and set the log level from user space. What various > SCSI drivers implement is not a log level but a log mask mechanism. How > about the following approach to associate a name with each bit in a log > mask, to export these names to user space and to make it possible to > enable/disable messages per log category: > * Introduce a variant of pr_debug() that allows to specify a textual > representation of the log category (a short string without spaces). > * Make the log category names available in > /sys/kernel/debug/dynamic_debug/... > * Today dynamic debug allows to enable/disable log messages by > specifying the source file name, function name, line number, module > name and/or format string. My proposal is to make it also possible to > enable/disable log messages based on the log category name. Many of these logging mechanisms are not just debug facilities. Perhaps a dynamic_debug control would be inappropriate. There have also been various custom scsi log level facilities like the blogic_msg for the very old BusLogic blogic_msg. These functions also sometimes write into some device-specific buffer. Perhaps the largest problem, if this is to be scsi only rather than system wide, is finding out what and how the various bits in a mask should be used. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h index 30a4606..3f0fbbf 100644 --- a/drivers/scsi/be2iscsi/be_main.h +++ b/drivers/scsi/be2iscsi/be_main.h @@ -1084,11 +1084,12 @@ struct hwi_context_memory { #define __beiscsi_log(phba, level, fmt, arg...) \ shost_printk(level, phba->shost, fmt, __LINE__, ##arg) -#define beiscsi_log(phba, level, mask, fmt, arg...) \ -do { \ - uint32_t log_value = phba->attr_log_enable; \ - if (((mask) & log_value) || (level[1] <= '3')) \ - __beiscsi_log(phba, level, fmt, ##arg); \ -} while (0); +#define beiscsi_log(phba, level, mask, prefix, fmt, ...) \ +do { \ + uint32_t log_value = phba->attr_log_enable; \ + if (((mask) & log_value) || (level[1] <= '3')) \ + __beiscsi_log(phba, level, prefix "_%d: " fmt, \ + ##__VA_ARGS__); \ +} while (0) #endif