Re: [PATCH] unnecessary removal of guest console characters

Re: [PATCH] unnecessary removal of guest console characters

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1868 bytes --]

Hi all,
I am attaching a patch that adds a new command line option to xenconsoled:

-o, --overflow-data=discard|keep

this option changes the behaviour when dealing with data that overflow
the max capacity of the buffer.
If overflow-data is set to discard (the default), the current behaviour
is used: we discard some data in the middle of the buffer.
If overflow-data is set to keep, we stop listening to the ring until we
free some space in the buffer.
This can cause the ring to fill up and the guest kernel internal buffer
to fill up as well. When this happens the guest kernel stops reading
characters from the console device so the application generating data
hangs. When xenconsoled resumes reading from the ring, the guest kernel
will be able to resume reading from the console device as well. At that
point the guest application will be allowed to continue.

Regards,

Stefano Stabellini

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Keir Fraser wrote:
> On 14/4/08 15:14, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:
> 
>> Keir Fraser, le Fri 11 Apr 2008 16:46:48 +0100, a écrit :
>>> The 'application' in this case is the kernel.
>> Mmm, in my understanding of printk, it is not blocking for the kernel,
>> and if a console is not able to catch up, the data simply gets
>> discarded (see __call_console_drivers, which doesn't check the result
>> of con->write(), which can hence be even 0 if the console is really
>> completely stuck).
> 
> Actually I was worried that we might depend on the ring being drained in a
> timely fashion in our own console driver implementation in Linux. Actually I
> think we don't (except that the handling of x_char looks suspect). Still, I
> don't know whether other guest types depend on the current behaviour. We
> could make the behaviour switchable I suppose.
> 
>  -- Keir
> 
> 




[-- Attachment #2: console-overflow2.patch --]
[-- Type: text/x-diff, Size: 3245 bytes --]

diff -r 84b5dee690f5 tools/console/daemon/io.c
--- a/tools/console/daemon/io.c	Mon Apr 21 14:59:25 2008 +0100
+++ b/tools/console/daemon/io.c	Mon Apr 21 16:51:32 2008 +0100
@@ -63,6 +63,7 @@ extern int log_time_hv;
 extern int log_time_hv;
 extern int log_time_guest;
 extern char *log_dir;
+extern int discard_overflowed_data;
 
 static int log_time_hv_needts = 1;
 static int log_time_guest_needts = 1;
@@ -201,7 +202,7 @@ static void buffer_append(struct domain 
 			      dom->domid, errno, strerror(errno));
 	}
 
