[PATCH 1/3] log: Add log_check_level function
This commit adds this predicate function which can be used to avoid the execution of code if a certain log level is not enabled. The function will only return 0 (false), if it is sure that a logging call for the same facility and level will not produce any output. This safety criterion shall ensure, that no logging output is lost due to the use of this predicate as a guard. On the other hand, even if the predicate returns != 0 (true), no logging output might get generated by a similar logging command. Note that the current implementation is not focussed on performance, which could be improved by using a lookup table instead of iterating through every target. Sponsored-by: On-Waves ehf --- include/osmocom/core/logging.h | 1 + src/logging.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/include/osmocom/core/logging.h b/include/osmocom/core/logging.h index 1c159d0..290b33d 100644 --- a/include/osmocom/core/logging.h +++ b/include/osmocom/core/logging.h @@ -198,6 +198,7 @@ void logp2(int subsys, unsigned int level, const char *file, int line, int cont, const char *format, ...) __attribute__ ((format (printf, 6, 7))); int log_init(const struct log_info *inf, void *talloc_ctx); +int log_check_level(int subsys, unsigned int level); /* context management */ void log_reset_context(void); diff --git a/src/logging.c b/src/logging.c index 876964a..c7b1999 100644 --- a/src/logging.c +++ b/src/logging.c @@ -865,4 +865,43 @@ int log_init(const struct log_info *inf, void *ctx) return 0; } +/*! \brief Check whether a log entry will be generated. + * \returns != 0 if a log entry might get generated by at least one target */ +int log_check_level(int subsys, unsigned int level) +{ + struct log_target *tar; + + if (subsys < 0) + subsys = subsys_lib2index(subsys); + + if (subsys > osmo_log_info->num_cat) + subsys = DLGLOBAL; + + /* TODO: The following could/should be cached (update on config) */ + + llist_for_each_entry(tar, &osmo_log_target_list, entry) { + struct log_category *category; + + category = &tar->categories[subsys]; + /* subsystem is not supposed to be logged */ + if (!category->enabled) + continue; + + /* Check the global log level */ + if (tar->loglevel != 0 && level < tar->loglevel) + continue; + + /* Check the category log level */ + if (tar->loglevel == 0 && category->loglevel != 0 && + level < category->loglevel) + continue; + + /* This might get logged (ignoring filters) */ + return 1; + } + + /* We are sure, that this will not be logged. */ + return 0; +} + /*! @} */ -- 1.9.1
Currently the LOGP/DEBUGP arguments are always evaluated even if no logging will happen at all. This can be expensive, for instance if hexdumps or pretty printed object names are generated. This causes high base load especially on embedded devices and is a major part of CPU usage e.g. of the osmo-pcu. This commit uses the log_check_level function to avoid the evaluation of the parameters if it is known in advance, that no logging entry will be generated. Sponsored-by: On-Waves ehf --- include/osmocom/core/logging.h | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/include/osmocom/core/logging.h b/include/osmocom/core/logging.h index 290b33d..e51487b 100644 --- a/include/osmocom/core/logging.h +++ b/include/osmocom/core/logging.h @@ -19,8 +19,18 @@ #define DEBUG #ifdef DEBUG -#define DEBUGP(ss, fmt, args...) logp(ss, __FILE__, __LINE__, 0, fmt, ## args) -#define DEBUGPC(ss, fmt, args...) logp(ss, __FILE__, __LINE__, 1, fmt, ## args) +#define DEBUGP(ss, fmt, args...) \ + do { \ + if (log_check_level(ss, LOGL_DEBUG)) \ + logp(ss, __FILE__, __LINE__, 0, fmt, ## args); \ + } while(0) + +#define DEBUGPC(ss, fmt, args...) \ + do { \ + if (log_check_level(ss, LOGL_DEBUG)) \ + logp(ss, __FILE__, __LINE__, 1, fmt, ## args); \ + } while(0) + #else #define DEBUGP(xss, fmt, args...) #define DEBUGPC(ss, fmt, args...) @@ -39,7 +49,10 @@ void logp(int subsys, const char *file, int line, int cont, const char *format, * \param[in] args variable argument list */ #define LOGP(ss, level, fmt, args...) \ - logp2(ss, level, __FILE__, __LINE__, 0, fmt, ##args) + do { \ + if (log_check_level(ss, level)) \ + logp2(ss, level, __FILE__, __LINE__, 0, fmt, ##args); \ + } while(0) /*! \brief Continue a log message through the Osmocom logging framework * \param[in] ss logging subsystem (e.g. \ref DLGLOBAL) @@ -48,7 +61,10 @@ void logp(int subsys, const char *file, int line, int cont, const char *format, * \param[in] args variable argument list */ #define LOGPC(ss, level, fmt, args...) \ - logp2(ss, level, __FILE__, __LINE__, 1, fmt, ##args) + do { \ + if (log_check_level(ss, level)) \ + logp2(ss, level, __FILE__, __LINE__, 1, fmt, ##args); \ + } while(0) /*! \brief different log levels */ #define LOGL_DEBUG 1 /*!< \brief debugging information */ -- 1.9.1
This commit adds OSMO_ASSERTs for mandatory conditions related to log_check_level, and fprintfs for optional conditions, since it is always safe for log_check_level to return != 0. Sponsored-by: On-Waves ehf --- tests/logging/logging_test.c | 10 ++++++++++ tests/logging/logging_test.err | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/logging/logging_test.c b/tests/logging/logging_test.c index b263f90..3c8bac4 100644 --- a/tests/logging/logging_test.c +++ b/tests/logging/logging_test.c @@ -78,16 +78,26 @@ int main(int argc, char **argv) log_parse_category_mask(stderr_target, "DRLL:DCC"); log_parse_category_mask(stderr_target, "DRLL"); DEBUGP(DCC, "You should not see this\n"); + if (log_check_level(DMM, LOGL_DEBUG) != 0) + fprintf(stderr, "log_check_level did not catch this case\n"); log_parse_category_mask(stderr_target, "DRLL:DCC"); DEBUGP(DRLL, "You should see this\n"); + OSMO_ASSERT(log_check_level(DRLL, LOGL_DEBUG) != 0); DEBUGP(DCC, "You should see this\n"); + OSMO_ASSERT(log_check_level(DCC, LOGL_DEBUG) != 0); DEBUGP(DMM, "You should not see this\n"); + if (log_check_level(DMM, LOGL_DEBUG) != 0) + fprintf(stderr, "log_check_level did not catch this case\n"); + OSMO_ASSERT(filter_called == 0); log_set_all_filter(stderr_target, 0); DEBUGP(DRLL, "You should not see this and filter is called\n"); OSMO_ASSERT(filter_called == 1); + if (log_check_level(DRLL, LOGL_DEBUG) != 0) + fprintf(stderr, + "log_check_level did not catch this case (filter)\n"); return 0; } diff --git a/tests/logging/logging_test.err b/tests/logging/logging_test.err index b59d2e8..c3b67cc 100644 --- a/tests/logging/logging_test.err +++ b/tests/logging/logging_test.err @@ -1,3 +1,3 @@ [1;31mYou should see this [0;m[1;32mYou should see this -[0;m \ No newline at end of file +[0;mlog_check_level did not catch this case (filter) -- 1.9.1
On 17 Nov 2015, at 11:52, Jacob Erlbeck <jerlbeck@sysmocom.de> wrote:
Hi Jacob, I am in favour of such a patch to prevent doing work (formatting arguments) when it will not be used.
+ /* This might get logged (ignoring filters) */
What is the reasoning for this part? The below does not work yet but shows the general idea. If the check routine already finds the first target that will generate an output, then let us use it? This is why I have put an output parameter in there and pass it a long. The thing that is not pretty is how to print "tar" and everything that follows it, naturally I would like to use a do {} while() loop but not roll the llist_entry by hand (or maybe I do). It is no look-up table but we avoid to duplicate the selection criteria and to iterate over targets (the one or two) we already have looked at? diff --git a/include/osmocom/core/logging.h b/include/osmocom/core/logging.h index e51487b..ed21e57 100644 --- a/include/osmocom/core/logging.h +++ b/include/osmocom/core/logging.h @@ -11,6 +11,8 @@ #include <stdarg.h> #include <osmocom/core/linuxlist.h> +struct log_target; + /*! \brief Maximum number of logging contexts */ #define LOG_MAX_CTX 8 /*! \brief Maximum number of logging filters */ @@ -18,17 +20,23 @@ #define DEBUG + + #ifdef DEBUG #define DEBUGP(ss, fmt, args...) \ do { \ - if (log_check_level(ss, LOGL_DEBUG)) \ - logp(ss, __FILE__, __LINE__, 0, fmt, ## args); \ + /* potentially shadow things */ \ + struct log_target *osmo_log_tgt; \ + if (log_check_level(ss, LOGL_DEBUG, &osmo_log_tgt)) \ + logp(ss, __FILE__, __LINE__, 0, osmo_log_tgt, fmt, ## args); \ } while(0) #define DEBUGPC(ss, fmt, args...) \ do { \ - if (log_check_level(ss, LOGL_DEBUG)) \ - logp(ss, __FILE__, __LINE__, 1, fmt, ## args); \ + /* potentially shadow things */ \ + struct log_target *osmo_log_tgt; \ + if (log_check_level(ss, LOGL_DEBUG, &osmo_log_tgt)) \ + logp(ss, __FILE__, __LINE__, 1, osmo_log_tgt, fmt, ## args); \ } while(0) #else @@ -38,9 +46,10 @@ void osmo_vlogp(int subsys, int level, const char *file, int line, - int cont, const char *format, va_list ap); + int cont, struct log_target *initial_target, + const char *format, va_list ap); -void logp(int subsys, const char *file, int line, int cont, const char *format, ...) __attribute__ ((format (printf, 5, 6))); +void logp(int subsys, const char *file, int line, int cont, struct log_target *initial_target, const char *format, ...) __attribute__ ((format (printf, 6, 7))); /*! \brief Log a new message through the Osmocom logging framework * \param[in] ss logging subsystem (e.g. \ref DLGLOBAL) @@ -50,8 +59,10 @@ void logp(int subsys, const char *file, int line, int cont, const char *format, */ #define LOGP(ss, level, fmt, args...) \ do { \ - if (log_check_level(ss, level)) \ - logp2(ss, level, __FILE__, __LINE__, 0, fmt, ##args); \ + /* potentially shadow things */ \ + struct log_target *osmo_log_tgt; \ + if (log_check_level(ss, level, &osmo_log_tgt)) \ + logp2(ss, level, __FILE__, __LINE__, 0, osmo_log_tgt, fmt, ##args); \ } while(0) /*! \brief Continue a log message through the Osmocom logging framework @@ -62,8 +73,10 @@ void logp(int subsys, const char *file, int line, int cont, const char *format, */ #define LOGPC(ss, level, fmt, args...) \ do { \ - if (log_check_level(ss, level)) \ - logp2(ss, level, __FILE__, __LINE__, 1, fmt, ##args); \ + /* potentially shadow things */ \ + struct log_target *osmo_log_tgt; \ + if (log_check_level(ss, level, &osmo_log_tgt)) \ + logp2(ss, level, __FILE__, __LINE__, 1, osmo_log_tgt, fmt, ##args); \ } while(0) /*! \brief different log levels */ @@ -107,8 +120,6 @@ struct log_context { void *ctx[LOG_MAX_CTX+1]; }; -struct log_target; - /*! \brief Log filter function */ typedef int log_filter(const struct log_context *ctx, struct log_target *target); @@ -211,10 +222,11 @@ struct log_target { /* use the above macros */ void logp2(int subsys, unsigned int level, const char *file, - int line, int cont, const char *format, ...) - __attribute__ ((format (printf, 6, 7))); + int line, int cont, struct log_target *iniial_target, + const char *format, ...) + __attribute__ ((format (printf, 7, 8))); int log_init(const struct log_info *inf, void *talloc_ctx); -int log_check_level(int subsys, unsigned int level); +int log_check_level(int subsys, unsigned int level, struct log_target **out_tar); /* context management */ void log_reset_context(void); diff --git a/src/logging.c b/src/logging.c index c7b1999..952b52e 100644 --- a/src/logging.c +++ b/src/logging.c @@ -310,35 +310,64 @@ err: target->output(target, level, buf); } -/*! \brief vararg version of logging function */ -void osmo_vlogp(int subsys, int level, const char *file, int line, - int cont, const char *format, va_list ap) +static inline int map_subsys(int subsys) { - struct log_target *tar; - if (subsys < 0) subsys = subsys_lib2index(subsys); if (subsys > osmo_log_info->num_cat) subsys = DLGLOBAL; + return subsys; +} - llist_for_each_entry(tar, &osmo_log_target_list, entry) { - struct log_category *category; - int output = 0; - va_list bp; +static inline int check_log_to_target(struct log_target *tar, int subsys, int level) +{ + struct log_category *category; - category = &tar->categories[subsys]; - /* subsystem is not supposed to be logged */ - if (!category->enabled) - continue; + category = &tar->categories[subsys]; - /* Check the global log level */ - if (tar->loglevel != 0 && level < tar->loglevel) - continue; + /* subsystem is not supposed to be logged */ + if (!category->enabled) + return 0; - /* Check the category log level */ - if (tar->loglevel == 0 && category->loglevel != 0 && - level < category->loglevel) + /* Check the global log level */ + if (tar->loglevel != 0 && level < tar->loglevel) + return 0; + + /* Check the category log level */ + if (tar->loglevel == 0 && category->loglevel != 0 && + level < category->loglevel) + return 0; + + /* TODO: Check the filter/selector too? */ + return 1; +} + +/*! \brief vararg version of logging function */ +void osmo_vlogp(int subsys, int level, const char *file, int line, + int cont, struct log_target *ini_tar, const char *format, va_list ap) +{ + struct log_target *tar = ini_tar; + + subsys = map_subsys(subsys); + + + if ((tar->filter_map & LOG_FILTER_ALL) == 0 + || (osmo_log_info->filter_fn && osmo_log_info->filter_fn(&log_context, tar))) { + va_list bp; + /* According to the manpage, vsnprintf leaves the value of ap + * in undefined state. Since _output uses vsnprintf and it may + * be called several times, we have to pass a copy of ap. */ + va_copy(bp, ap); + _output(tar, subsys, level, file, line, cont, format, bp); + va_end(bp); + } + + llist_for_each_entry_continue(tar, &osmo_log_target_list, entry) { + int output = 0; + va_list bp; + + if (!check_log_to_target(tar, subsys, level)) continue; /* Apply filters here... if that becomes messy we will @@ -363,22 +392,23 @@ void osmo_vlogp(int subsys, int level, const char *file, int line, /*! \brief logging function used by DEBUGP() macro */ void logp(int subsys, const char *file, int line, int cont, - const char *format, ...) + struct log_target *ini_tar, const char *format, ...) { va_list ap; va_start(ap, format); - osmo_vlogp(subsys, LOGL_DEBUG, file, line, cont, format, ap); + osmo_vlogp(subsys, LOGL_DEBUG, file, line, cont, ini_tar, format, ap); va_end(ap); } /*! \brief logging function used by LOGP() macro */ -void logp2(int subsys, unsigned int level, const char *file, int line, int cont, const char *format, ...) +void logp2(int subsys, unsigned int level, const char *file, int line, int cont, + struct log_target *ini_tar, const char *format, ...) { va_list ap; va_start(ap, format); - osmo_vlogp(subsys, level, file, line, cont, format, ap); + osmo_vlogp(subsys, level, file, line, cont, ini_tar, format, ap); va_end(ap); } @@ -867,40 +897,26 @@ int log_init(const struct log_info *inf, void *ctx) /*! \brief Check whether a log entry will be generated. * \returns != 0 if a log entry might get generated by at least one target */ -int log_check_level(int subsys, unsigned int level) +int log_check_level(int subsys, unsigned int level, struct log_target **out_tar) { struct log_target *tar; - if (subsys < 0) - subsys = subsys_lib2index(subsys); - - if (subsys > osmo_log_info->num_cat) - subsys = DLGLOBAL; + subsys = map_subsys(subsys); /* TODO: The following could/should be cached (update on config) */ llist_for_each_entry(tar, &osmo_log_target_list, entry) { - struct log_category *category; - - category = &tar->categories[subsys]; - /* subsystem is not supposed to be logged */ - if (!category->enabled) - continue; - - /* Check the global log level */ - if (tar->loglevel != 0 && level < tar->loglevel) - continue; - - /* Check the category log level */ - if (tar->loglevel == 0 && category->loglevel != 0 && - level < category->loglevel) + if (!check_log_to_target(tar, subsys, level)) continue; /* This might get logged (ignoring filters) */ + /* HHPF: Why? */ + *out_tar = tar; return 1; } /* We are sure, that this will not be logged. */ + *out_tar = NULL; return 0; } diff --git a/tests/logging/logging_test.c b/tests/logging/logging_test.c index 3c8bac4..7d0ba6b 100644 --- a/tests/logging/logging_test.c +++ b/tests/logging/logging_test.c @@ -68,6 +68,7 @@ const struct log_info log_info = { int main(int argc, char **argv) { struct log_target *stderr_target; + struct log_target *select_target; log_init(&log_info, NULL); stderr_target = log_target_create_stderr(); @@ -78,24 +79,31 @@ int main(int argc, char **argv) log_parse_category_mask(stderr_target, "DRLL:DCC"); log_parse_category_mask(stderr_target, "DRLL"); DEBUGP(DCC, "You should not see this\n"); - if (log_check_level(DMM, LOGL_DEBUG) != 0) + if (log_check_level(DMM, LOGL_DEBUG, &select_target) != 0) fprintf(stderr, "log_check_level did not catch this case\n"); + OSMO_ASSERT(select_target == stderr_target); log_parse_category_mask(stderr_target, "DRLL:DCC"); DEBUGP(DRLL, "You should see this\n"); - OSMO_ASSERT(log_check_level(DRLL, LOGL_DEBUG) != 0); + select_target = NULL; + OSMO_ASSERT(log_check_level(DRLL, LOGL_DEBUG, &select_target) != 0); + OSMO_ASSERT(select_target == stderr_target); DEBUGP(DCC, "You should see this\n"); - OSMO_ASSERT(log_check_level(DCC, LOGL_DEBUG) != 0); + select_target = NULL; + OSMO_ASSERT(log_check_level(DCC, LOGL_DEBUG, &select_target) != 0); + OSMO_ASSERT(select_target == stderr_target); DEBUGP(DMM, "You should not see this\n"); - if (log_check_level(DMM, LOGL_DEBUG) != 0) + select_target = NULL; + if (log_check_level(DMM, LOGL_DEBUG, &select_target) != 0) fprintf(stderr, "log_check_level did not catch this case\n"); + OSMO_ASSERT(select_target == stderr_target); OSMO_ASSERT(filter_called == 0); log_set_all_filter(stderr_target, 0); DEBUGP(DRLL, "You should not see this and filter is called\n"); OSMO_ASSERT(filter_called == 1); - if (log_check_level(DRLL, LOGL_DEBUG) != 0) + if (log_check_level(DRLL, LOGL_DEBUG, &select_target) != 0) fprintf(stderr, "log_check_level did not catch this case (filter)\n");
Hi Holger, On 21.12.2015 14:17, Holger Freyther wrote:
On 17 Nov 2015, at 11:52, Jacob Erlbeck <jerlbeck@sysmocom.de> wrote:
+ /* This might get logged (ignoring filters) */
What is the reasoning for this part?
I wanted to have a weaker criterion than 'return true if and only if something would be logged' to be more flexible when it comes to optimization. I had lookup tables in mind which cache the max level per facility, and which get updated when e.g. log_set_log_level is called. This would be O(1) consuming just a few CPU cycles per log, but would not help when filtering is used. And I didn't really understand filters when I wrote that commit. Including them into the log_check_level decision definitely makes sense if I want to use log DEBUG level stuff on an embedded or high load system.
The below does not work yet but shows the general idea. If the check routine already finds the first target that will generate an output, then let us use it? This is why I have put an output parameter in there and pass it a long.
Reusing the target pointer for logging seems like a nice feature, but it bloats the API and only gains anything, if something gets actually logged which is probably much more expensive than rescanning the probably very few targets that came in front and are in the CPU cache already anyway. So I wouldn't do that optimization unless profiling would tell me to do so. I thought of using log_check_level explicitly in user code, too. E.g. if a log statement would require some preparation that can be done at argument position. That gets a little clumsy with that additional parameter. I did some profiling with the CPU and the current log_check_level was way below the few remaining calls to logp.
The thing that is not pretty is how to print "tar" and everything that follows it, naturally I would like to use a do {} while() loop but not roll the llist_entry by hand (or maybe I do).
It is no look-up table but we avoid to duplicate the selection criteria and to iterate over targets (the one or two) we already have looked at?
I guess that a) not ignoring the filter and b) doing a O(1) pre-check (possibly inlined) will gain us more. Jacob -- - Jacob Erlbeck <jerlbeck@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
On 21 Dec 2015, at 15:13, Jacob Erlbeck <jerlbeck@sysmocom.de> wrote:
Hi Holger,
On 21.12.2015 14:17, Holger Freyther wrote:
On 17 Nov 2015, at 11:52, Jacob Erlbeck <jerlbeck@sysmocom.de> wrote:
+ /* This might get logged (ignoring filters) */
What is the reasoning for this part?
I wanted to have a weaker criterion than 'return true if and only if something would be logged' to be more flexible when it comes to optimization. I had lookup tables in mind which cache the max level per facility, and which get updated when e.g. log_set_log_level is called. This would be O(1) consuming just a few CPU cycles per log, but would not help when filtering is used.
And I didn't really understand filters when I wrote that commit. Including them into the log_check_level decision definitely makes sense if I want to use log DEBUG level stuff on an embedded or high load system.
Okay maybe I am missing a case here but it feels like we don't save anything by not calling it. So if the log level is good for output we return 1 and then do the full work. It doesn't really matter which function we do this work in. And as this doesn't matter we can leave it out. I posted a patch that removes the selection criteria and subsys code clone.
The below does not work yet but shows the general idea. If the check routine already finds the first target that will generate an output, then let us use it? This is why I have put an output parameter in there and pass it a long.
Reusing the target pointer for logging seems like a nice feature, but it bloats the API and only gains anything, if something gets actually logged which is probably much more expensive than rescanning the probably very few targets that came in front and are in the CPU cache already anyway. So I wouldn't do that optimization unless profiling would tell me to do so.
I consider LOGP, LOGPC, DEBUGP, DEBUGPC as the API. I don't think we have any direct callers of osmo_vlogp, logp, logp2. The change to start from the target that has been found is trivial and if we can avoid doing work without increasing the memory usage then we should do it. We do not have a libsomocore.map but if we do I would not export logp, logp2, osmo_vlogp and the log_check_level functions.
I thought of using log_check_level explicitly in user code, too. E.g. if a log statement would require some preparation that can be done at argument position. That gets a little clumsy with that additional parameter.
I did some profiling with the CPU and the current log_check_level was way below the few remaining calls to logp.
The thing that is not pretty is how to print "tar" and everything that follows it, naturally I would like to use a do {} while() loop but not roll the llist_entry by hand (or maybe I do).
It is no look-up table but we avoid to duplicate the selection criteria and to iterate over targets (the one or two) we already have looked at?
I guess that a) not ignoring the filter and b) doing a O(1) pre-check (possibly inlined) will gain us more.
I don't think we want to inline all pre-checks (list traversal). Your patch helps to avoid calling osmo_hexdump, gsm_lchan_name and others. I remove the code clones and continue (yeah redo the integer compares and a load) from where log_check_level stopped. It is a sound approach. cheers holger
On 21.12.2015 15:52, Holger Freyther wrote:
On 21 Dec 2015, at 15:13, Jacob Erlbeck <jerlbeck@sysmocom.de> wrote:
Hi Holger,
On 21.12.2015 14:17, Holger Freyther wrote:
On 17 Nov 2015, at 11:52, Jacob Erlbeck <jerlbeck@sysmocom.de> wrote:
+ /* This might get logged (ignoring filters) */
What is the reasoning for this part?
I wanted to have a weaker criterion than 'return true if and only if something would be logged' to be more flexible when it comes to optimization. I had lookup tables in mind which cache the max level per facility, and which get updated when e.g. log_set_log_level is called. This would be O(1) consuming just a few CPU cycles per log, but would not help when filtering is used.
And I didn't really understand filters when I wrote that commit. Including them into the log_check_level decision definitely makes sense if I want to use log DEBUG level stuff on an embedded or high load system.
Okay maybe I am missing a case here but it feels like we don't save anything by not calling it. So if the log level is good for output we return 1 and then do the full work. It doesn't really matter which function we do this work in.
And as this doesn't matter we can leave it out. I posted a patch that removes the selection criteria and subsys code clone.
Hmm, there seems to be some misunderstanding. While thinking about the filter stuff again, I liked the idea to include the filter test into log_check_level. It would made it possible to DEBUG-log e.g. single subscribers or MS without rendering the application unusable if the load were high (and filters really supported). So please don't remove it from your patch. Jacob -- - Jacob Erlbeck <jerlbeck@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
Hello list, Is this patch still considered for a merge ? It seems that embedded systems will largely benefit from this early check. It may improve osmo-pcu performance, as discussed in "osmo-bts / osmo-pcu profiling results" thread but also all binaries running on low resources systems. I have not received new test equipment yet, but I can give this patch a try as soon as possible if it helps. Thanks.
participants (3)
-
Holger Freyther -
Jacob Erlbeck -
Pierre Baudry