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

Groups > linux.kernel > #1372124 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [patch] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage  size David Rientjes <rientjes@google.com> - 2016-04-06 03:30 +0200
    Re: [patch] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage  size Nikolay Borisov <kernel@kyup.com> - 2016-04-06 09:30 +0200
      Re: [patch] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage  size Nikolay Borisov <kernel@kyup.com> - 2016-04-06 09:40 +0200
        Re: [patch] mm, hugetlb_cgroup: round limit_in_bytes down to  hugepage size Michal Hocko <mhocko@kernel.org> - 2016-04-06 11:20 +0200
        [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to hugepage  size David Rientjes <rientjes@google.com> - 2016-04-07 00:20 +0200
          Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to  hugepage size Michal Hocko <mhocko@kernel.org> - 2016-04-07 15:00 +0200
            Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to  hugepage size David Rientjes <rientjes@google.com> - 2016-04-14 22:30 +0200
              Re: [patch v2] mm, hugetlb_cgroup: round limit_in_bytes down to  hugepage size Michal Hocko <mhocko@kernel.org> - 2016-04-15 15:30 +0200

#1372124 — [patch] 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.

Round the hugetlb_cgroup limit down to hugepage size.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/hugetlb_cgroup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -288,6 +288,7 @@ static ssize_t hugetlb_cgroup_write(struct kernfs_open_file *of,
 
 	switch (MEMFILE_ATTR(of_cft(of)->private)) {
 	case RES_LIMIT:
+		nr_pages &= ~((1 << huge_page_order(&hstates[idx])) - 1);
 		mutex_lock(&hugetlb_limit_mutex);
 		ret = page_counter_limit(&h_cg->hugepage[idx], nr_pages);
 		mutex_unlock(&hugetlb_limit_mutex);

[toc] | [next] | [standalone]

#1372255

On 04/06/2016 04:25 AM, David Rientjes wrote:
> 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.
> 
> Round the hugetlb_cgroup limit down to hugepage size.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/hugetlb_cgroup.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> --- a/mm/hugetlb_cgroup.c
> +++ b/mm/hugetlb_cgroup.c
> @@ -288,6 +288,7 @@ static ssize_t hugetlb_cgroup_write(struct kernfs_open_file *of,
>  
>  	switch (MEMFILE_ATTR(of_cft(of)->private)) {
>  	case RES_LIMIT:
> +		nr_pages &= ~((1 << huge_page_order(&hstates[idx])) - 1);

Why not:

nr_pages = round_down(nr_pages, huge_page_order(&hstates[idx]));


>  		mutex_lock(&hugetlb_limit_mutex);
>  		ret = page_counter_limit(&h_cg->hugepage[idx], nr_pages);
>  		mutex_unlock(&hugetlb_limit_mutex);
> 

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

#1372258

On 04/06/2016 10:26 AM, Nikolay Borisov wrote:
> 
> 
> On 04/06/2016 04:25 AM, David Rientjes wrote:
>> 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.
>>
>> Round the hugetlb_cgroup limit down to hugepage size.
>>
>> Signed-off-by: David Rientjes <rientjes@google.com>
>> ---
>>  mm/hugetlb_cgroup.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -288,6 +288,7 @@ static ssize_t hugetlb_cgroup_write(struct kernfs_open_file *of,
>>  
>>  	switch (MEMFILE_ATTR(of_cft(of)->private)) {
>>  	case RES_LIMIT:
>> +		nr_pages &= ~((1 << huge_page_order(&hstates[idx])) - 1);
> 
> Why not:
> 
> nr_pages = round_down(nr_pages, huge_page_order(&hstates[idx]));

Oops, that should be:

round_down(nr_pages, 1 << huge_page_order(&hstates[idx]));

> 
> 
>>  		mutex_lock(&hugetlb_limit_mutex);
>>  		ret = page_counter_limit(&h_cg->hugepage[idx], nr_pages);
>>  		mutex_unlock(&hugetlb_limit_mutex);
>>

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

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

On Wed 06-04-16 10:33:19, Nikolay Borisov wrote:
> 
> 
> On 04/06/2016 10:26 AM, Nikolay Borisov wrote:
> > 
> > 
> > On 04/06/2016 04:25 AM, David Rientjes wrote:
> >> 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.
> >>
> >> Round the hugetlb_cgroup limit down to hugepage size.
> >>
> >> Signed-off-by: David Rientjes <rientjes@google.com>
> >> ---
> >>  mm/hugetlb_cgroup.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> >> --- a/mm/hugetlb_cgroup.c
> >> +++ b/mm/hugetlb_cgroup.c
> >> @@ -288,6 +288,7 @@ static ssize_t hugetlb_cgroup_write(struct kernfs_open_file *of,
> >>  
> >>  	switch (MEMFILE_ATTR(of_cft(of)->private)) {
> >>  	case RES_LIMIT:
> >> +		nr_pages &= ~((1 << huge_page_order(&hstates[idx])) - 1);
> > 
> > Why not:
> > 
> > nr_pages = round_down(nr_pages, huge_page_order(&hstates[idx]));
> 
> Oops, that should be:
> 
> round_down(nr_pages, 1 << huge_page_order(&hstates[idx]));

round_down is a bit nicer.

Anyway
Acked-by: Michal Hocko <mhocko@suse.com>

-- 
Michal Hocko
SUSE Labs

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

#1372891 — [patch v2] 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.

Round the hugetlb_cgroup limit down to hugepage size.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 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] | [next] | [standalone]

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

On Wed 06-04-16 15:10:23, David Rientjes 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?
-- 
Michal Hocko
SUSE Labs

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

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

On Thu, 7 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.

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

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

On Thu 14-04-16 13:22:30, David Rientjes wrote:
> On Thu, 7 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.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web