Re: [PATCH 2/7] res_counter: introduce ratelimiting attributes

[Andrea Righi <righi.andrea@gmail.com>]

On Tue, Apr 21, 2009 at 03:43:26PM +0530, Balbir Singh wrote:
> * Andrea Righi <righi.andrea@gmail.com> [2009-04-18 23:38:27]:
> 
> > Introduce attributes and functions in res_counter to implement throttling-based
> > cgroup subsystems.
> > 
> > The following attributes have been added to struct res_counter:
> >  * @policy:     the limiting policy / algorithm
> >  * @capacity:   the maximum capacity of the resource
> >  * @timestamp:  timestamp of the last accounted resource request
> > 
> 
> Units of each of the above would be desirable, without them it is hard
> to understand what you are trying to add. What is the unit of
> capacity?

Theoretically it can be any unit. At the moment it is used by the
io-throttle controller only for the token bucket strategy (@policy =
RATELIMIT_TOKEN_BUCKET) and it can be either bytes or IO operations.

Maybe I should add a comment like this.

> 
> > Currently the available policies are: token-bucket and leaky-bucket and the
> > attribute @capacity is only used by token-bucket policy (to represent the
> > bucket size).
> > 
> > The following function has been implemented to return the amount of time a
> > cgroup should sleep to remain within the defined resource limits.
> > 
> >   unsigned long long
> >   res_counter_ratelimit_sleep(struct res_counter *res, ssize_t val);
> > 
> > [ Note: only the interfaces needed by the cgroup IO controller are implemented
> > right now ]
> > 
> 
> This is a good RFC, but I would hold off merging till the subsystem
> gets in. Having said that I am not convinced about the subsystem
> sleeping, if the subsystem is not IO intensive, should it still sleep
> because it is over its IO b/w? This might make sense for the CPU
> controller, since not having CPU b/w does imply sleeping.
> 
> Could you please use the word throttle instead of sleep.

OK, will do in the next version.

