Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1382070 > unrolled thread

Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage size

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to  hugepage size David Rientjes <rientjes@google.com> - 2016-04-18 23:30 +0200
    Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to  hugepage size Andrew Morton <akpm@linux-foundation.org> - 2016-04-26 00:00 +0200
      [patch v3] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage  size David Rientjes <rientjes@google.com> - 2016-04-26 02:00 +0200

#1382070 — Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage size

On Fri, 15 Apr 2016, Michal Hocko wrote:

> > > > +static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup,
> > > > +				struct hugetlb_cgroup *parent_h_cgroup)
> > > > +{
> > > > +	int idx;
> > > > +
> > > > +	for (idx = 0; idx < HUGE_MAX_HSTATE; idx++) {
> > > > +		struct page_counter *counter = &h_cgroup->hugepage[idx];
> > > > +		struct page_counter *parent = NULL;
> > > > +		unsigned long limit;
> > > > +		int ret;
> > > > +
> > > > +		if (parent_h_cgroup)
> > > > +			parent = &parent_h_cgroup->hugepage[idx];
> > > > +		page_counter_init(counter, parent);
> > > > +
> > > > +		limit = round_down(PAGE_COUNTER_MAX,
> > > > +				   1 << huge_page_order(&hstates[idx]));
> > > > +		ret = page_counter_limit(counter, limit);
> > > > +		VM_BUG_ON(ret);
> > > > +	}
> > > > +}
> > > 
> > > I fail to see the point for this. Why would want to round down
> > > PAGE_COUNTER_MAX? It will never make a real difference. Or am I missing
> > > something?
> > 
> > Did you try the patch?
> > 
> > If we're rounding down the user value, it makes sense to be consistent 
> > with the upper bound default to specify intent.
> 
> The point I've tried to raise is why do we care and add a code if we can
> never reach that value? Does actually anybody checks for the alignment.

If the user modifies the value successfully, it can never be restored to 
the default since the write handler rounds down.  It's a matter of 
consistency for a long-term maintainable kernel and prevents bug reports.

[toc] | [next] | [standalone]

#1386945

On Mon, 18 Apr 2016 14:23:58 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:

> On Fri, 15 Apr 2016, Michal Hocko wrote:
> 
> > > > > +static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup,
> > > > > +				struct hugetlb_cgroup *parent_h_cgroup)
> > > > > +{
> > > > > +	int idx;
> > > > > +
> > > > > +	for (idx = 0; idx < HUGE_MAX_HSTATE; idx++) {
> > > > > +		struct page_counter *counter = &h_cgroup->hugepage[idx];
> > > > > +		struct page_counter *parent = NULL;
> > > > > +		unsigned long limit;
> > > > > +		int ret;
> > > > > +
> > > > > +		if (parent_h_cgroup)
> > > > > +			parent = &parent_h_cgroup->hugepage[idx];
> > > > > +		page_counter_init(counter, parent);
> > > > > +
> > > > > +		limit = round_down(PAGE_COUNTER_MAX,
> > > > > +				   1 << huge_page_order(&hstates[idx]));
> > > > > +		ret = page_counter_limit(counter, limit);
> > > > > +		VM_BUG_ON(ret);
> > > > > +	}
> > > > > +}
> > > > 
> > > > I fail to see the point for this. Why would want to round down
> > > > PAGE_COUNTER_MAX? It will never make a real difference. Or am I missing
> > > > something?
> > > 
> > > Did you try the patch?
> > > 
> > > If we're rounding down the user value, it makes sense to be consistent 
> > > with the upper bound default to specify intent.
> > 
> > The point I've tried to raise is why do we care and add a code if we can
> > never reach that value? Does actually anybody checks for the alignment.
> 
> If the user modifies the value successfully, it can never be restored to 
> the default since the write handler rounds down.  It's a matter of 
> consistency for a long-term maintainable kernel and prevents bug reports.

Can we please get the above reasoning into the changelog?

Also, the runtime effects of the patch are unclear - "not possible to
charge partial hugepages" sounds serious, but there's no cc:stable. 
Some clarification there also please.

[toc] | [prev] | [next] | [standalone]

#1387013 — [patch v3] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage size

The page_counter rounds limits down to page size values.  This makes
sense, except in the case of hugetlb_cgroup where it's not possible to
charge partial hugepages.  If the hugetlb_cgroup margin is less than the
hugepage size being charged, it will fail as expected.

Round the hugetlb_cgroup limit down to hugepage size, since it is the
effective limit of the cgroup.

For consistency, round down PAGE_COUNTER_MAX as well when a
hugetlb_cgroup is created: this prevents error reports when a user cannot
restore the value to the kernel default.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 v3: update changelog per akpm
     no stable backport needed

 mm/hugetlb_cgroup.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -67,26 +67,42 @@ static inline bool hugetlb_cgroup_have_usage(struct hugetlb_cgroup *h_cg)
 	return false;
 }
 
+static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup,
+				struct hugetlb_cgroup *parent_h_cgroup)
+{
+	int idx;
+
+	for (idx = 0; idx < HUGE_MAX_HSTATE; idx++) {
+		struct page_counter *counter = &h_cgroup->hugepage[idx];
+		struct page_counter *parent = NULL;
+		unsigned long limit;
+		int ret;
+
+		if (parent_h_cgroup)
+			parent = &parent_h_cgroup->hugepage[idx];
+		page_counter_init(counter, parent);
+
+		limit = round_down(PAGE_COUNTER_MAX,
+				   1 << huge_page_order(&hstates[idx]));
+		ret = page_counter_limit(counter, limit);
+		VM_BUG_ON(ret);
+	}
+}
+
 static struct cgroup_subsys_state *
 hugetlb_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 {
 	struct hugetlb_cgroup *parent_h_cgroup = hugetlb_cgroup_from_css(parent_css);
 	struct hugetlb_cgroup *h_cgroup;
-	int idx;
 
 	h_cgroup = kzalloc(sizeof(*h_cgroup), GFP_KERNEL);
 	if (!h_cgroup)
 		return ERR_PTR(-ENOMEM);
 
-	if (parent_h_cgroup) {
-		for (idx = 0; idx < HUGE_MAX_HSTATE; idx++)
-			page_counter_init(&h_cgroup->hugepage[idx],
-					  &parent_h_cgroup->hugepage[idx]);
-	} else {
+	if (!parent_h_cgroup)
 		root_h_cgroup = h_cgroup;
-		for (idx = 0; idx < HUGE_MAX_HSTATE; idx++)
-			page_counter_init(&h_cgroup->hugepage[idx], NULL);
-	}
+
+	hugetlb_cgroup_init(h_cgroup, parent_h_cgroup);
 	return &h_cgroup->css;
 }
 
@@ -285,6 +301,7 @@ static ssize_t hugetlb_cgroup_write(struct kernfs_open_file *of,
 		return ret;
 
 	idx = MEMFILE_IDX(of_cft(of)->private);
+	nr_pages = round_down(nr_pages, 1 << huge_page_order(&hstates[idx]));
 
 	switch (MEMFILE_ATTR(of_cft(of)->private)) {
 	case RES_LIMIT:

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web