DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com ECEA5C04B939
Message-ID: <1498575358.20270.114.camel@redhat.com>
Subject: Re: [PATCH 4/4] sched,fair: remove effective_load
From: Rik van Riel <riel@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, jhladky@redhat.com, mingo@kernel.org,
        mgorman@suse.de
Date: Tue, 27 Jun 2017 10:55:58 -0400
In-Reply-To: <20170627053906.GA7287@worktop>
References: <20170623165530.22514-1-riel@redhat.com>
         <20170623165530.22514-5-riel@redhat.com> <20170626144437.GB4941@worktop>
         <20170626144611.GA5775@worktop> <1498488941.13083.43.camel@redhat.com>
         <20170626150401.GC4941@worktop> <1498490454.13083.45.camel@redhat.com>
         <20170626161250.GD4941@worktop> <1498505689.13083.49.camel@redhat.com>
         <20170627053906.GA7287@worktop>
Content-Type: multipart/signed; micalg="pgp-sha256";
        protocol="application/pgp-signature"; boundary="=-n2XLQ4is+qDoYz2C9iBY"
Mime-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4495
Lines: 120


--=-n2XLQ4is+qDoYz2C9iBY
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, 2017-06-27 at 07:39 +0200, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 03:34:49PM -0400, Rik van Riel wrote:
> > On Mon, 2017-06-26 at 18:12 +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 26, 2017 at 11:20:54AM -0400, Rik van Riel wrote:
> > >=20
> > > > Oh, indeed.=C2=A0=C2=A0I guess in wake_affine() we should test
> > > > whether the CPUs are in the same NUMA node, rather than
> > > > doing cpus_share_cache() ?
> > >=20
> > > Well, since select_idle_sibling() is on LLC; the early test on
> > > cpus_share_cache(prev,this) seems to actually make sense.
> > >=20
> > > But then cutting out all the other bits seems wrong. Not in the
> > > least
> > > because !NUMA_BALACING should also still keep working.
> >=20
> > Even when !NUMA_BALANCING, I suspect it makes little sense
> > to compare the loads just one the cores in question, since
> > select_idle_sibling() will likely move the task somewhere
> > else.
> >=20
> > I suspect we want to compare the load on the whole LLC
> > for that reason, even with NUMA_BALANCING disabled.
>=20
> But we don't have that data around :/ One thing we could do is try
> and
> keep a copy of the last s*_lb_stats around in the sched_domain_shared
> stuff or something and try and use that.
>=20
> That way we can strictly keep things at the LLC level and not confuse
> things with NUMA.
>=20
> Similarly, we could use that same data to then avoid re-computing
> things
> for the NUMA domain as well and do away with numa_stats.

That does seem like a useful optimization, though
I guess we would have to invalidate the cached data
every time we actually move a task?

The current code simply walks all the CPUs in the
cpumask_t, and adds up capacity and load. Doing
that appears to be better than poor task placement
(Jirka's numbers speak for themselves), but optimizing
this code path does seem like a worthwhile goal.

I'll look into it.

> > > > Or, alternatively, have an update_numa_stats() variant
> > > > for numa_wake_affine() that works on the LLC level?
> > >=20
> > > I think we want to retain the existing behaviour for everything
> > > larger than LLC, and when NUMA_BALANCING, smaller than NUMA.
> >=20
> > What do you mean by this, exactly?
>=20
> As you noted, when prev and this are in the same LLC, it doesn't
> matter
> and select_idle_sibling() will do its thing. So anything smaller than
> the LLC need not do anything.
>=20
> When NUMA_BALANCING we have the numa_stats thing and we can, as you
> propose use that.
>=20
> If LLC < NUMA or !NUMA_BALANCING we have a region that needs to do
> _something_.

Agreed. I will fix this. Given that this is a bit
of a corner case, I guess I can fix this with follow-up
patches, to be merged into -tip before the whole series
is sent on to Linus?

> > > Also note that your use of task_h_load() in the new numa thing
> > > suffers
> > > from exactly the problem effective_load() is trying to solve.
> >=20
> > Are you saying task_h_load is wrong in task_numa_compare()
> > too, then?=C2=A0=C2=A0Should both use effective_load()?
>=20
> I need more than the few minutes I currently have, but probably. The
> question is of course, how much does it matter and how painful will
> it
> be to do it better.

I suspect it does not matter at all currenly, since the=C2=A0
load balancing code does not use effective_load, and
having the wake_affine logic calculate things differently
from the load balancer is likely to result in both pieces
of code fighting against each other.

I suspect we should either use task_h_load everywhere,
or effective_load everywhere, but not have a mix and
match situation where one is used in some places, and
the other in others.

--=20
All rights reversed
--=-n2XLQ4is+qDoYz2C9iBY
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAABCAAGBQJZUnH+AAoJEM553pKExN6DAkwIALP+sJTZ4pZqHN5raBd/sR5u
HhuJBFPndl9xmwsdbEJx8kkYJrX0Qo/I5707I4a9a2gbT41P9TfkTGr8w7BqG4YF
zwWAlrO3OS2ENCk9MHxYRaBXiD386zOadb297CFDBQNQOg6FvbvkJHPtq3xxTWtB
TzgvpkUw3XYphJc1HXWenU+nzyy6LTQtBMUNMcqzxaY7/3eunyiKxNKpDGp5PyOY
yr3ghlGrXRpN1j5MdN5Ft7LmHpMpWsVWTZwbKhUUl/C66aBz0cv2fTGoh0oGVz0u
Xfc8HdeQYEBtqtR/IRK1I0sZ8ZCwSROZAFowNbm2pLsw8uW/uDlwMAlOvckMnso=
=rnnP
-----END PGP SIGNATURE-----

--=-n2XLQ4is+qDoYz2C9iBY--