From d3b9f5b37f071b3f909e1735ee85b8047eb70c44 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Wed, 10 May 2017 17:36:08 -0600 Subject: [PATCH] Syslog buffering: Use IOBs to buffer data, not an on-stack buffer --- drivers/serial/serial.c | 3 +- drivers/syslog/Kconfig | 14 +-- drivers/syslog/syslog_stream.c | 164 +++++++++++++++++++++++---------- drivers/syslog/vsyslog.c | 21 ++++- include/nuttx/sched.h | 2 +- include/nuttx/streams.h | 34 ++++++- mm/iob/iob_alloc.c | 3 +- 7 files changed, 171 insertions(+), 70 deletions(-) diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c index 775fa3a97a..49417a9685 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -51,6 +51,7 @@ #include #include +#include #include #include #include @@ -365,7 +366,7 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer, * a little differently. */ - if (up_interrupt_context() || getpid() == 0) + if (up_interrupt_context() || sched_idletask()) { #ifdef CONFIG_SERIAL_REMOVABLE /* If the removable device is no longer connected, refuse to write to diff --git a/drivers/syslog/Kconfig b/drivers/syslog/Kconfig index de0d8a137f..60c136e5a7 100644 --- a/drivers/syslog/Kconfig +++ b/drivers/syslog/Kconfig @@ -86,26 +86,16 @@ config SYSLOG_BUFFER bool "Use buffered output" default n depends on SYSLOG_WRITE + select MM_IOB ---help--- Enables an buffering logic that will be used to serialize debug output from concurrent tasks. This enables allocation of one buffer - per thread, each of size CONFIG_SYSLOG_BUFSIZE. + per thread, each of size CONFIG_IOB_BUFSIZE. The use of SYSLOG buffering is optional. If not enabled, however, then the output from multiple tasks that attempt to generate SYSLOG output may be interleaved and difficult to read. -config SYSLOG_BUFSIZE - int "Output buffer size" - default 32 - range 1 65535 - depends on SYSLOG_BUFFER - ---help--- - If CONFIG_SYSLOG_BUFFER is enabled, then CONFIG_SYSLOG_BUFSIZE - provides the size of the per-thread output buffer. Setting - CONFIG_SYSLOG_BUFSIZE to a value less than one effectly disables - output SYSLOG buffering. - config SYSLOG_INTBUFFER bool "Use interrupt buffer" default n diff --git a/drivers/syslog/syslog_stream.c b/drivers/syslog/syslog_stream.c index 5fb8d19280..04bb0726b0 100644 --- a/drivers/syslog/syslog_stream.c +++ b/drivers/syslog/syslog_stream.c @@ -44,8 +44,9 @@ #include #include -#include #include +#include +#include #include "syslog.h" @@ -59,55 +60,67 @@ static void syslogstream_putc(FAR struct lib_outstream_s *this, int ch) { -#ifdef CONFIG_SYSLOG_BUFFER - FAR struct lib_syslogstream_s *stream = (FAR struct lib_syslogstream_s *)this; - /* Discard carriage returns */ if (ch != '\r') { - /* Add the incoming character to the buffer */ +#ifdef CONFIG_SYSLOG_BUFFER + FAR struct lib_syslogstream_s *stream; + FAR struct iob_s *iob; - stream->buf[stream->nbuf] = ch; - stream->nbuf++; - this->nput++; + DEBUGASSERT(this != NULL); + stream = (FAR struct lib_syslogstream_s *)this; + iob = stream->iob; - /* Is the buffer full? Did we encounter a new line? */ + /* Do we have an IO buffer? */ - if (stream->nbuf >= CONFIG_SYSLOG_BUFSIZE || ch == '\n') + if (iob != NULL) { - /* Yes.. then flush the buffer */ + /* Yes.. Add the incoming character to the buffer */ - (void)this->flush(this); - } - } -#else - int ret; - - /* Try writing until the write was successful or until an irrecoverable - * error occurs. - */ - - do - { - /* Write the character to the supported logging device. On failure, - * syslog_putc returns EOF with the errno value set; - */ - - ret = syslog_putc(ch); - if (ret != EOF) - { + iob->io_data[iob->io_len] = ch; + iob->io_len++; this->nput++; - return; - } - /* The special errno value -EINTR means that syslog_putc() was - * awakened by a signal. This is not a real error and must be - * ignored in this context. - */ - } - while (get_errno() == -EINTR); + /* Is the buffer full? Did we encounter a new line? */ + + if (iob->io_len >= CONFIG_IOB_BUFSIZE || ch == '\n') + { + /* Yes.. then flush the buffer */ + + (void)this->flush(this); + } + } + else #endif + { + int ret; + + /* Try writing until the write was successful or until an + * irrecoverable error occurs. + */ + + do + { + /* Write the character to the supported logging device. On + * failure, syslog_putc returns EOF with the errno value set; + */ + + ret = syslog_putc(ch); + if (ret != EOF) + { + this->nput++; + return; + } + + /* The special errno value -EINTR means that syslog_putc() was + * awakened by a signal. This is not a real error and must be + * ignored in this context. + */ + } + while (get_errno() == -EINTR); + } + } } /**************************************************************************** @@ -117,25 +130,31 @@ static void syslogstream_putc(FAR struct lib_outstream_s *this, int ch) #ifdef CONFIG_SYSLOG_BUFFER static int syslogstream_flush(FAR struct lib_outstream_s *this) { - FAR struct lib_syslogstream_s *stream = (FAR struct lib_syslogstream_s *)this; + FAR struct lib_syslogstream_s *stream; + FAR struct iob_s *iob; int ret = OK; - /* Is there anything buffered? */ + DEBUGASSERT(this != NULL); + stream = (FAR struct lib_syslogstream_s *)this; + iob = stream->iob; - if (stream->nbuf > 0) + /* Do we have an IO buffer? Is there anything buffered? */ + + if (iob != NULL && iob->io_len > 0) { /* Yes write the buffered data */ do { - ssize_t nbytes = syslog_write(stream->buf, stream->nbuf); + ssize_t nbytes = syslog_write((FAR const char *)iob->io_data, + (size_t)iob->io_len); if (nbytes < 0) { ret = -get_errno(); } else { - stream->nbuf = 0; + iob->io_len = 0; ret = OK; } } @@ -151,7 +170,7 @@ static int syslogstream_flush(FAR struct lib_outstream_s *this) ****************************************************************************/ /**************************************************************************** - * Name: syslogstream + * Name: syslogstream_create * * Description: * Initializes a stream for use with the configured syslog interface. @@ -159,23 +178,74 @@ static int syslogstream_flush(FAR struct lib_outstream_s *this) * * Input parameters: * stream - User allocated, uninitialized instance of struct - * lib_syslogstream_s to be initialized. + * lib_lowoutstream_s to be initialized. * * Returned Value: * None (User allocated instance initialized). * ****************************************************************************/ -void syslogstream(FAR struct lib_syslogstream_s *stream) +void syslogstream_create(FAR struct lib_syslogstream_s *stream) { + DEBUGASSERT(stream != NULL); + #ifdef CONFIG_SYSLOG_BUFFER + FAR struct iob_s *iob; + + /* Initialize the common fields */ + stream->public.put = syslogstream_putc; stream->public.flush = syslogstream_flush; stream->public.nput = 0; - stream->nbuf = 0; + + /* Allocate an IOB */ + + iob = iob_alloc(true); + stream->iob = iob; + + if (iob != NULL) + { + /* Initialize the IOB */ + + iob->io_len = 0; + iob->io_offset = 0; + iob->io_pktlen = 0; + } #else stream->public.put = syslogstream_putc; stream->public.flush = lib_noflush; stream->public.nput = 0; #endif } + +/**************************************************************************** + * Name: syslogstream_destroy + * + * Description: + * Free resources held by the syslog stream. + * + * Input parameters: + * stream - User allocated, uninitialized instance of struct + * lib_lowoutstream_s to be initialized. + * + * Returned Value: + * None (Resources freed). + * + ****************************************************************************/ + +#ifdef CONFIG_SYSLOG_BUFFER +void syslogstream_destroy(FAR struct lib_syslogstream_s *stream) +{ + DEBUGASSERT(stream != NULL); + + /* Verify that there is an IOB attached (there should be) */ + + if (stream->iob != NULL) + { + /* Free the IOB */ + + iob_free(stream->iob); + stream->iob = NULL; + } +} +#endif diff --git a/drivers/syslog/vsyslog.c b/drivers/syslog/vsyslog.c index 3ffc521181..cbb8588240 100644 --- a/drivers/syslog/vsyslog.c +++ b/drivers/syslog/vsyslog.c @@ -68,15 +68,17 @@ int _vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap) { struct lib_syslogstream_s stream; + int ret; + #ifdef CONFIG_SYSLOG_TIMESTAMP struct timespec ts; - int ret = -1; /* Get the current time. Since debug output may be generated very early * in the start-up sequence, hardware timer support may not yet be * available. */ + ret = -EAGAIN; if (OSINIT_HW_READY()) { /* Prefer monotonic when enabled, as it can be synchronized to @@ -114,7 +116,7 @@ int _vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap) { /* Use the normal SYSLOG stream */ - syslogstream(&stream); + syslogstream_create(&stream); } #if defined(CONFIG_SYSLOG_TIMESTAMP) @@ -124,5 +126,18 @@ int _vsyslog(int priority, FAR const IPTR char *fmt, FAR va_list *ap) ts.tv_sec, ts.tv_nsec/1000); #endif - return lib_vsprintf(&stream.public, fmt, *ap); + /* Generate the output */ + + ret = lib_vsprintf(&stream.public, fmt, *ap); + +#ifdef CONFIG_SYSLOG_BUFFER + /* Destroy the syslog stream buffer */ + + if (priority != LOG_EMERG) + { + syslogstream_destroy(&stream); + } +#endif + + return ret; } diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index a6031c3183..a1dbe6c5d6 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -760,7 +760,7 @@ FAR struct tcb_s *sched_self(void); void sched_foreach(sched_foreach_t handler, FAR void *arg); -/* Give a task ID, look up the corresponding TCB */ +/* Given a task ID, look up the corresponding TCB */ FAR struct tcb_s *sched_gettcb(pid_t pid); diff --git a/include/nuttx/streams.h b/include/nuttx/streams.h index c8eae2d625..ccb7054f7f 100644 --- a/include/nuttx/streams.h +++ b/include/nuttx/streams.h @@ -190,14 +190,17 @@ struct lib_rawsostream_s }; /* This is a special stream that does buffered character I/O. NOTE that is - * CONFIG_SYSLOG_BUFFER is not defined, it is the same as struct lib_outstream_s */ + * CONFIG_SYSLOG_BUFFER is not defined, it is the same as struct + * lib_outstream_s + */ + +struct iob_s; /* Forward reference */ struct lib_syslogstream_s { struct lib_outstream_s public; #ifdef CONFIG_SYSLOG_BUFFER - unsigned int nbuf; - char buf[CONFIG_SYSLOG_BUFSIZE]; + FAR struct iob_s *iob; #endif }; @@ -357,7 +360,7 @@ void lib_nullinstream(FAR struct lib_instream_s *nullinstream); void lib_nulloutstream(FAR struct lib_outstream_s *nulloutstream); /**************************************************************************** - * Name: syslogstream + * Name: syslogstream_create * * Description: * Initializes a stream for use with the configured syslog interface. @@ -372,7 +375,28 @@ void lib_nulloutstream(FAR struct lib_outstream_s *nulloutstream); * ****************************************************************************/ -void syslogstream(FAR struct lib_syslogstream_s *stream); +void syslogstream_create(FAR struct lib_syslogstream_s *stream); + +/**************************************************************************** + * Name: syslogstream_destroy + * + * Description: + * Free resources held by the syslog stream. + * + * Input parameters: + * stream - User allocated, uninitialized instance of struct + * lib_lowoutstream_s to be initialized. + * + * Returned Value: + * None (Resources freed). + * + ****************************************************************************/ + +#ifdef CONFIG_SYSLOG_BUFFER +void syslogstream_destroy(FAR struct lib_syslogstream_s *stream); +#else +# define syslogstream_destroy(s) +#endif /**************************************************************************** * Name: emergstream diff --git a/mm/iob/iob_alloc.c b/mm/iob/iob_alloc.c index a1c4d6de97..27de294670 100644 --- a/mm/iob/iob_alloc.c +++ b/mm/iob/iob_alloc.c @@ -45,6 +45,7 @@ #include #include +#include #include #include "iob.h" @@ -172,7 +173,7 @@ FAR struct iob_s *iob_alloc(bool throttled) { /* Were we called from the interrupt level? */ - if (up_interrupt_context()) + if (up_interrupt_context() || sched_idletask()) { /* Yes, then try to allocate an I/O buffer without waiting */