> 
> 
> > Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
> > ---
> >  include/linux/res_counter.h |   69 +++++++++++++++++++++++++++++++----------
> >  kernel/res_counter.c        |   72 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 124 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> > index 4c5bcf6..9bed6af 100644
> > --- a/include/linux/res_counter.h
> > +++ b/include/linux/res_counter.h
> > @@ -14,30 +14,36 @@
> >   */
> > 
> >  #include <linux/cgroup.h>
> > +#include <linux/jiffies.h>
> > 
> > -/*
> > - * The core object. the cgroup that wishes to account for some
> > - * resource may include this counter into its structures and use
> > - * the helpers described beyond
> > - */
> > +/* The various policies that can be used for ratelimiting resources */
> > +#define	RATELIMIT_LEAKY_BUCKET	0
> > +#define	RATELIMIT_TOKEN_BUCKET	1
> > 
> > +/**
> > + * struct res_counter - the core object to account cgroup resources
> > + *
> > + * @usage:	the current resource consumption level
> > + * @max_usage:	the maximal value of the usage from the counter creation
> > + * @limit:	the limit that usage cannot be exceeded
> > + * @failcnt:	the number of unsuccessful attempts to consume the resource
> > + * @policy:	the limiting policy / algorithm
> > + * @capacity:	the maximum capacity of the resource
> > + * @timestamp:	timestamp of the last accounted resource request
> > + * @lock:	the lock to protect all of the above.
> > + *		The routines below consider this to be IRQ-safe
> > + *
> > + * The cgroup that wishes to account for some resource may include this counter
> > + * into its structures and use the helpers described beyond.
> > + */
> >  struct res_counter {
> > -	/*
> > -	 * the current resource consumption level
> > -	 */
> >  	unsigned long long usage;
> > -	/*
> > -	 * the maximal value of the usage from the counter creation
> > -	 */
> >  	unsigned long long max_usage;
> > -	/*
> > -	 * the limit that usage cannot exceed
> > -	 */
> >  	unsigned long long limit;
> > -	/*
> > -	 * the number of unsuccessful attempts to consume the resource
> > -	 */
> 
> Don't understand why res_counter is removed? Am I reading the diff
> correctly?

It is not removed. I've just used the kernel-doc style comment
(Documentation/kernel-doc-nano-HOWTO.txt). I think Randy suggested this
in the past.

> 
> >  	unsigned long long failcnt;
> > +	unsigned long long policy;
> > +	unsigned long long capacity;
> > +	unsigned long long timestamp;
> >  	/*
> >  	 * the lock to protect all of the above.
> >  	 * the routines below consider this to be IRQ-safe
> > @@ -84,6 +90,9 @@ enum {
> >  	RES_USAGE,
> >  	RES_MAX_USAGE,
> >  	RES_LIMIT,
> > +	RES_POLICY,
> > +	RES_TIMESTAMP,
> > +	RES_CAPACITY,
> >  	RES_FAILCNT,
> >  };
> > 
> > @@ -130,6 +139,15 @@ static inline bool res_counter_limit_check_locked(struct res_counter *cnt)
> >  	return false;
> >  }
> > 
> > +static inline unsigned long long
> > +res_counter_ratelimit_delta_t(struct res_counter *res)
> > +{
> > +	return (long long)get_jiffies_64() - (long long)res->timestamp;
> > +}
> > +
> > +unsigned long long
> > +res_counter_ratelimit_sleep(struct res_counter *res, ssize_t val);
> > +
> >  /*
> >   * Helper function to detect if the cgroup is within it's limit or
> >   * not. It's currently called from cgroup_rss_prepare()
> > @@ -163,6 +181,23 @@ static inline void res_counter_reset_failcnt(struct res_counter *cnt)
> >  	spin_unlock_irqrestore(&cnt->lock, flags);
> >  }
> > 
> > +static inline int
> > +res_counter_ratelimit_set_limit(struct res_counter *cnt,
> > +			unsigned long long policy,
> > +			unsigned long long limit, unsigned long long max)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&cnt->lock, flags);
> > +	cnt->limit = limit;
> > +	cnt->capacity = max;
> > +	cnt->policy = policy;
> > +	cnt->timestamp = get_jiffies_64();
> > +	cnt->usage = 0;
> > +	spin_unlock_irqrestore(&cnt->lock, flags);
> > +	return 0;
> > +}
> > +
> >  static inline int res_counter_set_limit(struct res_counter *cnt,
> >  		unsigned long long limit)
> >  {
> > diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> > index bf8e753..b62319c 100644
> > --- a/kernel/res_counter.c
> > +++ b/kernel/res_counter.c
> > @@ -9,6 +9,7 @@
> > 
> >  #include <linux/types.h>
> >  #include <linux/parser.h>
> > +#include <linux/jiffies.h>
> >  #include <linux/fs.h>
> >  #include <linux/slab.h>
> >  #include <linux/res_counter.h>
> > @@ -20,6 +21,8 @@ void res_counter_init(struct res_counter *counter, struct res_counter *parent)
> >  	spin_lock_init(&counter->lock);
> >  	counter->limit = (unsigned long long)LLONG_MAX;
> >  	counter->parent = parent;
> > +	counter->capacity = (unsigned long long)LLONG_MAX;
> > +	counter->timestamp = get_jiffies_64();
> >  }
> > 
> >  int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
> > @@ -99,6 +102,12 @@ res_counter_member(struct res_counter *counter, int member)
> >  		return &counter->max_usage;
> >  	case RES_LIMIT:
> >  		return &counter->limit;
> > +	case RES_POLICY:
> > +		return &counter->policy;
> > +	case RES_TIMESTAMP:
> > +		return &counter->timestamp;
> > +	case RES_CAPACITY:
> > +		return &counter->capacity;
> >  	case RES_FAILCNT:
> >  		return &counter->failcnt;
> >  	};
> > @@ -163,3 +172,66 @@ int res_counter_write(struct res_counter *counter, int member,
> >  	spin_unlock_irqrestore(&counter->lock, flags);
> >  	return 0;
> >  }
> > +
> > +static unsigned long long
> > +ratelimit_leaky_bucket(struct res_counter *res, ssize_t val)
> > +{
> > +	unsigned long long delta, t;
> > +
> > +	res->usage += val;
> 
> Is this called from a protected context (w.r.t. res)?

Yes, it is called with res->lock held (look at
res_counter_ratelimit_sleep()).

I can add a comment anyway.

> 
> > +	delta = res_counter_ratelimit_delta_t(res);
> > +	if (!delta)
> > +		return 0;
> > +	t = res->usage * USEC_PER_SEC;
> > +	t = usecs_to_jiffies(div_u64(t, res->limit));
> > +	if (t > delta)
> > +		return t - delta;
> > +	/* Reset i/o statistics */
> > +	res->usage = 0;
> > +	res->timestamp = get_jiffies_64();
> > +	return 0;
> > +}
> > +
> > +static unsigned long long
> > +ratelimit_token_bucket(struct res_counter *res, ssize_t val)
> > +{
> > +	unsigned long long delta;
> > +	long long tok;
> > +
> > +	res->usage -= val;
> > +	delta = jiffies_to_msecs(res_counter_ratelimit_delta_t(res));
> > +	res->timestamp = get_jiffies_64();
> > +	tok = (long long)res->usage * MSEC_PER_SEC;
> > +	if (delta) {
> > +		long long max = (long long)res->capacity * MSEC_PER_SEC;
> > +
> > +		tok += delta * res->limit;
> > +		if (tok > max)
> > +			tok = max;
> 
> Use max_t() here

ok.

> 
> > +		res->usage = (unsigned long long)div_s64(tok, MSEC_PER_SEC);
> > +	}
> > +	return (tok < 0) ? msecs_to_jiffies(div_u64(-tok, res->limit)) : 0;
> > +}
> 
> I don't like the usage of MSEC and USEC for res->usage based on
> policy.

I used a different granularity only because in the io-throttle tests
token bucket worked better with USEC and leaky bucket with MSEC. But we
can generalize and encode this "granularity" information in a
res_counter->flags attribute.

> 
> > +
> > +unsigned long long
> > +res_counter_ratelimit_sleep(struct res_counter *res, ssize_t val)
> > +{
> > +	unsigned long long sleep = 0;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&res->lock, flags);
> > +	if (res->limit)
> > +		switch (res->policy) {
> > +		case RATELIMIT_LEAKY_BUCKET:
> > +			sleep = ratelimit_leaky_bucket(res, val);
> > +			break;
> > +		case RATELIMIT_TOKEN_BUCKET:
> > +			sleep = ratelimit_token_bucket(res, val);
> > +			break;
> > +		default:
> > +			WARN_ON(1);
> > +			break;
> > +		}
> > +	spin_unlock_irqrestore(&res->lock, flags);
> > +	return sleep;
> > +}
> > -- 
> > 1.5.6.3
> > 
> > 
> 
> -- 
> 	Balbir

Thanks for your comments!
-Andrea