Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751692AbdFZXLl (ORCPT ); Mon, 26 Jun 2017 19:11:41 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:33506 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751426AbdFZXLa (ORCPT ); Mon, 26 Jun 2017 19:11:30 -0400 Date: Tue, 27 Jun 2017 07:11:27 +0800 From: Wei Yang To: Borislav Petkov Cc: Wei Yang , kirill@shutemov.name, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, tj@kernel.org, rientjes@google.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2 1/3] x86/numa_emulation: fix potential memory leak Message-ID: <20170626231127.GA53180@WeideMacBook-Pro.local> Reply-To: Wei Yang References: <20170502130453.5933-1-richard.weiyang@gmail.com> <20170502130453.5933-2-richard.weiyang@gmail.com> <20170626153149.b2x5pcipzuzaguuw@pd.tnic> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="J2SCkAp4GZ/dPZZf" Content-Disposition: inline In-Reply-To: <20170626153149.b2x5pcipzuzaguuw@pd.tnic> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3705 Lines: 115 --J2SCkAp4GZ/dPZZf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 26, 2017 at 05:31:49PM +0200, Borislav Petkov wrote: >On Tue, May 02, 2017 at 09:04:51PM +0800, Wei Yang wrote: >> numa_emulation() needs to allocate a space for phys_dist[] temporarily, > >s/a // > >> while current code may miss to release this when dfl_phys_nid =3D=3D >> NUMA_NO_NODE. > >And when is "dfl_phys_nid =3D=3D NUMA_NO_NODE"? What does it mean actually? > It means numa emulation is not properly configured. >> It is observed in code review instead of in a real case. >> This patch fixes this by re-order the code path. >>=20 >> Signed-off-by: Wei Yang >> Acked-by: David Rientjes >> --- >> arch/x86/mm/numa_emulation.c | 36 ++++++++++++++++++------------------ >> 1 file changed, 18 insertions(+), 18 deletions(-) >>=20 >> diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c >> index a8f90ce3dedf..eb017c816de6 100644 >> --- a/arch/x86/mm/numa_emulation.c >> +++ b/arch/x86/mm/numa_emulation.c >> @@ -353,6 +353,24 @@ void __init numa_emulation(struct numa_meminfo *num= a_meminfo, int numa_dist_cnt) >> goto no_emu; >> } >> =20 >> + /* >> + * Determine the max emulated nid and the default phys nid to use >> + * for unmapped nodes. >> + */ >> + max_emu_nid =3D 0; >> + dfl_phys_nid =3D NUMA_NO_NODE; >> + for (i =3D 0; i < ARRAY_SIZE(emu_nid_to_phys); i++) { >> + if (emu_nid_to_phys[i] !=3D NUMA_NO_NODE) { >> + max_emu_nid =3D i; >> + if (dfl_phys_nid =3D=3D NUMA_NO_NODE) >> + dfl_phys_nid =3D emu_nid_to_phys[i]; >> + } >> + } >> + if (dfl_phys_nid =3D=3D NUMA_NO_NODE) { >> + pr_warn("NUMA: Warning: can't determine default physical node, disabl= ing emulation\n"); >> + goto no_emu; >> + } >> + > >Well, that function numa_emulation() does a looot of things and could >very well be split into subfunctions, which should make the whole path >more readable. > You are right. The whole function contains several blocks which could be split. While this patch focus on the memory leak issue. For readable code, = we could come up with a separate patch to refine it. >And this chunk you're moving is kinda begging to be a separate >function... Well, to this particular piece, have a for loop within a function doesn't l= ook like a big deal to me. So you prefer to take every for loop in this function out? Last but not the least, these are two issues: The problem this patch wants to address is the memory leak, while the conce= rn here you mentioned is the coding style. > >--=20 >Regards/Gruss, > Boris. > >Good mailing practices for 400: avoid top-posting and trim the reply. --=20 Wei Yang Help you, Help me --J2SCkAp4GZ/dPZZf Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJZUZSfAAoJEKcLNpZP5cTddXgP/jN3IdmO8SYyc4/uIJmjia5e 1sVB4E8Yy2xOFQGBolRuyqH/EKBhhaVOtWfZPgySEGmsdNhx+BU+hJ0wvqiyyQGK FQG/sCVFAMKn0t4yY0NVWW9JfiT7nyFzp7RRAedQoltV2+Hq3Q7F5rbXj/Y+3GwL mKYeyAWkf/F6Iibj1+zpciHu7l8QAKWhjQimLlfhZOkP1ny3Jv9JdnW/5Hj3DHrN poY/AQoLO4CGPIxGk4DC3Jn5W18+eqT8FunAMdHtQtAq9rS4YeXiKcChU+2KGK0g 90Gu3/tAQRalYY3fIok65tapu9+e8VHI8XEtIz1rff9qoOqDPBvHoZ8VRZ8mjgx6 qPNpSI4LfIeiErW80Kzp4Ku83gRPjkq+EzfJTGYfW09zOFL7QaSXNytXA7rld8Gy aPyXBH/heh6bnIMuKKJE49QTq+m8gCXj63iUOHXtMEpNTU546wMaNPZaaNJInMr8 NrOHgqckO/SMNAVWSvfu59A9GwhQY3GeBHD5TBV/K7s2CMA7xKuCnM5HsusW1dg+ ARL9aHWDEG9zG/K0xSpSYGhDCwKXxnJipqvqk8KpkNIHoE7c2lwUNMICFpbps33Q INoVJjB7tmZLrOGHdmXiDWRHd5LSV7TP9Urwr5+S00XsufQ8+/80cPHB7LcpeVVB jAus4WcfQOcv0qbFSUgc =LrAy -----END PGP SIGNATURE----- --J2SCkAp4GZ/dPZZf--