Re: [PATCH 1/6] res_counter: handle limit change

[Paul Menage]

On Fri, Jun 13, 2008 at 2:29 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> Add a support to shrink_usage_at_limit_change feature to res_counter.
> memcg will use this to drop pages.

Sorry for the delay in looking at this.

I think the basic idea is great.

>
> Change log: xxx -> v4 (new file.)
>  - cut out the limit-change part from hierarchy patch set.
>  - add "retry_count" arguments to shrink_usage(). This allows that we don't
>   have to set the default retry loop count.
>  - res_counter_check_under_val() is added to support subsystem.
>  - res_counter_init() is res_counter_init_ops(cnt, NULL)
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>
> ---
>  Documentation/controllers/resource_counter.txt |   19 +++++-
>  include/linux/res_counter.h                    |   33 ++++++++++-
>  kernel/res_counter.c                           |   74 ++++++++++++++++++++++++-
>  3 files changed, 121 insertions(+), 5 deletions(-)
>
> Index: linux-2.6.26-rc5-mm3/include/linux/res_counter.h
> ===================================================================
> --- linux-2.6.26-rc5-mm3.orig/include/linux/res_counter.h
> +++ linux-2.6.26-rc5-mm3/include/linux/res_counter.h
> @@ -21,6 +21,13 @@
>  * the helpers described beyond
>  */
>
> +struct res_counter;
> +struct res_counter_ops {
> +       /* called when the subsystem has to reduce the usage. */
> +       int (*shrink_usage)(struct res_counter *cnt, unsigned long long val,
> +                           int retry_count);
> +};

We should also add the limit/usage write strategy function in here too.


> +
>  struct res_counter {
>        /*
>         * the current resource consumption level
> @@ -39,6 +46,10 @@ struct res_counter {
>         */
>        unsigned long long failcnt;
>        /*
> +        * registered callbacks etc...for res_counter.
> +        */
> +       struct res_counter_ops ops;
> +       /*

As Pavel mentioned, a pointer would be better here.
> -void res_counter_init(struct res_counter *counter);
> +void res_counter_init_ops(struct res_counter *counter,
> +                               struct res_counter_ops *ops);
> +
> +static inline void res_counter_init(struct res_counter *counter)
> +{
> +       res_counter_init_ops(counter, NULL);
> +}

I would rather just see res_counter_init() take an ops parameter, and
update the (few) users of res_counter.


> +static int res_counter_resize_limit(struct res_counter *cnt,
> +                       unsigned long long val)
> +{
> +       int retry_count = 0;
> +       int ret = -EBUSY;
> +       unsigned long flags;
> +
> +       BUG_ON(!cnt->ops.shrink_usage);

As others have pointed out, there are some subsystems where usage
can't be shrunk. Maybe provide a "res_counter_unshrinkable()" function
that always returns -EBUSY and can be used by subsystems that can't
handle shrinking?

> @@ -133,11 +185,29 @@ ssize_t res_counter_write(struct res_cou
>                if (*end != '\0')
>                        goto out_free;
>        }
> +       switch (member) {
> +       case RES_LIMIT:
> +               if (counter->ops.shrink_usage) {
> +                       ret = res_counter_resize_limit(counter, tmp);
> +                       goto done;
> +               }
> +               break;
> +       default:
> +               /*
> +                * Considering future implementation, we'll have to handle
> +                * other members and "fallback" will not work well. So, we
> +                * avoid to make use of "default" here.
> +                */
> +               break;
> +       }

Would this be simpler as just

if (member == RES_LIMIT && counter->ops.shrink_usage) {
  ret = res_counter_resize_limit(counter, tmp);
} else {
  ...
}

Paul
--
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/