| Message ID | 20190905103951.36522-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add multiboot and smp | expand |
On 05.09.19 12:39, Janosch Frank wrote: > Linemode seems to add a newline for each sent message which makes > reading rather hard. Hence we add a small buffer and only print if > it's full or a newline is encountered. Except for when the string is > longer than the buffer, then we flush the buffer and print directly. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > --- > lib/s390x/sclp-console.c | 70 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 66 insertions(+), 4 deletions(-) > > diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c > index 19416b5..7397dc1 100644 > --- a/lib/s390x/sclp-console.c > +++ b/lib/s390x/sclp-console.c > @@ -13,6 +13,7 @@ > #include <asm/page.h> > #include <asm/arch_def.h> > #include <asm/io.h> > +#include <asm/spinlock.h> > #include "sclp.h" > > /* > @@ -87,6 +88,10 @@ static uint8_t _ascebc[256] = { > 0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF > }; > > +static char lm_buff[120]; > +static unsigned char lm_buff_off; > +static struct spinlock lm_buff_lock; > + > static void sclp_print_ascii(const char *str) > { > int len = strlen(str); > @@ -103,10 +108,10 @@ static void sclp_print_ascii(const char *str) > sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); > } > > -static void sclp_print_lm(const char *str) > +static void lm_print(const char *buff, int len) > { > unsigned char *ptr, *end, ch; > - unsigned int count, offset, len; > + unsigned int count, offset; > struct WriteEventData *sccb; > struct mdb *mdb; > struct mto *mto; > @@ -117,11 +122,10 @@ static void sclp_print_lm(const char *str) > end = (unsigned char *) sccb + 4096 - 1; > memset(sccb, 0, sizeof(*sccb)); > ptr = (unsigned char *) &sccb->msg.mdb.mto; > - len = strlen(str); > offset = 0; > do { > for (count = sizeof(*mto); offset < len; count++) { > - ch = str[offset++]; > + ch = buff[offset++]; > if (ch == 0x0a || ptr + count > end) > break; > ptr[count] = _ascebc[ch]; > @@ -148,6 +152,64 @@ static void sclp_print_lm(const char *str) > sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); > } > > + > +/* > + * In contrast to the ascii console, linemode produces a new > + * line with every write of data. The report() function uses > + * several printf() calls to generate a line of data which > + * would all end up on different lines. > + * > + * Hence we buffer here until we encounter a \n or the buffer > + * is full. That means that linemode output can look a bit > + * different from ascii and that it takes a bit longer for > + * lines to appear. > + */ > +static void sclp_print_lm(const char *str) > +{ > + int len; > + char *nl; > + > + spin_lock(&lm_buff_lock); > + > + len = strlen(str); > + /* > + * No use in copying into lm_buff, its time to flush the > + * buffer and print str until finished. > + */ > + if (len > sizeof(lm_buff)) { > + if (lm_buff_off) > + lm_print(lm_buff, lm_buff_off); > + lm_print(str, len); > + memset(lm_buff, 0 , sizeof(lm_buff)); > + lm_buff_off = 0; > + goto out; > + } > + > +fill: > + len = len < (sizeof(lm_buff) - lm_buff_off) ? len : (sizeof(lm_buff) - lm_buff_off); > + if ((lm_buff_off < sizeof(lm_buff) - 1)) { > + memcpy(&lm_buff[lm_buff_off], str, len); > + lm_buff_off += len; > + } > + /* Buffer not full and no newline */ > + nl = strchr(lm_buff, '\n'); > + if (lm_buff_off != sizeof(lm_buff) - 1 && !nl) > + goto out; > + > + lm_print(lm_buff, lm_buff_off); > + memset(lm_buff, 0 , sizeof(lm_buff)); > + lm_buff_off = 0; > + > + if (len < strlen(str)) { > + str = &str[len]; > + len = strlen(str); > + goto fill; > + } > + > +out: > + spin_unlock(&lm_buff_lock); > +} > + > /* > * SCLP needs to be initialized by setting a send and receive mask, > * indicating which messages the control program (we) want(s) to > Acked-by: David Hildenbrand <david@redhat.com>
On 05.09.19 12:39, Janosch Frank wrote: > Linemode seems to add a newline for each sent message which makes > reading rather hard. Hence we add a small buffer and only print if > it's full or a newline is encountered. Except for when the string is > longer than the buffer, then we flush the buffer and print directly. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > --- > lib/s390x/sclp-console.c | 70 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 66 insertions(+), 4 deletions(-) > > diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c > index 19416b5..7397dc1 100644 > --- a/lib/s390x/sclp-console.c > +++ b/lib/s390x/sclp-console.c > @@ -13,6 +13,7 @@ > #include <asm/page.h> > #include <asm/arch_def.h> > #include <asm/io.h> > +#include <asm/spinlock.h> > #include "sclp.h" > > /* > @@ -87,6 +88,10 @@ static uint8_t _ascebc[256] = { > 0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF > }; > > +static char lm_buff[120]; Just wondering, how did you come up with the 120? (my first guess would have been something around 80) > +static unsigned char lm_buff_off; > +static struct spinlock lm_buff_lock; > + > static void sclp_print_ascii(const char *str) > { > int len = strlen(str); > @@ -103,10 +108,10 @@ static void sclp_print_ascii(const char *str) > sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); > } > > -static void sclp_print_lm(const char *str) > +static void lm_print(const char *buff, int len) > { The rename of str->buff could have been avoided, however, the impact is rather small. > unsigned char *ptr, *end, ch; > - unsigned int count, offset, len; > + unsigned int count, offset; > struct WriteEventData *sccb; > struct mdb *mdb; > struct mto *mto; > @@ -117,11 +122,10 @@ static void sclp_print_lm(const char *str) > end = (unsigned char *) sccb + 4096 - 1; > memset(sccb, 0, sizeof(*sccb)); > ptr = (unsigned char *) &sccb->msg.mdb.mto; > - len = strlen(str); > offset = 0; > do { > for (count = sizeof(*mto); offset < len; count++) { > - ch = str[offset++]; > + ch = buff[offset++]; > if (ch == 0x0a || ptr + count > end) > break; > ptr[count] = _ascebc[ch]; > @@ -148,6 +152,64 @@ static void sclp_print_lm(const char *str) > sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); > } > > + > +/* > + * In contrast to the ascii console, linemode produces a new > + * line with every write of data. The report() function uses > + * several printf() calls to generate a line of data which > + * would all end up on different lines. > + * > + * Hence we buffer here until we encounter a \n or the buffer > + * is full. That means that linemode output can look a bit > + * different from ascii and that it takes a bit longer for > + * lines to appear. > + */ > +static void sclp_print_lm(const char *str) > +{ > + int len; > + char *nl; > + > + spin_lock(&lm_buff_lock); > + > + len = strlen(str); You could do that directly when declaring the variable, doesn't have to be under the lock. > + /* > + * No use in copying into lm_buff, its time to flush the > + * buffer and print str until finished. > + */ > + if (len > sizeof(lm_buff)) { I find ARRAY_SIZE(lm_buf) easier to understand than sizeof(lm_buff) > + if (lm_buff_off) > + lm_print(lm_buff, lm_buff_off); > + lm_print(str, len); > + memset(lm_buff, 0 , sizeof(lm_buff)); > + lm_buff_off = 0; > + goto out; > + } > + > +fill: Is there a way to remove this goto by using a simple while loop? > + len = len < (sizeof(lm_buff) - lm_buff_off) ? len : (sizeof(lm_buff) - lm_buff_off); > + if ((lm_buff_off < sizeof(lm_buff) - 1)) { Drop one set of () > + memcpy(&lm_buff[lm_buff_off], str, len); > + lm_buff_off += len; > + } > + /* Buffer not full and no newline */ > + nl = strchr(lm_buff, '\n'); Why do we have to search? Shouldn't a newline be the last copied character only? > + if (lm_buff_off != sizeof(lm_buff) - 1 && !nl) > + goto out; > + > + lm_print(lm_buff, lm_buff_off); > + memset(lm_buff, 0 , sizeof(lm_buff)); > + lm_buff_off = 0; > + > + if (len < strlen(str)) { > + str = &str[len]; > + len = strlen(str); > + goto fill; > + } This looks too complicated for my taste :) Or my caffeine level is low. I think we have the following cases: 1. String contains newline a) String fits into remaining buffer -> Copy into buffer, flush (last character is newline) b) String doesn't fit into remaining buffer -> Simply flush old buffer and print remaining string? 2. String doesn't contain newline. a) String fits into remaining buffer -> Copy into buffer, flush if full b) String doesn't fit into remaining buffer -> Simply flush old buffer and print remaining string? Optimizing for 1b) or 2b) isn't really worth it I guess - unless we want to wrap *any* string at 120 characters. But then, your pre-loop handling would also have to be modified. I think this allow to simplify your code a lot. (how often does it happen in our current tests that we exceed 120 characters?) > + > +out: > + spin_unlock(&lm_buff_lock); > +} > + > /* > * SCLP needs to be initialized by setting a send and receive mask, > * indicating which messages the control program (we) want(s) to >
On 9/11/19 9:57 AM, David Hildenbrand wrote: > On 05.09.19 12:39, Janosch Frank wrote: >> Linemode seems to add a newline for each sent message which makes >> reading rather hard. Hence we add a small buffer and only print if >> it's full or a newline is encountered. Except for when the string is >> longer than the buffer, then we flush the buffer and print directly. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> --- >> lib/s390x/sclp-console.c | 70 +++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 66 insertions(+), 4 deletions(-) >> >> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c >> index 19416b5..7397dc1 100644 >> --- a/lib/s390x/sclp-console.c >> +++ b/lib/s390x/sclp-console.c >> @@ -13,6 +13,7 @@ >> #include <asm/page.h> >> #include <asm/arch_def.h> >> #include <asm/io.h> >> +#include <asm/spinlock.h> >> #include "sclp.h" >> >> /* >> @@ -87,6 +88,10 @@ static uint8_t _ascebc[256] = { >> 0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF >> }; >> >> +static char lm_buff[120]; > > Just wondering, how did you come up with the 120? (my first guess would > have been something around 80) > >> +static unsigned char lm_buff_off; >> +static struct spinlock lm_buff_lock; >> + >> static void sclp_print_ascii(const char *str) >> { >> int len = strlen(str); >> @@ -103,10 +108,10 @@ static void sclp_print_ascii(const char *str) >> sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); >> } >> >> -static void sclp_print_lm(const char *str) >> +static void lm_print(const char *buff, int len) >> { > > The rename of str->buff could have been avoided, however, the impact is > rather small. > >> unsigned char *ptr, *end, ch; >> - unsigned int count, offset, len; >> + unsigned int count, offset; >> struct WriteEventData *sccb; >> struct mdb *mdb; >> struct mto *mto; >> @@ -117,11 +122,10 @@ static void sclp_print_lm(const char *str) >> end = (unsigned char *) sccb + 4096 - 1; >> memset(sccb, 0, sizeof(*sccb)); >> ptr = (unsigned char *) &sccb->msg.mdb.mto; >> - len = strlen(str); >> offset = 0; >> do { >> for (count = sizeof(*mto); offset < len; count++) { >> - ch = str[offset++]; >> + ch = buff[offset++]; >> if (ch == 0x0a || ptr + count > end) >> break; >> ptr[count] = _ascebc[ch]; >> @@ -148,6 +152,64 @@ static void sclp_print_lm(const char *str) >> sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); >> } >> >> + >> +/* >> + * In contrast to the ascii console, linemode produces a new >> + * line with every write of data. The report() function uses >> + * several printf() calls to generate a line of data which >> + * would all end up on different lines. >> + * >> + * Hence we buffer here until we encounter a \n or the buffer >> + * is full. That means that linemode output can look a bit >> + * different from ascii and that it takes a bit longer for >> + * lines to appear. >> + */ >> +static void sclp_print_lm(const char *str) >> +{ >> + int len; >> + char *nl; >> + >> + spin_lock(&lm_buff_lock); >> + >> + len = strlen(str); > > You could do that directly when declaring the variable, doesn't have to > be under the lock. > >> + /* >> + * No use in copying into lm_buff, its time to flush the >> + * buffer and print str until finished. >> + */ >> + if (len > sizeof(lm_buff)) { > > I find ARRAY_SIZE(lm_buf) easier to understand than sizeof(lm_buff) > >> + if (lm_buff_off) >> + lm_print(lm_buff, lm_buff_off); >> + lm_print(str, len); >> + memset(lm_buff, 0 , sizeof(lm_buff)); >> + lm_buff_off = 0; >> + goto out; >> + } >> + >> +fill: > > Is there a way to remove this goto by using a simple while loop? > >> + len = len < (sizeof(lm_buff) - lm_buff_off) ? len : (sizeof(lm_buff) - lm_buff_off); >> + if ((lm_buff_off < sizeof(lm_buff) - 1)) { > > Drop one set of () > >> + memcpy(&lm_buff[lm_buff_off], str, len); >> + lm_buff_off += len; >> + } >> + /* Buffer not full and no newline */ >> + nl = strchr(lm_buff, '\n'); > > Why do we have to search? Shouldn't a newline be the last copied > character only? > >> + if (lm_buff_off != sizeof(lm_buff) - 1 && !nl) >> + goto out; >> + >> + lm_print(lm_buff, lm_buff_off); >> + memset(lm_buff, 0 , sizeof(lm_buff)); >> + lm_buff_off = 0; >> + >> + if (len < strlen(str)) { >> + str = &str[len]; >> + len = strlen(str); >> + goto fill; >> + } > > This looks too complicated for my taste :) Or my caffeine level is low. > > I think we have the following cases: > > 1. String contains newline > a) String fits into remaining buffer > -> Copy into buffer, flush (last character is newline) > b) String doesn't fit into remaining buffer > -> Simply flush old buffer and print remaining string? > 2. String doesn't contain newline. > a) String fits into remaining buffer > -> Copy into buffer, flush if full > b) String doesn't fit into remaining buffer > -> Simply flush old buffer and print remaining string? > > Optimizing for 1b) or 2b) isn't really worth it I guess - unless we want > to wrap *any* string at 120 characters. But then, your pre-loop handling > would also have to be modified. I think this allow to simplify your code > a lot. > > (how often does it happen in our current tests that we exceed 120 > characters?) How about this? char *nl; int len = strlen(str), i = 0; spin_lock(&lm_buff_lock); while (len) { lm_buff[lm_buff_off] = str[i]; lm_buff_off++; len--; /* Buffer full or newline? */ if (str[i] == '\n' || lm_buff_off == (sizeof(lm_buff) - 1)) { lm_print(lm_buff, lm_buff_off); memset(lm_buff, 0 , sizeof(lm_buff)); lm_buff_off = 0; } i++; } spin_unlock(&lm_buff_lock); return; > >> + >> +out: >> + spin_unlock(&lm_buff_lock); >> +} >> + >> /* >> * SCLP needs to be initialized by setting a send and receive mask, >> * indicating which messages the control program (we) want(s) to >> > >
diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c index 19416b5..7397dc1 100644 --- a/lib/s390x/sclp-console.c +++ b/lib/s390x/sclp-console.c @@ -13,6 +13,7 @@ #include <asm/page.h> #include <asm/arch_def.h> #include <asm/io.h> +#include <asm/spinlock.h> #include "sclp.h" /* @@ -87,6 +88,10 @@ static uint8_t _ascebc[256] = { 0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF }; +static char lm_buff[120]; +static unsigned char lm_buff_off; +static struct spinlock lm_buff_lock; + static void sclp_print_ascii(const char *str) { int len = strlen(str); @@ -103,10 +108,10 @@ static void sclp_print_ascii(const char *str) sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); } -static void sclp_print_lm(const char *str) +static void lm_print(const char *buff, int len) { unsigned char *ptr, *end, ch; - unsigned int count, offset, len; + unsigned int count, offset; struct WriteEventData *sccb; struct mdb *mdb; struct mto *mto; @@ -117,11 +122,10 @@ static void sclp_print_lm(const char *str) end = (unsigned char *) sccb + 4096 - 1; memset(sccb, 0, sizeof(*sccb)); ptr = (unsigned char *) &sccb->msg.mdb.mto; - len = strlen(str); offset = 0; do { for (count = sizeof(*mto); offset < len; count++) { - ch = str[offset++]; + ch = buff[offset++]; if (ch == 0x0a || ptr + count > end) break; ptr[count] = _ascebc[ch]; @@ -148,6 +152,64 @@ static void sclp_print_lm(const char *str) sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb); } + +/* + * In contrast to the ascii console, linemode produces a new + * line with every write of data. The report() function uses + * several printf() calls to generate a line of data which + * would all end up on different lines. + * + * Hence we buffer here until we encounter a \n or the buffer + * is full. That means that linemode output can look a bit + * different from ascii and that it takes a bit longer for + * lines to appear. + */ +static void sclp_print_lm(const char *str) +{ + int len; + char *nl; + + spin_lock(&lm_buff_lock); + + len = strlen(str); + /* + * No use in copying into lm_buff, its time to flush the + * buffer and print str until finished. + */ + if (len > sizeof(lm_buff)) { + if (lm_buff_off) + lm_print(lm_buff, lm_buff_off); + lm_print(str, len); + memset(lm_buff, 0 , sizeof(lm_buff)); + lm_buff_off = 0; + goto out; + } + +fill: + len = len < (sizeof(lm_buff) - lm_buff_off) ? len : (sizeof(lm_buff) - lm_buff_off); + if ((lm_buff_off < sizeof(lm_buff) - 1)) { + memcpy(&lm_buff[lm_buff_off], str, len); + lm_buff_off += len; + } + /* Buffer not full and no newline */ + nl = strchr(lm_buff, '\n'); + if (lm_buff_off != sizeof(lm_buff) - 1 && !nl) + goto out; + + lm_print(lm_buff, lm_buff_off); + memset(lm_buff, 0 , sizeof(lm_buff)); + lm_buff_off = 0; + + if (len < strlen(str)) { + str = &str[len]; + len = strlen(str); + goto fill; + } + +out: + spin_unlock(&lm_buff_lock); +} + /* * SCLP needs to be initialized by setting a send and receive mask, * indicating which messages the control program (we) want(s) to