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

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

On Tue, Apr 21, 2009 at 09:15:34AM +0900, KAMEZAWA Hiroyuki wrote:
> On Sat, 18 Apr 2009 23:38:27 +0200
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > 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
> > 
> > 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 ]
> > 
> > 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
> > -	 */
> >  	unsigned long long failcnt;
> > +	unsigned long long policy;
> > +	unsigned long long capacity;
> > +	unsigned long long timestamp;
> >  
> Andrea, sizeof(struct res_counter) is getting close to 128bytes. (maybe someone adds more)
> Then, could you check "unsigned long or unsigned int" is allowed or not, again ?
> 
> It's very bad if cacheline of spinlock is different from data field, in future.

Regarding the new attributes, policy can be surely an unsigned int or
even less (now only 1 bit is used!), maybe we can just add an unsigned
int flags, and encode also potential future informations there.

Moreover, are we sure we really need an unsigned long long for failcnt?

Thanks,
-Andrea