Hi Keir,
Any feedback on this rate limiting patch for DomU consoles ?
Regards,
Dan.
On Mon, Sep 11, 2006 at 03:19:05PM +0100, Daniel P. Berrange wrote:
> On Wed, Aug 30, 2006 at 07:13:30PM +0100, Keir Fraser wrote:
> > On 29/8/06 4:59 pm, "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> >
> > > The patch sets
> > > - data size 5 kb
> > > - period 200 ms
> > > - delay 200 ms
> >
> > A few comments:
> > * I think the 'delay' parameter is not really useful. Think of this simply
> > as a simple credit-based scheduler that replenishes credit every 'period'.
> > So in this case you get 5kB every 200ms. If the domU offers more data in a
> > period, it must wait until its credit is replenished at the end of the
> > current 200ms period.
> > * I'm not sure bytes of data is the right thing to limit here. The main
> > thing that hoses domain0 is the repeated rescheduling of the console daemon,
> > and that is fundamentally caused by event-channel notifications. So it might
> > make sense to rate limit the number of times we read the event-channel port
> > from xc_evtchn_pending -- e.g., no more than 10 times a second (should be
> > plenty). This has a few advantages: 1. Looking just at data transferred
> > doesn't stop a malicious domain from hosing you with no-op event-channel
> > notifications; 2. This is a fundamental metric that can be measured and
> > rate-limited on all backend interfaces, so perhaps we can come up with some
> > common library of code that we apply to all backends/daemons.
>
> I've re-worked the patch based on this principle of "n events allowed
> in each time-slice", setting n=30 & the time-slice = 200ms. The code
> was actually much simpler than my previous patch so its definitely a
> winning strategy. Testing by running
>
> 'while /bin/true ; do echo t > /proc/sysrq-trigger; done'
>
> ..in one of the guest VMs on a 2.2 GHz Opteron, shows no significant CPU
> utilization attributed to xenconsoled. I've not examined whether this code
> can be put into a common library - it was simple enough to integrate in
> the xenconsoled event loop
>
> > It may turn out we need to rate limit on data *as well*, if it turns out
> > that sinking many kilobytes of data a second is prohibitively expensive, but
> > I doubt this will happen. For a start, the strict limiting of notifications
> > will encourage data to get queued up and improve batching of console data,
> > and batches of data should be processed quite efficiently. This same
> > argument extends to other backends (e.g., batching of requests in xenstored,
> > netback, blkback, etc).
>
> Based on initial testing it doesn't look like the data rate itself was causing
> any significant overhead, once the event channel port reads were limited.
>
>
> Signed-off by: Daniel P. Berrange <berrange@xxxxxxxxxx>
>
> Regards,
> Dan.
> --
> |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
> |=- Perl modules: http://search.cpan.org/~danberr/ -=|
> |=- Projects: http://freshmeat.net/~danielpb/ -=|
> |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
> diff -r bfd00b317815 tools/console/daemon/io.c
> --- a/tools/console/daemon/io.c Mon Sep 11 01:55:03 2006 +0100
> +++ b/tools/console/daemon/io.c Mon Sep 11 10:09:32 2006 -0400
> @@ -37,12 +37,18 @@
> #include <termios.h>
> #include <stdarg.h>
> #include <sys/mman.h>
> +#include <sys/time.h>
>
> #define MAX(a, b) (((a) > (b)) ? (a) : (b))
> #define MIN(a, b) (((a) < (b)) ? (a) : (b))
>
> /* Each 10 bits takes ~ 3 digits, plus one, plus one for nul terminator. */
> #define MAX_STRLEN(x) ((sizeof(x) * CHAR_BIT + CHAR_BIT-1) / 10 * 3 + 2)
> +
> +/* How many events are allowed in each time period */
> +#define RATE_LIMIT_ALLOWANCE 30
> +/* Duration of each time period in ms */
> +#define RATE_LIMIT_PERIOD 200
>
> struct buffer
> {
> @@ -65,6 +71,8 @@ struct domain
> evtchn_port_t local_port;
> int xce_handle;
> struct xencons_interface *interface;
> + int event_count;
> + long long next_period;
> };
>
> static struct domain *dom_head;
> @@ -306,6 +314,13 @@ static struct domain *create_domain(int
> {
> struct domain *dom;
> char *s;
> + struct timeval tv;
> +
> + if (gettimeofday(&tv, NULL) < 0) {
> + dolog(LOG_ERR, "Cannot get time of day %s:%s:L%d",
> + __FILE__, __FUNCTION__, __LINE__);
> + return NULL;
> + }
>
> dom = (struct domain *)malloc(sizeof(struct domain));
> if (dom == NULL) {
> @@ -330,6 +345,8 @@ static struct domain *create_domain(int
> dom->buffer.size = 0;
> dom->buffer.capacity = 0;
> dom->buffer.max_capacity = 0;
> + dom->event_count = 0;
> + dom->next_period = (tv.tv_sec * 1000) + (tv.tv_usec / 1000) +
> RATE_LIMIT_PERIOD;
> dom->next = NULL;
>
> dom->ring_ref = -1;
> @@ -512,9 +529,12 @@ static void handle_ring_read(struct doma
> if ((port = xc_evtchn_pending(dom->xce_handle)) == -1)
> return;
>
> + dom->event_count++;
> +
> buffer_append(dom);
>
> - (void)xc_evtchn_unmask(dom->xce_handle, port);
> + if (dom->event_count < RATE_LIMIT_ALLOWANCE)
> + (void)xc_evtchn_unmask(dom->xce_handle, port);
> }
>
> static void handle_xs(void)
> @@ -549,6 +569,9 @@ void handle_io(void)
> do {
> struct domain *d, *n;
> int max_fd = -1;
> + struct timeval timeout;
> + struct timeval tv;
> + long long now, next_timeout = 0;
>
> FD_ZERO(&readfds);
> FD_ZERO(&writefds);
> @@ -556,8 +579,33 @@ void handle_io(void)
> FD_SET(xs_fileno(xs), &readfds);
> max_fd = MAX(xs_fileno(xs), max_fd);
>
> + if (gettimeofday(&tv, NULL) < 0)
> + return;
> + now = (tv.tv_sec * 1000) + (tv.tv_usec / 1000);
> +
> + /* Re-calculate any event counter allowances & unblock
> + domains with new allowance */
> for (d = dom_head; d; d = d->next) {
> - if (d->xce_handle != -1) {
> + /* Add 5ms of fuzz since select() often returns
> + a couple of ms sooner than requested. Without
> + the fuzz we typically do an extra spin in select()
> + with a 1/2 ms timeout every other iteration */
> + if ((now+5) > d->next_period) {
> + d->next_period = now + RATE_LIMIT_PERIOD;
> + if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
> + (void)xc_evtchn_unmask(d->xce_handle,
> d->local_port);
> + }
> + d->event_count = 0;
> + }
> + }
> +
> + for (d = dom_head; d; d = d->next) {
> + if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
> + /* Determine if we're going to be the next time
> slice to expire */
> + if (!next_timeout ||
> + 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);
> @@ -573,16 +621,29 @@ void handle_io(void)
> }
> }
>
> - ret = select(max_fd + 1, &readfds, &writefds, 0, NULL);
> + /* If any domain has been rate limited, we need to work
> + out what timeout to supply to select */
> + if (next_timeout) {
> + long long duration = (next_timeout - now);
> + /* Shouldn't happen, but sanity check force greater
> than 0 */
> + if (duration <= 0)
> + duration = 1;
> + timeout.tv_sec = duration / 1000;
> + timeout.tv_usec = (duration - (timeout.tv_sec * 1000))
> * 1000;
> + }
> +
> + ret = select(max_fd + 1, &readfds, &writefds, 0, next_timeout ?
> &timeout : NULL);
>
> if (FD_ISSET(xs_fileno(xs), &readfds))
> handle_xs();
>
> for (d = dom_head; d; d = n) {
> n = d->next;
> - if (d->xce_handle != -1 &&
> - FD_ISSET(xc_evtchn_fd(d->xce_handle), &readfds))
> - handle_ring_read(d);
> + if (d->event_count < RATE_LIMIT_ALLOWANCE) {
> + if (d->xce_handle != -1 &&
> + FD_ISSET(xc_evtchn_fd(d->xce_handle),
> &readfds))
> + handle_ring_read(d);
> + }
>
> if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &readfds))
> handle_tty_read(d);
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home