linux-kernel - Re: [PATCH v3 3/4] mm/mempolicy: introduce MPOL_WEIGHTED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZbPWf9HbUNA1MELh@memverge.com>
Date: Fri, 26 Jan 2024 10:57:51 -0500
From: Gregory Price <gregory.price@...verge.com>
To: "Huang, Ying" <ying.huang@...el.com>
Cc: Gregory Price <gourry.memverge@...il.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
	corbet@....net, akpm@...ux-foundation.org, honggyu.kim@...com,
	rakie.kim@...com, hyeongtak.ji@...com, mhocko@...nel.org,
	vtavarespetr@...ron.com, jgroves@...ron.com,
	ravis.opensrc@...ron.com, sthanneeru@...ron.com,
	emirakhur@...ron.com, Hasan.Maruf@....com, seungjun.ha@...sung.com,
	hannes@...xchg.org, dan.j.williams@...el.com,
	Srinivasulu Thanneeru <sthanneeru.opensrc@...ron.com>
Subject: Re: [PATCH v3 3/4] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE
 for weighted interleaving

On Fri, Jan 26, 2024 at 03:10:49PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@...il.com> writes:
> 
> > +		} else if (pol == current->mempolicy &&
> > +				(pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
> > +			if (pol->cur_il_weight)
> > +				*policy = current->il_prev;
> > +			else
> > +				*policy = next_node_in(current->il_prev,
> > +						       pol->nodes);
> 
> It appears that my previous comments about this is ignored.
> 
> https://lore.kernel.org/linux-mm/875xzkv3x2.fsf@yhuang6-desk2.ccr.corp.intel.com/
> 
> Please correct me if I am wrong.
>

The fix is in the following patch.  I'd originally planned to squash the
atomic patch into this one, but decided against it because it probably
warranted isolated scrutiny.

@@ -973,8 +974,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
                        *policy = next_node_in(current->il_prev, pol->nodes);
                } else if (pol == current->mempolicy &&
                                (pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
-                       if (pol->cur_il_weight)
-                               *policy = current->il_prev;
+                       int cweight = atomic_read(&pol->cur_il_weight);
+
+                       if (cweight & 0xFF)
+                               *policy = cweight >> 8;

in this we return the node the weight applies to, otherwise we return
whatever is after il_prev.

I can pull this fix ahead.

> > +	/* if now at 0, move to next node and set up that node's weight */
> > +	if (unlikely(!policy->cur_il_weight)) {
> > +		me->il_prev = node;
> > +		next = next_node_in(node, policy->nodes);
> > +		rcu_read_lock();
> > +		table = rcu_dereference(iw_table);
> > +		/* detect system-default values */
> > +		weight = table ? table[next] : 1;
> > +		policy->cur_il_weight = weight ? weight : 1;
> > +		rcu_read_unlock();
> > +	}
> 
> It appears that the code could be more concise if we allow
> policy->cur_il_weight == 0.  Duplicated code are in
> alloc_pages_bulk_array_weighted_interleave() too.  Anyway, can we define
> some function to reduce duplicated code.
> 

This is kind of complicated by the next patch, which places the node and
the weight into the same field to resolve the stale weight issue.

In that patch (cur_il_weight = 0) means "cur_il_weight invalid",
because the weight part can only be 0 when:

a) an error occuring during bulk allocation
b) a rebind event

I'll take some time to think about whether we can do away with
task->il_prev (as your next patch notes mentioned).


> > +		/* Otherwise we adjust nr_pages down, and continue from there */
> > +		rem_pages -= pol->cur_il_weight;
> > +		pol->cur_il_weight = 0;
> 
> This break the rule to keep pol->cur_il_weight != 0 except after initial
> setup.  Is it OK?
> 

The only way cur_il_weight can leave this function 0 at this point is if
an error occurs (specifically the failure to kmalloc immediately next).

If we don't clear cur_il_weight here, then we have a stale weight, and
the next allocation pass will over-allocate on the current node.

This semantic also changes a bit in the next patch, but is basically the
same.  If il_weight is 0, then either an error occurred or a rebind
event occured.

> > +				/* resume from this node w/ remaining weight */
> > +				resume_node = prev_node;
> > +				resume_weight = weight - (node_pages % weight);
> 
> resume_weight = weight - delta; ?
>

ack

~Gregory