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

From: Balbir Singh
Date: Tue Apr 21 2009 - 06:18:21 EST


* Andrea Righi <righi.andrea@xxxxxxxxx> [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@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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.

--
Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/