* [PATCH v3 3/4] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-25 18:43 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linux-fsdevel, linux-api, corbet, akpm,
gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji, mhocko,
ying.huang, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <20240125184345.47074-1-gregory.price@memverge.com>
When a system has multiple NUMA nodes and it becomes bandwidth hungry,
using the current MPOL_INTERLEAVE could be an wise option.
However, if those NUMA nodes consist of different types of memory such
as socket-attached DRAM and CXL/PCIe attached DRAM, the round-robin
based interleave policy does not optimally distribute data to make use
of their different bandwidth characteristics.
Instead, interleave is more effective when the allocation policy follows
each NUMA nodes' bandwidth weight rather than a simple 1:1 distribution.
This patch introduces a new memory policy, MPOL_WEIGHTED_INTERLEAVE,
enabling weighted interleave between NUMA nodes. Weighted interleave
allows for proportional distribution of memory across multiple numa
nodes, preferably apportioned to match the bandwidth of each node.
For example, if a system has 1 CPU node (0), and 2 memory nodes (0,1),
with bandwidth of (100GB/s, 50GB/s) respectively, the appropriate
weight distribution is (2:1).
Weights for each node can be assigned via the new sysfs extension:
/sys/kernel/mm/mempolicy/weighted_interleave/
For now, the default value of all nodes will be `1`, which matches
the behavior of standard 1:1 round-robin interleave. An extension
will be added in the future to allow default values to be registered
at kernel and device bringup time.
The policy allocates a number of pages equal to the set weights. For
example, if the weights are (2,1), then 2 pages will be allocated on
node0 for every 1 page allocated on node1.
The new flag MPOL_WEIGHTED_INTERLEAVE can be used in set_mempolicy(2)
and mbind(2).
There are 3 integration points:
weighted_interleave_nodes:
Counts the number of allocations as they occur, and applies the
weight for the current node. When the weight reaches 0, switch
to the next node.
weighted_interleave_nid:
Gets the total weight of the nodemask as well as each individual
node weight, then calculates the node based on the given index.
bulk_array_weighted_interleave:
Gets the total weight of the nodemask as well as each individual
node weight, then calculates the number of "interleave rounds" as
well as any delta ("partial round"). Calculates the number of
pages for each node and allocates them.
If a node was scheduled for interleave via interleave_nodes, the
current weight (pol->cur_il_weight) will be allocated first, before
the remaining bulk calculation is done.
One piece of complexity is the interaction between a recent refactor
which split the logic to acquire the "ilx" (interleave index) of an
allocation and the actually application of the interleave. The
calculation of the `interleave index` is done by `get_vma_policy()`,
while the actual selection of the node will be later appliex by the
relevant weighted_interleave function.
Suggested-by: Hasan Al Maruf <Hasan.Maruf@amd.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Co-developed-by: Rakie Kim <rakie.kim@sk.com>
Signed-off-by: Rakie Kim <rakie.kim@sk.com>
Co-developed-by: Honggyu Kim <honggyu.kim@sk.com>
Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Co-developed-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Signed-off-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Co-developed-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
Co-developed-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
Signed-off-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
---
.../admin-guide/mm/numa_memory_policy.rst | 9 +
include/linux/mempolicy.h | 3 +
include/uapi/linux/mempolicy.h | 1 +
mm/mempolicy.c | 274 +++++++++++++++++-
4 files changed, 283 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/mm/numa_memory_policy.rst b/Documentation/admin-guide/mm/numa_memory_policy.rst
index eca38fa81e0f..a70f20ce1ffb 100644
--- a/Documentation/admin-guide/mm/numa_memory_policy.rst
+++ b/Documentation/admin-guide/mm/numa_memory_policy.rst
@@ -250,6 +250,15 @@ MPOL_PREFERRED_MANY
can fall back to all existing numa nodes. This is effectively
MPOL_PREFERRED allowed for a mask rather than a single node.
+MPOL_WEIGHTED_INTERLEAVE
+ This mode operates the same as MPOL_INTERLEAVE, except that
+ interleaving behavior is executed based on weights set in
+ /sys/kernel/mm/mempolicy/weighted_interleave/
+
+ Weighted interleave allocates pages on nodes according to a
+ weight. For example if nodes [0,1] are weighted [5,2], 5 pages
+ will be allocated on node0 for every 2 pages allocated on node1.
+
NUMA memory policy supports the following optional mode flags:
MPOL_F_STATIC_NODES
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
index 931b118336f4..c644d7bbd396 100644
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -54,6 +54,9 @@ struct mempolicy {
nodemask_t cpuset_mems_allowed; /* relative to these nodes */
nodemask_t user_nodemask; /* nodemask passed by user */
} w;
+
+ /* Weighted interleave settings */
+ u8 cur_il_weight;
};
/*
diff --git a/include/uapi/linux/mempolicy.h b/include/uapi/linux/mempolicy.h
index a8963f7ef4c2..1f9bb10d1a47 100644
--- a/include/uapi/linux/mempolicy.h
+++ b/include/uapi/linux/mempolicy.h
@@ -23,6 +23,7 @@ enum {
MPOL_INTERLEAVE,
MPOL_LOCAL,
MPOL_PREFERRED_MANY,
+ MPOL_WEIGHTED_INTERLEAVE,
MPOL_MAX, /* always last member of enum */
};
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b13c45a0bfcb..5a517511658e 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -19,6 +19,13 @@
* for anonymous memory. For process policy an process counter
* is used.
*
+ * weighted interleave
+ * Allocate memory interleaved over a set of nodes based on
+ * a set of weights (per-node), with normal fallback if it
+ * fails. Otherwise operates the same as interleave.
+ * Example: nodeset(0,1) & weights (2,1) - 2 pages allocated
+ * on node 0 for every 1 page allocated on node 1.
+ *
* bind Only allocate memory on a specific set of nodes,
* no fallback.
* FIXME: memory is allocated starting with the first node
@@ -314,6 +321,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags,
policy->mode = mode;
policy->flags = flags;
policy->home_node = NUMA_NO_NODE;
+ policy->cur_il_weight = 0;
return policy;
}
@@ -426,6 +434,10 @@ static const struct mempolicy_operations mpol_ops[MPOL_MAX] = {
.create = mpol_new_nodemask,
.rebind = mpol_rebind_preferred,
},
+ [MPOL_WEIGHTED_INTERLEAVE] = {
+ .create = mpol_new_nodemask,
+ .rebind = mpol_rebind_nodemask,
+ },
};
static bool migrate_folio_add(struct folio *folio, struct list_head *foliolist,
@@ -847,7 +859,8 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
old = current->mempolicy;
current->mempolicy = new;
- if (new && new->mode == MPOL_INTERLEAVE)
+ if (new && (new->mode == MPOL_INTERLEAVE ||
+ new->mode == MPOL_WEIGHTED_INTERLEAVE))
current->il_prev = MAX_NUMNODES-1;
task_unlock(current);
mpol_put(old);
@@ -873,6 +886,7 @@ static void get_policy_nodemask(struct mempolicy *pol, nodemask_t *nodes)
case MPOL_INTERLEAVE:
case MPOL_PREFERRED:
case MPOL_PREFERRED_MANY:
+ case MPOL_WEIGHTED_INTERLEAVE:
*nodes = pol->nodes;
break;
case MPOL_LOCAL:
@@ -957,6 +971,13 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
} else if (pol == current->mempolicy &&
pol->mode == MPOL_INTERLEAVE) {
*policy = next_node_in(current->il_prev, pol->nodes);
+ } else if (pol == current->mempolicy &&
+ (pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
+ if (pol->cur_il_weight)
+ *policy = current->il_prev;
+ else
+ *policy = next_node_in(current->il_prev,
+ pol->nodes);
} else {
err = -EINVAL;
goto out;
@@ -1769,7 +1790,8 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
* @vma: virtual memory area whose policy is sought
* @addr: address in @vma for shared policy lookup
* @order: 0, or appropriate huge_page_order for interleaving
- * @ilx: interleave index (output), for use only when MPOL_INTERLEAVE
+ * @ilx: interleave index (output), for use only when MPOL_INTERLEAVE or
+ * MPOL_WEIGHTED_INTERLEAVE
*
* Returns effective policy for a VMA at specified address.
* Falls back to current->mempolicy or system default policy, as necessary.
@@ -1786,7 +1808,8 @@ struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
pol = __get_vma_policy(vma, addr, ilx);
if (!pol)
pol = get_task_policy(current);
- if (pol->mode == MPOL_INTERLEAVE) {
+ if (pol->mode == MPOL_INTERLEAVE ||
+ pol->mode == MPOL_WEIGHTED_INTERLEAVE) {
*ilx += vma->vm_pgoff >> order;
*ilx += (addr - vma->vm_start) >> (PAGE_SHIFT + order);
}
@@ -1836,6 +1859,44 @@ bool apply_policy_zone(struct mempolicy *policy, enum zone_type zone)
return zone >= dynamic_policy_zone;
}
+static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
+{
+ unsigned int node, next;
+ struct task_struct *me = current;
+ u8 __rcu *table;
+ u8 weight;
+
+ node = next_node_in(me->il_prev, policy->nodes);
+ if (node == MAX_NUMNODES)
+ return node;
+
+ /* on first alloc after setting mempolicy, acquire first weight */
+ if (unlikely(!policy->cur_il_weight)) {
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* detect system-default values */
+ weight = table ? table[node] : 1;
+ policy->cur_il_weight = weight ? weight : 1;
+ rcu_read_unlock();
+ }
+
+ /* account for this allocation call */
+ policy->cur_il_weight--;
+
+ /* if now at 0, move to next node and set up that node's weight */
+ if (unlikely(!policy->cur_il_weight)) {
+ me->il_prev = node;
+ next = next_node_in(node, policy->nodes);
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* detect system-default values */
+ weight = table ? table[next] : 1;
+ policy->cur_il_weight = weight ? weight : 1;
+ rcu_read_unlock();
+ }
+ return node;
+}
+
/* Do dynamic interleaving for a process */
static unsigned int interleave_nodes(struct mempolicy *policy)
{
@@ -1870,6 +1931,9 @@ unsigned int mempolicy_slab_node(void)
case MPOL_INTERLEAVE:
return interleave_nodes(policy);
+ case MPOL_WEIGHTED_INTERLEAVE:
+ return weighted_interleave_nodes(policy);
+
case MPOL_BIND:
case MPOL_PREFERRED_MANY:
{
@@ -1908,6 +1972,39 @@ static unsigned int read_once_policy_nodemask(struct mempolicy *pol,
return nodes_weight(*mask);
}
+static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
+{
+ nodemask_t nodemask;
+ unsigned int target, nr_nodes;
+ u8 __rcu *table;
+ unsigned int weight_total = 0;
+ u8 weight;
+ int nid;
+
+ nr_nodes = read_once_policy_nodemask(pol, &nodemask);
+ if (!nr_nodes)
+ return numa_node_id();
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* calculate the total weight */
+ for_each_node_mask(nid, nodemask)
+ weight_total += table ? table[nid] : 1;
+
+ /* Calculate the node offset based on totals */
+ target = ilx % weight_total;
+ nid = first_node(nodemask);
+ while (target) {
+ weight = table ? table[nid] : 1;
+ if (target < weight)
+ break;
+ target -= weight;
+ nid = next_node_in(nid, nodemask);
+ }
+ rcu_read_unlock();
+ return nid;
+}
+
/*
* Do static interleaving for interleave index @ilx. Returns the ilx'th
* node in pol->nodes (starting from ilx=0), wrapping around if ilx
@@ -1968,6 +2065,11 @@ static nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *pol,
*nid = (ilx == NO_INTERLEAVE_INDEX) ?
interleave_nodes(pol) : interleave_nid(pol, ilx);
break;
+ case MPOL_WEIGHTED_INTERLEAVE:
+ *nid = (ilx == NO_INTERLEAVE_INDEX) ?
+ weighted_interleave_nodes(pol) :
+ weighted_interleave_nid(pol, ilx);
+ break;
}
return nodemask;
@@ -2029,6 +2131,7 @@ bool init_nodemask_of_mempolicy(nodemask_t *mask)
case MPOL_PREFERRED_MANY:
case MPOL_BIND:
case MPOL_INTERLEAVE:
+ case MPOL_WEIGHTED_INTERLEAVE:
*mask = mempolicy->nodes;
break;
@@ -2128,7 +2231,8 @@ struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
* If the policy is interleave or does not allow the current
* node in its nodemask, we allocate the standard way.
*/
- if (pol->mode != MPOL_INTERLEAVE &&
+ if ((pol->mode != MPOL_INTERLEAVE &&
+ pol->mode != MPOL_WEIGHTED_INTERLEAVE) &&
(!nodemask || node_isset(nid, *nodemask))) {
/*
* First, try to allocate THP only on local node, but
@@ -2264,6 +2368,156 @@ static unsigned long alloc_pages_bulk_array_interleave(gfp_t gfp,
return total_allocated;
}
+static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
+ struct mempolicy *pol, unsigned long nr_pages,
+ struct page **page_array)
+{
+ struct task_struct *me = current;
+ unsigned long total_allocated = 0;
+ unsigned long nr_allocated;
+ unsigned long rounds;
+ unsigned long node_pages, delta;
+ u8 weight, resume_weight;
+ u8 __rcu *table;
+ u8 *weights;
+ unsigned int weight_total = 0;
+ unsigned long rem_pages = nr_pages;
+ nodemask_t nodes;
+ int nnodes, node, resume_node, next_node;
+ int prev_node = me->il_prev;
+ int i;
+
+ if (!nr_pages)
+ return 0;
+
+ nnodes = read_once_policy_nodemask(pol, &nodes);
+ if (!nnodes)
+ return 0;
+
+ /* Continue allocating from most recent node and adjust the nr_pages */
+ if (pol->cur_il_weight) {
+ node = next_node_in(prev_node, nodes);
+ node_pages = pol->cur_il_weight;
+ if (node_pages > rem_pages)
+ node_pages = rem_pages;
+ nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
+ NULL, page_array);
+ page_array += nr_allocated;
+ total_allocated += nr_allocated;
+ /*
+ * if that's all the pages, no need to interleave, otherwise
+ * we need to set up the next interleave node/weight correctly.
+ */
+ if (rem_pages < pol->cur_il_weight) {
+ /* stay on current node, adjust cur_il_weight */
+ pol->cur_il_weight -= rem_pages;
+ return total_allocated;
+ } else if (rem_pages == pol->cur_il_weight) {
+ /* move to next node / weight */
+ me->il_prev = node;
+ next_node = next_node_in(node, nodes);
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ weight = table ? table[next_node] : 1;
+ /* detect system-default usage */
+ pol->cur_il_weight = weight ? weight : 1;
+ rcu_read_unlock();
+ return total_allocated;
+ }
+ /* Otherwise we adjust nr_pages down, and continue from there */
+ rem_pages -= pol->cur_il_weight;
+ pol->cur_il_weight = 0;
+ prev_node = node;
+ }
+
+ /* create a local copy of node weights to operate on outside rcu */
+ weights = kmalloc(nr_node_ids, GFP_KERNEL);
+ if (!weights)
+ return total_allocated;
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* If table is not registered, use system defaults */
+ if (table)
+ memcpy(weights, iw_table, nr_node_ids);
+ else
+ memset(weights, 1, nr_node_ids);
+ rcu_read_unlock();
+
+ /* calculate total, detect system default usage */
+ for_each_node_mask(node, nodes) {
+ /* detect system-default usage */
+ if (!weights[node])
+ weights[node] = 1;
+ weight_total += weights[node];
+ }
+
+ /*
+ * Now we can continue allocating from 0 instead of an offset
+ * We calculate the number of rounds and any partial rounds so
+ * that we minimize the number of calls to __alloc_pages_bulk
+ * This requires us to track which node we should resume from.
+ *
+ * if (rounds > 0) and (delta == 0), resume_node will always be
+ * the current value of prev_node, which may be NUMA_NO_NODE if
+ * this is the first allocation after a policy is replaced. The
+ * resume weight will be the weight of the next node.
+ *
+ * if (delta > 0) and delta is depleted exactly on a node-weight
+ * boundary, resume node will be the node last allocated from when
+ * delta reached 0.
+ *
+ * if (delta > 0) and delta is not depleted on a node-weight boundary,
+ * resume node will be the node prior to the node last allocated from.
+ *
+ * (rounds == 0) and (delta == 0) is not possible (earlier exit)
+ */
+ rounds = rem_pages / weight_total;
+ delta = rem_pages % weight_total;
+ resume_node = prev_node;
+ resume_weight = weights[next_node_in(prev_node, nodes)];
+ /* If no delta, we'll resume from current prev_node and first weight */
+ for (i = 0; i < nnodes; i++) {
+ node = next_node_in(prev_node, nodes);
+ weight = weights[node];
+ node_pages = weight * rounds;
+ /* If a delta exists, add this node's portion of the delta */
+ if (delta > weight) {
+ node_pages += weight;
+ delta -= weight;
+ resume_node = node;
+ } else if (delta) {
+ node_pages += delta;
+ if (delta == weight) {
+ /* resume from next node with its weight */
+ resume_node = node;
+ next_node = next_node_in(node, nodes);
+ resume_weight = weights[next_node];
+ } else {
+ /* resume from this node w/ remaining weight */
+ resume_node = prev_node;
+ resume_weight = weight - (node_pages % weight);
+ }
+ delta = 0;
+ }
+ /* node_pages can be 0 if an allocation fails and rounds == 0 */
+ if (!node_pages)
+ break;
+ nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
+ NULL, page_array);
+ page_array += nr_allocated;
+ total_allocated += nr_allocated;
+ if (total_allocated == nr_pages)
+ break;
+ prev_node = node;
+ }
+ /* resume allocating from the calculated node and weight */
+ me->il_prev = resume_node;
+ pol->cur_il_weight = resume_weight;
+ kfree(weights);
+ return total_allocated;
+}
+
static unsigned long alloc_pages_bulk_array_preferred_many(gfp_t gfp, int nid,
struct mempolicy *pol, unsigned long nr_pages,
struct page **page_array)
@@ -2304,6 +2558,10 @@ unsigned long alloc_pages_bulk_array_mempolicy(gfp_t gfp,
return alloc_pages_bulk_array_interleave(gfp, pol,
nr_pages, page_array);
+ if (pol->mode == MPOL_WEIGHTED_INTERLEAVE)
+ return alloc_pages_bulk_array_weighted_interleave(
+ gfp, pol, nr_pages, page_array);
+
if (pol->mode == MPOL_PREFERRED_MANY)
return alloc_pages_bulk_array_preferred_many(gfp,
numa_node_id(), pol, nr_pages, page_array);
@@ -2379,6 +2637,7 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
case MPOL_INTERLEAVE:
case MPOL_PREFERRED:
case MPOL_PREFERRED_MANY:
+ case MPOL_WEIGHTED_INTERLEAVE:
return !!nodes_equal(a->nodes, b->nodes);
case MPOL_LOCAL:
return true;
@@ -2515,6 +2774,10 @@ int mpol_misplaced(struct folio *folio, struct vm_area_struct *vma,
polnid = interleave_nid(pol, ilx);
break;
+ case MPOL_WEIGHTED_INTERLEAVE:
+ polnid = weighted_interleave_nid(pol, ilx);
+ break;
+
case MPOL_PREFERRED:
if (node_isset(curnid, pol->nodes))
goto out;
@@ -2889,6 +3152,7 @@ static const char * const policy_modes[] =
[MPOL_PREFERRED] = "prefer",
[MPOL_BIND] = "bind",
[MPOL_INTERLEAVE] = "interleave",
+ [MPOL_WEIGHTED_INTERLEAVE] = "weighted interleave",
[MPOL_LOCAL] = "local",
[MPOL_PREFERRED_MANY] = "prefer (many)",
};
@@ -2948,6 +3212,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
}
break;
case MPOL_INTERLEAVE:
+ case MPOL_WEIGHTED_INTERLEAVE:
/*
* Default to online nodes with memory if no nodelist
*/
@@ -3058,6 +3323,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
case MPOL_PREFERRED_MANY:
case MPOL_BIND:
case MPOL_INTERLEAVE:
+ case MPOL_WEIGHTED_INTERLEAVE:
nodes = pol->nodes;
break;
default:
--
2.39.1
^ permalink raw reply related
* [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Gregory Price @ 2024-01-25 18:43 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linux-fsdevel, linux-api, corbet, akpm,
gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji, mhocko,
ying.huang, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <20240125184345.47074-1-gregory.price@memverge.com>
In the prior patch, we carry only the current weight for a weighted
interleave round with us across calls through the allocator path.
node = next_node_in(current->il_prev, pol->nodemask)
pol->cur_il_weight <--- this weight applies to the above node
This separation of data can cause a race condition.
If a cgroup-initiated task migration or mems_allowed change occurs
from outside the context of the task, this can cause the weight to
become stale, meaning we may end using that weight to allocate
memory on the wrong node.
Example:
1) task A sets (cur_il_weight = 8) and (current->il_prev) to
node0. node1 is the next set bit in pol->nodemask
2) rebind event occurs, removing node1 from the nodemask.
node2 is now the next set bit in pol->nodemask
cur_il_weight is now stale.
3) allocation occurs, next_node_in(il_prev, nodes) returns
node2. cur_il_weight is now applied to the wrong node.
The upper level allocator logic must still enforce mems_allowed,
so this isn't dangerous, but it is innaccurate.
Just clearing the weight is insufficient, as it creates two more
race conditions. The root of the issue is the separation of weight
and node data between nodemask and cur_il_weight.
To solve this, update cur_il_weight to be an atomic_t, and place the
node that the weight applies to in the upper bits of the field:
atomic_t cur_il_weight
node bits 32:8
weight bits 7:0
Now retrieving or clearing the active interleave node and weight
is a single atomic operation, and we are not dependent on the
potentially changing state of (pol->nodemask) to determine what
node the weight applies to.
Two special observations:
- if the weight is non-zero, cur_il_weight must *always* have a
valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
This is because we steal the top byte for the weight.
- MAX_NUMNODES is presently limited to 1024 or less on every
architecture. This would permanently limit MAX_NUMNODES to
an absolute maximum of (1 << 24) to avoid overflows.
Per some reading and discussion, it appears that max nodes is
limited to 1024 so that zone type still fits in page flags, so
this method seemed preferable compared to the alternatives of
trying to make all or part of mempolicy RCU protected (which
may not be possible, since it is often referenced during code
chunks which call operations that may sleep).
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
include/linux/mempolicy.h | 2 +-
mm/mempolicy.c | 93 +++++++++++++++++++++++++--------------
2 files changed, 61 insertions(+), 34 deletions(-)
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
index c644d7bbd396..8108fc6e96ca 100644
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -56,7 +56,7 @@ struct mempolicy {
} w;
/* Weighted interleave settings */
- u8 cur_il_weight;
+ atomic_t cur_il_weight;
};
/*
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 5a517511658e..41b5fef0a6f5 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -321,7 +321,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags,
policy->mode = mode;
policy->flags = flags;
policy->home_node = NUMA_NO_NODE;
- policy->cur_il_weight = 0;
+ atomic_set(&policy->cur_il_weight, 0);
return policy;
}
@@ -356,6 +356,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes)
tmp = *nodes;
pol->nodes = tmp;
+ atomic_set(&pol->cur_il_weight, 0);
}
static void mpol_rebind_preferred(struct mempolicy *pol,
@@ -973,8 +974,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
*policy = next_node_in(current->il_prev, pol->nodes);
} else if (pol == current->mempolicy &&
(pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
- if (pol->cur_il_weight)
- *policy = current->il_prev;
+ int cweight = atomic_read(&pol->cur_il_weight);
+
+ if (cweight & 0xFF)
+ *policy = cweight >> 8;
else
*policy = next_node_in(current->il_prev,
pol->nodes);
@@ -1864,36 +1867,48 @@ static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
unsigned int node, next;
struct task_struct *me = current;
u8 __rcu *table;
+ int cur_weight;
u8 weight;
- node = next_node_in(me->il_prev, policy->nodes);
- if (node == MAX_NUMNODES)
- return node;
+ cur_weight = atomic_read(&policy->cur_il_weight);
+ node = cur_weight >> 8;
+ weight = cur_weight & 0xff;
- /* on first alloc after setting mempolicy, acquire first weight */
- if (unlikely(!policy->cur_il_weight)) {
+ /* If nodemask was rebound, just fetch the next node */
+ if (!weight || !node_isset(node, policy->nodes)) {
+ node = next_node_in(me->il_prev, policy->nodes);
+ /* can only happen if nodemask has become invalid */
+ if (node == MAX_NUMNODES)
+ return node;
rcu_read_lock();
table = rcu_dereference(iw_table);
/* detect system-default values */
weight = table ? table[node] : 1;
- policy->cur_il_weight = weight ? weight : 1;
+ weight = weight ? weight : 1;
rcu_read_unlock();
}
/* account for this allocation call */
- policy->cur_il_weight--;
+ weight--;
/* if now at 0, move to next node and set up that node's weight */
- if (unlikely(!policy->cur_il_weight)) {
+ if (unlikely(!weight)) {
me->il_prev = node;
next = next_node_in(node, policy->nodes);
- rcu_read_lock();
- table = rcu_dereference(iw_table);
- /* detect system-default values */
- weight = table ? table[next] : 1;
- policy->cur_il_weight = weight ? weight : 1;
- rcu_read_unlock();
- }
+ if (next != MAX_NUMNODES) {
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* detect system-default values */
+ weight = table ? table[next] : 1;
+ weight = weight ? weight : 1;
+ rcu_read_unlock();
+ cur_weight = (next << 8) | weight;
+ } else /* policy->nodes became invalid */
+ cur_weight = 0;
+ } else
+ cur_weight = (node << 8) | weight;
+
+ atomic_set(&policy->cur_il_weight, cur_weight);
return node;
}
@@ -2385,6 +2400,7 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
nodemask_t nodes;
int nnodes, node, resume_node, next_node;
int prev_node = me->il_prev;
+ int cur_node_and_weight = atomic_read(&pol->cur_il_weight);
int i;
if (!nr_pages)
@@ -2394,10 +2410,11 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
if (!nnodes)
return 0;
+ node = cur_node_and_weight >> 8;
+ weight = cur_node_and_weight & 0xff;
/* Continue allocating from most recent node and adjust the nr_pages */
- if (pol->cur_il_weight) {
- node = next_node_in(prev_node, nodes);
- node_pages = pol->cur_il_weight;
+ if (weight && node_isset(node, nodes)) {
+ node_pages = weight;
if (node_pages > rem_pages)
node_pages = rem_pages;
nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
@@ -2408,27 +2425,36 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
* if that's all the pages, no need to interleave, otherwise
* we need to set up the next interleave node/weight correctly.
*/
- if (rem_pages < pol->cur_il_weight) {
+ if (rem_pages < weight) {
/* stay on current node, adjust cur_il_weight */
- pol->cur_il_weight -= rem_pages;
+ weight -= rem_pages;
+ atomic_set(&pol->cur_il_weight, ((node << 8) | weight));
return total_allocated;
- } else if (rem_pages == pol->cur_il_weight) {
+ } else if (rem_pages == weight) {
/* move to next node / weight */
me->il_prev = node;
next_node = next_node_in(node, nodes);
- rcu_read_lock();
- table = rcu_dereference(iw_table);
- weight = table ? table[next_node] : 1;
- /* detect system-default usage */
- pol->cur_il_weight = weight ? weight : 1;
- rcu_read_unlock();
+ if (next_node == MAX_NUMNODES) {
+ next_node = 0;
+ weight = 0;
+ } else {
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ weight = table ? table[next_node] : 1;
+ /* detect system-default usage */
+ weight = weight ? weight : 1;
+ rcu_read_unlock();
+ }
+ atomic_set(&pol->cur_il_weight,
+ ((next_node << 8) | weight));
return total_allocated;
}
/* Otherwise we adjust nr_pages down, and continue from there */
- rem_pages -= pol->cur_il_weight;
- pol->cur_il_weight = 0;
+ rem_pages -= weight;
prev_node = node;
}
+ /* clear cur_il_weight in case of an allocation failure */
+ atomic_set(&pol->cur_il_weight, 0);
/* create a local copy of node weights to operate on outside rcu */
weights = kmalloc(nr_node_ids, GFP_KERNEL);
@@ -2513,7 +2539,8 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
}
/* resume allocating from the calculated node and weight */
me->il_prev = resume_node;
- pol->cur_il_weight = resume_weight;
+ resume_node = next_node_in(resume_node, nodes);
+ atomic_set(&pol->cur_il_weight, ((resume_node << 8) | resume_weight));
kfree(weights);
return total_allocated;
}
--
2.39.1
^ permalink raw reply related
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Andy Lutomirski @ 2024-01-25 18:55 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Andy Lutomirski, wine-devel, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, linux-api, André Almeida, Wolfram Sang,
Peter Zijlstra, Alexandre Julliard
In-Reply-To: <10405963.nUPlyArG6x@terabithia>
On Wed, Jan 24, 2024 at 7:42 PM Elizabeth Figura
<zfigura@codeweavers.com> wrote:
>
> On Wednesday, 24 January 2024 16:56:23 CST Elizabeth Figura wrote:
> > On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote:
> >
> > > On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura
> > > <zfigura@codeweavers.com> wrote:
> > >
> > > >
> > > >
> > > > ntsync uses a misc device as the simplest and least intrusive uAPI
> > > > interface.
> > >
> > > >
> > > >
> > > > Each file description on the device represents an isolated NT instance,
> > > > intended to correspond to a single NT virtual machine.
> > >
> > >
> > > If I understand this text right, and if I understood the code right,
> > > you're saying that each open instance of the device represents an
> > > entire universe of NT synchronization objects, and no security or
> > > isolation is possible between those objects. For single-process use,
> > > this seems fine. But fork() will be a bit odd (although NT doesn't
> > > really believe in fork, so maybe this is fine).
> > >
> > > Except that NT has *named* semaphores and such. And I'm pretty sure
> > > I've written GUI programs that use named synchronization objects (IIRC
> > > they were events, and this was a *very* common pattern, regularly
> > > discussed in MSDN, usenet, etc) to detect whether another instance of
> > > the program is running. And this all works on real Windows because
> > > sessions have sufficiently separated namespaces, and the security all
> > > works out about as any other security on Windows, etc. But
> > > implementing *that* on top of this
> > > file-description-plus-integer-equals-object will be fundamentally
> > > quite subject to one buggy program completely clobbering someone
> > > else's state.
> > >
> > > Would it make sense and scale appropriately for an NT synchronization
> > > *object* to be a Linux open file description? Then SCM_RIGHTS could
> > > pass them around, an RPC server could manage *named* objects, and
> > > they'd generally work just like other "Object Manager" objects like,
> > > say, files.
> >
> >
> > It's a sensible concern. I think when I discussed this with Alexandre
> > Julliard (the Wine maintainer, CC'd) the conclusion was this wasn't
> > something we were concerned about.
> >
> > While the current model *does* allow for processes to arbitrarily mess
> > with each other, accidentally or not, I think we're not concerned with
> > the scope of that than we are about implementing a whole scheduler in
> > user space.
> >
> > For one, you can't corrupt the wineserver state this way—wineserver
> > being sort of like a dedicated process that handles many of the things
> > that a kernel would, and so sometimes needs to set or reset events, or
> > perform NTSYNC_IOC_KILL_MUTEX, but never relies on ntsync object state.
> > Whereas trying to implement a scheduler in user space would involve the
> > wineserver taking locks, and hence other processes could deadlock.
> >
> > For two, it's probably a lot harder to mess with that internal state
> > accidentally.
> >
> > [There is also a potential problem where some broken applications
> > create a million (literally) sync objects. Making these into files runs
> > into NOFILE. We did specifically push distributions and systemd to
> > increase those limits because an older solution *did* use eventfds and
> > *did* run into those limits. Since that push was successful I don't
> > know if this is *actually* a concern anymore, but avoiding files is
> > probably not a bad thing either.]
>
> Of course, looking at it from a kernel maintainer's perspective, it wouldn't
> be insane to do this anyway. If we at some point do start to care about cross-
> process isolation in this way, or if another NT emulator wants to use this
> interface and does care about cross-process isolation, it'll be necessary. At
> least it'd make sense to make them separate files even if we don't implement
> granular permission handling just yet.
I'm not convinced that any complexity at all beyond using individual
files is needed for granular permission handling. Unless something
actually needs permission bits on different files pointing at the same
sync object (which I believe NT supports, but it's sort of an odd
concept and I'm not immediately convinced that anything uses it),
merely having individual files ought to do the trick. Handling of who
has permission to open a given named object can live in a daemon, and
I'd guess that Wine even already implements this.
And keeping everything together gives me flashbacks of Windows 95 and
Mac OS pre-X. Sure, in principle the software wasn't malicious, but
there was no shortage whatsoever of buggy crap out there, and systems
were quite unstable. Even just:
CreateSemaphore();
fork();
sleep a few seconds;
exit();
seems like it could corrupt the shared namespace world. (Obviously no
one would ever do that, right?)
Also, handle leaks:
while(true) {
make a subprocess, which creates a semaphore and crashes;
}
>
> The main question is, is NOFILE a realistic concern, and what other problems
> might there be, in terms of making these heavier objects? Besides memory usage
> I can't think of any, but of course I don't have much knowledge of this area.
Years ago there was some discussion of making struct file
lighter-weight for light-weight things that aren't files. And, in any
case, even the little integer indices in your code aren't free -- they
just aren't accounted as files.
And struct file isn't *that* bad. I bet it's not dramatically bigger,
or even smaller, than whatever the Windows kernel stores for a
semaphore handle.
--Andy
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Elizabeth Figura @ 2024-01-25 21:45 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, wine-devel, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, linux-api, André Almeida, Wolfram Sang,
Peter Zijlstra, Alexandre Julliard
In-Reply-To: <CALCETrVZFhH-dKCFpxj=nML2cn1EBc5wWHj9zhKK07TLSSqnDA@mail.gmail.com>
On Thursday, 25 January 2024 12:55:04 CST Andy Lutomirski wrote:
> On Wed, Jan 24, 2024 at 7:42 PM Elizabeth Figura
> <zfigura@codeweavers.com> wrote:
> >
> > On Wednesday, 24 January 2024 16:56:23 CST Elizabeth Figura wrote:
> > > On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote:
> > >
> > > > On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura
> > > > <zfigura@codeweavers.com> wrote:
> > > >
> > > > >
> > > > >
> > > > > ntsync uses a misc device as the simplest and least intrusive uAPI
> > > > > interface.
> > > >
> > > > >
> > > > >
> > > > > Each file description on the device represents an isolated NT instance,
> > > > > intended to correspond to a single NT virtual machine.
> > > >
> > > >
> > > > If I understand this text right, and if I understood the code right,
> > > > you're saying that each open instance of the device represents an
> > > > entire universe of NT synchronization objects, and no security or
> > > > isolation is possible between those objects. For single-process use,
> > > > this seems fine. But fork() will be a bit odd (although NT doesn't
> > > > really believe in fork, so maybe this is fine).
> > > >
> > > > Except that NT has *named* semaphores and such. And I'm pretty sure
> > > > I've written GUI programs that use named synchronization objects (IIRC
> > > > they were events, and this was a *very* common pattern, regularly
> > > > discussed in MSDN, usenet, etc) to detect whether another instance of
> > > > the program is running. And this all works on real Windows because
> > > > sessions have sufficiently separated namespaces, and the security all
> > > > works out about as any other security on Windows, etc. But
> > > > implementing *that* on top of this
> > > > file-description-plus-integer-equals-object will be fundamentally
> > > > quite subject to one buggy program completely clobbering someone
> > > > else's state.
> > > >
> > > > Would it make sense and scale appropriately for an NT synchronization
> > > > *object* to be a Linux open file description? Then SCM_RIGHTS could
> > > > pass them around, an RPC server could manage *named* objects, and
> > > > they'd generally work just like other "Object Manager" objects like,
> > > > say, files.
> > >
> > >
> > > It's a sensible concern. I think when I discussed this with Alexandre
> > > Julliard (the Wine maintainer, CC'd) the conclusion was this wasn't
> > > something we were concerned about.
> > >
> > > While the current model *does* allow for processes to arbitrarily mess
> > > with each other, accidentally or not, I think we're not concerned with
> > > the scope of that than we are about implementing a whole scheduler in
> > > user space.
> > >
> > > For one, you can't corrupt the wineserver state this way—wineserver
> > > being sort of like a dedicated process that handles many of the things
> > > that a kernel would, and so sometimes needs to set or reset events, or
> > > perform NTSYNC_IOC_KILL_MUTEX, but never relies on ntsync object state.
> > > Whereas trying to implement a scheduler in user space would involve the
> > > wineserver taking locks, and hence other processes could deadlock.
> > >
> > > For two, it's probably a lot harder to mess with that internal state
> > > accidentally.
> > >
> > > [There is also a potential problem where some broken applications
> > > create a million (literally) sync objects. Making these into files runs
> > > into NOFILE. We did specifically push distributions and systemd to
> > > increase those limits because an older solution *did* use eventfds and
> > > *did* run into those limits. Since that push was successful I don't
> > > know if this is *actually* a concern anymore, but avoiding files is
> > > probably not a bad thing either.]
> >
> > Of course, looking at it from a kernel maintainer's perspective, it wouldn't
> > be insane to do this anyway. If we at some point do start to care about cross-
> > process isolation in this way, or if another NT emulator wants to use this
> > interface and does care about cross-process isolation, it'll be necessary. At
> > least it'd make sense to make them separate files even if we don't implement
> > granular permission handling just yet.
>
> I'm not convinced that any complexity at all beyond using individual
> files is needed for granular permission handling. Unless something
> actually needs permission bits on different files pointing at the same
> sync object (which I believe NT supports, but it's sort of an odd
> concept and I'm not immediately convinced that anything uses it),
> merely having individual files ought to do the trick. Handling of who
> has permission to open a given named object can live in a daemon, and
> I'd guess that Wine even already implements this.
This is mostly correct. NT has file descriptors and descriptions (the
former is called a "handle"), though unlike Unix access bits are
specific to the *descriptor* (handle). I don't know if anything uses
it, but we do currently implement that basic functionality, so I can't
say that nothing does either.
So inasmuch as access to someone else's object is a concern, access to
your object with bits you don't have permission for could be a concern
along the same lines. However, from conversation with Alexandre I
believe it'd be fine to just implement those checks in user space.
> And keeping everything together gives me flashbacks of Windows 95 and
> Mac OS pre-X. Sure, in principle the software wasn't malicious, but
> there was no shortage whatsoever of buggy crap out there, and systems
> were quite unstable. Even just:
>
> CreateSemaphore();
> fork();
> sleep a few seconds;
> exit();
>
> seems like it could corrupt the shared namespace world. (Obviously no
> one would ever do that, right?)
>
> Also, handle leaks:
>
> while(true) {
> make a subprocess, which creates a semaphore and crashes;
> }
For whatever it's worth, this particular thing wouldn't be a concern;
Wine's "kernel" daemon already has to detect when a process dies and
close all its outstanding handles.
^ permalink raw reply
* [PATCH net-next v3 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-01-25 22:56 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, Joe Damato, Alexander Viro, Andrew Waterman,
Arnd Bergmann, Dominik Brodowski, Greg Kroah-Hartman, Jan Kara,
Jiri Slaby, Jonathan Corbet, Julien Panis,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure), Michael Ellerman,
Nathan Lynch, Palmer Dabbelt, Steve French, Thomas Huth,
Thomas Zimmermann
Greetings:
Welcome to v3. Cover letter updated from v2 to explain why ioctl and
adjusted my cc_cmd to try to get the correct people in addition to folks
who were added in v1 & v2. Labeled as net-next because it seems networking
related to me even though it is fs code.
TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
epoll with socket fds.") by allowing user applications to enable
epoll-based busy polling and set a busy poll packet budget on a per epoll
context basis.
This makes epoll-based busy polling much more usable for user
applications than the current system-wide sysctl and hardcoded budget.
To allow for this, two ioctls have been added for epoll contexts for
getting and setting a new struct, struct epoll_params.
ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
seemed that:
- Busy poll affects all existing epoll_wait and epoll_pwait variants in
the same way, so new verions of many syscalls might be needed. It
seems much simpler for users to use the correct
epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
or disable busy poll as needed. This also probably means less work to
get an existing epoll app using busy poll.
- previously added epoll_pwait2 helped to bring epoll closer to
existing syscalls (like pselect and ppoll) and this busy poll change
reflected as a new syscall would not have the same effect.
Note: patch 1/4 uses an xor so that busy poll is only enabled if the
per-context busy poll usecs is set or the system-wide sysctl. If both are
enabled, busy polling does not happen. Calling this out specifically incase
there are strong feelings about this one; I felt one xor the other made
sense, but I am open to changing it.
Longer explanation:
Presently epoll has support for a very useful form of busy poll based on
the incoming NAPI ID (see also: SO_INCOMING_NAPI_ID [2]).
This form of busy poll allows epoll_wait to drive NAPI packet processing
which allows for a few interesting user application designs which can
reduce latency and also potentially improve L2/L3 cache hit rates by
deferring NAPI until userland has finished its work.
The documentation available on this is, IMHO, a bit confusing so please
allow me to explain how one might use this:
1. Ensure each application thread has its own epoll instance mapping
1-to-1 with NIC RX queues. An n-tuple filter would likely be used to
direct connections with specific dest ports to these queues.
2. Optionally: Setup IRQ coalescing for the NIC RX queues where busy
polling will occur. This can help avoid the userland app from being
pre-empted by a hard IRQ while userland is running. Note this means that
userland must take care to call epoll_wait and not take too long in
userland since it now drives NAPI via epoll_wait.
3. Optionally: Consider using napi_defer_hard_irqs and gro_flush_timeout to
further restrict IRQ generation from the NIC. These settings are
system-wide so their impact must be carefully weighed against the running
applications.
4. Ensure that all incoming connections added to an epoll instance
have the same NAPI ID. This can be done with a BPF filter when
SO_REUSEPORT is used or getsockopt + SO_INCOMING_NAPI_ID when a single
accept thread is used which dispatches incoming connections to threads.
5. Lastly, busy poll must be enabled via a sysctl
(/proc/sys/net/core/busy_poll).
Please see Eric Dumazet's paper about busy polling [3] and a recent
academic paper about measured performance improvements of busy polling [4]
(albeit with a modification that is not currently present in the kernel)
for additional context.
The unfortunate part about step 5 above is that this enables busy poll
system-wide which affects all user applications on the system,
including epoll-based network applications which were not intended to
be used this way or applications where increased CPU usage for lower
latency network processing is unnecessary or not desirable.
If the user wants to run one low latency epoll-based server application
with epoll-based busy poll, but would like to run the rest of the
applications on the system (which may also use epoll) without busy poll,
this system-wide sysctl presents a significant problem.
This change preserves the system-wide sysctl, but adds a mechanism (via
ioctl) to enable or disable busy poll for epoll contexts as needed by
individual applications, making epoll-based busy poll more usable. Note
that this change includes an xor allowing only the per-context busy poll or
the system wide sysctl, not both. If both are enabled, busy polling does
not happen. Calling this out specifically incase there are strong feelings
about this one; I felt one xor the other made sense, but I am open to
changing it.
Thanks,
Joe
v2 -> v3:
- cover letter updated to mention why ioctl seems (to me) like a better
choice vs a new syscall.
- patch 3/4 was modified in 3 ways:
- when an unknown ioctl is received, -ENOIOCTLCMD is returned instead
of -EINVAL as the ioctl documentation requires.
- epoll_params.busy_poll_budget can only be set to a value larger than
NAPI_POLL_WEIGHT if code is run by privileged (CAP_NET_ADMIN) users.
Otherwise, -EPERM is returned.
- busy poll specific ioctl code moved out to its own function. On
kernels without busy poll support, -EOPNOTSUPP is returned. This also
makes the kernel build robot happier without littering the code with
more #ifdefs.
- dropped patch 4/4 after Eric Dumazet's review of it when it was sent
independently to the list [5].
v1 -> v2:
- cover letter updated to make a mention of napi_defer_hard_irqs and
gro_flush_timeout as an added step 3 and to cite both Eric Dumazet's
busy polling paper and a paper from University of Waterloo for
additional context. Specifically calling out the xor in patch 1/4
incase it is missed by reviewers.
- Patch 2/4 has its commit message updated, but no functional changes.
Commit message now describes that allowing for a settable budget helps
to improve throughput and is more consistent with other busy poll
mechanisms that allow a settable budget via SO_BUSY_POLL_BUDGET.
- Patch 3/4 was modified to check if the epoll_params.busy_poll_budget
exceeds NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
printed. This was done for consistency with netif_napi_add_weight,
which does the same.
- Patch 3/4 the struct epoll_params was updated to fix the type of the
data field; it was uint8_t and was changed to u8.
- Patch 4/4 added to check if SO_BUSY_POLL_BUDGET exceeds
NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
printed. This was done for consistency with netif_napi_add_weight,
which does the same.
[1]: https://lore.kernel.org/lkml/65b1cb7f73a6a_250560294bd@willemb.c.googlers.com.notmuch/
[2]: https://lore.kernel.org/lkml/20170324170836.15226.87178.stgit@localhost.localdomain/
[3]: https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf
[4]: https://dl.acm.org/doi/pdf/10.1145/3626780
[5]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/
Joe Damato (3):
eventpoll: support busy poll per epoll instance
eventpoll: Add per-epoll busy poll packet budget
eventpoll: Add epoll ioctl for epoll_params
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 122 +++++++++++++++++-
include/uapi/linux/eventpoll.h | 12 ++
3 files changed, 130 insertions(+), 5 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH net-next v3 1/3] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-01-25 22:56 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, Joe Damato, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240125225704.12781-1-jdamato@fastly.com>
Allow busy polling on a per-epoll context basis. The per-epoll context
usec timeout value is preferred, but the pre-existing system wide sysctl
value is still supported if it specified.
Note that this change uses an xor: either per epoll instance busy polling
is enabled on the epoll instance or system wide epoll is enabled. Enabling
both is disallowed.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
fs/eventpoll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 3534d36a1474..4503fec01278 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -227,6 +227,8 @@ struct eventpoll {
#ifdef CONFIG_NET_RX_BUSY_POLL
/* used to track busy poll napi_id */
unsigned int napi_id;
+ /* busy poll timeout */
+ u64 busy_poll_usecs;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
}
+/**
+ * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
+ * from the epoll instance ep is preferred, but if it is not set fallback to
+ * the system-wide global via busy_loop_timeout.
+ *
+ * @start_time: The start time used to compute the remaining time until timeout.
+ * @ep: Pointer to the eventpoll context.
+ *
+ * Return: true if the timeout has expired, false otherwise.
+ */
+static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
+{
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
+
+ if (bp_usec) {
+ unsigned long end_time = start_time + bp_usec;
+ unsigned long now = busy_loop_current_time();
+
+ return time_after(now, end_time);
+ } else {
+ return busy_loop_timeout(start_time);
+ }
+#endif
+ return true;
+}
+
#ifdef CONFIG_NET_RX_BUSY_POLL
+static bool ep_busy_loop_on(struct eventpoll *ep)
+{
+ return !!ep->busy_poll_usecs ^ net_busy_loop_on();
+}
+
static bool ep_busy_loop_end(void *p, unsigned long start_time)
{
struct eventpoll *ep = p;
- return ep_events_available(ep) || busy_loop_timeout(start_time);
+ return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
}
/*
@@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
- if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
+ if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
BUSY_POLL_BUDGET);
if (ep_events_available(ep))
@@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
struct socket *sock;
struct sock *sk;
- if (!net_busy_loop_on())
+ ep = epi->ep;
+ if (!ep_busy_loop_on(ep))
return;
sock = sock_from_file(epi->ffd.file);
@@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
return;
napi_id = READ_ONCE(sk->sk_napi_id);
- ep = epi->ep;
/* Non-NAPI IDs can be rejected
* or
@@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
{
}
+static inline bool ep_busy_loop_on(struct eventpoll *ep)
+{
+ return false;
+}
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
error = PTR_ERR(file);
goto out_free_fd;
}
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ ep->busy_poll_usecs = 0;
+#endif
ep->file = file;
fd_install(fd, file);
return fd;
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v3 2/3] eventpoll: Add per-epoll busy poll packet budget
From: Joe Damato @ 2024-01-25 22:56 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, Joe Damato, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240125225704.12781-1-jdamato@fastly.com>
When using epoll-based busy poll, the packet budget is hardcoded to
BUSY_POLL_BUDGET (8). Users may desire larger busy poll budgets, which
can potentially increase throughput when busy polling under high network
load.
Other busy poll methods allow setting the busy poll budget via
SO_BUSY_POLL_BUDGET, but epoll-based busy polling uses a hardcoded
value.
Fix this edge case by adding support for a per-epoll context busy poll
packet budget. If not specified, the default value (BUSY_POLL_BUDGET) is
used.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
fs/eventpoll.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 4503fec01278..40bd97477b91 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -229,6 +229,8 @@ struct eventpoll {
unsigned int napi_id;
/* busy poll timeout */
u64 busy_poll_usecs;
+ /* busy poll packet budget */
+ u16 busy_poll_budget;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -437,10 +439,14 @@ static bool ep_busy_loop_end(void *p, unsigned long start_time)
static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
+ u16 budget = READ_ONCE(ep->busy_poll_budget);
+
+ if (!budget)
+ budget = BUSY_POLL_BUDGET;
if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
- BUSY_POLL_BUDGET);
+ budget);
if (ep_events_available(ep))
return true;
/*
@@ -2098,6 +2104,7 @@ static int do_epoll_create(int flags)
}
#ifdef CONFIG_NET_RX_BUSY_POLL
ep->busy_poll_usecs = 0;
+ ep->busy_poll_budget = 0;
#endif
ep->file = file;
fd_install(fd, file);
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-25 22:56 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, Joe Damato, Jonathan Corbet, Alexander Viro, Jan Kara,
Michael Ellerman, Greg Kroah-Hartman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240125225704.12781-1-jdamato@fastly.com>
Add an ioctl for getting and setting epoll_params. User programs can use
this ioctl to get and set the busy poll usec time or packet budget
params for a specific epoll context.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 64 +++++++++++++++++++
include/uapi/linux/eventpoll.h | 12 ++++
3 files changed, 77 insertions(+)
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 457e16f06e04..b33918232f78 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -309,6 +309,7 @@ Code Seq# Include File Comments
0x89 0B-DF linux/sockios.h
0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
0x89 F0-FF linux/sockios.h SIOCDEVPRIVATE range
+0x8A 00-1F linux/eventpoll.h
0x8B all linux/wireless.h
0x8C 00-3F WiNRADiO driver
<http://www.winradio.com.au/>
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 40bd97477b91..73ae886efb8a 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -6,6 +6,8 @@
* Davide Libenzi <davidel@xmailserver.org>
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/sched/signal.h>
@@ -37,6 +39,7 @@
#include <linux/seq_file.h>
#include <linux/compat.h>
#include <linux/rculist.h>
+#include <linux/capability.h>
#include <net/busy_poll.h>
/*
@@ -495,6 +498,39 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
ep->napi_id = napi_id;
}
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct eventpoll *ep;
+ struct epoll_params epoll_params;
+ void __user *uarg = (void __user *) arg;
+
+ ep = file->private_data;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+ return -EFAULT;
+
+ if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
+ !capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
+ ep->busy_poll_budget = epoll_params.busy_poll_budget;
+ return 0;
+ case EPIOCGPARAMS:
+ memset(&epoll_params, 0, sizeof(epoll_params));
+ epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
+ epoll_params.busy_poll_budget = ep->busy_poll_budget;
+ if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
+ return -EFAULT;
+ return 0;
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
+
#else
static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
@@ -510,6 +546,12 @@ static inline bool ep_busy_loop_on(struct eventpoll *ep)
{
return false;
}
+
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -869,6 +911,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
ep_free(ep);
}
+static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret;
+
+ if (!is_file_epoll(file))
+ return -EINVAL;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ case EPIOCGPARAMS:
+ ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
static int ep_eventpoll_release(struct inode *inode, struct file *file)
{
struct eventpoll *ep = file->private_data;
@@ -975,6 +1037,8 @@ static const struct file_operations eventpoll_fops = {
.release = ep_eventpoll_release,
.poll = ep_eventpoll_poll,
.llseek = noop_llseek,
+ .unlocked_ioctl = ep_eventpoll_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
};
/*
diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
index cfbcc4cc49ac..8eb0fdbce995 100644
--- a/include/uapi/linux/eventpoll.h
+++ b/include/uapi/linux/eventpoll.h
@@ -85,4 +85,16 @@ struct epoll_event {
__u64 data;
} EPOLL_PACKED;
+struct epoll_params {
+ u64 busy_poll_usecs;
+ u16 busy_poll_budget;
+
+ /* for future fields */
+ u8 data[118];
+} EPOLL_PACKED;
+
+#define EPOLL_IOC_TYPE 0x8A
+#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
+#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
+
#endif /* _UAPI_LINUX_EVENTPOLL_H */
--
2.25.1
^ permalink raw reply related
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Greg Kroah-Hartman @ 2024-01-25 23:20 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240125225704.12781-4-jdamato@fastly.com>
On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -6,6 +6,8 @@
> * Davide Libenzi <davidel@xmailserver.org>
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
Why this addition? You do not add any pr_*() calls in this patch at all
that I can see.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Greg Kroah-Hartman @ 2024-01-25 23:21 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240125225704.12781-4-jdamato@fastly.com>
On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> +struct epoll_params {
> + u64 busy_poll_usecs;
> + u16 busy_poll_budget;
> +
> + /* for future fields */
> + u8 data[118];
> +} EPOLL_PACKED;
variables that cross the user/kernel boundry need to be __u64, __u16,
and __u8 here.
And why 118?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Greg Kroah-Hartman @ 2024-01-25 23:22 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240125225704.12781-4-jdamato@fastly.com>
On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> Add an ioctl for getting and setting epoll_params. User programs can use
> this ioctl to get and set the busy poll usec time or packet budget
> params for a specific epoll context.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> fs/eventpoll.c | 64 +++++++++++++++++++
> include/uapi/linux/eventpoll.h | 12 ++++
> 3 files changed, 77 insertions(+)
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 457e16f06e04..b33918232f78 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -309,6 +309,7 @@ Code Seq# Include File Comments
> 0x89 0B-DF linux/sockios.h
> 0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
> 0x89 F0-FF linux/sockios.h SIOCDEVPRIVATE range
> +0x8A 00-1F linux/eventpoll.h
> 0x8B all linux/wireless.h
> 0x8C 00-3F WiNRADiO driver
> <http://www.winradio.com.au/>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 40bd97477b91..73ae886efb8a 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -6,6 +6,8 @@
> * Davide Libenzi <davidel@xmailserver.org>
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/sched/signal.h>
> @@ -37,6 +39,7 @@
> #include <linux/seq_file.h>
> #include <linux/compat.h>
> #include <linux/rculist.h>
> +#include <linux/capability.h>
> #include <net/busy_poll.h>
>
> /*
> @@ -495,6 +498,39 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> ep->napi_id = napi_id;
> }
>
> +static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct eventpoll *ep;
> + struct epoll_params epoll_params;
> + void __user *uarg = (void __user *) arg;
> +
> + ep = file->private_data;
> +
> + switch (cmd) {
> + case EPIOCSPARAMS:
> + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> + return -EFAULT;
> +
> + if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> + !capable(CAP_NET_ADMIN))
> + return -EPERM;
> +
> + ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> + ep->busy_poll_budget = epoll_params.busy_poll_budget;
> + return 0;
> + case EPIOCGPARAMS:
> + memset(&epoll_params, 0, sizeof(epoll_params));
> + epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
> + epoll_params.busy_poll_budget = ep->busy_poll_budget;
> + if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> + return -EFAULT;
> + return 0;
> + default:
> + return -ENOIOCTLCMD;
> + }
> +}
> +
> #else
>
> static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
> @@ -510,6 +546,12 @@ static inline bool ep_busy_loop_on(struct eventpoll *ep)
> {
> return false;
> }
> +
> +static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + return -EOPNOTSUPP;
> +}
> #endif /* CONFIG_NET_RX_BUSY_POLL */
>
> /*
> @@ -869,6 +911,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
> ep_free(ep);
> }
>
> +static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + int ret;
> +
> + if (!is_file_epoll(file))
> + return -EINVAL;
> +
> + switch (cmd) {
> + case EPIOCSPARAMS:
> + case EPIOCGPARAMS:
> + ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int ep_eventpoll_release(struct inode *inode, struct file *file)
> {
> struct eventpoll *ep = file->private_data;
> @@ -975,6 +1037,8 @@ static const struct file_operations eventpoll_fops = {
> .release = ep_eventpoll_release,
> .poll = ep_eventpoll_poll,
> .llseek = noop_llseek,
> + .unlocked_ioctl = ep_eventpoll_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> };
>
> /*
> diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
> index cfbcc4cc49ac..8eb0fdbce995 100644
> --- a/include/uapi/linux/eventpoll.h
> +++ b/include/uapi/linux/eventpoll.h
> @@ -85,4 +85,16 @@ struct epoll_event {
> __u64 data;
> } EPOLL_PACKED;
>
> +struct epoll_params {
> + u64 busy_poll_usecs;
> + u16 busy_poll_budget;
> +
> + /* for future fields */
> + u8 data[118];
You forgot to validate that "data" is set to 0, which means that this
would be useless. Why have this at all?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-26 0:04 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <2024012559-appraiser-coerce-b32f@gregkh>
On Thu, Jan 25, 2024 at 03:20:37PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -6,6 +6,8 @@
> > * Davide Libenzi <davidel@xmailserver.org>
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
>
> Why this addition? You do not add any pr_*() calls in this patch at all
> that I can see.
Thanks, I've removed this for the next version. It was a remnant from a
previous version.
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-26 0:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <2024012558-coasting-unlatch-9315@gregkh>
On Thu, Jan 25, 2024 at 03:22:34PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > Add an ioctl for getting and setting epoll_params. User programs can use
> > this ioctl to get and set the busy poll usec time or packet budget
> > params for a specific epoll context.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> > .../userspace-api/ioctl/ioctl-number.rst | 1 +
> > fs/eventpoll.c | 64 +++++++++++++++++++
> > include/uapi/linux/eventpoll.h | 12 ++++
> > 3 files changed, 77 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 457e16f06e04..b33918232f78 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -309,6 +309,7 @@ Code Seq# Include File Comments
> > 0x89 0B-DF linux/sockios.h
> > 0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
> > 0x89 F0-FF linux/sockios.h SIOCDEVPRIVATE range
> > +0x8A 00-1F linux/eventpoll.h
> > 0x8B all linux/wireless.h
> > 0x8C 00-3F WiNRADiO driver
> > <http://www.winradio.com.au/>
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index 40bd97477b91..73ae886efb8a 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -6,6 +6,8 @@
> > * Davide Libenzi <davidel@xmailserver.org>
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > #include <linux/sched/signal.h>
> > @@ -37,6 +39,7 @@
> > #include <linux/seq_file.h>
> > #include <linux/compat.h>
> > #include <linux/rculist.h>
> > +#include <linux/capability.h>
> > #include <net/busy_poll.h>
> >
> > /*
> > @@ -495,6 +498,39 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> > ep->napi_id = napi_id;
> > }
> >
> > +static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct eventpoll *ep;
> > + struct epoll_params epoll_params;
> > + void __user *uarg = (void __user *) arg;
> > +
> > + ep = file->private_data;
> > +
> > + switch (cmd) {
> > + case EPIOCSPARAMS:
> > + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> > + return -EFAULT;
> > +
> > + if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> > + !capable(CAP_NET_ADMIN))
> > + return -EPERM;
> > +
> > + ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> > + ep->busy_poll_budget = epoll_params.busy_poll_budget;
> > + return 0;
> > + case EPIOCGPARAMS:
> > + memset(&epoll_params, 0, sizeof(epoll_params));
> > + epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
> > + epoll_params.busy_poll_budget = ep->busy_poll_budget;
> > + if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> > + return -EFAULT;
> > + return 0;
> > + default:
> > + return -ENOIOCTLCMD;
> > + }
> > +}
> > +
> > #else
> >
> > static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
> > @@ -510,6 +546,12 @@ static inline bool ep_busy_loop_on(struct eventpoll *ep)
> > {
> > return false;
> > }
> > +
> > +static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > #endif /* CONFIG_NET_RX_BUSY_POLL */
> >
> > /*
> > @@ -869,6 +911,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
> > ep_free(ep);
> > }
> >
> > +static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > + int ret;
> > +
> > + if (!is_file_epoll(file))
> > + return -EINVAL;
> > +
> > + switch (cmd) {
> > + case EPIOCSPARAMS:
> > + case EPIOCGPARAMS:
> > + ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int ep_eventpoll_release(struct inode *inode, struct file *file)
> > {
> > struct eventpoll *ep = file->private_data;
> > @@ -975,6 +1037,8 @@ static const struct file_operations eventpoll_fops = {
> > .release = ep_eventpoll_release,
> > .poll = ep_eventpoll_poll,
> > .llseek = noop_llseek,
> > + .unlocked_ioctl = ep_eventpoll_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > };
> >
> > /*
> > diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
> > index cfbcc4cc49ac..8eb0fdbce995 100644
> > --- a/include/uapi/linux/eventpoll.h
> > +++ b/include/uapi/linux/eventpoll.h
> > @@ -85,4 +85,16 @@ struct epoll_event {
> > __u64 data;
> > } EPOLL_PACKED;
> >
> > +struct epoll_params {
> > + u64 busy_poll_usecs;
> > + u16 busy_poll_budget;
> > +
> > + /* for future fields */
> > + u8 data[118];
>
> You forgot to validate that "data" is set to 0, which means that this
> would be useless. Why have this at all?
I included this because I (probably incorrectly) thought that there should
be some extra space in the struct for future additions if needed.
I am not sure if that is a recommended practice for this sort of thing or
not, but if it is I can add some validation.
Thanks for your time and effort in reviewing my code.
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-26 0:11 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <2024012551-anyone-demeaning-867b@gregkh>
On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > +struct epoll_params {
> > + u64 busy_poll_usecs;
> > + u16 busy_poll_budget;
> > +
> > + /* for future fields */
> > + u8 data[118];
> > +} EPOLL_PACKED;
>
> variables that cross the user/kernel boundry need to be __u64, __u16,
> and __u8 here.
I'll make that change for the next version, thank you.
> And why 118?
I chose this arbitrarily. I figured that a 128 byte struct would support 16
u64s in the event that other fields needed to be added in the future. 118
is what was left after the existing fields. There's almost certainly a
better way to do this - or perhaps it is unnecessary as per your other
message.
I am not sure if leaving extra space in the struct is a recommended
practice for ioctls or not - I thought I noticed some code that did and
some that didn't in the kernel so I err'd on the side of leaving the space
and probably did it in the worst way possible.
Thanks,
Joe
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Greg Kroah-Hartman @ 2024-01-26 0:23 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240126001128.GC1987@fastly.com>
On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:
> On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > > +struct epoll_params {
> > > + u64 busy_poll_usecs;
> > > + u16 busy_poll_budget;
> > > +
> > > + /* for future fields */
> > > + u8 data[118];
> > > +} EPOLL_PACKED;
> >
> > variables that cross the user/kernel boundry need to be __u64, __u16,
> > and __u8 here.
>
> I'll make that change for the next version, thank you.
>
> > And why 118?
>
> I chose this arbitrarily. I figured that a 128 byte struct would support 16
> u64s in the event that other fields needed to be added in the future. 118
> is what was left after the existing fields. There's almost certainly a
> better way to do this - or perhaps it is unnecessary as per your other
> message.
>
> I am not sure if leaving extra space in the struct is a recommended
> practice for ioctls or not - I thought I noticed some code that did and
> some that didn't in the kernel so I err'd on the side of leaving the space
> and probably did it in the worst way possible.
It's not really a good idea unless you know exactly what you are going
to do with it. Why not just have a new ioctl if you need new
information in the future? That's simpler, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-26 2:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Arnd Bergmann,
Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <2024012525-outdoors-district-2660@gregkh>
On Thu, Jan 25, 2024 at 04:23:58PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:
> > On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > > > +struct epoll_params {
> > > > + u64 busy_poll_usecs;
> > > > + u16 busy_poll_budget;
> > > > +
> > > > + /* for future fields */
> > > > + u8 data[118];
> > > > +} EPOLL_PACKED;
> > >
> > > variables that cross the user/kernel boundry need to be __u64, __u16,
> > > and __u8 here.
> >
> > I'll make that change for the next version, thank you.
> >
> > > And why 118?
> >
> > I chose this arbitrarily. I figured that a 128 byte struct would support 16
> > u64s in the event that other fields needed to be added in the future. 118
> > is what was left after the existing fields. There's almost certainly a
> > better way to do this - or perhaps it is unnecessary as per your other
> > message.
> >
> > I am not sure if leaving extra space in the struct is a recommended
> > practice for ioctls or not - I thought I noticed some code that did and
> > some that didn't in the kernel so I err'd on the side of leaving the space
> > and probably did it in the worst way possible.
>
> It's not really a good idea unless you know exactly what you are going
> to do with it. Why not just have a new ioctl if you need new
> information in the future? That's simpler, right?
Sure, that makes sense to me. I'll remove it in the v4 alongside the other
changes you've requested.
Thanks for your time and patience reviewing my code. I greatly appreciate
your helpful comments and feedback.
Thanks,
Joe
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Arnd Bergmann @ 2024-01-26 6:16 UTC (permalink / raw)
To: Joe Damato, Greg Kroah-Hartman
Cc: linux-kernel, Netdev, Chuck Lever, Jeff Layton, linux-api,
Christian Brauner, Eric Dumazet, David S . Miller,
alexander.duyck, Sridhar Samudrala, Jakub Kicinski,
Willem de Bruijn, weiwan, Jonathan Corbet, Alexander Viro,
Jan Kara, Michael Ellerman, Nathan Lynch, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Andrew Waterman,
Thomas Huth, Palmer Dabbelt, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240126023630.GA1235@fastly.com>
On Fri, Jan 26, 2024, at 03:36, Joe Damato wrote:
> On Thu, Jan 25, 2024 at 04:23:58PM -0800, Greg Kroah-Hartman wrote:
>> On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:
>> > On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
>> > > On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
>> > > > +struct epoll_params {
>> > > > + u64 busy_poll_usecs;
>> > > > + u16 busy_poll_budget;
>> > > > +
>> > > > + /* for future fields */
>> > > > + u8 data[118];
>> > > > +} EPOLL_PACKED;
>> > >
>
> Sure, that makes sense to me. I'll remove it in the v4 alongside the other
> changes you've requested.
>
> Thanks for your time and patience reviewing my code. I greatly appreciate
> your helpful comments and feedback.
Note that you should still pad the structure to its normal
alignment. On non-x86 targets this would currently mean a
multiple of 64 bits.
I would suggest dropping the EPOLL_PACKED here entirely and
just using a fully aligned structure on all architectures, like
struct epoll_params {
__aligned_u64 busy_poll_usecs;
__u16 busy_poll_budget;
__u8 __pad[6];
};
The explicit padding can help avoid leaking stack data when
a structure is copied back from kernel to userspace, so I would
just always use it in ioctl data structures.
Arnd
^ permalink raw reply
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Lance Yang @ 2024-01-26 6:16 UTC (permalink / raw)
To: akpm, Michal Hocko, Zach O'Keefe, Yang Shi, David Hildenbrand
Cc: songmuchun, peterx, mknyszek, minchan, linux-mm, linux-kernel,
linux-api
In-Reply-To: <CAK1f24=TfJvsDCEesaTa8rGP7ay62p6UiJem=XWnpFa9yfSA3A@mail.gmail.com>
I’d like to add another real use case.
In our company, we deploy applications using offline-online
hybrid deployment. This approach leverages the distinctive
resource utilization patterns of online services, utilizing idle
resources during various time periods by filling them with
offline jobs. This helps reduce the growing cost expenditures
for the enterprise.
Whether for online services or offline jobs, their requirements
for THP can be roughly categorized into three types:
* The first type aims to use huge pages as much as possible
and tolerates unpredictable stalls caused by direct reclaim
and/or compaction.
* The second type attempts to use huge pages but is relatively
latency-sensitive and cannot tolerate unpredictable stalls.
* The third type prefers not to use huge pages at all and is
extremely latency-sensitive.
After careful consideration, we decided to prioritize the
requirements of the first type and modify the THP settings
as follows:
echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
echo defer >/sys/kernel/mm/transparent_hugepage/defrag
With the introduction of MADV_COLLAPSE into the kernel,
it is no longer dependent on any sysfs setting under
/sys/kernel/mm/transparent_hugepage. MADV_COLLAPSE
offers the potential for fine-grained synchronous control over
the huge page allocation mechanism, marking a significant
enhancement for THP.
If the kernel supports a more relaxed (opportunistic)
MADV_COLLAPSE, we will modify the THP settings as follows:
echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
Then, we will use process_madvise(MADV_COLLAPSE, xx_relaxed_flag)
to address the requirements of the second type.
Why don't we favor madvise(MADV_COLLAPSE) for the first type
of requirements?
The main reason is that these requirements are typically for offline
jobs in the Hadoop ecosystem, such as MapReduce and Spark,
which run primarily on the JVM. IIRC, the JVM currently does not
support madvise(MADV_COLLAPSE). The second type of
requirements is all for our in-house developed online services.
For us, integrating a more relaxed (opportunistic)
MADV_COLLAPSE into our online services is relatively
straightforward.
By introducing various flags to MADV_COLLAPSE, we can offer
multiple synchronous allocation strategies for applications. This
fine-grained control may be more suitable for cloud-native
environments than the widespread settings under
/sys/kernel/mm/transparent_hugepage in sysfs.
Thanks for your time!
Lance
On Sun, Jan 21, 2024 at 11:12 AM Lance Yang <ioworker0@gmail.com> wrote:
>
> Hello Everyone,
>
> For applications actively utilizing THP, the defrag mode may
> not be a very user-friendly design. Here are the reasons:
> 1. Before marking the address space with
> MADV_HUGEPAGE,it is necessary to check if
> the current configuration of the defrag mode aligns with
> their preferences.
> 2. Once the defrag mode configuration changes, these
> applications may face the risk of unpredictable stalls.
>
> THP is an important feature of the Linux kernel that can
> significantly enhance memory access performance.
> However, due to the lack of fine-grained control over
> the huge page allocation strategy, many applications
> default to not using huge pages and even recommend
> users to disable THP. This situation is regrettable.
>
> With the introduction of MADV_COLLAPSE into the kernel,
> it is not affected by the defrag mode.
> MADV_COLLAPSE offers the potential for
> fine-grained synchronous control over the huge page
> allocation mechanism, marking a significant enhancement
> for THP.
>
> By adding flags to MADV_COLLAPSE, different
> synchronous allocation strategies can be provided to
> applications. This can instill confidence in them, allowing
> them to reconsider using THP and allocate huge pages
> according to their desired synchronous allocation strategy,
> without worrying about the defrag mode configuration.
>
> BR,
> Lance
>
>
> On Thu, Jan 18, 2024 at 8:03 PM Lance Yang <ioworker0@gmail.com> wrote:
> >
> > This idea was inspired by MADV_COLLAPSE introduced by Zach O'Keefe[1].
> >
> > Allow MADV_F_COLLAPSE_LIGHT behavior for process_madvise(2) if the caller
> > has CAP_SYS_ADMIN or is requesting the collapse of its own memory.
> >
> > The semantics of MADV_F_COLLAPSE_LIGHT are similar to MADV_COLLAPSE, but
> > it avoids direct reclaim and/or compaction, quickly failing on allocation
> > errors.
> >
> > This change enables a more flexible and efficient usage of memory collapse
> > operations, providing additional control to userspace applications for
> > system-wide THP optimization.
> >
> > Semantics
> >
> > This call is independent of the system-wide THP sysfs settings, but will
> > fail for memory marked VM_NOHUGEPAGE. If the ranges provided span
> > multiple VMAs, the semantics of the collapse over each VMA is independent
> > from the others. This implies a hugepage cannot cross a VMA boundary. If
> > collapse of a given hugepage-aligned/sized region fails, the operation may
> > continue to attempt collapsing the remainder of memory specified.
> >
> > The memory ranges provided must be page-aligned, but are not required to
> > be hugepage-aligned. If the memory ranges are not hugepage-aligned, the
> > start/end of the range will be clamped to the first/last hugepage-aligned
> > address covered by said range. The memory ranges must span at least one
> > hugepage-sized region.
> >
> > All non-resident pages covered by the range will first be
> > swapped/faulted-in, before being internally copied onto a freshly
> > allocated hugepage. Unmapped pages will have their data directly
> > initialized to 0 in the new hugepage. However, for every eligible
> > hugepage aligned/sized region to-be collapsed, at least one page must
> > currently be backed by memory (a PMD covering the address range must
> > already exist).
> >
> > Allocation for the new hugepage will not enter direct reclaim and/or
> > compaction, quickly failing if allocation fails. When the system has
> > multiple NUMA nodes, the hugepage will be allocated from the node providing
> > the most native pages. This operation operates on the current state of the
> > specified process and makes no persistent changes or guarantees on how pages
> > will be mapped, constructed, or faulted in the future.
> >
> > Use Cases
> >
> > An immediate user of this new functionality is the Go runtime heap allocator
> > that manages memory in hugepage-sized chunks. In the past, whether it was a
> > newly allocated chunk through mmap() or a reused chunk released by
> > madvise(MADV_DONTNEED), the allocator attempted to eagerly back memory with
> > huge pages using madvise(MADV_HUGEPAGE)[2] and madvise(MADV_COLLAPSE)[3]
> > respectively. However, both approaches resulted in performance issues; for
> > both scenarios, there could be entries into direct reclaim and/or compaction,
> > leading to unpredictable stalls[4]. Now, the allocator can confidently use
> > process_madvise(MADV_F_COLLAPSE_LIGHT) to attempt the allocation of huge pages.
> >
> > [1] https://github.com/torvalds/linux/commit/7d8faaf155454f8798ec56404faca29a82689c77
> > [2] https://github.com/golang/go/commit/8fa9e3beee8b0e6baa7333740996181268b60a3a
> > [3] https://github.com/golang/go/commit/9f9bb26880388c5bead158e9eca3be4b3a9bd2af
> > [4] https://github.com/golang/go/issues/63334
> >
> > [v1] https://lore.kernel.org/lkml/20240117050217.43610-1-ioworker0@gmail.com/
> >
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > Suggested-by: Zach O'Keefe <zokeefe@google.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > ---
> > V1 -> V2: Treat process_madvise(MADV_F_COLLAPSE_LIGHT) as the lighter-weight alternative
> > to madvise(MADV_COLLAPSE)
> >
> > arch/alpha/include/uapi/asm/mman.h | 1 +
> > arch/mips/include/uapi/asm/mman.h | 1 +
> > arch/parisc/include/uapi/asm/mman.h | 1 +
> > arch/xtensa/include/uapi/asm/mman.h | 1 +
> > include/linux/huge_mm.h | 5 +--
> > include/uapi/asm-generic/mman-common.h | 1 +
> > mm/khugepaged.c | 15 ++++++--
> > mm/madvise.c | 36 +++++++++++++++++---
> > tools/include/uapi/asm-generic/mman-common.h | 1 +
> > 9 files changed, 52 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> > index 763929e814e9..22f23ca04f1a 100644
> > --- a/arch/alpha/include/uapi/asm/mman.h
> > +++ b/arch/alpha/include/uapi/asm/mman.h
> > @@ -77,6 +77,7 @@
> > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> >
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> >
> > /* compatibility flags */
> > #define MAP_FILE 0
> > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
> > index c6e1fc77c996..acec0b643e9c 100644
> > --- a/arch/mips/include/uapi/asm/mman.h
> > +++ b/arch/mips/include/uapi/asm/mman.h
> > @@ -104,6 +104,7 @@
> > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> >
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> >
> > /* compatibility flags */
> > #define MAP_FILE 0
> > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
> > index 68c44f99bc93..812029c98cd7 100644
> > --- a/arch/parisc/include/uapi/asm/mman.h
> > +++ b/arch/parisc/include/uapi/asm/mman.h
> > @@ -71,6 +71,7 @@
> > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> >
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> >
> > #define MADV_HWPOISON 100 /* poison a page for testing */
> > #define MADV_SOFT_OFFLINE 101 /* soft offline page for testing */
> > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
> > index 1ff0c858544f..52ef463dd5b6 100644
> > --- a/arch/xtensa/include/uapi/asm/mman.h
> > +++ b/arch/xtensa/include/uapi/asm/mman.h
> > @@ -112,6 +112,7 @@
> > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> >
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> >
> > /* compatibility flags */
> > #define MAP_FILE 0
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 5adb86af35fc..075fdb5d481a 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -303,7 +303,7 @@ int hugepage_madvise(struct vm_area_struct *vma, unsigned long *vm_flags,
> > int advice);
> > int madvise_collapse(struct vm_area_struct *vma,
> > struct vm_area_struct **prev,
> > - unsigned long start, unsigned long end);
> > + unsigned long start, unsigned long end, int behavior);
> > void vma_adjust_trans_huge(struct vm_area_struct *vma, unsigned long start,
> > unsigned long end, long adjust_next);
> > spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma);
> > @@ -450,7 +450,8 @@ static inline int hugepage_madvise(struct vm_area_struct *vma,
> >
> > static inline int madvise_collapse(struct vm_area_struct *vma,
> > struct vm_area_struct **prev,
> > - unsigned long start, unsigned long end)
> > + unsigned long start, unsigned long end,
> > + int behavior)
> > {
> > return -EINVAL;
> > }
> > diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> > index 6ce1f1ceb432..92c67bc755da 100644
> > --- a/include/uapi/asm-generic/mman-common.h
> > +++ b/include/uapi/asm-generic/mman-common.h
> > @@ -78,6 +78,7 @@
> > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> >
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> >
> > /* compatibility flags */
> > #define MAP_FILE 0
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 2b219acb528e..2840051c0ae2 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -97,6 +97,8 @@ static struct kmem_cache *mm_slot_cache __ro_after_init;
> > struct collapse_control {
> > bool is_khugepaged;
> >
> > + int behavior;
> > +
> > /* Num pages scanned per node */
> > u32 node_load[MAX_NUMNODES];
> >
> > @@ -1058,10 +1060,16 @@ static int __collapse_huge_page_swapin(struct mm_struct *mm,
> > static int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
> > struct collapse_control *cc)
> > {
> > - gfp_t gfp = (cc->is_khugepaged ? alloc_hugepage_khugepaged_gfpmask() :
> > - GFP_TRANSHUGE);
> > int node = hpage_collapse_find_target_node(cc);
> > struct folio *folio;
> > + gfp_t gfp;
> > +
> > + if (cc->is_khugepaged)
> > + gfp = alloc_hugepage_khugepaged_gfpmask();
> > + else
> > + gfp = (cc->behavior == MADV_F_COLLAPSE_LIGHT ?
> > + GFP_TRANSHUGE_LIGHT :
> > + GFP_TRANSHUGE);
> >
> > if (!hpage_collapse_alloc_folio(&folio, gfp, node, &cc->alloc_nmask)) {
> > *hpage = NULL;
> > @@ -2697,7 +2705,7 @@ static int madvise_collapse_errno(enum scan_result r)
> > }
> >
> > int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > - unsigned long start, unsigned long end)
> > + unsigned long start, unsigned long end, int behavior)
> > {
> > struct collapse_control *cc;
> > struct mm_struct *mm = vma->vm_mm;
> > @@ -2718,6 +2726,7 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > if (!cc)
> > return -ENOMEM;
> > cc->is_khugepaged = false;
> > + cc->behavior = behavior;
> >
> > mmgrab(mm);
> > lru_add_drain_all();
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 912155a94ed5..9c40226505aa 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -60,6 +60,7 @@ static int madvise_need_mmap_write(int behavior)
> > case MADV_POPULATE_READ:
> > case MADV_POPULATE_WRITE:
> > case MADV_COLLAPSE:
> > + case MADV_F_COLLAPSE_LIGHT:
> > return 0;
> > default:
> > /* be safe, default to 1. list exceptions explicitly */
> > @@ -1082,8 +1083,9 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> > if (error)
> > goto out;
> > break;
> > + case MADV_F_COLLAPSE_LIGHT:
> > case MADV_COLLAPSE:
> > - return madvise_collapse(vma, prev, start, end);
> > + return madvise_collapse(vma, prev, start, end, behavior);
> > }
> >
> > anon_name = anon_vma_name(vma);
> > @@ -1178,6 +1180,7 @@ madvise_behavior_valid(int behavior)
> > case MADV_HUGEPAGE:
> > case MADV_NOHUGEPAGE:
> > case MADV_COLLAPSE:
> > + case MADV_F_COLLAPSE_LIGHT:
> > #endif
> > case MADV_DONTDUMP:
> > case MADV_DODUMP:
> > @@ -1194,6 +1197,17 @@ madvise_behavior_valid(int behavior)
> > }
> > }
> >
> > +
> > +static bool process_madvise_behavior_only(int behavior)
> > +{
> > + switch (behavior) {
> > + case MADV_F_COLLAPSE_LIGHT:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static bool process_madvise_behavior_valid(int behavior)
> > {
> > switch (behavior) {
> > @@ -1201,6 +1215,7 @@ static bool process_madvise_behavior_valid(int behavior)
> > case MADV_PAGEOUT:
> > case MADV_WILLNEED:
> > case MADV_COLLAPSE:
> > + case MADV_F_COLLAPSE_LIGHT:
> > return true;
> > default:
> > return false;
> > @@ -1368,6 +1383,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > * transparent huge pages so the existing pages will not be
> > * coalesced into THP and new pages will not be allocated as THP.
> > * MADV_COLLAPSE - synchronously coalesce pages into new THP.
> > + * MADV_F_COLLAPSE_LIGHT - only for process_madvise, avoids direct reclaim and/or
> > + * compaction.
> > * MADV_DONTDUMP - the application wants to prevent pages in the given range
> > * from being included in its core dump.
> > * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > @@ -1394,7 +1411,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > * -EBADF - map exists, but area maps something that isn't a file.
> > * -EAGAIN - a kernel resource was temporarily unavailable.
> > */
> > -int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> > +int _do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in,
> > + int behavior, bool is_process_madvise)
> > {
> > unsigned long end;
> > int error;
> > @@ -1405,6 +1423,9 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > if (!madvise_behavior_valid(behavior))
> > return -EINVAL;
> >
> > + if (!is_process_madvise && process_madvise_behavior_only(behavior))
> > + return -EINVAL;
> > +
> > if (!PAGE_ALIGNED(start))
> > return -EINVAL;
> > len = PAGE_ALIGN(len_in);
> > @@ -1448,9 +1469,14 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > return error;
> > }
> >
> > +int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> > +{
> > + return _do_madvise(mm, start, len_in, behavior, false);
> > +}
> > +
> > SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > {
> > - return do_madvise(current->mm, start, len_in, behavior);
> > + return _do_madvise(current->mm, start, len_in, behavior, false);
> > }
> >
> > SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > @@ -1504,8 +1530,8 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > total_len = iov_iter_count(&iter);
> >
> > while (iov_iter_count(&iter)) {
> > - ret = do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
> > - iter_iov_len(&iter), behavior);
> > + ret = _do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
> > + iter_iov_len(&iter), behavior, true);
> > if (ret < 0)
> > break;
> > iov_iter_advance(&iter, iter_iov_len(&iter));
> > diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
> > index 6ce1f1ceb432..92c67bc755da 100644
> > --- a/tools/include/uapi/asm-generic/mman-common.h
> > +++ b/tools/include/uapi/asm-generic/mman-common.h
> > @@ -78,6 +78,7 @@
> > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> >
> > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> >
> > /* compatibility flags */
> > #define MAP_FILE 0
> > --
> > 2.33.1
> >
^ permalink raw reply
* Re: [PATCH v3 3/4] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Huang, Ying @ 2024-01-26 7:10 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-kernel, linux-doc, linux-fsdevel, linux-api,
corbet, akpm, gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <20240125184345.47074-4-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> When a system has multiple NUMA nodes and it becomes bandwidth hungry,
> using the current MPOL_INTERLEAVE could be an wise option.
>
> However, if those NUMA nodes consist of different types of memory such
> as socket-attached DRAM and CXL/PCIe attached DRAM, the round-robin
> based interleave policy does not optimally distribute data to make use
> of their different bandwidth characteristics.
>
> Instead, interleave is more effective when the allocation policy follows
> each NUMA nodes' bandwidth weight rather than a simple 1:1 distribution.
>
> This patch introduces a new memory policy, MPOL_WEIGHTED_INTERLEAVE,
> enabling weighted interleave between NUMA nodes. Weighted interleave
> allows for proportional distribution of memory across multiple numa
> nodes, preferably apportioned to match the bandwidth of each node.
>
> For example, if a system has 1 CPU node (0), and 2 memory nodes (0,1),
> with bandwidth of (100GB/s, 50GB/s) respectively, the appropriate
> weight distribution is (2:1).
>
> Weights for each node can be assigned via the new sysfs extension:
> /sys/kernel/mm/mempolicy/weighted_interleave/
>
> For now, the default value of all nodes will be `1`, which matches
> the behavior of standard 1:1 round-robin interleave. An extension
> will be added in the future to allow default values to be registered
> at kernel and device bringup time.
>
> The policy allocates a number of pages equal to the set weights. For
> example, if the weights are (2,1), then 2 pages will be allocated on
> node0 for every 1 page allocated on node1.
>
> The new flag MPOL_WEIGHTED_INTERLEAVE can be used in set_mempolicy(2)
> and mbind(2).
>
> There are 3 integration points:
>
> weighted_interleave_nodes:
> Counts the number of allocations as they occur, and applies the
> weight for the current node. When the weight reaches 0, switch
> to the next node.
>
> weighted_interleave_nid:
> Gets the total weight of the nodemask as well as each individual
> node weight, then calculates the node based on the given index.
>
> bulk_array_weighted_interleave:
> Gets the total weight of the nodemask as well as each individual
> node weight, then calculates the number of "interleave rounds" as
> well as any delta ("partial round"). Calculates the number of
> pages for each node and allocates them.
>
> If a node was scheduled for interleave via interleave_nodes, the
> current weight (pol->cur_il_weight) will be allocated first, before
> the remaining bulk calculation is done.
>
> One piece of complexity is the interaction between a recent refactor
> which split the logic to acquire the "ilx" (interleave index) of an
> allocation and the actually application of the interleave. The
> calculation of the `interleave index` is done by `get_vma_policy()`,
> while the actual selection of the node will be later appliex by the
> relevant weighted_interleave function.
>
> Suggested-by: Hasan Al Maruf <Hasan.Maruf@amd.com>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> Co-developed-by: Rakie Kim <rakie.kim@sk.com>
> Signed-off-by: Rakie Kim <rakie.kim@sk.com>
> Co-developed-by: Honggyu Kim <honggyu.kim@sk.com>
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> Co-developed-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
> Signed-off-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
> Co-developed-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
> Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
> Co-developed-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
> Signed-off-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
> ---
> .../admin-guide/mm/numa_memory_policy.rst | 9 +
> include/linux/mempolicy.h | 3 +
> include/uapi/linux/mempolicy.h | 1 +
> mm/mempolicy.c | 274 +++++++++++++++++-
> 4 files changed, 283 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/numa_memory_policy.rst b/Documentation/admin-guide/mm/numa_memory_policy.rst
> index eca38fa81e0f..a70f20ce1ffb 100644
> --- a/Documentation/admin-guide/mm/numa_memory_policy.rst
> +++ b/Documentation/admin-guide/mm/numa_memory_policy.rst
> @@ -250,6 +250,15 @@ MPOL_PREFERRED_MANY
> can fall back to all existing numa nodes. This is effectively
> MPOL_PREFERRED allowed for a mask rather than a single node.
>
> +MPOL_WEIGHTED_INTERLEAVE
> + This mode operates the same as MPOL_INTERLEAVE, except that
> + interleaving behavior is executed based on weights set in
> + /sys/kernel/mm/mempolicy/weighted_interleave/
> +
> + Weighted interleave allocates pages on nodes according to a
> + weight. For example if nodes [0,1] are weighted [5,2], 5 pages
> + will be allocated on node0 for every 2 pages allocated on node1.
> +
> NUMA memory policy supports the following optional mode flags:
>
> MPOL_F_STATIC_NODES
> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
> index 931b118336f4..c644d7bbd396 100644
> --- a/include/linux/mempolicy.h
> +++ b/include/linux/mempolicy.h
> @@ -54,6 +54,9 @@ struct mempolicy {
> nodemask_t cpuset_mems_allowed; /* relative to these nodes */
> nodemask_t user_nodemask; /* nodemask passed by user */
> } w;
> +
> + /* Weighted interleave settings */
> + u8 cur_il_weight;
> };
>
> /*
> diff --git a/include/uapi/linux/mempolicy.h b/include/uapi/linux/mempolicy.h
> index a8963f7ef4c2..1f9bb10d1a47 100644
> --- a/include/uapi/linux/mempolicy.h
> +++ b/include/uapi/linux/mempolicy.h
> @@ -23,6 +23,7 @@ enum {
> MPOL_INTERLEAVE,
> MPOL_LOCAL,
> MPOL_PREFERRED_MANY,
> + MPOL_WEIGHTED_INTERLEAVE,
> MPOL_MAX, /* always last member of enum */
> };
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b13c45a0bfcb..5a517511658e 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -19,6 +19,13 @@
> * for anonymous memory. For process policy an process counter
> * is used.
> *
> + * weighted interleave
> + * Allocate memory interleaved over a set of nodes based on
> + * a set of weights (per-node), with normal fallback if it
> + * fails. Otherwise operates the same as interleave.
> + * Example: nodeset(0,1) & weights (2,1) - 2 pages allocated
> + * on node 0 for every 1 page allocated on node 1.
> + *
> * bind Only allocate memory on a specific set of nodes,
> * no fallback.
> * FIXME: memory is allocated starting with the first node
> @@ -314,6 +321,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags,
> policy->mode = mode;
> policy->flags = flags;
> policy->home_node = NUMA_NO_NODE;
> + policy->cur_il_weight = 0;
>
> return policy;
> }
> @@ -426,6 +434,10 @@ static const struct mempolicy_operations mpol_ops[MPOL_MAX] = {
> .create = mpol_new_nodemask,
> .rebind = mpol_rebind_preferred,
> },
> + [MPOL_WEIGHTED_INTERLEAVE] = {
> + .create = mpol_new_nodemask,
> + .rebind = mpol_rebind_nodemask,
> + },
> };
>
> static bool migrate_folio_add(struct folio *folio, struct list_head *foliolist,
> @@ -847,7 +859,8 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
>
> old = current->mempolicy;
> current->mempolicy = new;
> - if (new && new->mode == MPOL_INTERLEAVE)
> + if (new && (new->mode == MPOL_INTERLEAVE ||
> + new->mode == MPOL_WEIGHTED_INTERLEAVE))
> current->il_prev = MAX_NUMNODES-1;
> task_unlock(current);
> mpol_put(old);
> @@ -873,6 +886,7 @@ static void get_policy_nodemask(struct mempolicy *pol, nodemask_t *nodes)
> case MPOL_INTERLEAVE:
> case MPOL_PREFERRED:
> case MPOL_PREFERRED_MANY:
> + case MPOL_WEIGHTED_INTERLEAVE:
> *nodes = pol->nodes;
> break;
> case MPOL_LOCAL:
> @@ -957,6 +971,13 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
> } else if (pol == current->mempolicy &&
> pol->mode == MPOL_INTERLEAVE) {
> *policy = next_node_in(current->il_prev, pol->nodes);
> + } else if (pol == current->mempolicy &&
> + (pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
> + if (pol->cur_il_weight)
> + *policy = current->il_prev;
> + else
> + *policy = next_node_in(current->il_prev,
> + pol->nodes);
It appears that my previous comments about this is ignored.
https://lore.kernel.org/linux-mm/875xzkv3x2.fsf@yhuang6-desk2.ccr.corp.intel.com/
Please correct me if I am wrong.
> } else {
> err = -EINVAL;
> goto out;
> @@ -1769,7 +1790,8 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
> * @vma: virtual memory area whose policy is sought
> * @addr: address in @vma for shared policy lookup
> * @order: 0, or appropriate huge_page_order for interleaving
> - * @ilx: interleave index (output), for use only when MPOL_INTERLEAVE
> + * @ilx: interleave index (output), for use only when MPOL_INTERLEAVE or
> + * MPOL_WEIGHTED_INTERLEAVE
> *
> * Returns effective policy for a VMA at specified address.
> * Falls back to current->mempolicy or system default policy, as necessary.
> @@ -1786,7 +1808,8 @@ struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
> pol = __get_vma_policy(vma, addr, ilx);
> if (!pol)
> pol = get_task_policy(current);
> - if (pol->mode == MPOL_INTERLEAVE) {
> + if (pol->mode == MPOL_INTERLEAVE ||
> + pol->mode == MPOL_WEIGHTED_INTERLEAVE) {
> *ilx += vma->vm_pgoff >> order;
> *ilx += (addr - vma->vm_start) >> (PAGE_SHIFT + order);
> }
> @@ -1836,6 +1859,44 @@ bool apply_policy_zone(struct mempolicy *policy, enum zone_type zone)
> return zone >= dynamic_policy_zone;
> }
>
> +static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> +{
> + unsigned int node, next;
> + struct task_struct *me = current;
> + u8 __rcu *table;
> + u8 weight;
> +
> + node = next_node_in(me->il_prev, policy->nodes);
> + if (node == MAX_NUMNODES)
> + return node;
> +
> + /* on first alloc after setting mempolicy, acquire first weight */
> + if (unlikely(!policy->cur_il_weight)) {
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + /* detect system-default values */
> + weight = table ? table[node] : 1;
> + policy->cur_il_weight = weight ? weight : 1;
> + rcu_read_unlock();
> + }
> +
> + /* account for this allocation call */
> + policy->cur_il_weight--;
> +
> + /* if now at 0, move to next node and set up that node's weight */
> + if (unlikely(!policy->cur_il_weight)) {
> + me->il_prev = node;
> + next = next_node_in(node, policy->nodes);
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + /* detect system-default values */
> + weight = table ? table[next] : 1;
> + policy->cur_il_weight = weight ? weight : 1;
> + rcu_read_unlock();
> + }
It appears that the code could be more concise if we allow
policy->cur_il_weight == 0. Duplicated code are in
alloc_pages_bulk_array_weighted_interleave() too. Anyway, can we define
some function to reduce duplicated code.
> + return node;
> +}
> +
> /* Do dynamic interleaving for a process */
> static unsigned int interleave_nodes(struct mempolicy *policy)
> {
> @@ -1870,6 +1931,9 @@ unsigned int mempolicy_slab_node(void)
> case MPOL_INTERLEAVE:
> return interleave_nodes(policy);
>
> + case MPOL_WEIGHTED_INTERLEAVE:
> + return weighted_interleave_nodes(policy);
> +
> case MPOL_BIND:
> case MPOL_PREFERRED_MANY:
> {
> @@ -1908,6 +1972,39 @@ static unsigned int read_once_policy_nodemask(struct mempolicy *pol,
> return nodes_weight(*mask);
> }
>
> +static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
> +{
> + nodemask_t nodemask;
> + unsigned int target, nr_nodes;
> + u8 __rcu *table;
> + unsigned int weight_total = 0;
> + u8 weight;
> + int nid;
> +
> + nr_nodes = read_once_policy_nodemask(pol, &nodemask);
> + if (!nr_nodes)
> + return numa_node_id();
> +
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + /* calculate the total weight */
> + for_each_node_mask(nid, nodemask)
> + weight_total += table ? table[nid] : 1;
> +
> + /* Calculate the node offset based on totals */
> + target = ilx % weight_total;
> + nid = first_node(nodemask);
> + while (target) {
> + weight = table ? table[nid] : 1;
> + if (target < weight)
> + break;
> + target -= weight;
> + nid = next_node_in(nid, nodemask);
> + }
> + rcu_read_unlock();
> + return nid;
> +}
> +
> /*
> * Do static interleaving for interleave index @ilx. Returns the ilx'th
> * node in pol->nodes (starting from ilx=0), wrapping around if ilx
> @@ -1968,6 +2065,11 @@ static nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *pol,
> *nid = (ilx == NO_INTERLEAVE_INDEX) ?
> interleave_nodes(pol) : interleave_nid(pol, ilx);
> break;
> + case MPOL_WEIGHTED_INTERLEAVE:
> + *nid = (ilx == NO_INTERLEAVE_INDEX) ?
> + weighted_interleave_nodes(pol) :
> + weighted_interleave_nid(pol, ilx);
> + break;
> }
>
> return nodemask;
> @@ -2029,6 +2131,7 @@ bool init_nodemask_of_mempolicy(nodemask_t *mask)
> case MPOL_PREFERRED_MANY:
> case MPOL_BIND:
> case MPOL_INTERLEAVE:
> + case MPOL_WEIGHTED_INTERLEAVE:
> *mask = mempolicy->nodes;
> break;
>
> @@ -2128,7 +2231,8 @@ struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
> * If the policy is interleave or does not allow the current
> * node in its nodemask, we allocate the standard way.
> */
> - if (pol->mode != MPOL_INTERLEAVE &&
> + if ((pol->mode != MPOL_INTERLEAVE &&
> + pol->mode != MPOL_WEIGHTED_INTERLEAVE) &&
> (!nodemask || node_isset(nid, *nodemask))) {
> /*
> * First, try to allocate THP only on local node, but
> @@ -2264,6 +2368,156 @@ static unsigned long alloc_pages_bulk_array_interleave(gfp_t gfp,
> return total_allocated;
> }
>
> +static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> + struct mempolicy *pol, unsigned long nr_pages,
> + struct page **page_array)
> +{
> + struct task_struct *me = current;
> + unsigned long total_allocated = 0;
> + unsigned long nr_allocated;
> + unsigned long rounds;
> + unsigned long node_pages, delta;
> + u8 weight, resume_weight;
> + u8 __rcu *table;
> + u8 *weights;
> + unsigned int weight_total = 0;
> + unsigned long rem_pages = nr_pages;
> + nodemask_t nodes;
> + int nnodes, node, resume_node, next_node;
> + int prev_node = me->il_prev;
> + int i;
> +
> + if (!nr_pages)
> + return 0;
> +
> + nnodes = read_once_policy_nodemask(pol, &nodes);
> + if (!nnodes)
> + return 0;
> +
> + /* Continue allocating from most recent node and adjust the nr_pages */
> + if (pol->cur_il_weight) {
> + node = next_node_in(prev_node, nodes);
> + node_pages = pol->cur_il_weight;
> + if (node_pages > rem_pages)
> + node_pages = rem_pages;
> + nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
> + NULL, page_array);
> + page_array += nr_allocated;
> + total_allocated += nr_allocated;
> + /*
> + * if that's all the pages, no need to interleave, otherwise
> + * we need to set up the next interleave node/weight correctly.
> + */
> + if (rem_pages < pol->cur_il_weight) {
> + /* stay on current node, adjust cur_il_weight */
> + pol->cur_il_weight -= rem_pages;
> + return total_allocated;
> + } else if (rem_pages == pol->cur_il_weight) {
> + /* move to next node / weight */
> + me->il_prev = node;
> + next_node = next_node_in(node, nodes);
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + weight = table ? table[next_node] : 1;
> + /* detect system-default usage */
> + pol->cur_il_weight = weight ? weight : 1;
> + rcu_read_unlock();
> + return total_allocated;
> + }
> + /* Otherwise we adjust nr_pages down, and continue from there */
> + rem_pages -= pol->cur_il_weight;
> + pol->cur_il_weight = 0;
This break the rule to keep pol->cur_il_weight != 0 except after initial
setup. Is it OK?
> + prev_node = node;
> + }
> +
> + /* create a local copy of node weights to operate on outside rcu */
> + weights = kmalloc(nr_node_ids, GFP_KERNEL);
> + if (!weights)
> + return total_allocated;
> +
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + /* If table is not registered, use system defaults */
> + if (table)
> + memcpy(weights, iw_table, nr_node_ids);
> + else
> + memset(weights, 1, nr_node_ids);
> + rcu_read_unlock();
> +
> + /* calculate total, detect system default usage */
> + for_each_node_mask(node, nodes) {
> + /* detect system-default usage */
> + if (!weights[node])
> + weights[node] = 1;
> + weight_total += weights[node];
> + }
> +
> + /*
> + * Now we can continue allocating from 0 instead of an offset
> + * We calculate the number of rounds and any partial rounds so
> + * that we minimize the number of calls to __alloc_pages_bulk
> + * This requires us to track which node we should resume from.
> + *
> + * if (rounds > 0) and (delta == 0), resume_node will always be
> + * the current value of prev_node, which may be NUMA_NO_NODE if
> + * this is the first allocation after a policy is replaced. The
> + * resume weight will be the weight of the next node.
> + *
> + * if (delta > 0) and delta is depleted exactly on a node-weight
> + * boundary, resume node will be the node last allocated from when
> + * delta reached 0.
> + *
> + * if (delta > 0) and delta is not depleted on a node-weight boundary,
> + * resume node will be the node prior to the node last allocated from.
> + *
> + * (rounds == 0) and (delta == 0) is not possible (earlier exit)
> + */
> + rounds = rem_pages / weight_total;
> + delta = rem_pages % weight_total;
> + resume_node = prev_node;
> + resume_weight = weights[next_node_in(prev_node, nodes)];
> + /* If no delta, we'll resume from current prev_node and first weight */
> + for (i = 0; i < nnodes; i++) {
> + node = next_node_in(prev_node, nodes);
> + weight = weights[node];
> + node_pages = weight * rounds;
> + /* If a delta exists, add this node's portion of the delta */
> + if (delta > weight) {
> + node_pages += weight;
> + delta -= weight;
> + resume_node = node;
> + } else if (delta) {
> + node_pages += delta;
> + if (delta == weight) {
> + /* resume from next node with its weight */
> + resume_node = node;
> + next_node = next_node_in(node, nodes);
> + resume_weight = weights[next_node];
> + } else {
> + /* resume from this node w/ remaining weight */
> + resume_node = prev_node;
> + resume_weight = weight - (node_pages % weight);
resume_weight = weight - delta; ?
> + }
> + delta = 0;
> + }
> + /* node_pages can be 0 if an allocation fails and rounds == 0 */
> + if (!node_pages)
> + break;
> + nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
> + NULL, page_array);
> + page_array += nr_allocated;
> + total_allocated += nr_allocated;
> + if (total_allocated == nr_pages)
> + break;
> + prev_node = node;
> + }
> + /* resume allocating from the calculated node and weight */
> + me->il_prev = resume_node;
> + pol->cur_il_weight = resume_weight;
> + kfree(weights);
> + return total_allocated;
> +}
> +
> static unsigned long alloc_pages_bulk_array_preferred_many(gfp_t gfp, int nid,
> struct mempolicy *pol, unsigned long nr_pages,
> struct page **page_array)
> @@ -2304,6 +2558,10 @@ unsigned long alloc_pages_bulk_array_mempolicy(gfp_t gfp,
> return alloc_pages_bulk_array_interleave(gfp, pol,
> nr_pages, page_array);
>
> + if (pol->mode == MPOL_WEIGHTED_INTERLEAVE)
> + return alloc_pages_bulk_array_weighted_interleave(
> + gfp, pol, nr_pages, page_array);
> +
> if (pol->mode == MPOL_PREFERRED_MANY)
> return alloc_pages_bulk_array_preferred_many(gfp,
> numa_node_id(), pol, nr_pages, page_array);
> @@ -2379,6 +2637,7 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
> case MPOL_INTERLEAVE:
> case MPOL_PREFERRED:
> case MPOL_PREFERRED_MANY:
> + case MPOL_WEIGHTED_INTERLEAVE:
> return !!nodes_equal(a->nodes, b->nodes);
> case MPOL_LOCAL:
> return true;
> @@ -2515,6 +2774,10 @@ int mpol_misplaced(struct folio *folio, struct vm_area_struct *vma,
> polnid = interleave_nid(pol, ilx);
> break;
>
> + case MPOL_WEIGHTED_INTERLEAVE:
> + polnid = weighted_interleave_nid(pol, ilx);
> + break;
> +
> case MPOL_PREFERRED:
> if (node_isset(curnid, pol->nodes))
> goto out;
> @@ -2889,6 +3152,7 @@ static const char * const policy_modes[] =
> [MPOL_PREFERRED] = "prefer",
> [MPOL_BIND] = "bind",
> [MPOL_INTERLEAVE] = "interleave",
> + [MPOL_WEIGHTED_INTERLEAVE] = "weighted interleave",
> [MPOL_LOCAL] = "local",
> [MPOL_PREFERRED_MANY] = "prefer (many)",
> };
> @@ -2948,6 +3212,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
> }
> break;
> case MPOL_INTERLEAVE:
> + case MPOL_WEIGHTED_INTERLEAVE:
> /*
> * Default to online nodes with memory if no nodelist
> */
> @@ -3058,6 +3323,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> case MPOL_PREFERRED_MANY:
> case MPOL_BIND:
> case MPOL_INTERLEAVE:
> + case MPOL_WEIGHTED_INTERLEAVE:
> nodes = pol->nodes;
> break;
> default:
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Huang, Ying @ 2024-01-26 7:40 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-kernel, linux-doc, linux-fsdevel, linux-api,
corbet, akpm, gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <20240125184345.47074-5-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> In the prior patch, we carry only the current weight for a weighted
> interleave round with us across calls through the allocator path.
>
> node = next_node_in(current->il_prev, pol->nodemask)
> pol->cur_il_weight <--- this weight applies to the above node
>
> This separation of data can cause a race condition.
>
> If a cgroup-initiated task migration or mems_allowed change occurs
> from outside the context of the task, this can cause the weight to
> become stale, meaning we may end using that weight to allocate
> memory on the wrong node.
>
> Example:
> 1) task A sets (cur_il_weight = 8) and (current->il_prev) to
> node0. node1 is the next set bit in pol->nodemask
> 2) rebind event occurs, removing node1 from the nodemask.
> node2 is now the next set bit in pol->nodemask
> cur_il_weight is now stale.
> 3) allocation occurs, next_node_in(il_prev, nodes) returns
> node2. cur_il_weight is now applied to the wrong node.
>
> The upper level allocator logic must still enforce mems_allowed,
> so this isn't dangerous, but it is innaccurate.
>
> Just clearing the weight is insufficient, as it creates two more
> race conditions. The root of the issue is the separation of weight
> and node data between nodemask and cur_il_weight.
>
> To solve this, update cur_il_weight to be an atomic_t, and place the
> node that the weight applies to in the upper bits of the field:
>
> atomic_t cur_il_weight
> node bits 32:8
> weight bits 7:0
>
> Now retrieving or clearing the active interleave node and weight
> is a single atomic operation, and we are not dependent on the
> potentially changing state of (pol->nodemask) to determine what
> node the weight applies to.
>
> Two special observations:
> - if the weight is non-zero, cur_il_weight must *always* have a
> valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
> This is because we steal the top byte for the weight.
>
> - MAX_NUMNODES is presently limited to 1024 or less on every
> architecture. This would permanently limit MAX_NUMNODES to
> an absolute maximum of (1 << 24) to avoid overflows.
>
> Per some reading and discussion, it appears that max nodes is
> limited to 1024 so that zone type still fits in page flags, so
> this method seemed preferable compared to the alternatives of
> trying to make all or part of mempolicy RCU protected (which
> may not be possible, since it is often referenced during code
> chunks which call operations that may sleep).
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
> include/linux/mempolicy.h | 2 +-
> mm/mempolicy.c | 93 +++++++++++++++++++++++++--------------
> 2 files changed, 61 insertions(+), 34 deletions(-)
>
> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
> index c644d7bbd396..8108fc6e96ca 100644
> --- a/include/linux/mempolicy.h
> +++ b/include/linux/mempolicy.h
> @@ -56,7 +56,7 @@ struct mempolicy {
> } w;
>
> /* Weighted interleave settings */
> - u8 cur_il_weight;
> + atomic_t cur_il_weight;
If we use this field for node and weight, why not change the field name?
For example, cur_wil_node_weight.
> };
>
> /*
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 5a517511658e..41b5fef0a6f5 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -321,7 +321,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags,
> policy->mode = mode;
> policy->flags = flags;
> policy->home_node = NUMA_NO_NODE;
> - policy->cur_il_weight = 0;
> + atomic_set(&policy->cur_il_weight, 0);
>
> return policy;
> }
> @@ -356,6 +356,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes)
> tmp = *nodes;
>
> pol->nodes = tmp;
> + atomic_set(&pol->cur_il_weight, 0);
> }
>
> static void mpol_rebind_preferred(struct mempolicy *pol,
> @@ -973,8 +974,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
> *policy = next_node_in(current->il_prev, pol->nodes);
> } else if (pol == current->mempolicy &&
> (pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
> - if (pol->cur_il_weight)
> - *policy = current->il_prev;
> + int cweight = atomic_read(&pol->cur_il_weight);
> +
> + if (cweight & 0xFF)
> + *policy = cweight >> 8;
Please define some helper functions or macros instead of operate on bits
directly.
> else
> *policy = next_node_in(current->il_prev,
> pol->nodes);
If we record current node in pol->cur_il_weight, why do we still need
curren->il_prev. Can we only use pol->cur_il_weight? And if so, we can
even make current->il_prev a union.
--
Best Regards,
Huang, Ying
[snip]
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Christian Brauner @ 2024-01-26 9:42 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240125175113.GC5513@redhat.com>
On Thu, Jan 25, 2024 at 06:51:14PM +0100, Oleg Nesterov wrote:
> On 01/25, Christian Brauner wrote:
> >
> > > > When it is reaped is "mostly unrelated".
> > >
> > > Then why pidfd_poll() can't simply check !task || task->exit_state ?
> > >
> > > Nevermind. So, currently pidfd_poll() succeeds when the leader can be
> >
> > Hm, the comment right above mentions:
> >
> > /*
> > * Inform pollers only when the whole thread group exits.
> > * If the thread group leader exits before all other threads in the
> > * group, then poll(2) should block, similar to the wait(2) family.
> > */
> > > reaped, iow the whole thread group has exited.
>
> Yes, but the comment doesn't contradict with what I have said?
No, it doesn't. I'm trying to understand what you are suggesting though.
Are you saying !task || tas->exit_state is enough and we shouldn't use
the helper that was added in commit 38fd525a4c61 ("exit: Factor
thread_group_exited out of pidfd_poll"). If so what does that buy us
open-coding the check instead of using that helper? Is there an actual
bug here?
> > > But even if you are the
> > > parent, you can't expect that wait(WNOHANG) must succeed, the leader
> > > can be traced. I guess it is too late to change this behaviour.
> >
> > Hm, why is that an issue though?
>
> Well, I didn't say this is a problem. I simply do not know how/why people
> use pidfd_poll().
Sorry, I just have a hard time understanding what you wanted then. :)
"I guess it is too late to change this behavior." made it sound like a)
there's a problem and b) that you would prefer to change behavior. Thus,
it seems that wait(WNOHANG) hanging when a traced leader of an empty
thread-group has exited is a problem in your eyes.
>
> I mostly tried to explain why do I think that do_notify_pidfd() should
> be always called from exit_notify() path, not by release_task(), even
> if the task is not a leader.
>
> > Because a program would rely on WNOHANG to hang on
> > a ptraced leader? That seems esoteric imho.
>
> To me it would be usefule, but lets not discuss this now. The "patch"
Ok, that's good then. I would expect that at least stuff like rr makes
use of pidfd and they might rely on this behavior - although I haven't
checked their code.
> I sent doesn't change the current behaviour.
Yeah, I got that but it would still be useful to understand the wider
context you were adressing.
>
> > > What if we add the new PIDFD_THREAD flag? With this flag
> > >
> > > - sys_pidfd_open() doesn't require the must be a group leader
> >
> > Yes.
> >
> > >
> > > - pidfd_poll() succeeds when the task passes exit_notify() and
> > > becomes a zombie, even if it is a leader and has other threads.
> >
> > Iiuc, if an existing user creates a pidfd for a thread-group leader and
> > then polls that pidfd they would currently only get notified if the
> > thread-group is empty and the leader has exited.
> >
> > If we now start notifying when the thread-group leader exits but the
> > thread-group isn't empty then this would be a fairly big api change
>
> Hmm... again, this patch doesn't (shouldn't) change the current behavior.
>
> Please note "with this flag" above. If sys_pidfd_open() was called
> without PIDFD_THREAD, then sys_pidfd_open() still requires that the
> target task must be a group leader, and pidfd_poll() won't succeed
> until the leader exits and thread_group_empty() is true.
Yeah, I missed the PIDFD_THREAD flag suggestion. Sorry about that. Btw,
I'm not sure whether you remember that but when we originally did the
pidfd work you and I discussed thread support and already decided back
then that having a flag like PIDFD_THREAD would likely be the way to go.
The PIDFD_THREAD flag would be would be interesting because we could
make pidfd_send_signal() support this flag as well to allow sending a
signal to a specific thread. That's something that I had also wanted to
support. And I've been asked for this a few times already. What do you
think?
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Christian Brauner @ 2024-01-26 9:47 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240125140830.GA5513@redhat.com>
> Please the the incomplete/untested patch below.
>
> - The change in exit_notify() is sub-optimal, we can do better
> to avoid 2 do_notify_pidfd() calls from exit_notify(). But
> so far this is only for discussion, lets keep it simple.
>
> - __pidfd_prepare() needs some minor cleanups regardless of
> this change, I'll send the patch...
>
> What do you think?
>
> And why is thread_group_exited() exported?
>
> Oleg.
>
> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> index 5406fbc13074..2e6461459877 100644
> --- a/include/uapi/linux/pidfd.h
> +++ b/include/uapi/linux/pidfd.h
> @@ -7,6 +7,7 @@
> #include <linux/fcntl.h>
>
> /* Flags for pidfd_open(). */
> -#define PIDFD_NONBLOCK O_NONBLOCK
> +#define PIDFD_NONBLOCK O_NONBLOCK
> +#define PIDFD_THREAD O_EXCL // or anything else not used by anon_inode's
I like it!
The only request I would have is to not alias O_EXCL and PIDFD_THREAD.
Because it doesn't map as clearly as NONBLOCK did.
>
> #endif /* _UAPI_LINUX_PIDFD_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index dfb963d2f862..9f8526b7d717 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -752,6 +752,10 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> autoreap = true;
> }
>
> + /* unnecessary if do_notify_parent() was already called,
> + we can do better */
> + do_notify_pidfd(tsk);
> +
> if (autoreap) {
> tsk->exit_state = EXIT_DEAD;
> list_add(&tsk->ptrace_entry, &dead);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index c981fa6171c1..38f2c7423fb4 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -101,6 +101,7 @@
> #include <linux/user_events.h>
> #include <linux/iommu.h>
> #include <linux/rseq.h>
> +#include <uapi/linux/pidfd.h>
>
> #include <asm/pgalloc.h>
> #include <linux/uaccess.h>
> @@ -2068,12 +2069,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
> }
> #endif
>
> +static bool xxx_exited(struct pid *pid, int excl)
> +{
> + struct task_struct *task;
> + bool exited;
> +
> + rcu_read_lock();
> + task = pid_task(pid, PIDTYPE_PID);
> + exited = !task ||
> + (READ_ONCE(task->exit_state) && (excl || thread_group_empty(task)));
> + rcu_read_unlock();
> +
> + return exited;
> +}
> +
> /*
> * Poll support for process exit notification.
> */
> static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> {
> struct pid *pid = file->private_data;
> + int excl = file->f_flags & PIDFD_THREAD;
> __poll_t poll_flags = 0;
>
> poll_wait(file, &pid->wait_pidfd, pts);
> @@ -2083,7 +2099,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> * If the thread group leader exits before all other threads in the
> * group, then poll(2) should block, similar to the wait(2) family.
> */
> - if (thread_group_exited(pid))
> + if (xxx_exited(pid, excl))
> poll_flags = EPOLLIN | EPOLLRDNORM;
>
> return poll_flags;
> @@ -2129,7 +2145,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
> {
> int pidfd;
> struct file *pidfd_file;
> + unsigned excl = flags & PIDFD_THREAD;
>
> + flags &= ~PIDFD_THREAD;
> if (flags & ~(O_NONBLOCK | O_RDWR | O_CLOEXEC))
> return -EINVAL;
>
> @@ -2144,6 +2162,7 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
> return PTR_ERR(pidfd_file);
> }
> get_pid(pid); /* held by pidfd_file now */
> + pidfd_file->f_flags |= excl;
> *ret = pidfd_file;
> return pidfd;
> }
> @@ -2176,7 +2195,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
> */
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
> {
> - if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
> + unsigned excl = flags & PIDFD_THREAD;
> +
> + if (!pid || !pid_has_task(pid, excl ? PIDTYPE_PID : PIDTYPE_TGID))
> return -EINVAL;
>
> return __pidfd_prepare(pid, flags, ret);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index b52b10865454..5257197f9493 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> int fd;
> struct pid *p;
>
> - if (flags & ~PIDFD_NONBLOCK)
> + if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD))
> return -EINVAL;
>
> if (pid <= 0)
>
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Christian Brauner @ 2024-01-26 9:49 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbKqQl39WlqX8dgp@tycho.pizza>
On Thu, Jan 25, 2024 at 11:36:50AM -0700, Tycho Andersen wrote:
> On Thu, Jan 25, 2024 at 07:30:46PM +0100, Oleg Nesterov wrote:
> > On 01/25, Oleg Nesterov wrote:
> > >
> > > On 01/25, Tycho Andersen wrote:
> > > >
> > > > One of the things I don't like about PIDFD_THREAD is that it's hard to
> > > > tell whether an arbitrary thread is a leader or not. Right now we do
> > > > it by parsing /proc/pid/status, which shows all the stuff from
> > > > do_task_stat() that we don't care about but which is quite expensive
> > > > to compute. (Maybe there's a better way?)
> > > >
> > > > With PIDFD_THREAD we could could do it twice, once with the flag, get
> > > > EINVAL, and then do it again. But ideally we wouldn't have to.
> > >
> > > Too late for me, most probably I misunderstood.
> > >
> > > If you want the PIDFD_THREAD behaviour, you can always use this flag
> > > without any check...
>
> Sorry, I hadn't read the patch. If it's ok to use PIDFD_THREAD on a
> leader, then we can just always specify it. (We don't care about the
> behavior of pidfd_poll().)
>
> > > Could you spell?
> >
> > Just in case, we can even add PIDFD_AUTO (modulo naming) which acts as
> > PIDFD_THREAD if the target task is not a leader or 0 (current behaviour)
> > otherwise. Trivial.
>
> Yep, or given the above, maybe it'll work as-is, thank you.
Yes, let's rather do the explicit PIDFD_THREAD.
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Christian Brauner @ 2024-01-26 10:07 UTC (permalink / raw)
To: Joe Damato
Cc: Greg Kroah-Hartman, linux-kernel, netdev, chuck.lever, jlayton,
linux-api, edumazet, davem, alexander.duyck, sridhar.samudrala,
kuba, willemdebruijn.kernel, weiwan, Jonathan Corbet,
Alexander Viro, Jan Kara, Michael Ellerman, Nathan Lynch,
Steve French, Thomas Zimmermann, Jiri Slaby, Julien Panis,
Arnd Bergmann, Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240126023630.GA1235@fastly.com>
On Thu, Jan 25, 2024 at 06:36:30PM -0800, Joe Damato wrote:
> On Thu, Jan 25, 2024 at 04:23:58PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:
> > > On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > > > > +struct epoll_params {
> > > > > + u64 busy_poll_usecs;
> > > > > + u16 busy_poll_budget;
> > > > > +
> > > > > + /* for future fields */
> > > > > + u8 data[118];
> > > > > +} EPOLL_PACKED;
> > > >
> > > > variables that cross the user/kernel boundry need to be __u64, __u16,
> > > > and __u8 here.
> > >
> > > I'll make that change for the next version, thank you.
> > >
> > > > And why 118?
> > >
> > > I chose this arbitrarily. I figured that a 128 byte struct would support 16
> > > u64s in the event that other fields needed to be added in the future. 118
> > > is what was left after the existing fields. There's almost certainly a
> > > better way to do this - or perhaps it is unnecessary as per your other
> > > message.
> > >
> > > I am not sure if leaving extra space in the struct is a recommended
> > > practice for ioctls or not - I thought I noticed some code that did and
> > > some that didn't in the kernel so I err'd on the side of leaving the space
> > > and probably did it in the worst way possible.
> >
> > It's not really a good idea unless you know exactly what you are going
> > to do with it. Why not just have a new ioctl if you need new
> > information in the future? That's simpler, right?
>
> Sure, that makes sense to me. I'll remove it in the v4 alongside the other
> changes you've requested.
Fwiw, we do support extensible ioctls since they encode the size. Take a
look at kernel/seccomp.c. It's a clean extensible interface built on top
of the copy_struct_from_user() pattern we added for system calls
(openat(), clone3() etc.):
static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
struct seccomp_filter *filter = file->private_data;
void __user *buf = (void __user *)arg;
/* Fixed-size ioctls */
switch (cmd) {
case SECCOMP_IOCTL_NOTIF_RECV:
return seccomp_notify_recv(filter, buf);
case SECCOMP_IOCTL_NOTIF_SEND:
return seccomp_notify_send(filter, buf);
case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR:
case SECCOMP_IOCTL_NOTIF_ID_VALID:
return seccomp_notify_id_valid(filter, buf);
case SECCOMP_IOCTL_NOTIF_SET_FLAGS:
return seccomp_notify_set_flags(filter, arg);
}
/* Extensible Argument ioctls */
#define EA_IOCTL(cmd) ((cmd) & ~(IOC_INOUT | IOCSIZE_MASK))
switch (EA_IOCTL(cmd)) {
case EA_IOCTL(SECCOMP_IOCTL_NOTIF_ADDFD):
return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));
default:
return -EINVAL;
}
}
static long seccomp_notify_addfd(struct seccomp_filter *filter,
struct seccomp_notif_addfd __user *uaddfd,
unsigned int size)
{
struct seccomp_notif_addfd addfd;
struct seccomp_knotif *knotif;
struct seccomp_kaddfd kaddfd;
int ret;
BUILD_BUG_ON(sizeof(addfd) < SECCOMP_NOTIFY_ADDFD_SIZE_VER0);
BUILD_BUG_ON(sizeof(addfd) != SECCOMP_NOTIFY_ADDFD_SIZE_LATEST);
if (size < SECCOMP_NOTIFY_ADDFD_SIZE_VER0 || size >= PAGE_SIZE)
return -EINVAL;
ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
if (ret)
return ret;
^ permalink raw reply
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Lance Yang @ 2024-01-26 10:15 UTC (permalink / raw)
To: akpm, Michal Hocko, Zach O'Keefe, Yang Shi, David Hildenbrand
Cc: songmuchun, peterx, mknyszek, minchan, linux-mm, linux-kernel,
linux-api
In-Reply-To: <CAK1f24nS8MEA3wcS4za-uSp7ZBxvd+xqMRf8-u=m5uCvTs8yJQ@mail.gmail.com>
I would like to correct the information provided in my previous
email and also provide some additional information.
On Fri, Jan 26, 2024 at 2:16 PM Lance Yang <ioworker0@gmail.com> wrote:
>
> I’d like to add another real use case.
>
> In our company, we deploy applications using offline-online
> hybrid deployment. This approach leverages the distinctive
> resource utilization patterns of online services, utilizing idle
> resources during various time periods by filling them with
> offline jobs. This helps reduce the growing cost expenditures
> for the enterprise.
>
> Whether for online services or offline jobs, their requirements
> for THP can be roughly categorized into three types:
>
> * The first type aims to use huge pages as much as possible
> and tolerates unpredictable stalls caused by direct reclaim
> and/or compaction.
> * The second type attempts to use huge pages but is relatively
> latency-sensitive and cannot tolerate unpredictable stalls.
> * The third type prefers not to use huge pages at all and is
> extremely latency-sensitive.
>
> After careful consideration, we decided to prioritize the
> requirements of the first type and modify the THP settings
> as follows:
>
> echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> echo defer >/sys/kernel/mm/transparent_hugepage/defrag
>
> With the introduction of MADV_COLLAPSE into the kernel,
> it is no longer dependent on any sysfs setting under
> /sys/kernel/mm/transparent_hugepage. MADV_COLLAPSE
> offers the potential for fine-grained synchronous control over
> the huge page allocation mechanism, marking a significant
> enhancement for THP.
>
> If the kernel supports a more relaxed (opportunistic)
> MADV_COLLAPSE, we will modify the THP settings as follows:
>
> echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
The correct THP settings should be:
echo always >/sys/kernel/mm/transparent_hugepage/enabled
echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
>
> Then, we will use process_madvise(MADV_COLLAPSE, xx_relaxed_flag)
> to address the requirements of the second type.
>
> Why don't we favor madvise(MADV_COLLAPSE) for the first type
> of requirements?
> The main reason is that these requirements are typically for offline
> jobs in the Hadoop ecosystem, such as MapReduce and Spark,
> which run primarily on the JVM. IIRC, the JVM currently does not
> support madvise(MADV_COLLAPSE). The second type of
To add, there are also some offline jobs that rely on PyTorch for
machine learning model training tasks. IIRC, PyTorch also does
not support madvise(MADV_COLLAPSE).
Thanks,
Lance
> requirements is all for our in-house developed online services.
> For us, integrating a more relaxed (opportunistic)
> MADV_COLLAPSE into our online services is relatively
> straightforward.
>
> By introducing various flags to MADV_COLLAPSE, we can offer
> multiple synchronous allocation strategies for applications. This
> fine-grained control may be more suitable for cloud-native
> environments than the widespread settings under
> /sys/kernel/mm/transparent_hugepage in sysfs.
>
> Thanks for your time!
> Lance
>
> On Sun, Jan 21, 2024 at 11:12 AM Lance Yang <ioworker0@gmail.com> wrote:
> >
> > Hello Everyone,
> >
> > For applications actively utilizing THP, the defrag mode may
> > not be a very user-friendly design. Here are the reasons:
> > 1. Before marking the address space with
> > MADV_HUGEPAGE,it is necessary to check if
> > the current configuration of the defrag mode aligns with
> > their preferences.
> > 2. Once the defrag mode configuration changes, these
> > applications may face the risk of unpredictable stalls.
> >
> > THP is an important feature of the Linux kernel that can
> > significantly enhance memory access performance.
> > However, due to the lack of fine-grained control over
> > the huge page allocation strategy, many applications
> > default to not using huge pages and even recommend
> > users to disable THP. This situation is regrettable.
> >
> > With the introduction of MADV_COLLAPSE into the kernel,
> > it is not affected by the defrag mode.
> > MADV_COLLAPSE offers the potential for
> > fine-grained synchronous control over the huge page
> > allocation mechanism, marking a significant enhancement
> > for THP.
> >
> > By adding flags to MADV_COLLAPSE, different
> > synchronous allocation strategies can be provided to
> > applications. This can instill confidence in them, allowing
> > them to reconsider using THP and allocate huge pages
> > according to their desired synchronous allocation strategy,
> > without worrying about the defrag mode configuration.
> >
> > BR,
> > Lance
> >
> >
> > On Thu, Jan 18, 2024 at 8:03 PM Lance Yang <ioworker0@gmail.com> wrote:
> > >
> > > This idea was inspired by MADV_COLLAPSE introduced by Zach O'Keefe[1].
> > >
> > > Allow MADV_F_COLLAPSE_LIGHT behavior for process_madvise(2) if the caller
> > > has CAP_SYS_ADMIN or is requesting the collapse of its own memory.
> > >
> > > The semantics of MADV_F_COLLAPSE_LIGHT are similar to MADV_COLLAPSE, but
> > > it avoids direct reclaim and/or compaction, quickly failing on allocation
> > > errors.
> > >
> > > This change enables a more flexible and efficient usage of memory collapse
> > > operations, providing additional control to userspace applications for
> > > system-wide THP optimization.
> > >
> > > Semantics
> > >
> > > This call is independent of the system-wide THP sysfs settings, but will
> > > fail for memory marked VM_NOHUGEPAGE. If the ranges provided span
> > > multiple VMAs, the semantics of the collapse over each VMA is independent
> > > from the others. This implies a hugepage cannot cross a VMA boundary. If
> > > collapse of a given hugepage-aligned/sized region fails, the operation may
> > > continue to attempt collapsing the remainder of memory specified.
> > >
> > > The memory ranges provided must be page-aligned, but are not required to
> > > be hugepage-aligned. If the memory ranges are not hugepage-aligned, the
> > > start/end of the range will be clamped to the first/last hugepage-aligned
> > > address covered by said range. The memory ranges must span at least one
> > > hugepage-sized region.
> > >
> > > All non-resident pages covered by the range will first be
> > > swapped/faulted-in, before being internally copied onto a freshly
> > > allocated hugepage. Unmapped pages will have their data directly
> > > initialized to 0 in the new hugepage. However, for every eligible
> > > hugepage aligned/sized region to-be collapsed, at least one page must
> > > currently be backed by memory (a PMD covering the address range must
> > > already exist).
> > >
> > > Allocation for the new hugepage will not enter direct reclaim and/or
> > > compaction, quickly failing if allocation fails. When the system has
> > > multiple NUMA nodes, the hugepage will be allocated from the node providing
> > > the most native pages. This operation operates on the current state of the
> > > specified process and makes no persistent changes or guarantees on how pages
> > > will be mapped, constructed, or faulted in the future.
> > >
> > > Use Cases
> > >
> > > An immediate user of this new functionality is the Go runtime heap allocator
> > > that manages memory in hugepage-sized chunks. In the past, whether it was a
> > > newly allocated chunk through mmap() or a reused chunk released by
> > > madvise(MADV_DONTNEED), the allocator attempted to eagerly back memory with
> > > huge pages using madvise(MADV_HUGEPAGE)[2] and madvise(MADV_COLLAPSE)[3]
> > > respectively. However, both approaches resulted in performance issues; for
> > > both scenarios, there could be entries into direct reclaim and/or compaction,
> > > leading to unpredictable stalls[4]. Now, the allocator can confidently use
> > > process_madvise(MADV_F_COLLAPSE_LIGHT) to attempt the allocation of huge pages.
> > >
> > > [1] https://github.com/torvalds/linux/commit/7d8faaf155454f8798ec56404faca29a82689c77
> > > [2] https://github.com/golang/go/commit/8fa9e3beee8b0e6baa7333740996181268b60a3a
> > > [3] https://github.com/golang/go/commit/9f9bb26880388c5bead158e9eca3be4b3a9bd2af
> > > [4] https://github.com/golang/go/issues/63334
> > >
> > > [v1] https://lore.kernel.org/lkml/20240117050217.43610-1-ioworker0@gmail.com/
> > >
> > > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > > Suggested-by: Zach O'Keefe <zokeefe@google.com>
> > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > ---
> > > V1 -> V2: Treat process_madvise(MADV_F_COLLAPSE_LIGHT) as the lighter-weight alternative
> > > to madvise(MADV_COLLAPSE)
> > >
> > > arch/alpha/include/uapi/asm/mman.h | 1 +
> > > arch/mips/include/uapi/asm/mman.h | 1 +
> > > arch/parisc/include/uapi/asm/mman.h | 1 +
> > > arch/xtensa/include/uapi/asm/mman.h | 1 +
> > > include/linux/huge_mm.h | 5 +--
> > > include/uapi/asm-generic/mman-common.h | 1 +
> > > mm/khugepaged.c | 15 ++++++--
> > > mm/madvise.c | 36 +++++++++++++++++---
> > > tools/include/uapi/asm-generic/mman-common.h | 1 +
> > > 9 files changed, 52 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> > > index 763929e814e9..22f23ca04f1a 100644
> > > --- a/arch/alpha/include/uapi/asm/mman.h
> > > +++ b/arch/alpha/include/uapi/asm/mman.h
> > > @@ -77,6 +77,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
> > > index c6e1fc77c996..acec0b643e9c 100644
> > > --- a/arch/mips/include/uapi/asm/mman.h
> > > +++ b/arch/mips/include/uapi/asm/mman.h
> > > @@ -104,6 +104,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
> > > index 68c44f99bc93..812029c98cd7 100644
> > > --- a/arch/parisc/include/uapi/asm/mman.h
> > > +++ b/arch/parisc/include/uapi/asm/mman.h
> > > @@ -71,6 +71,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > #define MADV_HWPOISON 100 /* poison a page for testing */
> > > #define MADV_SOFT_OFFLINE 101 /* soft offline page for testing */
> > > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
> > > index 1ff0c858544f..52ef463dd5b6 100644
> > > --- a/arch/xtensa/include/uapi/asm/mman.h
> > > +++ b/arch/xtensa/include/uapi/asm/mman.h
> > > @@ -112,6 +112,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > > index 5adb86af35fc..075fdb5d481a 100644
> > > --- a/include/linux/huge_mm.h
> > > +++ b/include/linux/huge_mm.h
> > > @@ -303,7 +303,7 @@ int hugepage_madvise(struct vm_area_struct *vma, unsigned long *vm_flags,
> > > int advice);
> > > int madvise_collapse(struct vm_area_struct *vma,
> > > struct vm_area_struct **prev,
> > > - unsigned long start, unsigned long end);
> > > + unsigned long start, unsigned long end, int behavior);
> > > void vma_adjust_trans_huge(struct vm_area_struct *vma, unsigned long start,
> > > unsigned long end, long adjust_next);
> > > spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma);
> > > @@ -450,7 +450,8 @@ static inline int hugepage_madvise(struct vm_area_struct *vma,
> > >
> > > static inline int madvise_collapse(struct vm_area_struct *vma,
> > > struct vm_area_struct **prev,
> > > - unsigned long start, unsigned long end)
> > > + unsigned long start, unsigned long end,
> > > + int behavior)
> > > {
> > > return -EINVAL;
> > > }
> > > diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> > > index 6ce1f1ceb432..92c67bc755da 100644
> > > --- a/include/uapi/asm-generic/mman-common.h
> > > +++ b/include/uapi/asm-generic/mman-common.h
> > > @@ -78,6 +78,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > index 2b219acb528e..2840051c0ae2 100644
> > > --- a/mm/khugepaged.c
> > > +++ b/mm/khugepaged.c
> > > @@ -97,6 +97,8 @@ static struct kmem_cache *mm_slot_cache __ro_after_init;
> > > struct collapse_control {
> > > bool is_khugepaged;
> > >
> > > + int behavior;
> > > +
> > > /* Num pages scanned per node */
> > > u32 node_load[MAX_NUMNODES];
> > >
> > > @@ -1058,10 +1060,16 @@ static int __collapse_huge_page_swapin(struct mm_struct *mm,
> > > static int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
> > > struct collapse_control *cc)
> > > {
> > > - gfp_t gfp = (cc->is_khugepaged ? alloc_hugepage_khugepaged_gfpmask() :
> > > - GFP_TRANSHUGE);
> > > int node = hpage_collapse_find_target_node(cc);
> > > struct folio *folio;
> > > + gfp_t gfp;
> > > +
> > > + if (cc->is_khugepaged)
> > > + gfp = alloc_hugepage_khugepaged_gfpmask();
> > > + else
> > > + gfp = (cc->behavior == MADV_F_COLLAPSE_LIGHT ?
> > > + GFP_TRANSHUGE_LIGHT :
> > > + GFP_TRANSHUGE);
> > >
> > > if (!hpage_collapse_alloc_folio(&folio, gfp, node, &cc->alloc_nmask)) {
> > > *hpage = NULL;
> > > @@ -2697,7 +2705,7 @@ static int madvise_collapse_errno(enum scan_result r)
> > > }
> > >
> > > int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > > - unsigned long start, unsigned long end)
> > > + unsigned long start, unsigned long end, int behavior)
> > > {
> > > struct collapse_control *cc;
> > > struct mm_struct *mm = vma->vm_mm;
> > > @@ -2718,6 +2726,7 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > > if (!cc)
> > > return -ENOMEM;
> > > cc->is_khugepaged = false;
> > > + cc->behavior = behavior;
> > >
> > > mmgrab(mm);
> > > lru_add_drain_all();
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 912155a94ed5..9c40226505aa 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -60,6 +60,7 @@ static int madvise_need_mmap_write(int behavior)
> > > case MADV_POPULATE_READ:
> > > case MADV_POPULATE_WRITE:
> > > case MADV_COLLAPSE:
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > return 0;
> > > default:
> > > /* be safe, default to 1. list exceptions explicitly */
> > > @@ -1082,8 +1083,9 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> > > if (error)
> > > goto out;
> > > break;
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > case MADV_COLLAPSE:
> > > - return madvise_collapse(vma, prev, start, end);
> > > + return madvise_collapse(vma, prev, start, end, behavior);
> > > }
> > >
> > > anon_name = anon_vma_name(vma);
> > > @@ -1178,6 +1180,7 @@ madvise_behavior_valid(int behavior)
> > > case MADV_HUGEPAGE:
> > > case MADV_NOHUGEPAGE:
> > > case MADV_COLLAPSE:
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > #endif
> > > case MADV_DONTDUMP:
> > > case MADV_DODUMP:
> > > @@ -1194,6 +1197,17 @@ madvise_behavior_valid(int behavior)
> > > }
> > > }
> > >
> > > +
> > > +static bool process_madvise_behavior_only(int behavior)
> > > +{
> > > + switch (behavior) {
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > + return true;
> > > + default:
> > > + return false;
> > > + }
> > > +}
> > > +
> > > static bool process_madvise_behavior_valid(int behavior)
> > > {
> > > switch (behavior) {
> > > @@ -1201,6 +1215,7 @@ static bool process_madvise_behavior_valid(int behavior)
> > > case MADV_PAGEOUT:
> > > case MADV_WILLNEED:
> > > case MADV_COLLAPSE:
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > return true;
> > > default:
> > > return false;
> > > @@ -1368,6 +1383,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > > * transparent huge pages so the existing pages will not be
> > > * coalesced into THP and new pages will not be allocated as THP.
> > > * MADV_COLLAPSE - synchronously coalesce pages into new THP.
> > > + * MADV_F_COLLAPSE_LIGHT - only for process_madvise, avoids direct reclaim and/or
> > > + * compaction.
> > > * MADV_DONTDUMP - the application wants to prevent pages in the given range
> > > * from being included in its core dump.
> > > * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > > @@ -1394,7 +1411,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > > * -EBADF - map exists, but area maps something that isn't a file.
> > > * -EAGAIN - a kernel resource was temporarily unavailable.
> > > */
> > > -int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> > > +int _do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in,
> > > + int behavior, bool is_process_madvise)
> > > {
> > > unsigned long end;
> > > int error;
> > > @@ -1405,6 +1423,9 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > > if (!madvise_behavior_valid(behavior))
> > > return -EINVAL;
> > >
> > > + if (!is_process_madvise && process_madvise_behavior_only(behavior))
> > > + return -EINVAL;
> > > +
> > > if (!PAGE_ALIGNED(start))
> > > return -EINVAL;
> > > len = PAGE_ALIGN(len_in);
> > > @@ -1448,9 +1469,14 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > > return error;
> > > }
> > >
> > > +int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> > > +{
> > > + return _do_madvise(mm, start, len_in, behavior, false);
> > > +}
> > > +
> > > SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > > {
> > > - return do_madvise(current->mm, start, len_in, behavior);
> > > + return _do_madvise(current->mm, start, len_in, behavior, false);
> > > }
> > >
> > > SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > > @@ -1504,8 +1530,8 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > > total_len = iov_iter_count(&iter);
> > >
> > > while (iov_iter_count(&iter)) {
> > > - ret = do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
> > > - iter_iov_len(&iter), behavior);
> > > + ret = _do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
> > > + iter_iov_len(&iter), behavior, true);
> > > if (ret < 0)
> > > break;
> > > iov_iter_advance(&iter, iter_iov_len(&iter));
> > > diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
> > > index 6ce1f1ceb432..92c67bc755da 100644
> > > --- a/tools/include/uapi/asm-generic/mman-common.h
> > > +++ b/tools/include/uapi/asm-generic/mman-common.h
> > > @@ -78,6 +78,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > --
> > > 2.33.1
> > >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox