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

[Andrea Righi]

On Tue, Apr 21, 2009 at 03:46:59PM +0530, Balbir Singh wrote:
> * Andrea Righi <righi.andrea at gmail.com> [2009-04-21 11:55:26]:
> 
> > 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 at 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 at 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?
> >
> 
> No we don't. But having it helps the members align well on a 8 byte
> boundary. For all you know the compiler might do that anyway, unless
> we pack the structure.
> 
> Why does policy need to be unsigned long long? Can't it be a boolean
> for now? Token or leaky? We can consider unioning of some fields like
> soft_limit when added along with the proposed fields.

Adding an unsigned int flags to encode policy and other potential future
stuff seems much better. Also agreed by Kame.

Maybe the difference between a soft_limit and hard_limit can be also
encoded in the flags.

Moreover, max_usage is not used in ratelimited resources. And capacity
is not used in all the other cases (it has been introduced only for
token-bucket ratelimited resources).

I think we can easily union max_usage and capacity at least.

-Andrea
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers