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 == >> NUMA_NO_NODE. > >And when is "dfl_phys_nid == 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. >> >> Signed-off-by: Wei Yang <[email protected]> >> Acked-by: David Rientjes <[email protected]> >> --- >> arch/x86/mm/numa_emulation.c | 36 ++++++++++++++++++------------------ >> 1 file changed, 18 insertions(+), 18 deletions(-) >> >> 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 >> *numa_meminfo, int numa_dist_cnt) >> goto no_emu; >> } >> >> + /* >> + * Determine the max emulated nid and the default phys nid to use >> + * for unmapped nodes. >> + */ >> + max_emu_nid = 0; >> + dfl_phys_nid = NUMA_NO_NODE; >> + for (i = 0; i < ARRAY_SIZE(emu_nid_to_phys); i++) { >> + if (emu_nid_to_phys[i] != NUMA_NO_NODE) { >> + max_emu_nid = i; >> + if (dfl_phys_nid == NUMA_NO_NODE) >> + dfl_phys_nid = emu_nid_to_phys[i]; >> + } >> + } >> + if (dfl_phys_nid == NUMA_NO_NODE) { >> + pr_warn("NUMA: Warning: can't determine default physical node, >> disabling 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 look 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 concern here you mentioned is the coding style. > >-- >Regards/Gruss, > Boris. > >Good mailing practices for 400: avoid top-posting and trim the reply. -- Wei Yang Help you, Help me
signature.asc
Description: PGP signature