-	if (buffer->max_capacity &&
+	if (discard_overflowed_data && buffer->max_capacity &&
 	    buffer->size > buffer->max_capacity) {
 		/* Discard the middle of the data. */
 
@@ -228,6 +229,11 @@ static void buffer_advance(struct buffer
 	if (buffer->consumed == buffer->size) {
 		buffer->consumed = 0;
 		buffer->size = 0;
+		if (buffer->max_capacity &&
+		    buffer->capacity > buffer->max_capacity) {
+			buffer->data = realloc(buffer->data, buffer->max_capacity);
+			buffer->capacity = buffer->max_capacity;
+		}
 	}
 }
 
@@ -1005,9 +1011,13 @@ void handle_io(void)
 				    d->next_period < next_timeout)
 					next_timeout = d->next_period;
 			} else if (d->xce_handle != -1) {
-				int evtchn_fd = xc_evtchn_fd(d->xce_handle);
-				FD_SET(evtchn_fd, &readfds);
-				max_fd = MAX(evtchn_fd, max_fd);
+				if (discard_overflowed_data ||
+				    !d->buffer.max_capacity ||
+				    d->buffer.size < d->buffer.max_capacity) {
+					int evtchn_fd = xc_evtchn_fd(d->xce_handle);
+					FD_SET(evtchn_fd, &readfds);
+					max_fd = MAX(evtchn_fd, max_fd);
+				}
 			}
 
 			if (d->master_fd != -1) {
diff -r 84b5dee690f5 tools/console/daemon/main.c
--- a/tools/console/daemon/main.c	Mon Apr 21 14:59:25 2008 +0100
+++ b/tools/console/daemon/main.c	Mon Apr 21 16:51:54 2008 +0100
@@ -38,6 +38,7 @@ int log_time_hv = 0;
 int log_time_hv = 0;
 int log_time_guest = 0;
 char *log_dir = NULL;
+int discard_overflowed_data = 1;
 
 static void handle_hup(int sig)
 {
@@ -46,7 +47,7 @@ static void handle_hup(int sig)
 
 static void usage(char *name)
 {
-	printf("Usage: %s [-h] [-V] [-v] [-i] [--log=none|guest|hv|all] [--log-dir=DIR] [--pid-file=PATH] [-t, --timestamp=none|guest|hv|all]\n", name);
+	printf("Usage: %s [-h] [-V] [-v] [-i] [--log=none|guest|hv|all] [--log-dir=DIR] [--pid-file=PATH] [-t, --timestamp=none|guest|hv|all] [-o, --overflow-data=discard|keep]\n", name);
 }
 
 static void version(char *name)
@@ -56,7 +57,7 @@ static void version(char *name)
 
 int main(int argc, char **argv)
 {
-	const char *sopts = "hVvit:";
+	const char *sopts = "hVvit:o:";
 	struct option lopts[] = {
 		{ "help", 0, 0, 'h' },
 		{ "version", 0, 0, 'V' },
@@ -66,6 +67,7 @@ int main(int argc, char **argv)
 		{ "log-dir", 1, 0, 'r' },
 		{ "pid-file", 1, 0, 'p' },
 		{ "timestamp", 1, 0, 't' },
+		{ "overflow-data", 1, 0, 'o'},
 		{ 0 },
 	};
 	bool is_interactive = false;
@@ -119,6 +121,13 @@ int main(int argc, char **argv)
 				log_time_hv = 0;
 			}
 			break;
+		case 'o':
+			if (!strcmp(optarg, "keep")) {
+				discard_overflowed_data = 0;
+			} else if (!strcmp(optarg, "discard")) {
+				discard_overflowed_data = 1;
+			}
+			break;
 		case '?':
 			fprintf(stderr,
 				"Try `%s --help' for more information\n",



[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH] unnecessary removal of guest console characters

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

Hi all,
currently when max_capacity is set in xenconsoled and the buffer if 
completely filled then some characters in the buffer are discarded (even 
though the rate limitation makes this unlikely to happen).
A better way to handle this scenario would be to stop reading from the 
ring until the buffer has some free space.
I am attaching a simple patch that does exactly this.
Best Regards,

Stefano Stabellini

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

[-- Attachment #2: xenconsoled.patch --]
[-- Type: text/x-diff, Size: 1776 bytes --]

diff -r 082d3886fded tools/console/daemon/io.c
--- a/tools/console/daemon/io.c	Fri Apr 11 09:14:03 2008 +0100
+++ b/tools/console/daemon/io.c	Fri Apr 11 14:12:51 2008 +0100
@@ -200,21 +200,6 @@ static void buffer_append(struct domain 
 			      "on domain %d: %d (%s)\n",
 			      dom->domid, errno, strerror(errno));
 	}
-
-	if (buffer->max_capacity &&
-	    buffer->size > buffer->max_capacity) {
-		/* Discard the middle of the data. */
-
-		size_t over = buffer->size - buffer->max_capacity;
-		char *maxpos = buffer->data + buffer->max_capacity;
-
-		memmove(maxpos - over, maxpos, over);
-		buffer->data = realloc(buffer->data, buffer->max_capacity);
-		buffer->size = buffer->capacity = buffer->max_capacity;
-
-		if (buffer->consumed > buffer->max_capacity - over)
-			buffer->consumed = buffer->max_capacity - over;
-	}
 }
 
 static bool buffer_empty(struct buffer *buffer)
@@ -228,6 +213,11 @@ static void buffer_advance(struct buffer
 	if (buffer->consumed == buffer->size) {
 		buffer->consumed = 0;
 		buffer->size = 0;
+		if (buffer->max_capacity &&
+		    buffer->capacity > buffer->max_capacity) {
+			buffer->data = realloc(buffer->data, buffer->max_capacity);
+			buffer->capacity = buffer->max_capacity;
+		}
 	}
 }
 
@@ -1005,9 +995,12 @@ void handle_io(void)
 				    d->next_period < next_timeout)
 					next_timeout = d->next_period;
 			} else if (d->xce_handle != -1) {
-				int evtchn_fd = xc_evtchn_fd(d->xce_handle);
-				FD_SET(evtchn_fd, &readfds);
-				max_fd = MAX(evtchn_fd, max_fd);
+				if (!d->buffer.max_capacity ||
+				    d->buffer.size < d->buffer.max_capacity) {
+					int evtchn_fd = xc_evtchn_fd(d->xce_handle);
+					FD_SET(evtchn_fd, &readfds);
+					max_fd = MAX(evtchn_fd, max_fd);
+				}
 			}
 
 			if (d->master_fd != -1) {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] unnecessary removal of guest console characters

