* [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 2/4] mm/mempolicy: refactor a read-once mechanism into a function for re-use
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>
move the use of barrier() to force policy->nodemask onto the stack into
a function `read_once_policy_nodemask` so that it may be re-used.
Suggested-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
mm/mempolicy.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index f1627d45b0c8..b13c45a0bfcb 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1894,6 +1894,20 @@ unsigned int mempolicy_slab_node(void)
}
}
+static unsigned int read_once_policy_nodemask(struct mempolicy *pol,
+ nodemask_t *mask)
+{
+ /*
+ * barrier stabilizes the nodemask locally so that it can be iterated
+ * over safely without concern for changes. Allocators validate node
+ * selection does not violate mems_allowed, so this is safe.
+ */
+ barrier();
+ memcpy(mask, &pol->nodes, sizeof(nodemask_t));
+ barrier();
+ return nodes_weight(*mask);
+}
+
/*
* Do static interleaving for interleave index @ilx. Returns the ilx'th
* node in pol->nodes (starting from ilx=0), wrapping around if ilx
@@ -1901,20 +1915,12 @@ unsigned int mempolicy_slab_node(void)
*/
static unsigned int interleave_nid(struct mempolicy *pol, pgoff_t ilx)
{
- nodemask_t nodemask = pol->nodes;
+ nodemask_t nodemask;
unsigned int target, nnodes;
int i;
int nid;
- /*
- * The barrier will stabilize the nodemask in a register or on
- * the stack so that it will stop changing under the code.
- *
- * Between first_node() and next_node(), pol->nodes could be changed
- * by other threads. So we put pol->nodes in a local stack.
- */
- barrier();
- nnodes = nodes_weight(nodemask);
+ nnodes = read_once_policy_nodemask(pol, &nodemask);
if (!nnodes)
return numa_node_id();
target = ilx % nnodes;
--
2.39.1
^ permalink raw reply related
* [PATCH v3 1/4] mm/mempolicy: implement the sysfs-based weighted_interleave interface
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>
From: Rakie Kim <rakie.kim@sk.com>
This patch provides a way to set interleave weight information under
sysfs at /sys/kernel/mm/mempolicy/weighted_interleave/nodeN
The sysfs structure is designed as follows.
$ tree /sys/kernel/mm/mempolicy/
/sys/kernel/mm/mempolicy/ [1]
└── weighted_interleave [2]
├── node0 [3]
└── node1
Each file above can be explained as follows.
[1] mm/mempolicy: configuration interface for mempolicy subsystem
[2] weighted_interleave/: config interface for weighted interleave policy
[3] weighted_interleave/nodeN: weight for nodeN
If a node value is set to `0`, the system-default value will be used.
As of this patch, the system-default for all nodes is always 1.
Suggested-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Rakie Kim <rakie.kim@sk.com>
Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Co-developed-by: Gregory Price <gregory.price@memverge.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Co-developed-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Signed-off-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
---
.../ABI/testing/sysfs-kernel-mm-mempolicy | 4 +
...fs-kernel-mm-mempolicy-weighted-interleave | 25 ++
mm/mempolicy.c | 224 ++++++++++++++++++
3 files changed, 253 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
new file mode 100644
index 000000000000..2dcf24f4384a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
@@ -0,0 +1,4 @@
+What: /sys/kernel/mm/mempolicy/
+Date: December 2023
+Contact: Linux memory management mailing list <linux-mm@kvack.org>
+Description: Interface for Mempolicy
diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
new file mode 100644
index 000000000000..0062b02703ff
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
@@ -0,0 +1,25 @@
+What: /sys/kernel/mm/mempolicy/weighted_interleave/
+Date: January 2024
+Contact: Linux memory management mailing list <linux-mm@kvack.org>
+Description: Configuration Interface for the Weighted Interleave policy
+
+What: /sys/kernel/mm/mempolicy/weighted_interleave/nodeN
+Date: January 2024
+Contact: Linux memory management mailing list <linux-mm@kvack.org>
+Description: Weight configuration interface for nodeN
+
+ The interleave weight for a memory node (N). These weights are
+ utilized by taskss which have set their mempolicy to
+ MPOL_WEIGHTED_INTERLEAVE.
+
+ These weights only affect new allocations, and changes at runtime
+ will not cause migrations on already allocated pages.
+
+ The minimum weight for a node is always 1.
+
+ Minimum weight: 1
+ Maximum weight: 255
+
+ Writing an empty string or `0` will reset the weight to the
+ system default. The system default may be set by the kernel
+ or drivers at boot or during hotplug events.
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 10a590ee1c89..f1627d45b0c8 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -131,6 +131,17 @@ static struct mempolicy default_policy = {
static struct mempolicy preferred_node_policy[MAX_NUMNODES];
+/*
+ * iw_table is the sysfs-set interleave weight table, a value of 0 denotes
+ * system-default value should be used. A NULL iw_table also denotes that
+ * system-default values should be used. Until the system-default table
+ * is implemented, the system-default is always 1.
+ *
+ * iw_table is RCU protected
+ */
+static u8 __rcu *iw_table;
+static DEFINE_MUTEX(iw_table_lock);
+
/**
* numa_nearest_node - Find nearest node by state
* @node: Node id to start the search
@@ -3067,3 +3078,216 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
nodemask_pr_args(&nodes));
}
+
+#ifdef CONFIG_SYSFS
+struct iw_node_attr {
+ struct kobj_attribute kobj_attr;
+ int nid;
+};
+
+static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct iw_node_attr *node_attr;
+ u8 weight;
+ u8 __rcu *table;
+
+ node_attr = container_of(attr, struct iw_node_attr, kobj_attr);
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ weight = table ? table[node_attr->nid] : 1;
+ rcu_read_unlock();
+
+ return sysfs_emit(buf, "%d\n", weight);
+}
+
+static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct iw_node_attr *node_attr;
+ u8 __rcu *new;
+ u8 __rcu *old;
+ u8 weight = 0;
+
+ node_attr = container_of(attr, struct iw_node_attr, kobj_attr);
+ if (count == 0 || sysfs_streq(buf, ""))
+ weight = 0;
+ else if (kstrtou8(buf, 0, &weight))
+ return -EINVAL;
+
+ /*
+ * The default weight is 1, for now. When the kernel-internal
+ * default weight array is implemented, 0 will be a directive to
+ * the allocators to use the system-default weight instead.
+ */
+ if (!weight)
+ weight = 1;
+
+ new = kmalloc(nr_node_ids, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ mutex_lock(&iw_table_lock);
+ old = rcu_dereference_protected(iw_table,
+ lockdep_is_held(&iw_table_lock));
+ if (old)
+ memcpy(new, old, nr_node_ids);
+ else
+ memset(new, 1, nr_node_ids);
+ new[node_attr->nid] = weight;
+ rcu_assign_pointer(iw_table, new);
+ mutex_unlock(&iw_table_lock);
+ synchronize_rcu();
+ kfree(old);
+ return count;
+}
+
+static struct iw_node_attr **node_attrs;
+
+static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
+ struct kobject *parent)
+{
+ if (!node_attr)
+ return;
+ sysfs_remove_file(parent, &node_attr->kobj_attr.attr);
+ kfree(node_attr->kobj_attr.attr.name);
+ kfree(node_attr);
+}
+
+static void sysfs_wi_release(struct kobject *wi_kobj)
+{
+ int i;
+
+ for (i = 0; i < nr_node_ids; i++)
+ sysfs_wi_node_release(node_attrs[i], wi_kobj);
+ kobject_put(wi_kobj);
+}
+
+static const struct kobj_type wi_ktype = {
+ .sysfs_ops = &kobj_sysfs_ops,
+ .release = sysfs_wi_release,
+};
+
+static int add_weight_node(int nid, struct kobject *wi_kobj)
+{
+ struct iw_node_attr *node_attr;
+ char *name;
+
+ node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
+ if (!node_attr)
+ return -ENOMEM;
+
+ name = kasprintf(GFP_KERNEL, "node%d", nid);
+ if (!name) {
+ kfree(node_attr);
+ return -ENOMEM;
+ }
+
+ sysfs_attr_init(&node_attr->kobj_attr.attr);
+ node_attr->kobj_attr.attr.name = name;
+ node_attr->kobj_attr.attr.mode = 0644;
+ node_attr->kobj_attr.show = node_show;
+ node_attr->kobj_attr.store = node_store;
+ node_attr->nid = nid;
+
+ if (sysfs_create_file(wi_kobj, &node_attr->kobj_attr.attr)) {
+ kfree(node_attr->kobj_attr.attr.name);
+ kfree(node_attr);
+ pr_err("failed to add attribute to weighted_interleave\n");
+ return -ENOMEM;
+ }
+
+ node_attrs[nid] = node_attr;
+ return 0;
+}
+
+static int add_weighted_interleave_group(struct kobject *root_kobj)
+{
+ struct kobject *wi_kobj;
+ int nid, err;
+
+ wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
+ if (!wi_kobj)
+ return -ENOMEM;
+
+ err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj,
+ "weighted_interleave");
+ if (err) {
+ kfree(wi_kobj);
+ return err;
+ }
+
+ for_each_node_state(nid, N_POSSIBLE) {
+ err = add_weight_node(nid, wi_kobj);
+ if (err) {
+ pr_err("failed to add sysfs [node%d]\n", nid);
+ break;
+ }
+ }
+ if (err)
+ kobject_put(wi_kobj);
+ return 0;
+}
+
+static void mempolicy_kobj_release(struct kobject *kobj)
+{
+ u8 __rcu *old;
+
+ mutex_lock(&iw_table_lock);
+ old = rcu_dereference_protected(iw_table,
+ lockdep_is_held(&iw_table_lock));
+ rcu_assign_pointer(iw_table, NULL);
+ mutex_unlock(&iw_table_lock);
+ synchronize_rcu();
+ kfree(old);
+ kfree(node_attrs);
+ kfree(kobj);
+}
+
+static const struct kobj_type mempolicy_ktype = {
+ .release = mempolicy_kobj_release
+};
+
+static int __init mempolicy_sysfs_init(void)
+{
+ int err;
+ static struct kobject *mempolicy_kobj;
+
+ mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
+ if (!mempolicy_kobj) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+
+ node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
+ GFP_KERNEL);
+ if (!node_attrs) {
+ err = -ENOMEM;
+ goto mempol_out;
+ }
+
+ err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
+ "mempolicy");
+ if (err)
+ goto node_out;
+
+ err = add_weighted_interleave_group(mempolicy_kobj);
+ if (err) {
+ pr_err("mempolicy sysfs structure failed to initialize\n");
+ kobject_put(mempolicy_kobj);
+ return err;
+ }
+
+ return err;
+node_out:
+ kfree(node_attrs);
+mempol_out:
+ kfree(mempolicy_kobj);
+err_out:
+ pr_err("failed to add mempolicy kobject to the system\n");
+ return err;
+}
+
+late_initcall(mempolicy_sysfs_init);
+#endif /* CONFIG_SYSFS */
--
2.39.1
^ permalink raw reply related
* [PATCH v3 0/4] mm/mempolicy: weighted interleave mempolicy and sysfs extension
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,
Hasan Al Maruf, Hao Wang, Michal Hocko, Zhongkun He,
Frank van der Linden, John Groves, Jonathan Cameron, Andi Kleen
Hi Andrew - this version added a fix for a stale weight
issue that can occur on cgroup migration, and some fixups
recommended by Ying Huang. There is an additional patch
about the stale weight due to the introduction of atomics
that may warrant some explicit scrutiny before pulling.
v3: stale value fix, bulk allocator fixups, sysfs simplification
---
Weighted interleave is a new interleave policy intended to make
use of heterogeneous memory environments appearing with CXL.
The existing interleave mechanism does an even round-robin
distribution of memory across all nodes in a nodemask, while
weighted interleave distributes memory across nodes according
to a provided weight. (Weight = # of page allocations per round)
Weighted interleave is intended to reduce average latency when
bandwidth is pressured - therefore increasing total throughput.
In other words: It allows greater use of the total available
bandwidth in a heterogeneous hardware environment (different
hardware provides different bandwidth capacity).
As bandwidth is pressured, latency increases - first linearly
and then exponentially. By keeping bandwidth usage distributed
according to available bandwidth, we therefore can reduce the
average latency of a cacheline fetch.
A good explanation of the bandwidth vs latency response curve:
https://mahmoudhatem.wordpress.com/2017/11/07/memory-bandwidth-vs-latency-response-curve/
From the article:
```
Constant region:
The latency response is fairly constant for the first 40%
of the sustained bandwidth.
Linear region:
In between 40% to 80% of the sustained bandwidth, the
latency response increases almost linearly with the bandwidth
demand of the system due to contention overhead by numerous
memory requests.
Exponential region:
Between 80% to 100% of the sustained bandwidth, the memory
latency is dominated by the contention latency which can be
as much as twice the idle latency or more.
Maximum sustained bandwidth :
Is 65% to 75% of the theoretical maximum bandwidth.
```
As a general rule of thumb:
* If bandwidth usage is low, latency does not increase. It is
optimal to place data in the nearest (lowest latency) device.
* If bandwidth usage is high, latency increases. It is optimal
to place data such that bandwidth use is optimized per-device.
This is the top line goal: Provide a user a mechanism to target using
the "maximum sustained bandwidth" of each hardware component in a
heterogenous memory system.
For example, the stream benchmark demonstrates that 1:1 (default)
interleave is actively harmful, while weighted interleave can be
beneficial. Default interleave distributes data such that too much
pressure is placed on devices with lower available bandwidth.
Stream Benchmark (High level results, 1 Socket + 1 CXL Device)
Default interleave : -78% (slower than DRAM)
Global weighting : -6% to +4% (workload dependant)
Targeted weights : +2.5% to +4% (consistently better than DRAM)
Global means the task-policy was set (set_mempolicy), while targeted
means VMA policies were set (mbind2). We see weighted interleave
is not always beneficial when applied globally, but is always
beneficial when applied to bandwidth-driving memory regions.
We implement sysfs entries for "system global" weights which can be
set by a daemon or administrator.
There are 3 patches in this set:
1) Implement system-global interleave weights as sysfs extension
in mm/mempolicy.c. These weights are RCU protected, and a
default weight set is provided (all weights are 1 by default).
In future work, we intend to expose an interface for HMAT/CDAT
information to be used during boot to set reasonable system
default values based on the memory configuration of the system
discovered at boot or during device hotplug.
2) A mild refactor of some interleave-logic for re-use in the
new weighted interleave logic.
3) MPOL_WEIGHTED_INTERLEAVE extension for set_mempolicy/mbind
Included below are some performance and LTP test information,
and a sample numactl branch which can be used for testing.
= Performance summary =
(tests may have different configurations, see extended info below)
1) MLC (W2) : +38% over DRAM. +264% over default interleave.
MLC (W5) : +40% over DRAM. +226% over default interleave.
2) Stream : -6% to +4% over DRAM, +430% over default interleave.
3) XSBench : +19% over DRAM. +47% over default interleave.
= LTP Testing Summary =
existing mempolicy & mbind tests: pass
mempolicy & mbind + weighted interleave (global weights): pass
= version history
v3:
- MAJOR: Changes cur_weight to be an atomic and to carry the
current interleave node+weight, rather than just weight
this fixes a stale-weight issue when a rebind occurs.
- minor doc updates
- sysfs: remove module_exit path, not needed
- sysfs: allocate node_attrs rather than static MAX_NUMNODES array
- interleave_nodes: handle cur_weight=0 conditions explicitly
- bulk allocator: prev_node should be initialized to me->il_prev
- bulk alloactor: weight collection logic style fixes
- bulk allocator: bulk allocator for loop style fixes
- bulk allocator: corner case fixes for resume_node/weight
v2:
- MAJOR: Torture tested bulk allocator, fixed edge conditions
tracking the next me->il_node. Added documentation.
Prior version was stable, but the resulting me->il_node
could be wrong under certain circumstances.
- naming: iw_table_mtx -> iw_table_lock
- RCU: use synchronize+kfree and simplify the weight structure
- default: remove default table, since it's static for now
- sysfs setup: simplify setup, if table==NULL presume 1's
- node_store: only allocate (sizeof(u8) * nr_node_ids)
- allocators: update to deal with NULL table pointer
- read_once: __builtin_memcpy -> memcpy
- formatting
v1:
- RCU: This version protects the weight array with RCU.
- ktest fix: proper include (types.h) in uapi header
- doc: make mpol_params in docs reflect definition
- doc: mempolicy.c comments in MPOL_WEIGHTED_INTERLEAVE patch
- Dropped task-local weights and syscalls from the proposal
until affirmative use cases for task-local weights appear.
Link: https://lore.kernel.org/linux-mm/20240103224209.2541-1-gregory.price@memverge.com/
=====================================================================
Performance tests - MLC
From - Ravi Jonnalagadda <ravis.opensrc@micron.com>
Hardware: Single-socket, multiple CXL memory expanders.
Workload: W2
Data Signature: 2:1 read:write
DRAM only bandwidth (GBps): 298.8
DRAM + CXL (default interleave) (GBps): 113.04
DRAM + CXL (weighted interleave)(GBps): 412.5
Gain over DRAM only: 1.38x
Gain over default interleave: 2.64x
Workload: W5
Data Signature: 1:1 read:write
DRAM only bandwidth (GBps): 273.2
DRAM + CXL (default interleave) (GBps): 117.23
DRAM + CXL (weighted interleave)(GBps): 382.7
Gain over DRAM only: 1.4x
Gain over default interleave: 2.26x
=====================================================================
Performance test - Stream
From - Gregory Price <gregory.price@memverge.com>
Hardware: Single socket, single CXL expander
numactl extension: https://github.com/gmprice/numactl/tree/weighted_interleave_master
Summary: 64 threads, ~18GB workload, 3GB per array, executed 100 times
Default interleave : -78% (slower than DRAM)
Global weighting : -6% to +4% (workload dependant)
mbind2 weights : +2.5% to +4% (consistently better than DRAM)
dram only:
numactl --cpunodebind=1 --membind=1 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Function Direction BestRateMBs AvgTime MinTime MaxTime
Copy: 0->0 200923.2 0.032662 0.031853 0.033301
Scale: 0->0 202123.0 0.032526 0.031664 0.032970
Add: 0->0 208873.2 0.047322 0.045961 0.047884
Triad: 0->0 208523.8 0.047262 0.046038 0.048414
CXL-only:
numactl --cpunodebind=1 -w --membind=2 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Copy: 0->0 22209.7 0.288661 0.288162 0.289342
Scale: 0->0 22288.2 0.287549 0.287147 0.288291
Add: 0->0 24419.1 0.393372 0.393135 0.393735
Triad: 0->0 24484.6 0.392337 0.392083 0.394331
Based on the above, the optimal weights are ~9:1
echo 9 > /sys/kernel/mm/mempolicy/weighted_interleave/node1
echo 1 > /sys/kernel/mm/mempolicy/weighted_interleave/node2
default interleave:
numactl --cpunodebind=1 --interleave=1,2 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Copy: 0->0 44666.2 0.143671 0.143285 0.144174
Scale: 0->0 44781.6 0.143256 0.142916 0.143713
Add: 0->0 48600.7 0.197719 0.197528 0.197858
Triad: 0->0 48727.5 0.197204 0.197014 0.197439
global weighted interleave:
numactl --cpunodebind=1 -w --interleave=1,2 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Copy: 0->0 190085.9 0.034289 0.033669 0.034645
Scale: 0->0 207677.4 0.031909 0.030817 0.033061
Add: 0->0 202036.8 0.048737 0.047516 0.053409
Triad: 0->0 217671.5 0.045819 0.044103 0.046755
targted regions w/ global weights (modified stream to mbind2 malloc'd regions))
numactl --cpunodebind=1 --membind=1 ./stream_c.exe -b --ntimes 100 --array-size 400M --malloc
Copy: 0->0 205827.0 0.031445 0.031094 0.031984
Scale: 0->0 208171.8 0.031320 0.030744 0.032505
Add: 0->0 217352.0 0.045087 0.044168 0.046515
Triad: 0->0 216884.8 0.045062 0.044263 0.046982
=====================================================================
Performance tests - XSBench
From - Hyeongtak Ji <hyeongtak.ji@sk.com>
Hardware: Single socket, Single CXL memory Expander
NUMA node 0: 56 logical cores, 128 GB memory
NUMA node 2: 96 GB CXL memory
Threads: 56
Lookups: 170,000,000
Summary: +19% over DRAM. +47% over default interleave.
Performance tests - XSBench
1. dram only
$ numactl -m 0 ./XSBench -s XL –p 5000000
Runtime: 36.235 seconds
Lookups/s: 4,691,618
2. default interleave
$ numactl –i 0,2 ./XSBench –s XL –p 5000000
Runtime: 55.243 seconds
Lookups/s: 3,077,293
3. weighted interleave
numactl –w –i 0,2 ./XSBench –s XL –p 5000000
Runtime: 29.262 seconds
Lookups/s: 5,809,513
=====================================================================
LTP Tests: https://github.com/gmprice/ltp/tree/mempolicy2
= Existing tests
set_mempolicy, get_mempolicy, mbind
MPOL_WEIGHTED_INTERLEAVE added manually to test basic functionality
but did not adjust tests for weighting. Basically the weights were
set to 1, which is the default, and it should behavior like standard
MPOL_INTERLEAVE if logic is correct.
== set_mempolicy01 : passed 18, failed 0
== set_mempolicy02 : passed 10, failed 0
== set_mempolicy03 : passed 64, failed 0
== set_mempolicy04 : passed 32, failed 0
== set_mempolicy05 - n/a on non-x86
== set_mempolicy06 : passed 10, failed 0
this is set_mempolicy02 + MPOL_WEIGHTED_INTERLEAVE
== set_mempolicy07 : passed 32, failed 0
set_mempolicy04 + MPOL_WEIGHTED_INTERLEAVE
== get_mempolicy01 : passed 12, failed 0
change: added MPOL_WEIGHTED_INTERLEAVE
== get_mempolicy02 : passed 2, failed 0
== mbind01 : passed 15, failed 0
added MPOL_WEIGHTED_INTERLEAVE
== mbind02 : passed 4, failed 0
added MPOL_WEIGHTED_INTERLEAVE
== mbind03 : passed 16, failed 0
added MPOL_WEIGHTED_INTERLEAVE
== mbind04 : passed 48, failed 0
added MPOL_WEIGHTED_INTERLEAVE
=====================================================================
numactl (set_mempolicy) w/ global weighting test
numactl fork: https://github.com/gmprice/numactl/tree/weighted_interleave_master
command: numactl -w --interleave=0,1 ./eatmem
result (weights 1:1):
0176a000 weighted interleave:0-1 heap anon=65793 dirty=65793 active=0 N0=32897 N1=32896 kernelpagesize_kB=4
7fceeb9ff000 weighted interleave:0-1 anon=65537 dirty=65537 active=0 N0=32768 N1=32769 kernelpagesize_kB=4
50% distribution is correct
result (weights 5:1):
01b14000 weighted interleave:0-1 heap anon=65793 dirty=65793 active=0 N0=54828 N1=10965 kernelpagesize_kB=4
7f47a1dff000 weighted interleave:0-1 anon=65537 dirty=65537 active=0 N0=54614 N1=10923 kernelpagesize_kB=4
16.666% distribution is correct
result (weights 1:5):
01f07000 weighted interleave:0-1 heap anon=65793 dirty=65793 active=0 N0=10966 N1=54827 kernelpagesize_kB=4
7f17b1dff000 weighted interleave:0-1 anon=65537 dirty=65537 active=0 N0=10923 N1=54614 kernelpagesize_kB=4
16.666% distribution is correct
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main (void)
{
char* mem = malloc(1024*1024*256);
memset(mem, 1, 1024*1024*256);
for (int i = 0; i < ((1024*1024*256)/4096); i++)
{
mem = malloc(4096);
mem[0] = 1;
}
printf("done\n");
getchar();
return 0;
}
=====================================================================
Suggested-by: Gregory Price <gregory.price@memverge.com>
Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Suggested-by: Hasan Al Maruf <hasanalmaruf@fb.com>
Suggested-by: Hao Wang <haowang3@fb.com>
Suggested-by: Ying Huang <ying.huang@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Michal Hocko <mhocko@suse.com>
Suggested-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
Suggested-by: Frank van der Linden <fvdl@google.com>
Suggested-by: John Groves <john@jagalactic.com>
Suggested-by: Vinicius Tavares Petrucci <vtavarespetr@micron.com>
Suggested-by: Srinivasulu Thanneeru <sthanneeru@micron.com>
Suggested-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Suggested-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Suggested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Gregory Price (3):
mm/mempolicy: refactor a read-once mechanism into a function for
re-use
mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted
interleaving
mm/mempolicy: change cur_il_weight to atomic and carry the node with
it
Rakie Kim (1):
mm/mempolicy: implement the sysfs-based weighted_interleave interface
.../ABI/testing/sysfs-kernel-mm-mempolicy | 4 +
...fs-kernel-mm-mempolicy-weighted-interleave | 25 +
.../admin-guide/mm/numa_memory_policy.rst | 9 +
include/linux/mempolicy.h | 3 +
include/uapi/linux/mempolicy.h | 1 +
mm/mempolicy.c | 551 +++++++++++++++++-
6 files changed, 579 insertions(+), 14 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
--
2.39.1
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-25 18:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240125183045.GE5513@redhat.com>
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.
Tycho
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-25 18:30 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240125182505.GD5513@redhat.com>
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...
>
> 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.
Oleg.
^ permalink raw reply
* Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-01-25 18:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel, linux-api, Arnd Bergmann
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <d8631ec7-046e-4ef7-a1ff-71e4ecebe706@app.fastmail.com>
On Thursday, 25 January 2024 11:02:26 CST Arnd Bergmann wrote:
> On Wed, Jan 24, 2024, at 23:28, Elizabeth Figura wrote:
> > On Wednesday, 24 January 2024 13:52:52 CST Arnd Bergmann wrote:
> >> On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote:
> >> > That'd be nicer in general. I think there was some documentation that
> >> > advised using timespec64 for new ioctl interfaces but it may have been
> >> > outdated or misread.
> >>
> >> It's probably something I wrote. It depends a bit on
> >> whether you have an absolute or relative timeout. If
> >> the timeout is relative to the current time as I understand
> >> it is here, a 64-bit number seems more logical to me.
> >>
> >> For absolute times, I would usually use a __kernel_timespec,
> >> especially if it's CLOCK_REALTIME. In this case you would
> >> also need to specify the time domain.
> >
> > Currently the interface does pass it as an absolute time, with the
> > domain implicitly being MONOTONIC. This particular choice comes from
> > process/botching-up-ioctls.rst, which is admittedly focused around GPU
> > ioctls, but the rationale of having easily restartable ioctls applies
> > here too.
>
> Ok, I was thinking of Documentation/driver-api/ioctl.rst, which
> has similar recommendations.
>
> > (E.g. Wine does play games with signals, so we do want to be able to
> > interrupt arbitrary waits with EINTR. The "usual" fast path for ntsync
> > waits won't hit that, but we want to have it work.)
> >
> > On the other hand, if we can pass the timeout as relative, and write it
> > back on exit like ppoll() does [assuming that's not proscribed], that
> > would presumably be slightly better for performance.
>
> I've seen arguments go either way between absolute and relative
> times, just pick whatever works best for you here.
>
> > When writing the patch I just picked the recommended option, and didn't
> > bother doing any micro-optimizations afterward.
> >
> > What's the rationale for using timespec for absolute or written-back
> > timeouts, instead of dealing in ns directly? I'm afraid it's not
> > obvious to me.
>
> There is no hard rule either way, I mainly didn't like the
> indirect pointer to the timespec that you have here. For
> traditional unix-style interfaces, a timespec with CLOCK_REALTIME
> times usually makes sense since that is what user space is
> already using elsewhere, but you probably don't need to
> worry about that. In theory, the single u64 CLOCK_REALTIME
> nanoseconds have the problem of no longer working after year
> 2262, but with CLOCK_MONOTONIC that is not a concern anyway.
>
> Between embedding a __u64 nanosecond value and embedding
> a __kernel_timespec, I would pick whichever avoids converting
> a __u64 back into a timespec, as that is an expensive
> operation at least on 32-bit code.
Makes sense. I'll probably switch to using a relative and written-back u64
then, thanks!
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-25 18:25 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbKigMNQM0Yklc/5@tycho.pizza>
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...
Could you spell?
Oleg.
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Elizabeth Figura @ 2024-01-25 18:21 UTC (permalink / raw)
To: Andy Lutomirski, wine-devel, Arnd Bergmann
Cc: Greg Kroah-Hartman, linux-kernel, linux-api, André Almeida,
Wolfram Sang, Peter Zijlstra, Alexandre Julliard
In-Reply-To: <63b3828d-8482-4435-9c98-50578bbbbe07@app.fastmail.com>
On Thursday, 25 January 2024 10:47:49 CST Arnd Bergmann wrote:
> On Thu, Jan 25, 2024, at 04:42, Elizabeth Figura 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:
> >> [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 can think of a few other possible benefits of going with
> per-mutex file descriptors:
>
> - being able to use poll() for waiting on them individually in
> combination with other file descriptor based events (socket,
> signalfd, pidfd, ...)
I can say for sure this isn't going to be useful for Wine, at least not with
the current design.
It also doesn't really mesh well with the NT design in the first place.
NTSYNC_IOC_WAIT_ANY differs from poll() in two major ways: it consumes state
of most object types, and (as coded here) it needs the owner thread ID to be
specifically passed for mutexes.
Anyway, as Alexandre has informed me I clearly have misunderstood our
requirements, so I'm going to try to put together something using files
instead.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-25 18:03 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, 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:
> > > 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.
Thanks for sending your patch, I'll take a look at it (probably
tomorrow at this rate).
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.
Still, if that's the only way that makes sense, that's fine.
Tycho
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-25 17:51 UTC (permalink / raw)
To: Christian Brauner
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240125-tricksen-baugrube-3f78c487a23a@brauner>
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?
> > 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().
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"
I sent doesn't change the current behaviour.
> > 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.
Oleg.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Christian Brauner @ 2024-01-25 17:17 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>
> > 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. 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? And if it is an issue why shouldn't we
be able to change it? Because a program would rely on WNOHANG to hang on
a ptraced leader? That seems esoteric imho. I might just misunderstand.
>
> 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 and
I would expect this to cause regressions as that surely is something
that userspace relies on. Am I understand this right?
^ permalink raw reply
* Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Arnd Bergmann @ 2024-01-25 17:02 UTC (permalink / raw)
To: Elizabeth Figura, Greg Kroah-Hartman, linux-kernel, linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <8367053.NyiUUSuA9g@camazotz>
On Wed, Jan 24, 2024, at 23:28, Elizabeth Figura wrote:
> On Wednesday, 24 January 2024 13:52:52 CST Arnd Bergmann wrote:
>> On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote:
>> > That'd be nicer in general. I think there was some documentation that advised
>> > using timespec64 for new ioctl interfaces but it may have been outdated or
>> > misread.
>>
>> It's probably something I wrote. It depends a bit on
>> whether you have an absolute or relative timeout. If
>> the timeout is relative to the current time as I understand
>> it is here, a 64-bit number seems more logical to me.
>>
>> For absolute times, I would usually use a __kernel_timespec,
>> especially if it's CLOCK_REALTIME. In this case you would
>> also need to specify the time domain.
>
> Currently the interface does pass it as an absolute time, with the
> domain implicitly being MONOTONIC. This particular choice comes from
> process/botching-up-ioctls.rst, which is admittedly focused around GPU
> ioctls, but the rationale of having easily restartable ioctls applies
> here too.
Ok, I was thinking of Documentation/driver-api/ioctl.rst, which
has similar recommendations.
> (E.g. Wine does play games with signals, so we do want to be able to
> interrupt arbitrary waits with EINTR. The "usual" fast path for ntsync
> waits won't hit that, but we want to have it work.)
>
> On the other hand, if we can pass the timeout as relative, and write it
> back on exit like ppoll() does [assuming that's not proscribed], that
> would presumably be slightly better for performance.
I've seen arguments go either way between absolute and relative
times, just pick whatever works best for you here.
> When writing the patch I just picked the recommended option, and didn't
> bother doing any micro-optimizations afterward.
>
> What's the rationale for using timespec for absolute or written-back
> timeouts, instead of dealing in ns directly? I'm afraid it's not
> obvious to me.
There is no hard rule either way, I mainly didn't like the
indirect pointer to the timespec that you have here. For
traditional unix-style interfaces, a timespec with CLOCK_REALTIME
times usually makes sense since that is what user space is
already using elsewhere, but you probably don't need to
worry about that. In theory, the single u64 CLOCK_REALTIME
nanoseconds have the problem of no longer working after year
2262, but with CLOCK_MONOTONIC that is not a concern anyway.
Between embedding a __u64 nanosecond value and embedding
a __kernel_timespec, I would pick whichever avoids converting
a __u64 back into a timespec, as that is an expensive
operation at least on 32-bit code.
Arnd
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Arnd Bergmann @ 2024-01-25 16:47 UTC (permalink / raw)
To: Elizabeth Figura, Andy Lutomirski, wine-devel
Cc: Greg Kroah-Hartman, linux-kernel, linux-api, André Almeida,
Wolfram Sang, Peter Zijlstra, Alexandre Julliard
In-Reply-To: <10405963.nUPlyArG6x@terabithia>
On Thu, Jan 25, 2024, at 04:42, Elizabeth Figura 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:
>>
>> [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 can think of a few other possible benefits of going with
per-mutex file descriptors:
- being able to use poll() for waiting on them individually in
combination with other file descriptor based events (socket,
signalfd, pidfd, ...)
- replacing your logic around xarray with something a bit
simpler. As far as I can tell, your code is all correct here,
but it would be easier to understand if it looked more like
other code I'm familiar with.
> 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.
I would think that RLIMIT_NOFILE is a sensible way of
managing this, at least this way it's possible to prevent
exhausting memory with too many mutexes, but still raising
the limit if you need more than whatever default one might
come up with.
Arnd
^ permalink raw reply
* Re: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
From: Willem de Bruijn @ 2024-01-25 14:11 UTC (permalink / raw)
To: Joe Damato, Willem de Bruijn
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba, weiwan
In-Reply-To: <20240125042746.GA1294@fastly.com>
Joe Damato wrote:
> On Wed, Jan 24, 2024 at 09:46:23PM -0500, Willem de Bruijn wrote:
> > 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>
> >
> > Please be sure to include the lists and people suggested by
> > `get_maintainer.pl -f fs/eventpoll.c`.
>
> Thanks - I must have done something wrong when trying to get the maintainer
> list.
>
> Should I resend this v2? Not sure what the appropriate thing to do is in
> this case. My apologies.
If you don't get additional feedback in a few days and still prefer
this option that might be an approach.
After reading the below thread, compare the different possible APIs
and either revise the code or perhaps add a small paragraph why you
think this is the preferred path.
> > Adding ioctls is generally discouraged.
> >
> > As this affects the behavior of epoll_wait, should this just be a
> > flag to (a new variant of) epoll_wait?
>
> I have no strong preference either way. It seems to me that adding a new
> system call is a fairly significant change vs adding an ioctl, but I am
> open to whatever is preferred by the maintainers.
>
> I have no idea who would need to weigh-in to make this decision.
>
> > Speaking from some experience with adding epoll_pwait2. I initially
> > there added a stateful change that would affect wait behavior. The
> > sensible feedback as the time was to just change the behavior of the
> > syscall it affected. Even if that requires a syscall (which is not
> > that different from an ioctl, if better defined).
> >
> > The discussion in that thread may be informative to decide on API:
> > https://lwn.net/ml/linux-kernel/20201116161001.1606608-1-willemdebruijn.kernel@gmail.com/
>
> Interesting thread, thanks for sending.
>
> > Agreed on the overall principle that it is preferable to be able to
> > enable busypolling selectively. We already do for SO_BUSY_POLL and
> > sysctl busy_read.
>
> Thanks for taking a look and providing feedback.
>
> >
> > > ---
> > > .../userspace-api/ioctl/ioctl-number.rst | 1 +
> > > fs/eventpoll.c | 47 +++++++++++++++++++
> > > include/uapi/linux/eventpoll.h | 12 +++++
> > > 3 files changed, 60 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..c1ee0fe01da1 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>
> > > @@ -869,6 +871,49 @@ 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;
> > > + struct eventpoll *ep;
> > > + struct epoll_params epoll_params;
> > > + void __user *uarg = (void __user *) arg;
> > > +
> > > + if (!is_file_epoll(file))
> > > + return -EINVAL;
> > > +
> > > + ep = file->private_data;
> > > +
> > > + switch (cmd) {
> > > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > > + case EPIOCSPARAMS:
> > > + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> > > + return -EFAULT;
> > > +
> > > + if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
> > > + pr_err("busy poll budget %u exceeds suggested maximum %u\n",
> > > + epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
> > > +
> > > + 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;
> > > +#endif
> > > + 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 +1020,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
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-25 14:08 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbArN3EYRfhrNs3o@tycho.pizza>
Add Eric.
On 01/23, Tycho Andersen wrote:
>
> On Tue, Jan 23, 2024 at 08:56:08PM +0100, Oleg Nesterov wrote:
>
> > So. When do we want to do do_notify_pidfd() ? Whe the task (leader or not)
> > becomes a zombie (passes exit_notify) or when it is reaped by release_task?
>
> It seems like we'd want it when exit_notify() is called in principle,
> since that's when the pid actually dies.
No the pid "dies" after this task is reaped, until then its nr is still
in use and pid_task(PIDTYPE_PID) returns the exiting/exited task.
> 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
reaped, iow the whole thread group has exited. 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.
What if we add the new PIDFD_THREAD flag? With this flag
- sys_pidfd_open() doesn't require the must be a group leader
- pidfd_poll() succeeds when the task passes exit_notify() and
becomes a zombie, even if it is a leader and has other threads.
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
#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 related
* Re: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
From: kernel test robot @ 2024-01-25 13:12 UTC (permalink / raw)
To: Joe Damato, netdev, linux-kernel
Cc: oe-kbuild-all, chuck.lever, jlayton, linux-api, brauner, edumazet,
davem, alexander.duyck, sridhar.samudrala, kuba, weiwan,
Joe Damato
In-Reply-To: <20240125003014.43103-4-jdamato@fastly.com>
Hi Joe,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/eventpoll-support-busy-poll-per-epoll-instance/20240125-083418
base: net-next/main
patch link: https://lore.kernel.org/r/20240125003014.43103-4-jdamato%40fastly.com
patch subject: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20240125/202401252141.eUlgsF08-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240125/202401252141.eUlgsF08-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401252141.eUlgsF08-lkp@intel.com/
All warnings (new ones prefixed by >>):
fs/eventpoll.c: In function 'ep_eventpoll_ioctl':
>> fs/eventpoll.c:879:22: warning: unused variable 'uarg' [-Wunused-variable]
879 | void __user *uarg = (void __user *) arg;
| ^~~~
>> fs/eventpoll.c:878:29: warning: unused variable 'epoll_params' [-Wunused-variable]
878 | struct epoll_params epoll_params;
| ^~~~~~~~~~~~
fs/eventpoll.c:877:27: warning: variable 'ep' set but not used [-Wunused-but-set-variable]
877 | struct eventpoll *ep;
| ^~
vim +/uarg +879 fs/eventpoll.c
873
874 static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
875 {
876 int ret;
877 struct eventpoll *ep;
> 878 struct epoll_params epoll_params;
> 879 void __user *uarg = (void __user *) arg;
880
881 if (!is_file_epoll(file))
882 return -EINVAL;
883
884 ep = file->private_data;
885
886 switch (cmd) {
887 #ifdef CONFIG_NET_RX_BUSY_POLL
888 case EPIOCSPARAMS:
889 if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
890 return -EFAULT;
891
892 if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
893 pr_err("busy poll budget %u exceeds suggested maximum %u\n",
894 epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
895
896 ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
897 ep->busy_poll_budget = epoll_params.busy_poll_budget;
898 return 0;
899
900 case EPIOCGPARAMS:
901 memset(&epoll_params, 0, sizeof(epoll_params));
902 epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
903 epoll_params.busy_poll_budget = ep->busy_poll_budget;
904 if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
905 return -EFAULT;
906
907 return 0;
908 #endif
909 default:
910 ret = -EINVAL;
911 break;
912 }
913
914 return ret;
915 }
916
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [RFC PATCH 4/9] ntsync: Introduce NTSYNC_IOC_PUT_SEM.
From: Nikolay Borisov @ 2024-01-25 8:59 UTC (permalink / raw)
To: Elizabeth Figura, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel,
linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <20240124004028.16826-5-zfigura@codeweavers.com>
On 24.01.24 г. 2:40 ч., Elizabeth Figura wrote:
> This corresponds to the NT syscall NtReleaseSemaphore().
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> ---
> drivers/misc/ntsync.c | 76 +++++++++++++++++++++++++++++++++++++
> include/uapi/linux/ntsync.h | 2 +
> 2 files changed, 78 insertions(+)
>
> diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> index 3287b94be351..d1c91c2a4f1a 100644
> --- a/drivers/misc/ntsync.c
> +++ b/drivers/misc/ntsync.c
> @@ -21,9 +21,11 @@ enum ntsync_type {
> struct ntsync_obj {
> struct rcu_head rhead;
> struct kref refcount;
> + spinlock_t lock;
>
> enum ntsync_type type;
>
> + /* The following fields are protected by the object lock. */
> union {
> struct {
> __u32 count;
> @@ -36,6 +38,19 @@ struct ntsync_device {
> struct xarray objects;
> };
>
> +static struct ntsync_obj *get_obj(struct ntsync_device *dev, __u32 id)
> +{
> + struct ntsync_obj *obj;
> +
> + rcu_read_lock();
> + obj = xa_load(&dev->objects, id);
> + if (obj && !kref_get_unless_zero(&obj->refcount))
> + obj = NULL;
> + rcu_read_unlock();
> +
> + return obj;
> +}
> +
> static void destroy_obj(struct kref *ref)
> {
> struct ntsync_obj *obj = container_of(ref, struct ntsync_obj, refcount);
> @@ -48,6 +63,18 @@ static void put_obj(struct ntsync_obj *obj)
> kref_put(&obj->refcount, destroy_obj);
> }
>
> +static struct ntsync_obj *get_obj_typed(struct ntsync_device *dev, __u32 id,
> + enum ntsync_type type)
> +{
> + struct ntsync_obj *obj = get_obj(dev, id);
> +
> + if (obj && obj->type != type) {
> + put_obj(obj);
> + return NULL;
> + }
> + return obj;
> +}
> +
> static int ntsync_char_open(struct inode *inode, struct file *file)
> {
> struct ntsync_device *dev;
> @@ -81,6 +108,7 @@ static int ntsync_char_release(struct inode *inode, struct file *file)
> static void init_obj(struct ntsync_obj *obj)
> {
> kref_init(&obj->refcount);
> + spin_lock_init(&obj->lock);
> }
>
> static int ntsync_create_sem(struct ntsync_device *dev, void __user *argp)
> @@ -131,6 +159,52 @@ static int ntsync_delete(struct ntsync_device *dev, void __user *argp)
> return 0;
> }
>
> +/*
> + * Actually change the semaphore state, returning -EOVERFLOW if it is made
> + * invalid.
> + */
> +static int put_sem_state(struct ntsync_obj *sem, __u32 count)
nit: Just a general observation - those functions that contains the
specific type in their name could take the exact object i.e struct ntsem
which will make the code somewhat more clear. Of course, this would mean
that the struct definition in patch 3 should be changed to also contain
a tag name.
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Alexandre Julliard @ 2024-01-25 7:41 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Andy Lutomirski, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel,
linux-api, wine-devel, André Almeida, Wolfram Sang,
Arkadiusz Hiler, Peter Zijlstra
In-Reply-To: <5907233.BlLQTPImNI@camazotz>
Elizabeth Figura <zfigura@codeweavers.com> writes:
> 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.
I may have misunderstood something in that dicussion then, because it
would definitely be a concern. It's OK for a process to be able to mess
up the state of any object that it has an NT handle to, but it shouldn't
be possible to mess up the state of unrelated objects in other processes
simply by passing the wrong integer id.
The concern is not so much about a malicious process going out of its
way to corrupt others, because it could do that through the NT API just
as well. But if a wayward pointer corrupts the client-side handle cache,
that shouldn't take down the entire session.
--
Alexandre Julliard
julliard@winehq.org
^ permalink raw reply
* Re: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-25 4:27 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba, weiwan
In-Reply-To: <65b1cb7f73a6a_250560294bd@willemb.c.googlers.com.notmuch>
On Wed, Jan 24, 2024 at 09:46:23PM -0500, Willem de Bruijn wrote:
> 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>
>
> Please be sure to include the lists and people suggested by
> `get_maintainer.pl -f fs/eventpoll.c`.
Thanks - I must have done something wrong when trying to get the maintainer
list.
Should I resend this v2? Not sure what the appropriate thing to do is in
this case. My apologies.
> Adding ioctls is generally discouraged.
>
> As this affects the behavior of epoll_wait, should this just be a
> flag to (a new variant of) epoll_wait?
I have no strong preference either way. It seems to me that adding a new
system call is a fairly significant change vs adding an ioctl, but I am
open to whatever is preferred by the maintainers.
I have no idea who would need to weigh-in to make this decision.
> Speaking from some experience with adding epoll_pwait2. I initially
> there added a stateful change that would affect wait behavior. The
> sensible feedback as the time was to just change the behavior of the
> syscall it affected. Even if that requires a syscall (which is not
> that different from an ioctl, if better defined).
>
> The discussion in that thread may be informative to decide on API:
> https://lwn.net/ml/linux-kernel/20201116161001.1606608-1-willemdebruijn.kernel@gmail.com/
Interesting thread, thanks for sending.
> Agreed on the overall principle that it is preferable to be able to
> enable busypolling selectively. We already do for SO_BUSY_POLL and
> sysctl busy_read.
Thanks for taking a look and providing feedback.
>
> > ---
> > .../userspace-api/ioctl/ioctl-number.rst | 1 +
> > fs/eventpoll.c | 47 +++++++++++++++++++
> > include/uapi/linux/eventpoll.h | 12 +++++
> > 3 files changed, 60 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..c1ee0fe01da1 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>
> > @@ -869,6 +871,49 @@ 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;
> > + struct eventpoll *ep;
> > + struct epoll_params epoll_params;
> > + void __user *uarg = (void __user *) arg;
> > +
> > + if (!is_file_epoll(file))
> > + return -EINVAL;
> > +
> > + ep = file->private_data;
> > +
> > + switch (cmd) {
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > + case EPIOCSPARAMS:
> > + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> > + return -EFAULT;
> > +
> > + if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
> > + pr_err("busy poll budget %u exceeds suggested maximum %u\n",
> > + epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
> > +
> > + 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;
> > +#endif
> > + 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 +1020,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
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Elizabeth Figura @ 2024-01-25 3:42 UTC (permalink / raw)
To: Andy Lutomirski, wine-devel
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, linux-api,
wine-devel, André Almeida, Wolfram Sang, Peter Zijlstra,
Alexandre Julliard, Elizabeth Figura
In-Reply-To: <5907233.BlLQTPImNI@camazotz>
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.
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.
Alternatively, maybe there's another more lightweight way to store per-process
data?
^ permalink raw reply
* Re: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
From: Willem de Bruijn @ 2024-01-25 2:46 UTC (permalink / raw)
To: Joe Damato, netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, Joe Damato
In-Reply-To: <20240125003014.43103-4-jdamato@fastly.com>
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>
Please be sure to include the lists and people suggested by
`get_maintainer.pl -f fs/eventpoll.c`.
Adding ioctls is generally discouraged.
As this affects the behavior of epoll_wait, should this just be a
flag to (a new variant of) epoll_wait?
Speaking from some experience with adding epoll_pwait2. I initially
there added a stateful change that would affect wait behavior. The
sensible feedback as the time was to just change the behavior of the
syscall it affected. Even if that requires a syscall (which is not
that different from an ioctl, if better defined).
The discussion in that thread may be informative to decide on API:
https://lwn.net/ml/linux-kernel/20201116161001.1606608-1-willemdebruijn.kernel@gmail.com/
Agreed on the overall principle that it is preferable to be able to
enable busypolling selectively. We already do for SO_BUSY_POLL and
sysctl busy_read.
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> fs/eventpoll.c | 47 +++++++++++++++++++
> include/uapi/linux/eventpoll.h | 12 +++++
> 3 files changed, 60 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..c1ee0fe01da1 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>
> @@ -869,6 +871,49 @@ 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;
> + struct eventpoll *ep;
> + struct epoll_params epoll_params;
> + void __user *uarg = (void __user *) arg;
> +
> + if (!is_file_epoll(file))
> + return -EINVAL;
> +
> + ep = file->private_data;
> +
> + switch (cmd) {
> +#ifdef CONFIG_NET_RX_BUSY_POLL
> + case EPIOCSPARAMS:
> + if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> + return -EFAULT;
> +
> + if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
> + pr_err("busy poll budget %u exceeds suggested maximum %u\n",
> + epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
> +
> + 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;
> +#endif
> + 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 +1020,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
* [net-next v2 4/4] net: print error if SO_BUSY_POLL_BUDGET is large
From: Joe Damato @ 2024-01-25 0:30 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, Joe Damato
In-Reply-To: <20240125003014.43103-1-jdamato@fastly.com>
When drivers call netif_napi_add_weight with a weight that is larger
than NAPI_POLL_WEIGHT, the networking code allows the larger weight, but
prints an error.
Replicate this check for SO_BUSY_POLL_BUDGET; check if the user
specified amount exceeds NAPI_POLL_WEIGHT, allow it anyway, but print an
error.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
net/core/sock.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/sock.c b/net/core/sock.c
index 158dbdebce6a..ed243bd0dd77 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1153,6 +1153,9 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
return -EPERM;
if (val < 0 || val > U16_MAX)
return -EINVAL;
+ if (val > NAPI_POLL_WEIGHT)
+ pr_err("SO_BUSY_POLL_BUDGET %u exceeds suggested maximum %u\n", val,
+ NAPI_POLL_WEIGHT);
WRITE_ONCE(sk->sk_busy_poll_budget, val);
return 0;
#endif
--
2.25.1
^ permalink raw reply related
* [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-25 0:30 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, Joe Damato
In-Reply-To: <20240125003014.43103-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 | 47 +++++++++++++++++++
include/uapi/linux/eventpoll.h | 12 +++++
3 files changed, 60 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..c1ee0fe01da1 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>
@@ -869,6 +871,49 @@ 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;
+ struct eventpoll *ep;
+ struct epoll_params epoll_params;
+ void __user *uarg = (void __user *) arg;
+
+ if (!is_file_epoll(file))
+ return -EINVAL;
+
+ ep = file->private_data;
+
+ switch (cmd) {
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ case EPIOCSPARAMS:
+ if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+ return -EFAULT;
+
+ if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
+ pr_err("busy poll budget %u exceeds suggested maximum %u\n",
+ epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
+
+ 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;
+#endif
+ 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 +1020,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
* [net-next v2 2/4] eventpoll: Add per-epoll busy poll packet budget
From: Joe Damato @ 2024-01-25 0:30 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, Joe Damato
In-Reply-To: <20240125003014.43103-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
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