| 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
| ||
|
Message-ID: <6bd3789f-4dee-a184-d415-4ad77f0f98b7@oracle.com>
Date: Wed, 13 Oct 2021 15:06:31 -0700
From: Mike Kravetz <mike.kravetz@...cle.com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>,
akpm@...ux-foundation.org
Cc: mhocko@...nel.org, guro@...com, corbet@....net,
yaozhenguo1@...il.com, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH] hugetlb: Support node specified when using cma for
gigantic hugepages
On 10/9/21 10:24 PM, Baolin Wang wrote:
> Now the size of CMA area for gigantic hugepages runtime allocation is
> balanced for all online nodes, but we also want to specify the size of
> CMA per-node, or only one node in some cases, which are similar with
> commit 86acc55c3d32 ("hugetlbfs: extend the definition of hugepages
> parameter to support node allocation")[1].
>
> Thus this patch adds node format for 'hugetlb_cma' parameter to support
> specifying the size of CMA per-node. An example is as follows:
>
> hugetlb_cma=0:5G,2:5G
>
> which means allocating 5G size of CMA area on node 0 and node 2
> respectively.
>
> [1]
> https://lkml.kernel.org/r/20211005054729.86457-1-yaozhenguo1@gmail.com
>
> Signed-off-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 6 +-
> mm/hugetlb.c | 79 +++++++++++++++++++++----
> 2 files changed, 73 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 3ad8e9d0..a147faa5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1587,8 +1587,10 @@
> registers. Default set by CONFIG_HPET_MMAP_DEFAULT.
>
> hugetlb_cma= [HW,CMA] The size of a CMA area used for allocation
> - of gigantic hugepages.
> - Format: nn[KMGTPE]
> + of gigantic hugepages. Or using node format, the size
> + of a CMA area per node can be specified.
> + Format: nn[KMGTPE] or (node format)
> + <node>:nn[KMGTPE][,<node>:nn[KMGTPE]]
>
> Reserve a CMA area of given size and allocate gigantic
> hugepages using the CMA allocator. If enabled, the
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 6d2f4c2..8b4e409 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -50,6 +50,7 @@
>
> #ifdef CONFIG_CMA
> static struct cma *hugetlb_cma[MAX_NUMNODES];
> +static unsigned long hugetlb_cma_size_in_node[MAX_NUMNODES] __initdata;
> static bool hugetlb_cma_page(struct page *page, unsigned int order)
> {
> return cma_pages_valid(hugetlb_cma[page_to_nid(page)], page,
> @@ -62,6 +63,7 @@ static bool hugetlb_cma_page(struct page *page, unsigned int order)
> }
> #endif
> static unsigned long hugetlb_cma_size __initdata;
> +static nodemask_t hugetlb_cma_nodes_allowed = NODE_MASK_NONE;
>
> /*
> * Minimum page order among possible hugepage sizes, set to a proper value
> @@ -3497,9 +3499,15 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
>
> if (nid == NUMA_NO_NODE) {
> /*
> + * If we've specified the size of CMA area per node,
> + * should use it firstly.
> + */
> + if (hstate_is_gigantic(h) && !nodes_empty(hugetlb_cma_nodes_allowed))
> + n_mask = &hugetlb_cma_nodes_allowed;
> + /*
IIUC, this changes the behavior for 'balanced' gigantic huge page pool
allocations if per-node hugetlb_cma is specified. It will now only
attempt to allocate gigantic pages on nodes where CMA was reserved.
Even if we run out of space on the node, it will not go to other nodes
as before. Is that correct?
I do not believe we want this change in behavior. IMO, if the user is
doing node specific CMA reservations, then the user should use the node
specific sysfs file for pool allocations on that node.
> * global hstate attribute
> */
> - if (!(obey_mempolicy &&
> + else if (!(obey_mempolicy &&
> init_nodemask_of_mempolicy(&nodes_allowed)))
> n_mask = &node_states[N_MEMORY];
> else
> @@ -6745,7 +6753,38 @@ void hugetlb_unshare_all_pmds(struct vm_area_struct *vma)
>
> static int __init cmdline_parse_hugetlb_cma(char *p)
> {
> - hugetlb_cma_size = memparse(p, &p);
> + int nid, count = 0;
> + unsigned long tmp;
> + char *s = p;
> +
> + while (*s) {
> + if (sscanf(s, "%lu%n", &tmp, &count) != 1)
> + break;
> +
> + if (s[count] == ':') {
> + nid = tmp;
> + if (nid < 0 || nid >= MAX_NUMNODES)
> + break;
nid can only be compared to MAX_NUMNODES because this an early param
before numa is setup and we do not know exactly how many nodes there
are. Is this correct?
Suppose one specifies an invaid node. For example, on my 2 node system
I use the option 'hugetlb_cma=2:2G'. This is not flagged as an error
during processing and 1G CMA is reserved on node 0 and 1G is reserved
on node 1. Is that by design, or just chance?
We should be able to catch this in hugetlb_cma_reserve. For the example
above, I think we should flag this as an error and not reserve any CMA.
> +
> + s += count + 1;
> + tmp = memparse(s, &s);
> + hugetlb_cma_size_in_node[nid] = tmp;
> + hugetlb_cma_size += tmp;
> +
> + /*
> + * Skip the separator if have one, otherwise
> + * break the parsing.
> + */
> + if (*s == ',')
> + s++;
> + else
> + break;
> + } else {
> + hugetlb_cma_size = memparse(p, &p);
> + break;
> + }
> + }
> +
> return 0;
> }
>
> @@ -6754,6 +6793,7 @@ static int __init cmdline_parse_hugetlb_cma(char *p)
> void __init hugetlb_cma_reserve(int order)
> {
> unsigned long size, reserved, per_node;
> + bool node_specific_cma_alloc = false;
> int nid;
>
> cma_reserve_called = true;
> @@ -6767,20 +6807,37 @@ void __init hugetlb_cma_reserve(int order)
> return;
> }
Earlier in hugetlb_cma_reserve (not in the context here), there is this
code:
if (hugetlb_cma_size < (PAGE_SIZE << order)) {
pr_warn("hugetlb_cma: cma area should be at least %lu MiB\n",
(PAGE_SIZE << order) / SZ_1M);
hugetlb_cma_size = 0;
return;
}
That causes an early exit if hugetlb_cma_size is too small for a
gigantic page.
On my 2 node x86 system with 1G gigantic pages, I can specify
'hugetlb_cma=0:512M,1:512M'. This does not trigger the above early exit
because total hugetlb_cma_size is 1G. It does end up reserving 1G on
node 0 and nothing on node 1. I do not believe this is by design. We
should validate the specified per-node sizes as well.
--
Mike Kravetz
>
> - /*
> - * If 3 GB area is requested on a machine with 4 numa nodes,
> - * let's allocate 1 GB on first three nodes and ignore the last one.
> - */
> - per_node = DIV_ROUND_UP(hugetlb_cma_size, nr_online_nodes);
> - pr_info("hugetlb_cma: reserve %lu MiB, up to %lu MiB per node\n",
> - hugetlb_cma_size / SZ_1M, per_node / SZ_1M);
> + for_each_node_state(nid, N_ONLINE) {
> + if (hugetlb_cma_size_in_node[nid] > 0) {
> + node_specific_cma_alloc = true;
> + break;
> + }
> + }
> +
> + if (!node_specific_cma_alloc) {
> + /*
> + * If 3 GB area is requested on a machine with 4 numa nodes,
> + * let's allocate 1 GB on first three nodes and ignore the last one.
> + */
> + per_node = DIV_ROUND_UP(hugetlb_cma_size, nr_online_nodes);
> + pr_info("hugetlb_cma: reserve %lu MiB, up to %lu MiB per node\n",
> + hugetlb_cma_size / SZ_1M, per_node / SZ_1M);
> + }
>
> reserved = 0;
> for_each_node_state(nid, N_ONLINE) {
> int res;
> char name[CMA_MAX_NAME];
>
> - size = min(per_node, hugetlb_cma_size - reserved);
> + if (node_specific_cma_alloc) {
> + if (hugetlb_cma_size_in_node[nid] <= 0)
> + continue;
> +
> + size = hugetlb_cma_size_in_node[nid];
> + } else {
> + size = min(per_node, hugetlb_cma_size - reserved);
> + }
> +
> size = round_up(size, PAGE_SIZE << order);
>
> snprintf(name, sizeof(name), "hugetlb%d", nid);
> @@ -6799,6 +6856,8 @@ void __init hugetlb_cma_reserve(int order)
> continue;
> }
>
> + if (node_specific_cma_alloc)
> + node_set(nid, hugetlb_cma_nodes_allowed);
> reserved += size;
> pr_info("hugetlb_cma: reserved %lu MiB on node %d\n",
> size / SZ_1M, nid);
>
Powered by blists - more mailing lists