[kvm-unit-tests,v2,3/6] s390x: Add linemode buffer to fix newline on every print
diff mbox series

Message ID 20190905103951.36522-4-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: Add multiboot and smp
Related show

Commit Message

Janosch Frank Sept. 5, 2019, 10:39 a.m. UTC
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(-)

Comments

David Hildenbrand Sept. 9, 2019, 9:02 a.m. UTC | #1
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>
David Hildenbrand Sept. 11, 2019, 7:57 a.m. UTC | #2
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
>
Janosch Frank Sept. 19, 2019, 11:07 a.m. UTC | #3
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
>>
> 
>

Patch
diff mbox series

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