linux-kernel - Re: [RFC][PATCH 2/2] memcg: hardwall hierarhcy for memcg

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]

Date:	Wed, 4 Jun 2008 01:59:12 -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>
Subject: Re: [RFC][PATCH 2/2] memcg: hardwall hierarhcy for memcg

On Tue, Jun 3, 2008 at 10:03 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@...fujitsu.com> wrote:
> @@ -792,6 +798,89 @@ int mem_cgroup_shrink_usage(struct mm_st
>  }
>
>  /*
> + * Memory Controller hierarchy support.
> + */
> +
> +/*
> + * shrink usage to be res->usage + val < res->limit.
> + */
> +
> +int memcg_shrink_val(struct res_counter *cnt, unsigned long long val)
> +{
> +       struct mem_cgroup *memcg = container_of(cnt, struct mem_cgroup, res);
> +       unsigned long flags;
> +       int ret = 1;
> +       int progress = 1;
> +
> +retry:
> +       spin_lock_irqsave(&cnt->lock, flags);
> +       /* Need to shrink ? */
> +       if (cnt->usage + val <= cnt->limit)
> +               ret = 0;
> +       spin_unlock_irqrestore(&cnt->lock, flags);

Can't this logic be in res_counter itself? I.e. the callback can
assume that some shrinking needs to be done, and should just do it and
return. The res_counter can handle retrying if necessary.

> +/*
> + * For Hard Wall Hierarchy.
> + */
> +
> +int mem_cgroup_resize_callback(struct res_counter *cnt,
> +                       unsigned long long val, int what)
> +{
> +       unsigned long flags, borrow;
> +       unsigned long long diffs;
> +       int ret = 0;
> +
> +       BUG_ON(what != RES_LIMIT);
> +
> +       /* Is this under hierarchy ? */
> +       if (!cnt->parent) {
> +               spin_lock_irqsave(&cnt->lock, flags);
> +               cnt->limit = val;
> +               spin_unlock_irqrestore(&cnt->lock, flags);
> +               return 0;
> +       }
> +
> +       spin_lock_irqsave(&cnt->lock, flags);
> +       if (val > cnt->limit) {
> +               diffs = val - cnt->limit;
> +               borrow = 1;
> +       } else {
> +               diffs = cnt->limit - val;
> +               borrow = 0;
> +       }
> +       spin_unlock_irqrestore(&cnt->lock, flags);
> +
> +       if (borrow)
> +               ret = res_counter_move_resource(cnt,diffs,
> +                                       memcg_shrink_val,
> +                                       MEM_CGROUP_RECLAIM_RETRIES);
> +       else
> +               ret = res_counter_return_resource(cnt, diffs,
> +                                       memcg_shrink_val,
> +                                       MEM_CGROUP_RECLAIM_RETRIES);
> +       return ret;
> +}

Again, a lot of this function seems like generic logic that should be
in res_counter. The only bit that's memory specific is the
memcg_shrink_val, and maybe that could just be passed when creating
the res_counter. Perhaps we should have a res_counter_ops structure
with operations like "parse" for parsing strings into numbers
(currently called "write_strategy") and "reclaim" for trying to shrink
the usage.

> @@ -896,11 +987,44 @@ static ssize_t mem_cgroup_write(struct c
>                                struct file *file, const char __user *userbuf,
>                                size_t nbytes, loff_t *ppos)
>  {
> -       return res_counter_write(&mem_cgroup_from_cont(cont)->res,
> +       struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> +
> +       if (cft->private != RES_LIMIT
> +               || !cont->parent
> +               || memcg->hierarchy_model == MEMCG_NO_HIERARCHY)

The res_counter already knows whether it has a parent, so these checks
shouldn't be necessary.

> @@ -1096,6 +1238,12 @@ static void mem_cgroup_destroy(struct cg
>        int node;
>        struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
>
> +       if (cont->parent &&
> +           mem->hierarchy_model == MEMCG_HARDWALL_HIERARCHY) {
> +               /* we did what we can...just returns what we borrow */
> +               res_counter_return_resource(&mem->res, -1, NULL, 0);
> +       }
> +

Should we also re-account any remaining child usage to the parent?

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/