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@xxxxxxxxxxxxx>
>> Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
>> ---
>> 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
>>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
