Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: David Rientjes <rientjes@google.com>
To: Mina Almasry <almasrymina@google.com>
Cc: mike.kravetz@oracle.com, shakeelb@google.com, shuah@kernel.org,
	gthelen@google.com, Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-kselftest@vger.kernel.org, cgroups@vger.kernel.org,
	aneesh.kumar@linux.vnet.ibm.com
Subject: Re: [PATCH v10 2/8] hugetlb_cgroup: add interface for charge/uncharge hugetlb reservations
Date: Wed, 29 Jan 2020 13:21:52 -0800 (PST)
Message-ID: <alpine.DEB.2.21.2001291312490.175731@chino.kir.corp.google.com> (raw)
In-Reply-To: <20200115012651.228058-2-almasrymina@google.com>

On Tue, 14 Jan 2020, Mina Almasry wrote:

> diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
> index 063962f6dfc6a..eab8a70d5bcb5 100644
> --- a/include/linux/hugetlb_cgroup.h
> +++ b/include/linux/hugetlb_cgroup.h
> @@ -20,29 +20,37 @@
>  struct hugetlb_cgroup;
>  /*
>   * Minimum page order trackable by hugetlb cgroup.
> - * At least 3 pages are necessary for all the tracking information.
> + * At least 4 pages are necessary for all the tracking information.
>   */
>  #define HUGETLB_CGROUP_MIN_ORDER	2

I always struggle with a way to document and protect these types of 
usages.  In this case, we are using the private filed of tail pages; in 
thp code, we enumerate these usages separately in struct page: see "Tail 
pages of compound page" comment in the union.  Using the private field is 
fine to store a pointer to the hugetlb_cgroup, but I'm wondering if we can 
document or protect against future patches not understanding this usage.  
Otherwise it's implicit beyond this comment.

Maybe an expanded comment here is the only thing that is needed because 
it's unique to struct hugetlb_cgroup that describes what struct page 
represents for the second, third, and (now) fourth tail page.

Or, we could go even more explicit and define an enum that defines the 
third tail page is for hugetlb_cgroup limits and fourth is for 
hugetlb_cgroup reservation limits.

enum {
...
	H_CG_LIMIT,
	H_CG_RESV,
	MAX_H_CG_PRIVATE,
};

and then do a

BUILD_BUG_ON(MAX_H_CG_PRIVATE > (1 << HUGETLB_CGROUP_MIN_ORDER));

somewhere?  And then use H_CG_RESV (or a better name) as the offset from 
the head page in the code?

> 
>  #ifdef CONFIG_CGROUP_HUGETLB
> 
> -static inline struct hugetlb_cgroup *hugetlb_cgroup_from_page(struct page *page)
> +static inline struct hugetlb_cgroup *hugetlb_cgroup_from_page(struct page *page,
> +							      bool reserved)

When implementing specs, I immediately see "bool reserved" or 
"int reserved:1" as reserved for future use :)  Maybe simple enough to 
just name these formals as resv?

Otherwise looks like a simple conversion of existing functionality and 
extension to the new limits.

Acked-by: David Rientjes <rientjes@google.com>

  parent reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-15  1:26 [PATCH v10 1/8] hugetlb_cgroup: Add hugetlb_cgroup reservation counter Mina Almasry
2020-01-15  1:26 ` [PATCH v10 2/8] hugetlb_cgroup: add interface for charge/uncharge hugetlb reservations Mina Almasry
2020-01-17 19:26   ` Mike Kravetz
2020-01-17 19:34     ` Mina Almasry
2020-01-29 21:21   ` David Rientjes [this message]
2020-01-30  0:41     ` Mike Kravetz
2020-01-15  1:26 ` [PATCH v10 3/8] hugetlb_cgroup: add reservation accounting for private mappings Mina Almasry
2020-01-17 22:57   ` Mike Kravetz
2020-01-29 21:28   ` David Rientjes
2020-02-03 23:17     ` Mina Almasry
2020-01-15  1:26 ` [PATCH v10 4/8] hugetlb: disable region_add file_region coalescing Mina Almasry
2020-01-21 18:50   ` Mike Kravetz
2020-01-15  1:26 ` [PATCH v10 5/8] hugetlb_cgroup: add accounting for shared mappings Mina Almasry
2020-01-15  1:26 ` [PATCH v10 6/8] hugetlb_cgroup: support noreserve mappings Mina Almasry
2020-01-15  1:26 ` [PATCH v10 7/8] hugetlb_cgroup: Add hugetlb_cgroup reservation tests Mina Almasry
2020-01-23  9:15   ` Sandipan Das
2020-01-23 20:05     ` Mina Almasry
2020-01-29 21:00     ` David Rientjes
2020-01-30  6:11       ` Sandipan Das
2020-02-03 23:18         ` Mina Almasry
2020-01-15  1:26 ` [PATCH v10 8/8] hugetlb_cgroup: Add hugetlb_cgroup reservation docs Mina Almasry
2020-01-16 22:44 ` [PATCH v10 1/8] hugetlb_cgroup: Add hugetlb_cgroup reservation counter Mike Kravetz
2020-01-29 21:09 ` David Rientjes
2020-02-03 23:16   ` Mina Almasry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.2001291312490.175731@chino.kir.corp.google.com \
    --to=rientjes@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=almasrymina@google.com \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=cgroups@vger.kernel.org \
    --cc=gthelen@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mike.kravetz@oracle.com \
    --cc=shakeelb@google.com \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git