* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Huang, Ying @ 2024-01-23 3:02 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-kernel, linux-doc, linux-fsdevel, linux-api,
corbet, akpm, gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <20240119175730.15484-4-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> When a system has multiple NUMA nodes and it becomes bandwidth hungry,
> using the current MPOL_INTERLEAVE could be an wise option.
>
> However, if those NUMA nodes consist of different types of memory such
> as socket-attached DRAM and CXL/PCIe attached DRAM, the round-robin
> based interleave policy does not optimally distribute data to make use
> of their different bandwidth characteristics.
>
> Instead, interleave is more effective when the allocation policy follows
> each NUMA nodes' bandwidth weight rather than a simple 1:1 distribution.
>
> This patch introduces a new memory policy, MPOL_WEIGHTED_INTERLEAVE,
> enabling weighted interleave between NUMA nodes. Weighted interleave
> allows for proportional distribution of memory across multiple numa
> nodes, preferably apportioned to match the bandwidth of each node.
>
> For example, if a system has 1 CPU node (0), and 2 memory nodes (0,1),
> with bandwidth of (100GB/s, 50GB/s) respectively, the appropriate
> weight distribution is (2:1).
>
> Weights for each node can be assigned via the new sysfs extension:
> /sys/kernel/mm/mempolicy/weighted_interleave/
>
> For now, the default value of all nodes will be `1`, which matches
> the behavior of standard 1:1 round-robin interleave. An extension
> will be added in the future to allow default values to be registered
> at kernel and device bringup time.
>
> The policy allocates a number of pages equal to the set weights. For
> example, if the weights are (2,1), then 2 pages will be allocated on
> node0 for every 1 page allocated on node1.
>
> The new flag MPOL_WEIGHTED_INTERLEAVE can be used in set_mempolicy(2)
> and mbind(2).
>
> There are 3 integration points:
>
> weighted_interleave_nodes:
> Counts the number of allocations as they occur, and applies the
> weight for the current node. When the weight reaches 0, switch
> to the next node.
>
> weighted_interleave_nid:
> Gets the total weight of the nodemask as well as each individual
> node weight, then calculates the node based on the given index.
>
> bulk_array_weighted_interleave:
> Gets the total weight of the nodemask as well as each individual
> node weight, then calculates the number of "interleave rounds" as
> well as any delta ("partial round"). Calculates the number of
> pages for each node and allocates them.
>
> If a node was scheduled for interleave via interleave_nodes, the
> current weight (pol->cur_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 | 5 +
> include/uapi/linux/mempolicy.h | 1 +
> mm/mempolicy.c | 234 +++++++++++++++++-
> 4 files changed, 246 insertions(+), 3 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..c1a083eb0dd5 100644
> --- a/include/linux/mempolicy.h
> +++ b/include/linux/mempolicy.h
> @@ -54,6 +54,11 @@ struct mempolicy {
> nodemask_t cpuset_mems_allowed; /* relative to these nodes */
> nodemask_t user_nodemask; /* nodemask passed by user */
> } w;
> +
> + /* Weighted interleave settings */
> + struct {
> + u8 cur_weight;
> + } wil;
> };
>
> /*
> 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 427bddf115df..aa3b2389d3e0 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
> @@ -313,6 +320,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->wil.cur_weight = 0;
>
> return policy;
> }
> @@ -425,6 +433,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,
> @@ -846,7 +858,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);
> @@ -872,6 +885,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:
> @@ -956,6 +970,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->wil.cur_weight)
> + *policy = current->il_prev;
> + else
> + *policy = next_node_in(current->il_prev,
> + pol->nodes);
> } else {
> err = -EINVAL;
> goto out;
> @@ -1785,7 +1806,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) {
Should change the comments above get_vma_policy() definition too.
> *ilx += vma->vm_pgoff >> order;
> *ilx += (addr - vma->vm_start) >> (PAGE_SHIFT + order);
> }
> @@ -1835,6 +1857,28 @@ 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 next;
> + struct task_struct *me = current;
> + u8 __rcu *table;
> +
> + next = next_node_in(me->il_prev, policy->nodes);
> + if (next == MAX_NUMNODES)
> + return next;
> +
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + if (!policy->wil.cur_weight)
> + policy->wil.cur_weight = table ? table[next] : 1;
> + rcu_read_unlock();
> +
> + policy->wil.cur_weight--;
> + if (!policy->wil.cur_weight)
> + me->il_prev = next;
> + return next;
> +}
> +
> /* Do dynamic interleaving for a process */
> static unsigned int interleave_nodes(struct mempolicy *policy)
> {
> @@ -1869,6 +1913,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:
> {
> @@ -1907,6 +1954,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
> @@ -1967,6 +2047,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;
> @@ -2028,6 +2113,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;
>
> @@ -2127,7 +2213,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
> @@ -2263,6 +2350,135 @@ 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, weight_nodes, resume_node;
> + int prev_node = NUMA_NO_NODE;
It appears that we should initialize prev_node with me->il_prev?
Details are as below.
> + bool delta_depleted = false;
> + 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->wil.cur_weight) {
> + node = next_node_in(me->il_prev, nodes);
> + node_pages = pol->wil.cur_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 */
> + if (rem_pages <= pol->wil.cur_weight) {
> + pol->wil.cur_weight -= rem_pages;
If "pol->wil.cur_weight == 0" here, we need to change me->il_prev?
> + return total_allocated;
> + }
> + /* Otherwise we adjust nr_pages down, and continue from there */
> + rem_pages -= pol->wil.cur_weight;
> + pol->wil.cur_weight = 0;
> + prev_node = node;
> + }
else {
prev_node = me->il_prev;
}
> +
> + /* fetch the weights for this operation and calculate total weight */
> + weights = kmalloc(nnodes, GFP_KERNEL);
> + if (!weights)
> + return total_allocated;
> +
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + weight_nodes = 0;
We can replace "weight_nodes" with "i" and use a "for" loop?
> + while (weight_nodes < nnodes) {
> + node = next_node_in(prev_node, nodes);
IIUC, "node" will not change in the loop, so all "weight" below will be
the same value. To keep it simple, I think we can just copy weights
from the global iw_table and consider the default value?
> + weight = table ? table[node] : 1;
> + weights[weight_nodes++] = weight;
> + weight_total += weight;
> + }
> + rcu_read_unlock();
> +
> + /*
> + * 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 me->il_prev
> + *
> + * 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;
> + /* 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[i];
> + 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;
> + resume_weight = i < (nnodes - 1) ? weights[i+1] :
> + weights[0];
> + /* stop tracking iff (delta == weight) */
> + delta_depleted = !delta;
> + } else if (delta) { /* <= weight */
The comment is unnecessary and wrong.
> + /* if delta depleted, resume from this node */
> + node_pages += delta;
> + delta = 0;
> + resume_node = prev_node;
> + resume_weight = weight - (node_pages % weight);
> + delta_depleted = true; /* stop tracking */
> + } else if (!delta_depleted) {
> + /* if there was no delta, track last allocated node */
> + resume_node = node;
> + resume_weight = i < (nnodes - 1) ? weights[i+1] :
> + weights[0];
> + }
Can the above code be simplified as something like below?
resume_node = prev_node;
resume_weight = 0;
for (...) {
...
if (delta > weight) {
node_pages += weight;
delta -= weight;
} else if (delta) {
node_pages += delta;
/* if delta depleted, resume from this node */
if (delta < weight) {
resume_node = prev_node;
resume_weight = weight - delta;
} else {
resume_node = node;
}
delta = 0;
}
...
}
--
Best Regards,
Huang, Ying
> + /* 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->wil.cur_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)
> @@ -2303,6 +2519,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);
> @@ -2378,6 +2598,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;
> @@ -2514,6 +2735,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;
> @@ -2888,6 +3113,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)",
> };
> @@ -2947,6 +3173,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
> */
> @@ -3057,6 +3284,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:
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-23 4:54 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <87jzo0vjkk.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Tue, Jan 23, 2024 at 11:02:03AM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > + int prev_node = NUMA_NO_NODE;
>
> It appears that we should initialize prev_node with me->il_prev?
> Details are as below.
>
yeah good catch, was a rebase error from my tested code, where this is
the case. patching now.
> > + if (rem_pages <= pol->wil.cur_weight) {
> > + pol->wil.cur_weight -= rem_pages;
>
> If "pol->wil.cur_weight == 0" here, we need to change me->il_prev?
>
you are right, and also need to fetch the next cur_weight. Seems I
missed this specific case in my tests. (had this tested with a single
node but not 2, so it looked right).
Added to my test suite.
> We can replace "weight_nodes" with "i" and use a "for" loop?
>
> > + while (weight_nodes < nnodes) {
> > + node = next_node_in(prev_node, nodes);
>
> IIUC, "node" will not change in the loop, so all "weight" below will be
> the same value. To keep it simple, I think we can just copy weights
> from the global iw_table and consider the default value?
>
another rebase error here from my tested code, this should have been
node = prev_node;
while (...)
node = next_node_in(node, nodes);
I can change it to a for loop as suggested, but for more info on why I
did it this way, see the chunk below
> > + } else if (!delta_depleted) {
> > + /* if there was no delta, track last allocated node */
> > + resume_node = node;
> > + resume_weight = i < (nnodes - 1) ? weights[i+1] :
> > + weights[0];
^ this line acquires the weight of the *NEXT* node
another chunk prior to this does the same
thing. I suppose i can use next_node_in()
instead and just copy the entire weigh array
though, if that is preferable.
> > + }
>
> Can the above code be simplified as something like below?
>
> resume_node = prev_node;
> resume_weight = 0;
> for (...) {
> ...
> if (delta > weight) {
> node_pages += weight;
> delta -= weight;
> } else if (delta) {
> node_pages += delta;
> /* if delta depleted, resume from this node */
> if (delta < weight) {
> resume_node = prev_node;
> resume_weight = weight - delta;
> } else {
> resume_node = node;
> }
> delta = 0;
> }
> ...
> }
>
I'll take another look at it, but this logic is annoying because of the
corner case: me->il_prev can be NUMA_NO_NODE or an actual numa node.
If it's NUMA_NO_NODE, then the logic you have above will say "the next
node has no remaining weights assigned" and skip it on the next call to
weighted_interleave_nid or weighted_interleave_nodes.
This is incorrect - we want the weight of the first node to be
resume_weight, which is what this chunk does:
if (delta >= weight) {
/* if delta == weight, get next node weight */
resume_weight = i < (nnodes - 1) ? weights[i+1] : weights[0];
else if (delta) { /* delta < weight */
/* there's a remaining weight, use the that for resume weight */
resume_weight = weight - (node_pages % weight);
} else if (!delta_depleted) {
/* there was never a delta, track the last node and get the weight
* of the node AFTER that node, that's the resume weight */
resume_weight = i < (nnodes - 1) ? weights[i+1] : weights[0];
}
If il_prev is an actual node, and delta == 0, we want to return with
(il_prev = prev_node) but with the weight set to the weight of the
first node we're about to allocate from.
This is the reason for the annoying logic here: We have to come out of
this loop with the actual node and the actual weight.
I'll try to clean it up further and get my test suite to pass.
~Gregory
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-23 5:16 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <Za9GiqsZtcfKXc5m@memverge.com>
On Mon, Jan 22, 2024 at 11:54:34PM -0500, Gregory Price wrote:
> >
> > Can the above code be simplified as something like below?
> >
> > resume_node = prev_node;
--- resume_weight = 0;
+++ resume_weight = weights[node];
> > for (...) {
> > ...
> > }
> >
>
> I'll take another look at it, but this logic is annoying because of the
> corner case: me->il_prev can be NUMA_NO_NODE or an actual numa node.
>
After a quick look, as long as no one objects to (me->il_prev) remaining
NUMA_NO_NODE while having a weight assigned to pol->wil.cur_weight, then
this looks like it can be simplified as above.
I don't think it's harmful, but i'll have to take a quick look at what
happens on rebind to make sure we don't have a stale weight.
~Gregory
^ permalink raw reply
* [PATCH] selftests/landlock:Fix net_test build issues with old libc
From: Hu Yadi @ 2024-01-23 6:26 UTC (permalink / raw)
To: jmorris, serge, shuah, mathieu.desnoyers, mic, amir73il, brauner,
avagin
Cc: linux-api, linux-kernel, linux-security-module, linux-kselftest,
514118380, konstantin.meskhidze, Hu.Yadi
From: "Hu.Yadi" <hu.yadi@h3c.com>
Fixes: a549d055a22e ("selftests/landlock: Add network tests")
one issues comes up while building selftest/landlock/net_test on my side
(gcc 7.3/glibc-2.28/kernel-4.19)
net_test.c: In function ‘set_service’:
net_test.c:91:45: warning: implicit declaration of function ‘gettid’; [-Wimplicit-function-declaration]
"_selftests-landlock-net-tid%d-index%d", gettid(),
^~~~~~
getgid
net_test.c:(.text+0x4e0): undefined reference to `gettid'
Signed-off-by: Hu Yadi <hu.yadi@h3c.com>
Suggested-by: Jiao <jiaoxupo@h3c.com>
Reviewed-by: Berlin <berlin@h3c.com>
---
tools/testing/selftests/landlock/net_test.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 929e21c4db05..6cc1bb1a9166 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -18,9 +18,15 @@
#include <sys/prctl.h>
#include <sys/socket.h>
#include <sys/un.h>
-
+#include <sys/syscall.h>
#include "common.h"
+
+static pid_t sys_gettid(void)
+{
+ return syscall(__NR_gettid);
+}
+
const short sock_port_start = (1 << 10);
static const char loopback_ipv4[] = "127.0.0.1";
@@ -88,7 +94,7 @@ static int set_service(struct service_fixture *const srv,
case AF_UNIX:
srv->unix_addr.sun_family = prot.domain;
sprintf(srv->unix_addr.sun_path,
- "_selftests-landlock-net-tid%d-index%d", gettid(),
+ "_selftests-landlock-net-tid%d-index%d", sys_gettid(),
index);
srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
srv->unix_addr.sun_path[0] = '\0';
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Huang, Ying @ 2024-01-23 8:13 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <Za9GiqsZtcfKXc5m@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Tue, Jan 23, 2024 at 11:02:03AM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > + int prev_node = NUMA_NO_NODE;
>>
>> It appears that we should initialize prev_node with me->il_prev?
>> Details are as below.
>>
>
> yeah good catch, was a rebase error from my tested code, where this is
> the case. patching now.
>
>> > + if (rem_pages <= pol->wil.cur_weight) {
>> > + pol->wil.cur_weight -= rem_pages;
>>
>> If "pol->wil.cur_weight == 0" here, we need to change me->il_prev?
>>
> you are right, and also need to fetch the next cur_weight. Seems I
> missed this specific case in my tests. (had this tested with a single
> node but not 2, so it looked right).
>
> Added to my test suite.
>
>> We can replace "weight_nodes" with "i" and use a "for" loop?
>>
>> > + while (weight_nodes < nnodes) {
>> > + node = next_node_in(prev_node, nodes);
>>
>> IIUC, "node" will not change in the loop, so all "weight" below will be
>> the same value. To keep it simple, I think we can just copy weights
>> from the global iw_table and consider the default value?
>>
>
> another rebase error here from my tested code, this should have been
> node = prev_node;
> while (...)
> node = next_node_in(node, nodes);
>
> I can change it to a for loop as suggested, but for more info on why I
> did it this way, see the chunk below
>
>> > + } else if (!delta_depleted) {
>> > + /* if there was no delta, track last allocated node */
>> > + resume_node = node;
>> > + resume_weight = i < (nnodes - 1) ? weights[i+1] :
>> > + weights[0];
> ^ this line acquires the weight of the *NEXT* node
> another chunk prior to this does the same
> thing. I suppose i can use next_node_in()
> instead and just copy the entire weigh array
> though, if that is preferable.
Yes. I think copy the entire weight array make code logic simpler.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Huang, Ying @ 2024-01-23 8:35 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <Za9LnN59SBWwdFdW@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Mon, Jan 22, 2024 at 11:54:34PM -0500, Gregory Price wrote:
>> >
>> > Can the above code be simplified as something like below?
>> >
>> > resume_node = prev_node;
> --- resume_weight = 0;
> +++ resume_weight = weights[node];
>> > for (...) {
>> > ...
>> > }
>> >
>>
>> I'll take another look at it, but this logic is annoying because of the
>> corner case: me->il_prev can be NUMA_NO_NODE or an actual numa node.
>>
>
> After a quick look, as long as no one objects to (me->il_prev) remaining
> NUMA_NO_NODE
MAX_NUMNODES-1 ?
> while having a weight assigned to pol->wil.cur_weight,
I think that it is OK.
And, IIUC, pol->wil.cur_weight can be 0, as in
weighted_interleave_nodes(), if it's 0, it will be assigned to default
weight for the node.
> then
> this looks like it can be simplified as above.
>
> I don't think it's harmful, but i'll have to take a quick look at what
> happens on rebind to make sure we don't have a stale weight.
Make sense.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Huang, Ying @ 2024-01-23 8:40 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-kernel, linux-doc, linux-fsdevel, linux-api,
corbet, akpm, gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <20240119175730.15484-4-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> When a system has multiple NUMA nodes and it becomes bandwidth hungry,
> using the current MPOL_INTERLEAVE could be an wise option.
>
> However, if those NUMA nodes consist of different types of memory such
> as socket-attached DRAM and CXL/PCIe attached DRAM, the round-robin
> based interleave policy does not optimally distribute data to make use
> of their different bandwidth characteristics.
>
> Instead, interleave is more effective when the allocation policy follows
> each NUMA nodes' bandwidth weight rather than a simple 1:1 distribution.
>
> This patch introduces a new memory policy, MPOL_WEIGHTED_INTERLEAVE,
> enabling weighted interleave between NUMA nodes. Weighted interleave
> allows for proportional distribution of memory across multiple numa
> nodes, preferably apportioned to match the bandwidth of each node.
>
> For example, if a system has 1 CPU node (0), and 2 memory nodes (0,1),
> with bandwidth of (100GB/s, 50GB/s) respectively, the appropriate
> weight distribution is (2:1).
>
> Weights for each node can be assigned via the new sysfs extension:
> /sys/kernel/mm/mempolicy/weighted_interleave/
>
> For now, the default value of all nodes will be `1`, which matches
> the behavior of standard 1:1 round-robin interleave. An extension
> will be added in the future to allow default values to be registered
> at kernel and device bringup time.
>
> The policy allocates a number of pages equal to the set weights. For
> example, if the weights are (2,1), then 2 pages will be allocated on
> node0 for every 1 page allocated on node1.
>
> The new flag MPOL_WEIGHTED_INTERLEAVE can be used in set_mempolicy(2)
> and mbind(2).
>
> There are 3 integration points:
>
> weighted_interleave_nodes:
> Counts the number of allocations as they occur, and applies the
> weight for the current node. When the weight reaches 0, switch
> to the next node.
>
> weighted_interleave_nid:
> Gets the total weight of the nodemask as well as each individual
> node weight, then calculates the node based on the given index.
>
> bulk_array_weighted_interleave:
> Gets the total weight of the nodemask as well as each individual
> node weight, then calculates the number of "interleave rounds" as
> well as any delta ("partial round"). Calculates the number of
> pages for each node and allocates them.
>
> If a node was scheduled for interleave via interleave_nodes, the
> current weight (pol->cur_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 | 5 +
> include/uapi/linux/mempolicy.h | 1 +
> mm/mempolicy.c | 234 +++++++++++++++++-
> 4 files changed, 246 insertions(+), 3 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..c1a083eb0dd5 100644
> --- a/include/linux/mempolicy.h
> +++ b/include/linux/mempolicy.h
> @@ -54,6 +54,11 @@ struct mempolicy {
> nodemask_t cpuset_mems_allowed; /* relative to these nodes */
> nodemask_t user_nodemask; /* nodemask passed by user */
> } w;
> +
> + /* Weighted interleave settings */
> + struct {
> + u8 cur_weight;
> + } wil;
> };
>
> /*
> 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 427bddf115df..aa3b2389d3e0 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
> @@ -313,6 +320,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->wil.cur_weight = 0;
>
> return policy;
> }
> @@ -425,6 +433,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,
> @@ -846,7 +858,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);
> @@ -872,6 +885,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:
> @@ -956,6 +970,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->wil.cur_weight)
> + *policy = current->il_prev;
> + else
> + *policy = next_node_in(current->il_prev,
> + pol->nodes);
Per my understanding, we should always use "*policy = next_node_in()"
here, as in weighted_interleave_nodes().
> } else {
> err = -EINVAL;
> goto out;
> @@ -1785,7 +1806,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);
> }
> @@ -1835,6 +1857,28 @@ 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 next;
> + struct task_struct *me = current;
> + u8 __rcu *table;
> +
> + next = next_node_in(me->il_prev, policy->nodes);
> + if (next == MAX_NUMNODES)
> + return next;
> +
> + rcu_read_lock();
> + table = rcu_dereference(iw_table);
> + if (!policy->wil.cur_weight)
> + policy->wil.cur_weight = table ? table[next] : 1;
> + rcu_read_unlock();
> +
> + policy->wil.cur_weight--;
> + if (!policy->wil.cur_weight)
> + me->il_prev = next;
> + return next;
> +}
> +
[snip]
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH] selftests/landlock:Fix net_test build issues with old libc
From: Mickaël Salaün @ 2024-01-23 11:05 UTC (permalink / raw)
To: Hu Yadi
Cc: jmorris, serge, shuah, mathieu.desnoyers, amir73il, brauner,
avagin, linux-api, linux-kernel, linux-security-module,
linux-kselftest, 514118380, konstantin.meskhidze
In-Reply-To: <20240123062621.25082-1-hu.yadi@h3c.com>
Thanks, I tweaked a bit and merged this patch in my next branch.
On Tue, Jan 23, 2024 at 02:26:21PM +0800, Hu Yadi wrote:
> From: "Hu.Yadi" <hu.yadi@h3c.com>
There is an extra "." here, I fixed it. You don't need to add this From
field if it matches your email's From one.
>
> Fixes: a549d055a22e ("selftests/landlock: Add network tests")
>
> one issues comes up while building selftest/landlock/net_test on my side
> (gcc 7.3/glibc-2.28/kernel-4.19)
>
> net_test.c: In function ‘set_service’:
> net_test.c:91:45: warning: implicit declaration of function ‘gettid’; [-Wimplicit-function-declaration]
> "_selftests-landlock-net-tid%d-index%d", gettid(),
> ^~~~~~
> getgid
> net_test.c:(.text+0x4e0): undefined reference to `gettid'
>
> Signed-off-by: Hu Yadi <hu.yadi@h3c.com>
> Suggested-by: Jiao <jiaoxupo@h3c.com>
> Reviewed-by: Berlin <berlin@h3c.com>
> ---
> tools/testing/selftests/landlock/net_test.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 929e21c4db05..6cc1bb1a9166 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -18,9 +18,15 @@
> #include <sys/prctl.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> -
> +#include <sys/syscall.h>
> #include "common.h"
>
> +
> +static pid_t sys_gettid(void)
> +{
> + return syscall(__NR_gettid);
> +}
> +
> const short sock_port_start = (1 << 10);
>
> static const char loopback_ipv4[] = "127.0.0.1";
> @@ -88,7 +94,7 @@ static int set_service(struct service_fixture *const srv,
> case AF_UNIX:
> srv->unix_addr.sun_family = prot.domain;
> sprintf(srv->unix_addr.sun_path,
> - "_selftests-landlock-net-tid%d-index%d", gettid(),
> + "_selftests-landlock-net-tid%d-index%d", sys_gettid(),
> index);
> srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
> srv->unix_addr.sun_path[0] = '\0';
> --
> 2.23.0
>
>
^ permalink raw reply
* 回复: [PATCH v4] selftests/landlock:Fix two build issues
From: Huyadi @ 2024-01-23 12:04 UTC (permalink / raw)
To: 'Mickaël Salaün'
Cc: jmorris@namei.org, serge@hallyn.com, shuah@kernel.org,
mathieu.desnoyers@efficios.com, amir73il@gmail.com,
brauner@kernel.org, avagin@google.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, 514118380@qq.com,
konstantin.meskhidze@huawei.com
In-Reply-To: <20240119.Ugaehae2ze5b@digikod.net>
>> Changes v3 -> v2:
>> - add helper of gettid instead of __NR_gettid
>> - add gcc/glibc version info in comments Changes v1 -> v2:
>> - fix whitespace error
>> - replace SYS_gettid with _NR_gettid
>>
>> tools/testing/selftests/landlock/fs_test.c | 5 ++++-
>> tools/testing/selftests/landlock/net_test.c | 7 ++++++-
>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c
>> b/tools/testing/selftests/landlock/fs_test.c
>> index 18e1f86a6234..a992cf7c0ad1 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -4572,7 +4572,10 @@ FIXTURE_VARIANT(layout3_fs)
>> /* clang-format off */
>> FIXTURE_VARIANT_ADD(layout3_fs, tmpfs) {
>> /* clang-format on */
>> - .mnt = mnt_tmp,
>> + .mnt = {
>> + .type = "tmpfs",
>> + .data = "size=4m,mode=700",
>> + },
>
>I requested some changes here.
>
Could you give me some inspiration how to fix it?
it looks fine to me to assign value as above, which consistent with other pseudo FS tests.
Thanks in advance.
>> .file_path = file1_s1d1,
>> };
>>
^ permalink raw reply
* Re: [PATCH v4 5/6] add listmount(2) syscall
From: John Paul Adrian Glaubitz @ 2024-01-23 14:14 UTC (permalink / raw)
To: linux
Cc: amir73il, arnd, christian, dhowells, fweimer, kzak, linux-api,
linux-fsdevel, linux-kernel, linux-man, linux-security-module,
mattlloydhouse, mszeredi, raven, torvalds, viro, inux-sh
In-Reply-To: <75b87a85-7d2c-4078-91e3-024ea36cfb42@roeck-us.net>
Hi Guenter,
> with this patch in the tree, all sh4 builds fail with ICE.
>
> during RTL pass: final
> In file included from fs/namespace.c:11:
> fs/namespace.c: In function '__se_sys_listmount':
> include/linux/syscalls.h:258:9: internal compiler error: in change_address_1, at emit-rtl.c:2275
>
> I tested with gcc 8.2, 11.3, 11.4, and 12.3. The compiler version
> does not make a difference. Has anyone else seen the same problem ?
> If so, any idea what to do about it ?
I'm not seeing any problems building the SH kernel except some -Werror=missing-prototypes warnings.
I'm using gcc 11.1 from here [1].
Adrian
PS: Please always CC linux-sh and the SH maintainers when reporting issues.
> [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH v4 5/6] add listmount(2) syscall
From: John Paul Adrian Glaubitz @ 2024-01-23 14:14 UTC (permalink / raw)
To: linux
Cc: amir73il, arnd, christian, dhowells, fweimer, kzak, linux-api,
linux-fsdevel, linux-kernel, linux-man, linux-security-module,
mattlloydhouse, mszeredi, raven, torvalds, viro, linux-sh
In-Reply-To: <75b87a85-7d2c-4078-91e3-024ea36cfb42@roeck-us.net>
Hi Guenter,
> with this patch in the tree, all sh4 builds fail with ICE.
>
> during RTL pass: final
> In file included from fs/namespace.c:11:
> fs/namespace.c: In function '__se_sys_listmount':
> include/linux/syscalls.h:258:9: internal compiler error: in change_address_1, at emit-rtl.c:2275
>
> I tested with gcc 8.2, 11.3, 11.4, and 12.3. The compiler version
> does not make a difference. Has anyone else seen the same problem ?
> If so, any idea what to do about it ?
I'm not seeing any problems building the SH kernel except some -Werror=missing-prototypes warnings.
I'm using gcc 11.1 from here [1].
Adrian
PS: Please always CC linux-sh and the SH maintainers when reporting issues.
> [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH v4 5/6] add listmount(2) syscall
From: Guenter Roeck @ 2024-01-23 15:31 UTC (permalink / raw)
To: John Paul Adrian Glaubitz
Cc: amir73il, arnd, christian, dhowells, fweimer, kzak, linux-api,
linux-fsdevel, linux-kernel, linux-man, linux-security-module,
mattlloydhouse, mszeredi, raven, torvalds, viro, inux-sh
In-Reply-To: <20240123141408.3756120-1-glaubitz@physik.fu-berlin.de>
On 1/23/24 06:14, John Paul Adrian Glaubitz wrote:
> Hi Guenter,
>
>> with this patch in the tree, all sh4 builds fail with ICE.
>>
>> during RTL pass: final
>> In file included from fs/namespace.c:11:
>> fs/namespace.c: In function '__se_sys_listmount':
>> include/linux/syscalls.h:258:9: internal compiler error: in change_address_1, at emit-rtl.c:2275
>>
>> I tested with gcc 8.2, 11.3, 11.4, and 12.3. The compiler version
>> does not make a difference. Has anyone else seen the same problem ?
>> If so, any idea what to do about it ?
>
> I'm not seeing any problems building the SH kernel except some -Werror=missing-prototypes warnings.
>
The problem has been fixed thanks to some guidance from Linus. I did disable
CONFIG_WERROR for sh (and a few other architectures) because it now always
results in pointless build failures on test builds due to commit 0fcb70851fbf
("Makefile.extrawarn: turn on missing-prototypes globally").
> I'm using gcc 11.1 from here [1].
>
> Adrian
>
> PS: Please always CC linux-sh and the SH maintainers when reporting issues.
>
When reporting anything in the linux kernel, it is always a tight rope between
copying too few or too many people or mailing lists. I have been scolded for both.
Replying to the original patch historically results in the fewest complaints,
so I think I'll stick to that.
Guenter
^ permalink raw reply
* [PATCH v3 0/3] pidfds for non thread group leaders
From: Tycho Andersen @ 2024-01-23 15:34 UTC (permalink / raw)
To: Christian Brauner; +Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen
From: Tycho Andersen <tandersen@netflix.com>
Hi all,
Finally picking this back up. I looked for a location where we could put
the do_notify_pidfd() call so that we'd only need one for an
embarassingly long time, but couldn't find anything that worked. So, we
have a new call in a similar location in v1, just using the actual
helper. Perhaps this makes sense, since we're adding a new feature.
Hopefully all the tests even pass this time! I looked for a bit but
couldn't figure out why they don't exit(0) when something failed. I'll
keep looking at that.
Thoughts welcome,
Tycho
Tycho Andersen (3):
pidfd: allow pidfd_open() on non-thread-group leaders
selftests/pidfd: add non-thread-group leader tests
clone: allow CLONE_THREAD | CLONE_PIDFD together
include/linux/sched/signal.h | 1 +
kernel/exit.c | 11 +
kernel/fork.c | 7 +-
kernel/pid.c | 11 +-
kernel/signal.c | 2 +-
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 3 +-
.../selftests/pidfd/pidfd_non_tgl_test.c | 600 ++++++++++++++++++
8 files changed, 619 insertions(+), 17 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
base-commit: 610347effc2ecb5ededf5037e82240b151f883ab
--
2.34.1
^ permalink raw reply
* [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-23 15:34 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen,
Tycho Andersen
In-Reply-To: <20240123153452.170866-1-tycho@tycho.pizza>
From: Tycho Andersen <tandersen@netflix.com>
We are using the pidfd family of syscalls with the seccomp userspace
notifier. When some thread triggers a seccomp notification, we want to do
some things to its context (munge fd tables via pidfd_getfd(), maybe write
to its memory, etc.). However, threads created with ~CLONE_FILES or
~CLONE_VM mean that we can't use the pidfd family of syscalls for this
purpose, since their fd table or mm are distinct from the thread group
leader's. In this patch, we relax this restriction for pidfd_open().
In order to avoid dangling poll() users we need to notify pidfd waiters
when individual threads die, but once we do that all the other machinery
seems to work ok viz. the tests. But I suppose there are more cases than
just this one.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
--
v2: unify pidfd notification to all go through do_notify_pidfd() inside of
__exit_signals() suggested by Oleg.
Link to v1: https://lore.kernel.org/all/20231130163946.277502-1-tycho@tycho.pizza/
v3: go back to two separate call sites, the exiting one in
do_notify_parent(), and a new one in release_task(), when a thread is
not the thread group leader.
---
include/linux/sched/signal.h | 1 +
kernel/exit.c | 11 +++++++++++
kernel/fork.c | 4 +---
kernel/pid.c | 11 +----------
kernel/signal.c | 2 +-
5 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 4b7664c56208..d752f003a69a 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -333,6 +333,7 @@ extern int kill_pid_usb_asyncio(int sig, int errno, sigval_t addr, struct pid *,
extern int kill_pgrp(struct pid *pid, int sig, int priv);
extern int kill_pid(struct pid *pid, int sig, int priv);
extern __must_check bool do_notify_parent(struct task_struct *, int);
+extern void do_notify_pidfd(struct task_struct *task);
extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
extern void force_sig(int);
extern void force_fatal_sig(int);
diff --git a/kernel/exit.c b/kernel/exit.c
index 3988a02efaef..90117d7861f4 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -256,6 +256,17 @@ void release_task(struct task_struct *p)
write_lock_irq(&tasklist_lock);
ptrace_release_task(p);
thread_pid = get_pid(p->thread_pid);
+
+ /*
+ * If we're not the leader, notify any waiters on our pidfds. Note that
+ * we don't want to notify the leader until /everyone/ in the thread
+ * group is dead, viz. the condition below.
+ *
+ * We have to do this here, since __exit_signal() will
+ * __unhash_processes(), and break do_notify_pidfd()'s lookup.
+ */
+ if (!thread_group_leader(p))
+ do_notify_pidfd(p);
__exit_signal(p);
/*
diff --git a/kernel/fork.c b/kernel/fork.c
index 47ff3b35352e..44969cd472f0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2157,8 +2157,6 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
* Allocate a new file that stashes @pid and reserve a new pidfd number in the
* caller's file descriptor table. The pidfd is reserved but not installed yet.
*
- * The helper verifies that @pid is used as a thread group leader.
- *
* If this function returns successfully the caller is responsible to either
* call fd_install() passing the returned pidfd and pidfd file as arguments in
* order to install the pidfd into its file descriptor table or they must use
@@ -2176,7 +2174,7 @@ 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))
+ if (!pid)
return -EINVAL;
return __pidfd_prepare(pid, flags, ret);
diff --git a/kernel/pid.c b/kernel/pid.c
index b52b10865454..b55c0adf457b 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -552,11 +552,6 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
* Return the task associated with @pidfd. The function takes a reference on
* the returned task. The caller is responsible for releasing that reference.
*
- * Currently, the process identified by @pidfd is always a thread-group leader.
- * This restriction currently exists for all aspects of pidfds including pidfd
- * creation (CLONE_PIDFD cannot be used with CLONE_THREAD) and pidfd polling
- * (only supports thread group leaders).
- *
* Return: On success, the task_struct associated with the pidfd.
* On error, a negative errno number will be returned.
*/
@@ -615,11 +610,7 @@ int pidfd_create(struct pid *pid, unsigned int flags)
* @flags: flags to pass
*
* This creates a new pid file descriptor with the O_CLOEXEC flag set for
- * the process identified by @pid. Currently, the process identified by
- * @pid must be a thread-group leader. This restriction currently exists
- * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot
- * be used with CLONE_THREAD) and pidfd polling (only supports thread group
- * leaders).
+ * the process identified by @pid.
*
* Return: On success, a cloexec pidfd is returned.
* On error, a negative errno number will be returned.
diff --git a/kernel/signal.c b/kernel/signal.c
index c9c57d053ce4..3e3c9d0fa3a5 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2019,7 +2019,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
return ret;
}
-static void do_notify_pidfd(struct task_struct *task)
+void do_notify_pidfd(struct task_struct *task)
{
struct pid *pid;
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/3] selftests/pidfd: add non-thread-group leader tests
From: Tycho Andersen @ 2024-01-23 15:34 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen,
Tycho Andersen
In-Reply-To: <20240123153452.170866-1-tycho@tycho.pizza>
From: Tycho Andersen <tandersen@netflix.com>
This adds a family of tests for various behaviors for pidfds of
non-thread-group leaders.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
---
v2: unchanged
v3: add some tests for the situation that Christian described in one of the
previous threads, see the comments for a link + details.
---
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 3 +-
.../selftests/pidfd/pidfd_non_tgl_test.c | 583 ++++++++++++++++++
3 files changed, 586 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
index 973198a3ec3d..e7532e84a34a 100644
--- a/tools/testing/selftests/pidfd/.gitignore
+++ b/tools/testing/selftests/pidfd/.gitignore
@@ -6,3 +6,4 @@ pidfd_wait
pidfd_fdinfo_test
pidfd_getfd_test
pidfd_setns_test
+pidfd_non_tgl_test
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index d731e3e76d5b..50e3aa9de05a 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -2,7 +2,8 @@
CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \
- pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test
+ pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test \
+ pidfd_non_tgl_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
new file mode 100644
index 000000000000..c0624b127fab
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
@@ -0,0 +1,583 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <sys/socket.h>
+#include <limits.h>
+#include <string.h>
+#include <signal.h>
+#include <syscall.h>
+#include <sched.h>
+#include <poll.h>
+
+#include "../kselftest.h"
+#include "pidfd.h"
+
+static int sys_waitid(int which, pid_t pid, siginfo_t *info, int options,
+ struct rusage *ru)
+{
+ return syscall(__NR_waitid, which, pid, info, options, ru);
+}
+
+// glibc defaults to 8MB stacks
+#define STACK_SIZE (8 * 1024 * 1024)
+static char stack[STACK_SIZE];
+
+static int thread_sleep(void *)
+{
+ while (1)
+ sleep(100);
+ return 1;
+}
+
+static int fork_task_with_thread(int (*fn)(void *), int sk_pair[2],
+ pid_t *tgl, pid_t *thread, int *tgl_pidfd,
+ int *thread_pidfd)
+{
+ *tgl_pidfd = *thread_pidfd = -1;
+
+ *tgl = fork();
+ if (*tgl < 0) {
+ perror("fork");
+ return -1;
+ }
+
+ if (!*tgl) {
+ int flags = CLONE_THREAD | CLONE_VM | CLONE_SIGHAND;
+ pid_t t;
+
+ t = clone(fn, stack + STACK_SIZE, flags, sk_pair);
+ if (t < 0) {
+ perror("clone");
+ exit(1);
+ }
+
+ close(sk_pair[1]);
+
+ if (write(sk_pair[0], &t, sizeof(t)) != sizeof(t)) {
+ perror("read");
+ exit(1);
+ }
+
+ // wait to get killed for various reasons by the tests.
+ while (1)
+ sleep(100);
+ }
+
+ errno = 0;
+ if (read(sk_pair[1], thread, sizeof(*thread)) != sizeof(*thread)) {
+ perror("read");
+ goto cleanup;
+ }
+
+ *tgl_pidfd = sys_pidfd_open(*tgl, 0);
+ if (*tgl_pidfd < 0) {
+ perror("pidfd_open tgl");
+ goto cleanup;
+ }
+
+ *thread_pidfd = sys_pidfd_open(*thread, 0);
+ if (*thread_pidfd < 0) {
+ perror("pidfd");
+ goto cleanup;
+ }
+
+ return 0;
+
+cleanup:
+ kill(*tgl, SIGKILL);
+ if (*tgl_pidfd >= 0)
+ close(*tgl_pidfd);
+ if (*thread_pidfd >= 0)
+ close(*thread_pidfd);
+ return -1;
+}
+
+static int test_non_tgl_basic(void)
+{
+ pid_t tgl, thread;
+ int sk_pair[2], status;
+ int tgl_pidfd = -1, thread_pidfd = -1;
+ int ret = KSFT_FAIL;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ ksft_print_msg("socketpair failed %s\n", strerror(errno));
+ return KSFT_FAIL;
+ }
+
+ if (fork_task_with_thread(thread_sleep, sk_pair, &tgl, &thread,
+ &tgl_pidfd, &thread_pidfd) < 0) {
+ return KSFT_FAIL;
+ }
+
+ /*
+ * KILL of a thread should still kill everyone
+ */
+ if (sys_pidfd_send_signal(thread_pidfd, SIGKILL, NULL, 0)) {
+ perror("pidfd_send_signal");
+ goto cleanup;
+ }
+
+ errno = 0;
+ if (waitpid(tgl, &status, 0) != tgl) {
+ perror("waitpid tgl");
+ goto cleanup;
+ }
+
+ if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) {
+ ksft_print_msg("bad exit status %x\n", status);
+ goto cleanup;
+ }
+
+ ret = KSFT_PASS;
+
+cleanup:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+ close(tgl_pidfd);
+ close(thread_pidfd);
+ return ret;
+}
+
+static int thread_exec(void *arg)
+{
+ int *sk_pair = arg;
+ pid_t thread;
+
+ if (read(sk_pair[0], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("read");
+ exit(1);
+ }
+
+ execlp("/bin/true", "/bin/true", NULL);
+ return 1;
+}
+
+static int test_non_tgl_exec(void)
+{
+ pid_t tgl, thread;
+ int sk_pair[2];
+ int tgl_pidfd = -1, thread_pidfd = -1;
+ int ret = KSFT_FAIL, ready;
+ struct pollfd pollfd;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ ksft_print_msg("socketpair failed %s\n", strerror(errno));
+ return KSFT_FAIL;
+ }
+
+ if (fork_task_with_thread(thread_exec, sk_pair, &tgl, &thread,
+ &tgl_pidfd, &thread_pidfd) < 0) {
+ return KSFT_FAIL;
+ }
+
+ if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("read");
+ goto cleanup;
+ }
+
+ // thread will exec(), so this pidfd should eventually be dead (i.e.
+ // poll should return).
+ pollfd.fd = thread_pidfd;
+ pollfd.events = POLLIN;
+
+ ready = poll(&pollfd, 1, -1);
+ if (ready == -1) {
+ perror("poll");
+ goto cleanup;
+ }
+
+ if (ready != 1) {
+ ksft_print_msg("bad poll result %d\n", ready);
+ goto cleanup;
+ }
+
+ if (pollfd.revents != POLLIN) {
+ ksft_print_msg("bad poll revents: %x\n", pollfd.revents);
+ goto cleanup;
+ }
+
+ errno = 0;
+ if (sys_pidfd_getfd(thread_pidfd, 0, 0) > 0) {
+ ksft_print_msg("got a real fd");
+ goto cleanup;
+ }
+
+ if (errno != ESRCH) {
+ ksft_print_msg("polling invalid thread didn't give ESRCH");
+ goto cleanup;
+ }
+
+ ret = KSFT_PASS;
+
+cleanup:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+ close(tgl_pidfd);
+ close(thread_pidfd);
+ kill(tgl, SIGKILL);
+ waitpid(tgl, NULL, 0);
+ return ret;
+}
+
+int thread_wait_exit(void *arg)
+{
+ int *sk_pair = arg;
+ pid_t thread;
+
+ if (read(sk_pair[0], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("read");
+ exit(1);
+ }
+
+ return 0;
+}
+
+static int test_non_tgl_poll_exit(void)
+{
+ pid_t tgl, thread, writer;
+ int sk_pair[2], ready;
+ int tgl_pidfd = -1, thread_pidfd = -1;
+ int ret = KSFT_FAIL;
+ struct pollfd pollfd;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ ksft_print_msg("socketpair failed %s\n", strerror(errno));
+ return KSFT_FAIL;
+ }
+
+ if (fork_task_with_thread(thread_wait_exit, sk_pair, &tgl, &thread,
+ &tgl_pidfd, &thread_pidfd) < 0) {
+ return KSFT_FAIL;
+ }
+
+ writer = fork();
+ if (writer < 0) {
+ perror("fork");
+ return KSFT_FAIL;
+ }
+
+ if (writer == 0) {
+ sleep(3);
+ if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("read");
+ exit(1);
+ }
+ exit(0);
+ }
+
+ // thread will exit, so this pidfd should eventually be dead (i.e.
+ // poll should return).
+ pollfd.fd = thread_pidfd;
+ pollfd.events = POLLIN;
+
+ ready = poll(&pollfd, 1, -1);
+ if (ready == -1) {
+ perror("poll");
+ goto cleanup;
+ }
+
+ if (ready != 1) {
+ ksft_print_msg("bad poll result %d\n", ready);
+ goto cleanup;
+ }
+
+ if (pollfd.revents != POLLIN) {
+ ksft_print_msg("bad poll revents: %x\n", pollfd.revents);
+ goto cleanup;
+ }
+
+ if (pollfd.fd != thread_pidfd) {
+ ksft_print_msg("bad poll fd: %x\n", pollfd.fd);
+ goto cleanup;
+ }
+
+ ret = KSFT_PASS;
+
+cleanup:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+ close(tgl_pidfd);
+ close(thread_pidfd);
+ kill(tgl, SIGKILL);
+ waitpid(tgl, NULL, 0);
+ waitpid(thread, NULL, 0);
+ return ret;
+}
+
+
+static int test_non_tgl_exit_poll(void)
+{
+ pid_t tgl, thread;
+ int sk_pair[2];
+ int tgl_pidfd = -1, thread_pidfd = -1;
+ int ret = KSFT_FAIL, ready;
+ struct pollfd pollfd;
+ siginfo_t siginfo;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ ksft_print_msg("socketpair failed %s\n", strerror(errno));
+ return KSFT_FAIL;
+ }
+
+ if (fork_task_with_thread(thread_wait_exit, sk_pair, &tgl, &thread,
+ &tgl_pidfd, &thread_pidfd) < 0) {
+ return KSFT_FAIL;
+ }
+
+ if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("write");
+ goto cleanup;
+ }
+
+ // thread will exit, so this pidfd should eventually be dead (i.e.
+ // poll should return).
+ pollfd.fd = thread_pidfd;
+ pollfd.events = POLLIN;
+
+ ready = poll(&pollfd, 1, -1);
+ if (ready == -1) {
+ perror("poll");
+ goto cleanup;
+ }
+
+ if (ready != 1) {
+ ksft_print_msg("bad poll result %d\n", ready);
+ goto cleanup;
+ }
+
+ if (pollfd.revents != POLLIN) {
+ ksft_print_msg("bad poll revents: %x\n", pollfd.revents);
+ goto cleanup;
+ }
+
+ errno = 0;
+ if (sys_pidfd_getfd(thread_pidfd, 0, 0) > 0) {
+ ksft_print_msg("got a real pidfd");
+ goto cleanup;
+ }
+
+ if (errno != ESRCH) {
+ ksft_print_msg("polling invalid thread didn't give ESRCH");
+ goto cleanup;
+ }
+
+ // ok, but the thread group *leader* should still be alive
+ pollfd.fd = tgl_pidfd;
+ ready = poll(&pollfd, 1, 1);
+ if (ready == -1) {
+ perror("poll");
+ goto cleanup;
+ }
+
+ if (ready != 0) {
+ ksft_print_msg("polling leader returned something?! %x", pollfd.revents);
+ goto cleanup;
+ }
+
+ errno = 0;
+ if (sys_waitid(P_PIDFD, thread_pidfd, &siginfo, WEXITED, NULL) == 0) {
+ ksft_print_msg("got a real wait value");
+ goto cleanup;
+ }
+
+ if (errno != ECHILD) {
+ ksft_print_msg("waiting invalid thread didn't give ECHILD: %d\n", errno);
+ goto cleanup;
+ }
+
+ close(thread_pidfd);
+
+ ret = KSFT_PASS;
+
+cleanup:
+ kill(tgl, SIGKILL);
+ waitpid(tgl, NULL, 0);
+ waitpid(thread, NULL, 0);
+ return ret;
+}
+
+/*
+ * This is a test for the first half of the scenario in
+ * https://lore.kernel.org/all/20231207-avancieren-unbezahlbar-9258f45ec3ec@brauner/
+ *
+ * i.e we expect:
+ *
+ * waitid(P_PIDFD, tg_pidfd) waitid(P_PIDFD, t_pidfd)
+ * tg_pid[PIDTYPE_TGID] == tsk1 t_pid[PIDTYPE_TGID] == NULL
+ * => succeeds => fails
+ */
+static int test_non_tgl_waitid_exit(void)
+{
+ pid_t tgl, thread, writer;
+ int sk_pair[2], status;
+ int tgl_pidfd = -1, thread_pidfd = -1;
+ int ret = KSFT_FAIL;
+ siginfo_t siginfo;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ ksft_print_msg("socketpair failed %s\n", strerror(errno));
+ return KSFT_FAIL;
+ }
+
+ if (fork_task_with_thread(thread_wait_exit, sk_pair, &tgl, &thread,
+ &tgl_pidfd, &thread_pidfd) < 0) {
+ return KSFT_FAIL;
+ }
+
+ writer = fork();
+ if (writer < 0) {
+ perror("fork");
+ return KSFT_FAIL;
+ }
+
+ if (writer == 0) {
+ sleep(3);
+ if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("read");
+ exit(1);
+ }
+ exit(0);
+ }
+
+ errno = 0;
+ if (sys_waitid(P_PIDFD, thread_pidfd, &siginfo, __WALL | WEXITED, NULL) == 0
+ || errno != ECHILD) {
+ ksft_print_msg("waitid didn't give ECHILD");
+ goto cleanup;
+ }
+ close(thread_pidfd);
+
+ kill(tgl, SIGKILL);
+
+ errno = 0;
+ if (sys_waitid(P_PIDFD, tgl_pidfd, &siginfo, __WALL | WEXITED, NULL) != 0) {
+ perror("waitid");
+ ksft_print_msg("waitid failed %d\n", errno);
+ goto cleanup;
+ }
+ close(tgl_pidfd);
+
+ if (siginfo.si_code != CLD_KILLED || siginfo.si_status != SIGKILL) {
+ ksft_print_msg("bad exit code %x status %x\n", siginfo.si_code, siginfo.si_status);
+ goto cleanup;
+ }
+
+ ret = KSFT_PASS;
+
+cleanup:
+ kill(tgl, SIGKILL);
+ waitpid(tgl, &status, 0);
+ waitpid(writer, NULL, 0);
+ return ret;
+}
+
+/*
+ * This is a test for the first second half of the scenario in
+ * https://lore.kernel.org/all/20231207-avancieren-unbezahlbar-9258f45ec3ec@brauner/
+ *
+ * i.e after the exec, we expect:
+ *
+ * waitid(P_PIDFD, tg_pidfd) waitid(P_PIDFD, t_pid)
+ * tg_pid[PIDTYPE_TGID] == tsk2 t_pid[PIDTYPE_TGID] == NULL
+ * => succeeds => fails
+ */
+static int test_non_tgl_waitid_exec(void)
+{
+ pid_t tgl, thread, writer;
+ int sk_pair[2], status;
+ int tgl_pidfd = -1, thread_pidfd = -1;
+ int ret = KSFT_FAIL;
+ siginfo_t siginfo;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ ksft_print_msg("socketpair failed %s\n", strerror(errno));
+ return KSFT_FAIL;
+ }
+
+ if (fork_task_with_thread(thread_exec, sk_pair, &tgl, &thread,
+ &tgl_pidfd, &thread_pidfd) < 0) {
+ return KSFT_FAIL;
+ }
+
+ writer = fork();
+ if (writer < 0) {
+ perror("fork");
+ return KSFT_FAIL;
+ }
+
+ if (writer == 0) {
+ sleep(3);
+ if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+ perror("read");
+ exit(1);
+ }
+ exit(0);
+ }
+
+ errno = 0;
+ if (sys_waitid(P_PIDFD, thread_pidfd, &siginfo, __WALL | WEXITED, NULL) == 0
+ || errno != ECHILD) {
+ ksft_print_msg("waitid didn't give ECHILD");
+ goto cleanup;
+ }
+ close(thread_pidfd);
+
+ kill(tgl, SIGKILL);
+
+ errno = 0;
+ if (sys_waitid(P_PIDFD, tgl_pidfd, &siginfo, __WALL | WEXITED, NULL) != 0) {
+ perror("waitid");
+ ksft_print_msg("waitid failed %d\n", errno);
+ goto cleanup;
+ }
+ close(tgl_pidfd);
+
+ if (siginfo.si_code != CLD_KILLED || siginfo.si_status != SIGKILL) {
+ ksft_print_msg("bad exit code %x status %x\n", siginfo.si_code, siginfo.si_status);
+ goto cleanup;
+ }
+
+ ret = KSFT_PASS;
+cleanup:
+ kill(tgl, SIGKILL);
+ kill(thread, SIGKILL);
+ waitpid(tgl, &status, 0);
+ waitpid(thread, &status, 0);
+ waitpid(writer, &status, 0);
+ return ret;
+}
+
+#define T(x) { x, #x }
+struct pidfd_non_tgl_test {
+ int (*fn)();
+ const char *name;
+} tests[] = {
+ T(test_non_tgl_basic),
+ T(test_non_tgl_exec),
+ T(test_non_tgl_poll_exit),
+ T(test_non_tgl_exit_poll),
+ T(test_non_tgl_waitid_exit),
+ T(test_non_tgl_waitid_exec),
+};
+#undef T
+
+int main(int argc, char *argv[])
+{
+ int i, ret = EXIT_SUCCESS;
+
+ for (i = 0; i < ARRAY_SIZE(tests); i++) {
+ switch (tests[i].fn()) {
+ case KSFT_PASS:
+ ksft_test_result_pass("%s\n", tests[i].name);
+ break;
+ case KSFT_SKIP:
+ ksft_test_result_skip("%s\n", tests[i].name);
+ break;
+ default:
+ ret = EXIT_FAILURE;
+ ksft_test_result_fail("%s\n", tests[i].name);
+ break;
+ }
+ }
+
+ return ret;
+}
--
2.34.1
^ permalink raw reply related
* [PATCH v3 3/3] clone: allow CLONE_THREAD | CLONE_PIDFD together
From: Tycho Andersen @ 2024-01-23 15:34 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen,
Tycho Andersen
In-Reply-To: <20240123153452.170866-1-tycho@tycho.pizza>
From: Tycho Andersen <tandersen@netflix.com>
This removes the restriction of CLONE_THREAD | CLONE_PIDFD being specified
together. Assuming the previous patch sorts out all the thorny issues this
should be safe. I've left it as a separate patch since it is not strictly
necessary as a usecase for us, but might be nice? Perhaps we want to wait
until someone actually needs it though.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
---
kernel/fork.c | 3 +--
.../selftests/pidfd/pidfd_non_tgl_test.c | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 44969cd472f0..25fccf7c08a7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2296,9 +2296,8 @@ __latent_entropy struct task_struct *copy_process(
/*
* - CLONE_DETACHED is blocked so that we can potentially
* reuse it later for CLONE_PIDFD.
- * - CLONE_THREAD is blocked until someone really needs it.
*/
- if (clone_flags & (CLONE_DETACHED | CLONE_THREAD))
+ if (clone_flags & CLONE_DETACHED)
return ERR_PTR(-EINVAL);
}
diff --git a/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
index c0624b127fab..a34208c2307e 100644
--- a/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
@@ -546,6 +546,22 @@ static int test_non_tgl_waitid_exec(void)
return ret;
}
+static int test_clone_thread_pidfd(void)
+{
+ pid_t pid;
+ int flags = CLONE_THREAD | CLONE_VM | CLONE_SIGHAND | CLONE_PIDFD;
+ int pidfd;
+
+ pid = clone(thread_sleep, stack + STACK_SIZE, flags, NULL, &pidfd);
+ if (pid < 0) {
+ perror("clone");
+ return KSFT_FAIL;
+ }
+
+ close(pidfd);
+ return KSFT_PASS;
+}
+
#define T(x) { x, #x }
struct pidfd_non_tgl_test {
int (*fn)();
@@ -557,6 +573,7 @@ struct pidfd_non_tgl_test {
T(test_non_tgl_exit_poll),
T(test_non_tgl_waitid_exit),
T(test_non_tgl_waitid_exec),
+ T(test_clone_thread_pidfd),
};
#undef T
--
2.34.1
^ permalink raw reply related
* Re: 回复: [PATCH v4] selftests/landlock:Fix two build issues
From: Mickaël Salaün @ 2024-01-23 15:32 UTC (permalink / raw)
To: Huyadi
Cc: jmorris@namei.org, serge@hallyn.com, shuah@kernel.org,
mathieu.desnoyers@efficios.com, amir73il@gmail.com,
brauner@kernel.org, avagin@google.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, 514118380@qq.com,
konstantin.meskhidze@huawei.com
In-Reply-To: <adec399e50c74b30b59480d92c431241@h3c.com>
On Tue, Jan 23, 2024 at 12:04:17PM +0000, Huyadi wrote:
>
> >> Changes v3 -> v2:
> >> - add helper of gettid instead of __NR_gettid
> >> - add gcc/glibc version info in comments Changes v1 -> v2:
> >> - fix whitespace error
> >> - replace SYS_gettid with _NR_gettid
> >>
> >> tools/testing/selftests/landlock/fs_test.c | 5 ++++-
> >> tools/testing/selftests/landlock/net_test.c | 7 ++++++-
> >> 2 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/landlock/fs_test.c
> >> b/tools/testing/selftests/landlock/fs_test.c
> >> index 18e1f86a6234..a992cf7c0ad1 100644
> >> --- a/tools/testing/selftests/landlock/fs_test.c
> >> +++ b/tools/testing/selftests/landlock/fs_test.c
> >> @@ -4572,7 +4572,10 @@ FIXTURE_VARIANT(layout3_fs)
> >> /* clang-format off */
> >> FIXTURE_VARIANT_ADD(layout3_fs, tmpfs) {
> >> /* clang-format on */
> >> - .mnt = mnt_tmp,
> >> + .mnt = {
> >> + .type = "tmpfs",
> >> + .data = "size=4m,mode=700",
> >> + },
> >
> >I requested some changes here.
> >
>
> Could you give me some inspiration how to fix it?
> it looks fine to me to assign value as above, which consistent with other pseudo FS tests.
> Thanks in advance.
Just add and use this for the two tmpfs data:
#define MNT_TMP_DATA "size=4m,mode=700"
You can also make the mnt_tmp variable static const.
>
> >> .file_path = file1_s1d1,
> >> };
> >>
>
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-23 19:56 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen
In-Reply-To: <20240123153452.170866-2-tycho@tycho.pizza>
Too late for me, but I don't understand this patch after a quick glance.
perhaps I missed something...
On 01/23, Tycho Andersen wrote:
>
> @@ -256,6 +256,17 @@ void release_task(struct task_struct *p)
> write_lock_irq(&tasklist_lock);
> ptrace_release_task(p);
> thread_pid = get_pid(p->thread_pid);
> +
> + /*
> + * If we're not the leader, notify any waiters on our pidfds. Note that
> + * we don't want to notify the leader until /everyone/ in the thread
> + * group is dead, viz. the condition below.
> + *
> + * We have to do this here, since __exit_signal() will
> + * __unhash_processes(), and break do_notify_pidfd()'s lookup.
> + */
> + if (!thread_group_leader(p))
> + do_notify_pidfd(p);
This doesn't look consistent.
If the task is a group leader do_notify_pidfd() is called by exit_notify()
when it becomes a zombie (if no other threads), before it is reaped by its
parent (unless autoreap).
If it is a sub-thread, it is called by release_task() above. Note that a
sub-thread can become a zombie too if it is traced.
> __exit_signal(p);
and, do_notify_pidfd() is called before __exit_signal() which does
__unhash_process() -> detach_pid(PIDTYPE_PID).
Doesn't this mean that pidfd_poll() can hang? thread_group_exited()
won't return true after do_notify_pidfd() above, not to mention that
thread_group_empty() is not possible if !thread_group_leader().
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?
Either way pidfd_poll() needs more changes with this patch and it can't
use thread_group_exited(). If do_notify_pidfd() is called by release_task()
after __exit_signal(), it can just check pid_has_task(PIDTYPE_PID).
Oleg.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-23 21:10 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen
In-Reply-To: <20240123195608.GB9978@redhat.com>
On Tue, Jan 23, 2024 at 08:56:08PM +0100, Oleg Nesterov wrote:
> Too late for me, but I don't understand this patch after a quick glance.
> perhaps I missed something...
Thanks for taking a look.
> On 01/23, Tycho Andersen wrote:
> >
> > @@ -256,6 +256,17 @@ void release_task(struct task_struct *p)
> > write_lock_irq(&tasklist_lock);
> > ptrace_release_task(p);
> > thread_pid = get_pid(p->thread_pid);
> > +
> > + /*
> > + * If we're not the leader, notify any waiters on our pidfds. Note that
> > + * we don't want to notify the leader until /everyone/ in the thread
> > + * group is dead, viz. the condition below.
> > + *
> > + * We have to do this here, since __exit_signal() will
> > + * __unhash_processes(), and break do_notify_pidfd()'s lookup.
> > + */
> > + if (!thread_group_leader(p))
> > + do_notify_pidfd(p);
>
> This doesn't look consistent.
>
> If the task is a group leader do_notify_pidfd() is called by exit_notify()
> when it becomes a zombie (if no other threads), before it is reaped by its
> parent (unless autoreap).
There is another path, also in release_task(), that I was trying to
mirror since it deals explicitly with sub-threads but,
> If it is a sub-thread, it is called by release_task() above. Note that a
> sub-thread can become a zombie too if it is traced.
I didn't know about this.
> > __exit_signal(p);
>
> and, do_notify_pidfd() is called before __exit_signal() which does
> __unhash_process() -> detach_pid(PIDTYPE_PID).
>
> Doesn't this mean that pidfd_poll() can hang? thread_group_exited()
> won't return true after do_notify_pidfd() above, not to mention that
> thread_group_empty() is not possible if !thread_group_leader().
I was wondering about this too, but the test_non_tgl_poll_exit test in
the next patch tests exactly this and works as expected.
> 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. When it is reaped is "mostly
unrelated". Something like,
1. in the "normal" exit_notify() paths via do_notify_parent()
2. if none of those cases are true (aka the final else in
exit_notify()) and the thread is not ptraced
3. via release_task() finally if this was the thread group leader and
it died before some sub-thread
then in pidfd_poll(), we can do:
if (!tsk || (tsk->exit_state >= 0) || thread_group_exited())
do_notify_pidfd();
?
> Either way pidfd_poll() needs more changes with this patch and it can't
> use thread_group_exited(). If do_notify_pidfd() is called by release_task()
> after __exit_signal(), it can just check pid_has_task(PIDTYPE_PID).
I suppose this is why my test works, since pid_task(PIDTYPE_PID) is null
after release_task(). But if we want it to happen earlier, we'll have
to do something like the above.
Tycho
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-23 21:27 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <87a5owv454.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Tue, Jan 23, 2024 at 04:35:19PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
>
> > On Mon, Jan 22, 2024 at 11:54:34PM -0500, Gregory Price wrote:
> >> >
> >> > Can the above code be simplified as something like below?
> >> >
> >> > resume_node = prev_node;
> > --- resume_weight = 0;
> > +++ resume_weight = weights[node];
> >> > for (...) {
> >> > ...
> >> > }
> >> >
> >>
> >> I'll take another look at it, but this logic is annoying because of the
> >> corner case: me->il_prev can be NUMA_NO_NODE or an actual numa node.
> >>
> >
> > After a quick look, as long as no one objects to (me->il_prev) remaining
> > NUMA_NO_NODE
>
> MAX_NUMNODES-1 ?
>
When setting a new policy, the il_prev gets set to NUMA_NO_NODE. It's
not harmful and is just (-1), which is functionally the same as
(MAX_NUMNODES-1) for the purpose of iterating the nodemask with
next_node_in(). So it's fine to set (resume_node = me->il_prev)
as discussed.
I have a cleaned up function I'll push when i fix up a few other spots.
> > while having a weight assigned to pol->wil.cur_weight,
>
> I think that it is OK.
>
> And, IIUC, pol->wil.cur_weight can be 0, as in
> weighted_interleave_nodes(), if it's 0, it will be assigned to default
> weight for the node.
>
cur_weight is different than the global weights. cur_weight tells us
how many pages are remaining to allocate for the current node.
(cur_weight = 0) can happen in two scenarios:
- initial setting of mempolicy (NUMA_NO_NODE w/ cur_weight=0)
- weighted_interleave_nodes decrements it down to 0
Now that i'm looking at it - the second condition should not exist, and
we can eliminate it. The logic in weighted_interleave_nodes is actually
annoyingly unclear at the moment, so I'm going to re-factor it a bit to
be more explicit.
~Gregory
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-23 22:22 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen
In-Reply-To: <ZbArN3EYRfhrNs3o@tycho.pizza>
I am already sleeping. I'll try to reply to other parts of your email
tomorrow but I am not sure, I will be very busy with family duties.
On 01/23, Tycho Andersen wrote:
>
> > > __exit_signal(p);
> >
> > and, do_notify_pidfd() is called before __exit_signal() which does
> > __unhash_process() -> detach_pid(PIDTYPE_PID).
> >
> > Doesn't this mean that pidfd_poll() can hang? thread_group_exited()
> > won't return true after do_notify_pidfd() above, not to mention that
> > thread_group_empty() is not possible if !thread_group_leader().
>
> I was wondering about this too, but the test_non_tgl_poll_exit test in
> the next patch tests exactly this and works as expected.
Well, if release_task() completes __exit_signal() before the woken task
does thread_group_exited(), pid_task(PIDTYPE_PID) will return 0 and
pidfd_poll() won't hang.
But to be honest I can't understand test_non_tgl_poll_exit() at all. I don't
even understand why the process/thread created by fork_task_with_thread()
should ever exit. And why it creates the "writer" child... Never mind, too
late for me to read the code.
Oleg.
^ permalink raw reply
* Re: [RFC PATCH 2/9] ntsync: Reserve a minor device number and ioctl range.
From: Greg Kroah-Hartman @ 2024-01-24 0:54 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, linux-kernel, linux-api, wine-devel,
André Almeida, Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra
In-Reply-To: <20240124004028.16826-3-zfigura@codeweavers.com>
On Tue, Jan 23, 2024 at 06:40:21PM -0600, Elizabeth Figura wrote:
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> ---
Note, we can't take patches without any changelog text, and you don't
want us to :)
> Documentation/admin-guide/devices.txt | 3 ++-
> Documentation/userspace-api/ioctl/ioctl-number.rst | 2 ++
> drivers/misc/ntsync.c | 3 ++-
> include/linux/miscdevice.h | 1 +
> 4 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/devices.txt b/Documentation/admin-guide/devices.txt
> index 94c98be1329a..041404397ee5 100644
> --- a/Documentation/admin-guide/devices.txt
> +++ b/Documentation/admin-guide/devices.txt
> @@ -376,8 +376,9 @@
> 240 = /dev/userio Serio driver testing device
> 241 = /dev/vhost-vsock Host kernel driver for virtio vsock
> 242 = /dev/rfkill Turning off radio transmissions (rfkill)
> + 243 = /dev/ntsync NT synchronization primitive device
>
> - 243-254 Reserved for local use
> + 244-254 Reserved for local use
Why do you need a fixed minor number? Can't your userspace handle
dynamic numbers? What systems require a static value?
thanks,
greg k-h
^ permalink raw reply
* [RFC PATCH 3/9] ntsync: Introduce NTSYNC_IOC_CREATE_SEM and NTSYNC_IOC_DELETE.
From: Elizabeth Figura @ 2024-01-24 0:40 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra, Elizabeth Figura
In-Reply-To: <20240124004028.16826-1-zfigura@codeweavers.com>
These correspond to the NT syscalls NtCreateSemaphore() and NtClose().
Unlike those functions, however, these ioctls do not handle object names, or
lookup of existing objects, or handle reference counting, but simply create the
underlying primitive. The user space emulator is expected to implement those
functions if they are required.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 117 ++++++++++++++++++++++++++++++++++++
include/uapi/linux/ntsync.h | 25 ++++++++
2 files changed, 142 insertions(+)
create mode 100644 include/uapi/linux/ntsync.h
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 84b498e2b2d5..3287b94be351 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -8,23 +8,140 @@
#include <linux/fs.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/xarray.h>
+#include <uapi/linux/ntsync.h>
#define NTSYNC_NAME "ntsync"
+enum ntsync_type {
+ NTSYNC_TYPE_SEM,
+};
+
+struct ntsync_obj {
+ struct rcu_head rhead;
+ struct kref refcount;
+
+ enum ntsync_type type;
+
+ union {
+ struct {
+ __u32 count;
+ __u32 max;
+ } sem;
+ } u;
+};
+
+struct ntsync_device {
+ struct xarray objects;
+};
+
+static void destroy_obj(struct kref *ref)
+{
+ struct ntsync_obj *obj = container_of(ref, struct ntsync_obj, refcount);
+
+ kfree_rcu(obj, rhead);
+}
+
+static void put_obj(struct ntsync_obj *obj)
+{
+ kref_put(&obj->refcount, destroy_obj);
+}
+
static int ntsync_char_open(struct inode *inode, struct file *file)
{
+ struct ntsync_device *dev;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ xa_init_flags(&dev->objects, XA_FLAGS_ALLOC);
+
+ file->private_data = dev;
return nonseekable_open(inode, file);
}
static int ntsync_char_release(struct inode *inode, struct file *file)
{
+ struct ntsync_device *dev = file->private_data;
+ struct ntsync_obj *obj;
+ unsigned long id;
+
+ xa_for_each(&dev->objects, id, obj)
+ put_obj(obj);
+
+ xa_destroy(&dev->objects);
+
+ kfree(dev);
+
+ return 0;
+}
+
+static void init_obj(struct ntsync_obj *obj)
+{
+ kref_init(&obj->refcount);
+}
+
+static int ntsync_create_sem(struct ntsync_device *dev, void __user *argp)
+{
+ struct ntsync_sem_args __user *user_args = argp;
+ struct ntsync_sem_args args;
+ struct ntsync_obj *sem;
+ __u32 id;
+ int ret;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+
+ if (args.count > args.max)
+ return -EINVAL;
+
+ sem = kzalloc(sizeof(*sem), GFP_KERNEL);
+ if (!sem)
+ return -ENOMEM;
+
+ init_obj(sem);
+ sem->type = NTSYNC_TYPE_SEM;
+ sem->u.sem.count = args.count;
+ sem->u.sem.max = args.max;
+
+ ret = xa_alloc(&dev->objects, &id, sem, xa_limit_32b, GFP_KERNEL);
+ if (ret < 0) {
+ kfree(sem);
+ return ret;
+ }
+
+ return put_user(id, &user_args->sem);
+}
+
+static int ntsync_delete(struct ntsync_device *dev, void __user *argp)
+{
+ struct ntsync_obj *obj;
+ __u32 id;
+
+ if (get_user(id, (__u32 __user *)argp))
+ return -EFAULT;
+
+ obj = xa_erase(&dev->objects, id);
+ if (!obj)
+ return -EINVAL;
+
+ put_obj(obj);
return 0;
}
static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
unsigned long parm)
{
+ struct ntsync_device *dev = file->private_data;
+ void __user *argp = (void __user *)parm;
+
switch (cmd) {
+ case NTSYNC_IOC_CREATE_SEM:
+ return ntsync_create_sem(dev, argp);
+ case NTSYNC_IOC_DELETE:
+ return ntsync_delete(dev, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
new file mode 100644
index 000000000000..d97afc138dcc
--- /dev/null
+++ b/include/uapi/linux/ntsync.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Kernel support for NT synchronization primitive emulation
+ *
+ * Copyright (C) 2021-2022 Elizabeth Figura
+ */
+
+#ifndef __LINUX_NTSYNC_H
+#define __LINUX_NTSYNC_H
+
+#include <linux/types.h>
+
+struct ntsync_sem_args {
+ __u32 sem;
+ __u32 count;
+ __u32 max;
+};
+
+#define NTSYNC_IOC_BASE 0xf7
+
+#define NTSYNC_IOC_CREATE_SEM _IOWR(NTSYNC_IOC_BASE, 0, \
+ struct ntsync_sem_args)
+#define NTSYNC_IOC_DELETE _IOW (NTSYNC_IOC_BASE, 1, __u32)
+
+#endif
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 6/9] ntsync: Introduce NTSYNC_IOC_WAIT_ALL.
From: Elizabeth Figura @ 2024-01-24 0:40 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra, Elizabeth Figura
In-Reply-To: <20240124004028.16826-1-zfigura@codeweavers.com>
This corresponds to part of the functionality of the NT syscall
NtWaitForMultipleObjects(). Specifically, it implements the behaviour where
the third argument (wait_any) is FALSE, and it does not yet handle alertable
waits.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 241 ++++++++++++++++++++++++++++++++++--
include/uapi/linux/ntsync.h | 2 +
2 files changed, 235 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 2e8d3c2d51a4..2685363fae9e 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -23,7 +23,34 @@ struct ntsync_obj {
struct kref refcount;
spinlock_t lock;
+ /*
+ * any_waiters is protected by the object lock, but all_waiters is
+ * protected by the device wait_all_lock.
+ */
struct list_head any_waiters;
+ struct list_head all_waiters;
+
+ /*
+ * Hint describing how many tasks are queued on this object in a
+ * wait-all operation.
+ *
+ * Any time we do a wake, we may need to wake "all" waiters as well as
+ * "any" waiters. In order to atomically wake "all" waiters, we must
+ * lock all of the objects, and that means grabbing the wait_all_lock
+ * below (and, due to lock ordering rules, before locking this object).
+ * However, wait-all is a rare operation, and grabbing the wait-all
+ * lock for every wake would create unnecessary contention. Therefore we
+ * first check whether all_hint is zero, and, if it is, we skip trying
+ * to wake "all" waiters.
+ *
+ * This hint isn't protected by any lock. It might change during the
+ * course of a wake, but there's no meaningful race there; it's only a
+ * hint.
+ *
+ * Since wait requests must originate from user-space threads, we're
+ * limited here by PID_MAX_LIMIT, so there's no risk of saturation.
+ */
+ atomic_t all_hint;
enum ntsync_type type;
@@ -54,11 +81,25 @@ struct ntsync_q {
*/
atomic_t signaled;
+ bool all;
__u32 count;
struct ntsync_q_entry entries[];
};
struct ntsync_device {
+ /*
+ * Wait-all operations must atomically grab all objects, and be totally
+ * ordered with respect to each other and wait-any operations. If one
+ * thread is trying to acquire several objects, another thread cannot
+ * touch the object at the same time.
+ *
+ * We achieve this by grabbing multiple object locks at the same time.
+ * However, this creates a lock ordering problem. To solve that problem,
+ * wait_all_lock is taken first whenever multiple objects must be locked
+ * at the same time.
+ */
+ spinlock_t wait_all_lock;
+
struct xarray objects;
};
@@ -107,6 +148,8 @@ static int ntsync_char_open(struct inode *inode, struct file *file)
if (!dev)
return -ENOMEM;
+ spin_lock_init(&dev->wait_all_lock);
+
xa_init_flags(&dev->objects, XA_FLAGS_ALLOC);
file->private_data = dev;
@@ -132,8 +175,81 @@ static int ntsync_char_release(struct inode *inode, struct file *file)
static void init_obj(struct ntsync_obj *obj)
{
kref_init(&obj->refcount);
+ atomic_set(&obj->all_hint, 0);
spin_lock_init(&obj->lock);
INIT_LIST_HEAD(&obj->any_waiters);
+ INIT_LIST_HEAD(&obj->all_waiters);
+}
+
+static bool is_signaled(struct ntsync_obj *obj, __u32 owner)
+{
+ lockdep_assert_held(&obj->lock);
+
+ switch (obj->type) {
+ case NTSYNC_TYPE_SEM:
+ return !!obj->u.sem.count;
+ }
+
+ WARN(1, "bad object type %#x\n", obj->type);
+ return false;
+}
+
+/*
+ * "locked_obj" is an optional pointer to an object which is already locked and
+ * should not be locked again. This is necessary so that changing an object's
+ * state and waking it can be a single atomic operation.
+ */
+static void try_wake_all(struct ntsync_device *dev, struct ntsync_q *q,
+ struct ntsync_obj *locked_obj)
+{
+ __u32 count = q->count;
+ bool can_wake = true;
+ __u32 i;
+
+ lockdep_assert_held(&dev->wait_all_lock);
+ if (locked_obj)
+ lockdep_assert_held(&locked_obj->lock);
+
+ for (i = 0; i < count; i++) {
+ if (q->entries[i].obj != locked_obj)
+ spin_lock_nest_lock(&q->entries[i].obj->lock, &dev->wait_all_lock);
+ }
+
+ for (i = 0; i < count; i++) {
+ if (!is_signaled(q->entries[i].obj, q->owner)) {
+ can_wake = false;
+ break;
+ }
+ }
+
+ if (can_wake && atomic_cmpxchg(&q->signaled, -1, 0) == -1) {
+ for (i = 0; i < count; i++) {
+ struct ntsync_obj *obj = q->entries[i].obj;
+
+ switch (obj->type) {
+ case NTSYNC_TYPE_SEM:
+ obj->u.sem.count--;
+ break;
+ }
+ }
+ wake_up_process(q->task);
+ }
+
+ for (i = 0; i < count; i++) {
+ if (q->entries[i].obj != locked_obj)
+ spin_unlock(&q->entries[i].obj->lock);
+ }
+}
+
+static void try_wake_all_obj(struct ntsync_device *dev, struct ntsync_obj *obj)
+{
+ struct ntsync_q_entry *entry;
+
+ lockdep_assert_held(&dev->wait_all_lock);
+ lockdep_assert_held(&obj->lock);
+
+ list_for_each_entry(entry, &obj->all_waiters, node)
+ try_wake_all(dev, entry->q, obj);
}
static void try_wake_any_sem(struct ntsync_obj *sem)
@@ -234,14 +350,29 @@ static int ntsync_put_sem(struct ntsync_device *dev, void __user *argp)
if (!sem)
return -EINVAL;
- spin_lock(&sem->lock);
+ if (atomic_read(&sem->all_hint) > 0) {
+ spin_lock(&dev->wait_all_lock);
+ spin_lock_nest_lock(&sem->lock, &dev->wait_all_lock);
- prev_count = sem->u.sem.count;
- ret = put_sem_state(sem, args.count);
- if (!ret)
- try_wake_any_sem(sem);
+ prev_count = sem->u.sem.count;
+ ret = put_sem_state(sem, args.count);
+ if (!ret) {
+ try_wake_all_obj(dev, sem);
+ try_wake_any_sem(sem);
+ }
- spin_unlock(&sem->lock);
+ spin_unlock(&sem->lock);
+ spin_unlock(&dev->wait_all_lock);
+ } else {
+ spin_lock(&sem->lock);
+
+ prev_count = sem->u.sem.count;
+ ret = put_sem_state(sem, args.count);
+ if (!ret)
+ try_wake_any_sem(sem);
+
+ spin_unlock(&sem->lock);
+ }
put_obj(sem);
@@ -278,7 +409,7 @@ static int ntsync_schedule(const struct ntsync_q *q, ktime_t *timeout)
* Also, calculate the relative timeout.
*/
static int setup_wait(struct ntsync_device *dev,
- const struct ntsync_wait_args *args,
+ const struct ntsync_wait_args *args, bool all,
ktime_t *ret_timeout, struct ntsync_q **ret_q)
{
const __u32 count = args->count;
@@ -321,6 +452,7 @@ static int setup_wait(struct ntsync_device *dev,
q->task = current;
q->owner = args->owner;
atomic_set(&q->signaled, -1);
+ q->all = all;
q->count = count;
for (i = 0; i < count; i++) {
@@ -330,6 +462,16 @@ static int setup_wait(struct ntsync_device *dev,
if (!obj)
goto err;
+ if (all) {
+ /* Check that the objects are all distinct. */
+ for (j = 0; j < i; j++) {
+ if (obj == q->entries[j].obj) {
+ put_obj(obj);
+ goto err;
+ }
+ }
+ }
+
entry->obj = obj;
entry->q = q;
entry->index = i;
@@ -370,7 +512,7 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp)
if (copy_from_user(&args, argp, sizeof(args)))
return -EFAULT;
- ret = setup_wait(dev, &args, &timeout, &q);
+ ret = setup_wait(dev, &args, false, &timeout, &q);
if (ret < 0)
return ret;
@@ -432,6 +574,87 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp)
return ret;
}
+static int ntsync_wait_all(struct ntsync_device *dev, void __user *argp)
+{
+ struct ntsync_wait_args args;
+ struct ntsync_q *q;
+ ktime_t timeout;
+ int signaled;
+ __u32 i;
+ int ret;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+
+ ret = setup_wait(dev, &args, true, &timeout, &q);
+ if (ret < 0)
+ return ret;
+
+ /* queue ourselves */
+
+ spin_lock(&dev->wait_all_lock);
+
+ for (i = 0; i < args.count; i++) {
+ struct ntsync_q_entry *entry = &q->entries[i];
+ struct ntsync_obj *obj = entry->obj;
+
+ atomic_inc(&obj->all_hint);
+
+ /*
+ * obj->all_waiters is protected by dev->wait_all_lock rather
+ * than obj->lock, so there is no need to acquire it here.
+ */
+ list_add_tail(&entry->node, &obj->all_waiters);
+ }
+
+ /* check if we are already signaled */
+
+ try_wake_all(dev, q, NULL);
+
+ spin_unlock(&dev->wait_all_lock);
+
+ /* sleep */
+
+ ret = ntsync_schedule(q, args.timeout ? &timeout : NULL);
+
+ /* and finally, unqueue */
+
+ spin_lock(&dev->wait_all_lock);
+
+ for (i = 0; i < args.count; i++) {
+ struct ntsync_q_entry *entry = &q->entries[i];
+ struct ntsync_obj *obj = entry->obj;
+
+ /*
+ * obj->all_waiters is protected by dev->wait_all_lock rather
+ * than obj->lock, so there is no need to acquire it here.
+ */
+ list_del(&entry->node);
+
+ atomic_dec(&obj->all_hint);
+
+ put_obj(obj);
+ }
+
+ spin_unlock(&dev->wait_all_lock);
+
+ signaled = atomic_read(&q->signaled);
+ if (signaled != -1) {
+ struct ntsync_wait_args __user *user_args = argp;
+
+ /* even if we caught a signal, we need to communicate success */
+ ret = 0;
+
+ if (put_user(signaled, &user_args->index))
+ ret = -EFAULT;
+ } else if (!ret) {
+ ret = -ETIMEDOUT;
+ }
+
+ kfree(q);
+ return ret;
+}
+
static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
unsigned long parm)
{
@@ -445,6 +668,8 @@ static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
return ntsync_delete(dev, argp);
case NTSYNC_IOC_PUT_SEM:
return ntsync_put_sem(dev, argp);
+ case NTSYNC_IOC_WAIT_ALL:
+ return ntsync_wait_all(dev, argp);
case NTSYNC_IOC_WAIT_ANY:
return ntsync_wait_any(dev, argp);
default:
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 10f07da7864e..a5bed5a39b21 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -36,5 +36,7 @@ struct ntsync_wait_args {
struct ntsync_sem_args)
#define NTSYNC_IOC_WAIT_ANY _IOWR(NTSYNC_IOC_BASE, 3, \
struct ntsync_wait_args)
+#define NTSYNC_IOC_WAIT_ALL _IOWR(NTSYNC_IOC_BASE, 4, \
+ struct ntsync_wait_args)
#endif
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 4/9] ntsync: Introduce NTSYNC_IOC_PUT_SEM.
From: Elizabeth Figura @ 2024-01-24 0:40 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra, Elizabeth Figura
In-Reply-To: <20240124004028.16826-1-zfigura@codeweavers.com>
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)
+{
+ lockdep_assert_held(&sem->lock);
+
+ if (sem->u.sem.count + count < sem->u.sem.count ||
+ sem->u.sem.count + count > sem->u.sem.max)
+ return -EOVERFLOW;
+
+ sem->u.sem.count += count;
+ return 0;
+}
+
+static int ntsync_put_sem(struct ntsync_device *dev, void __user *argp)
+{
+ struct ntsync_sem_args __user *user_args = argp;
+ struct ntsync_sem_args args;
+ struct ntsync_obj *sem;
+ __u32 prev_count;
+ int ret;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+
+ sem = get_obj_typed(dev, args.sem, NTSYNC_TYPE_SEM);
+ if (!sem)
+ return -EINVAL;
+
+ spin_lock(&sem->lock);
+
+ prev_count = sem->u.sem.count;
+ ret = put_sem_state(sem, args.count);
+
+ spin_unlock(&sem->lock);
+
+ put_obj(sem);
+
+ if (!ret && put_user(prev_count, &user_args->count))
+ ret = -EFAULT;
+
+ return ret;
+}
+
static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
unsigned long parm)
{
@@ -142,6 +216,8 @@ static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
return ntsync_create_sem(dev, argp);
case NTSYNC_IOC_DELETE:
return ntsync_delete(dev, argp);
+ case NTSYNC_IOC_PUT_SEM:
+ return ntsync_put_sem(dev, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index d97afc138dcc..8c610d65f8ef 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -21,5 +21,7 @@ struct ntsync_sem_args {
#define NTSYNC_IOC_CREATE_SEM _IOWR(NTSYNC_IOC_BASE, 0, \
struct ntsync_sem_args)
#define NTSYNC_IOC_DELETE _IOW (NTSYNC_IOC_BASE, 1, __u32)
+#define NTSYNC_IOC_PUT_SEM _IOWR(NTSYNC_IOC_BASE, 2, \
+ struct ntsync_sem_args)
#endif
--
2.43.0
^ 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