[Keir Fraser]

Buffering happens in the daemon, not in the inter-domain ring. If we switch
to your strategy then overflow leads to the guest hanging rather than
characters being dropped. And this could happen, for example, if no client
is connected to xenconsoled and emptying the buffers. So I know which
strategy I prefer! Large buffers in the backend daemon, plus rate-limiting
of bursts, is the right answer.

 -- Keir

On 11/4/08 14:46, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
wrote:

> Hi all,
> currently when max_capacity is set in xenconsoled and the buffer if
> completely filled then some characters in the buffer are discarded (even
> though the rate limitation makes this unlikely to happen).
> A better way to handle this scenario would be to stop reading from the
> ring until the buffer has some free space.
> I am attaching a simple patch that does exactly this.
> Best Regards,
> 
> Stefano Stabellini
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> diff -r 082d3886fded tools/console/daemon/io.c
> --- a/tools/console/daemon/io.c Fri Apr 11 09:14:03 2008 +0100
> +++ b/tools/console/daemon/io.c Fri Apr 11 14:12:51 2008 +0100
> @@ -200,21 +200,6 @@ static void buffer_append(struct domain
>      "on domain %d: %d (%s)\n",
>      dom->domid, errno, strerror(errno));
> }
> -
> - if (buffer->max_capacity &&
> -     buffer->size > buffer->max_capacity) {
> -  /* Discard the middle of the data. */
> -
> -  size_t over = buffer->size - buffer->max_capacity;
> -  char *maxpos = buffer->data + buffer->max_capacity;
> -
> -  memmove(maxpos - over, maxpos, over);
> -  buffer->data = realloc(buffer->data, buffer->max_capacity);
> -  buffer->size = buffer->capacity = buffer->max_capacity;
> -
> -  if (buffer->consumed > buffer->max_capacity - over)
> -   buffer->consumed = buffer->max_capacity - over;
> - }
>  }
>  
>  static bool buffer_empty(struct buffer *buffer)
> @@ -228,6 +213,11 @@ static void buffer_advance(struct buffer
> if (buffer->consumed == buffer->size) {
> buffer->consumed = 0;
> buffer->size = 0;
> +  if (buffer->max_capacity &&
> +      buffer->capacity > buffer->max_capacity) {
> +   buffer->data = realloc(buffer->data, buffer->max_capacity);
> +   buffer->capacity = buffer->max_capacity;
> +  }
> }
>  }
>  
> @@ -1005,9 +995,12 @@ void handle_io(void)
>    d->next_period < next_timeout)
> next_timeout = d->next_period;
> } else if (d->xce_handle != -1) {
> -    int evtchn_fd = xc_evtchn_fd(d->xce_handle);
> -    FD_SET(evtchn_fd, &readfds);
> -    max_fd = MAX(evtchn_fd, max_fd);
> +    if (!d->buffer.max_capacity ||
> +        d->buffer.size < d->buffer.max_capacity) {
> +     int evtchn_fd = xc_evtchn_fd(d->xce_handle);
> +     FD_SET(evtchn_fd, &readfds);
> +     max_fd = MAX(evtchn_fd, max_fd);
> +    }
> }
>  
> if (d->master_fd != -1) {
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] unnecessary removal of guest console characters

[Samuel Thibault]

Keir Fraser, le Fri 11 Apr 2008 16:03:57 +0100, a écrit :
> Buffering happens in the daemon, not in the inter-domain ring. If we switch
> to your strategy then overflow leads to the guest hanging rather than
> characters being dropped.

The guest itself doesn't hang, the application inside the guest that
produces the output does hang, and that's what I would expect if I am
not looking at it, rather than the risk of loosing data because of a
non-end-to-end flow control.

Samuel

Re: [PATCH] unnecessary removal of guest console characters

[Keir Fraser]

The 'application' in this case is the kernel. The data in this case is
console logs and possibly output from an interactive login. Responsiveness
of the former is critical, even, I would say, if that's at the expense of
integrity of the latter.

 -- Keir

On 11/4/08 16:11, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:

> Keir Fraser, le Fri 11 Apr 2008 16:03:57 +0100, a écrit :
>> Buffering happens in the daemon, not in the inter-domain ring. If we switch
>> to your strategy then overflow leads to the guest hanging rather than
>> characters being dropped.
> 
> The guest itself doesn't hang, the application inside the guest that
> produces the output does hang, and that's what I would expect if I am
> not looking at it, rather than the risk of loosing data because of a
> non-end-to-end flow control.
> 
> Samuel