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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6599ad830806192209j2a3909faob223b72de3d28b81@mail.gmail.com>
Date:	Thu, 19 Jun 2008 22:09:20 -0700
From:	"Paul Menage" <menage@...gle.com>
To:	"KAMEZAWA Hiroyuki" <kamezawa.hiroyu@...fujitsu.com>
Cc:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"balbir@...ux.vnet.ibm.com" <balbir@...ux.vnet.ibm.com>,
	"xemul@...nvz.org" <xemul@...nvz.org>,
	"yamamoto@...inux.co.jp" <yamamoto@...inux.co.jp>,
	"nishimura@....nes.nec.co.jp" <nishimura@....nes.nec.co.jp>,
	"lizf@...fujitsu.com" <lizf@...fujitsu.com>
Subject: Re: [PATCH 1/6] res_counter: handle limit change

On Fri, Jun 13, 2008 at 2:29 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@...fujitsu.com> 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@...fujitsu.com>
>
> ---
>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/