search

pmodels / mpich

Official MPICH Repository
http://www.mpich.org
Other
535 stars 280 forks source link

PAMI memory leaks #1947

Closed mpichbot closed 7 years ago

mpichbot commented 7 years ago

Originally by robl on 2013-10-04 16:52:01 -0500

The pami device appears to be either leaking resources, or it is managing resources in a way that confuses the MPICH memory tracing tools.

These four memory allocations in MPIDI_Comm_coll_query: http://trac.mpich.org/projects/mpich/browser/src/mpid/pamid/src/comm/mpid_selectcolls.c#L617

617       if(num_algorithms[0] || num_algorithms[1])
618       {
619          comm->mpid.coll_algorithm[i][0] = (pami_algorithm_t *)
620                MPIU_Malloc(sizeof(pami_algorithm_t) * num_algorithms[0]);
621          comm->mpid.coll_metadata[i][0] = (pami_metadata_t *)
622                MPIU_Malloc(sizeof(pami_metadata_t) * num_algorithms[0]);
623          comm->mpid.coll_algorithm[i][1] = (pami_algorithm_t *)
624                MPIU_Malloc(sizeof(pami_algorithm_t) * num_algorithms[1]);
625          comm->mpid.coll_metadata[i][1] = (pami_metadata_t *)
626                MPIU_Malloc(sizeof(pami_metadata_t) * num_algorithms[1]);
627          comm->mpid.coll_count[i][0] = num_algorithms[0];
628          comm->mpid.coll_count[i][1] = num_algorithms[1];

and this memory allocation in MPID_VCRT_Create: http://trac.mpich.org/projects/mpich/browser/src/mpid/pamid/src/mpid_vc.c#L61

61  int MPID_VCRT_Create(int size, MPID_VCRT *vcrt_ptr)
62  {
63      struct MPIDI_VCRT * vcrt;
64      int i,result;
65  
66      vcrt = MPIU_Malloc(sizeof(struct MPIDI_VCRT) + size*sizeof(MPID_VCR));
67      for(i = 0; i < size; i++)
68          vcrt->vcr_table[i] = MPIU_Malloc(sizeof(struct MPID_VCR_t));

MikeB, BobC, and I had a short email discussion about this back in July 2013 on the mpich-ibm list but it seems to have been set aside for more pressing concerns.

mpichbot commented 7 years ago

Originally by blocksom on 2013-10-07 08:52:14 -0500

... adding comments from the mailing list:

I think this came up before. Those are supposed to be freed in comm destroy. Is that not happening?

http://trac.mpich.org/projects/mpich/browser/src/mpid/pamid/src/comm/mpid_comm.c#L389

mpichbot commented 7 years ago

Originally by robl on 2013-10-07 14:57:33 -0500

Yeah, those look right to me as well, but this is not a part of MPICH I spend any time thinking about.

I did a little (just a little) poking around with cscope: Let's walk up the calling functions. Cscope seems to think it's pretty linear for most of the way:

inlined function MPIR_Comm_release

or MPIR_Comm_release_always

mpichbot commented 7 years ago

Originally by robl on 2013-10-07 15:01:04 -0500

The question then is "are the MPICH memory tracing tools defective" or "is pami cleanup code not getting called".

I briefly explored using valgrind to get a second opinion on the resource leaks, but I don't have access to pami-on-linux.

bgclang has a "memory address checker" but that's more for memory errors, not memory leaks. It's also apparently a bit of work to get pami compiling with bgclang.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-09 16:26:09 -0500

Is there a test case? ctxdup isn't showing me any problems.

mpichbot commented 7 years ago

Originally by robl on 2013-10-09 16:59:02 -0500

Attachment added: leak-check.c (1.2 KiB) a simple test to demonstrate resource leaks on blue gene

mpichbot commented 7 years ago

Originally by robl on 2013-10-09 17:01:14 -0500

I've added a simple test case. open and close a file. don't even have to write to it. ROMIO is doing a lot more under the covers than would appear at first glance: duping communicators, placing attributes on COMM_SELF, things like this, but the test is --enable-g=all clean under ch3, and gives me this on bluegene:

leaked context IDs detected: mask=0x166a340 mask[0]=0xfffffff8
[0] 0 at [0x00000019c58255b8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c58254f8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c58253f8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5825338], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5825218], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c5825158], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 160 at [0x00000019c5824ff8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 16 at [0x00000019c5824f38], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 80 at [0x00000019c5824dd8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 8 at [0x00000019c5824d18], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5824c18], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5824b58], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 80 at [0x00000019c5824a58], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 8 at [0x00000019c5824998], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5824898], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c58247d8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 80 at [0x00000019c58246d8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 8 at [0x00000019c5824618], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5824518], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5824458], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5824398], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c58242d8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 160 at [0x00000019c5824178], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 16 at [0x00000019c58240b8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5823f98], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c5823ed8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5823dd8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5823d18], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5823bf8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c5823b38], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5823a38], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5823978], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5823858], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c5823798], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5823698], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c58235d8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c58234b8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c58233f8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c58232f8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5823238], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5823118], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
[0] 0 at [0x00000019c5823058], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
[0] 80 at [0x00000019c5822f58], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
[0] 8 at [0x00000019c5822e98], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
[0] 0 at [0x00000019c5822d78], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]

... and so on.

mpichbot commented 7 years ago

Originally by robl on 2013-10-10 09:08:46 -0500

Ooooh! here's an even simpler one. Just dup a communicator then free it.

leaked context IDs detected: mask=0x1666700 mask[0]=0xfffffff8
mpichbot commented 7 years ago

Originally by robl on 2013-10-10 09:09:57 -0500

Attachment added: leak-test-dupcomm.c (1.2 KiB) even simpler test case: just dup COMM_WORLD, then free it.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-10 09:21:25 -0500

I suspect a couple things and I'll take a look at the ROMIO problem.

Also, the other developer knows about the MPID_VCRT_Create leaks and can comment on them.

mpichbot commented 7 years ago

Originally by robl on 2013-10-10 10:27:12 -0500

Hi Bob. BG ROMIO is not leaking communicators, or else we'd see a message about COMM resources not being released. Something like this:

In direct memory block for handle type COMM, 1 handles are still allocated

so of course the duped communicator itself is not leaking. but something in pami's communicator cleanup code is, well, not cleaning up.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-10 10:50:48 -0500

Ok, I haven't used --enable-g=all so I'm not clear on what it's telling me.

And you're on the master branch? We know comm world (and the associated pami algorithm storage) isn't freed. And there's a VCRT free that is commented out. I think I need to wait for those fixes from PE and retest.

And just to add to the confusion, I'm running the BGQ V1R2M1 branch where we've made comm free/destroy non-blocking. Context id's and VCRT's are freed when the geometry is asynchronously destroyed and the cb_done is called. But I haven't yet seen a real leak of the pami algorithm storage or context ids (except comm world).

void geom_destroy_cb_done(void *ctxt, void *data, pami_result_t err)
{
   int mpi_errno = MPI_SUCCESS;
   MPIDI_Post_geom_destroy_t *geom_destroy_post = (MPIDI_Post_geom_destroy_t *) data;
/* 
   When this patch moves to master... free PE's copy of the task list.
   pami_task_t *tasks = geom_destroy_post->tasks;
   MPID_VCR_FREE_LPIDS(tasks);
*/
   /* BGQ (and maybe other platforms) shares the vcrt with MPICH so release it now.*/
   mpi_errno = MPID_VCRT_Release(geom_destroy_post->vcrt, 0 /* isDisconnect */);
   if (mpi_errno != MPI_SUCCESS) 
   {
      MPID_Abort( 0, mpi_errno, 1, "PAMID failed to release VCRT" );
   // MPIU_ERR_POP(mpi_errno);
   }
   MPIR_Free_contextid( geom_destroy_post->recvcontext_id ); 
   MPIU_TestFree(&data);
}
mpichbot commented 7 years ago

Originally by robl on 2013-10-10 12:26:32 -0500

Replying to bobc:

Ok, I haven't used --enable-g=all so I'm not clear on what it's telling me.

Our in-tree memory debugging/tracing facility is kind of a pain in the neck, especially if you are used to the clear reporting of valgrind, purify, or tools like that. However, of those options, which one works on Blue Gene at scale?

I'm sorry for pushing relatively hard for what is a relatively minor leak, but once MPICH is clean, it makes it a lot easier to find the leaks in higher level libraries.

And you're on the master branch?

Always. Thanks to Mike Blocksome's heroic efforts to drive down the deltas between IBM's tree and master, I just work directly on master. Seems like it's also, IP-wise, a lot easier for IBM to take from MPICH than it is for me to contribute into the various IBM branches.

We know comm world (and the associated pami algorithm storage) isn't freed. And there's a VCRT free that is commented out. I think I need to wait for those fixes from PE and retest.

Well, considering a simple point-to-point testcase was not sufficient to trigger the error, whatever internal comm-world cleanup code MPICH common code calls seems to be sufficient?

And just to add to the confusion, I'm running the BGQ V1R2M1 branch where we've made comm free/destroy non-blocking. Context id's and VCRT's are freed when the geometry is asynchronously destroyed and the cb_done is called.

I think the way this memory debugging facility works is it traverses its bookkeeping at MPI_Finalize(), so as long as the context ids and vcrts are freed at some point, everyone will be happy.

But I haven't yet seen a real leak of the pami algorithm storage or context ids (except comm world).

I don't know what you mean by "real leak": in C, it's either a leak or it's not, right? Well, we can dicker over calling a fixed amount of memory left behind at exit a "real leak" or not, but as I said earlier in this reply, tidying up these memory allocations means we can focus on the libraries building on top of MPICH and not get distracted with 200 lines of carping about mpich internals.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-10 13:44:05 -0500

Ok, what your leak-test-dupcomm.c is telling me is 

stdout[0]: leaked context IDs detected: mask=0x1664918 mask[0]=0xfffffff8

which is 

/* the first three values are already used (comm_world, comm_self,
       and the internal-only copy of comm_world) */

Right? No 'real leak' -- i.e. not the dup'd comm :) Just what the PE guys are already working on. We've ignored them (even on BG/P I think) because they live until finalize and we don't particularly care (?!) what happens after finalize. But yes they should be cleaned up to avoid this sort of mess and debug/tool issues.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-10 14:27:13 -0500

I removed the dup to see what happens, but the debug check isn't even called when the testcase is just init and finalize. (Put a print in the function). So I didn't prove anything.

$ grep -c MPIU_CheckContextIDsOnFinalize leak*comm.out
leak-test-dupcomm.out:1
leak-test-nodupcomm.out:0
mpichbot commented 7 years ago

Originally by robl on 2013-10-10 15:14:11 -0500

the debug check isn't even called when the testcase is just init and finalize

Yeah, fun feature about MPICH: I don't know the specifics but parts of its initialization are deferred until a "heavy" enough call is made. i.e. not everything happens in MPI_Init. (At least that was the case 5 years ago when I was experimenting with dynamic processes and the name publishing interface)

mpichbot commented 7 years ago

Originally by haizhu on 2013-10-11 15:41:45 -0500

I put out a patch to fix the memory leak issue in MPID_VCRT_Create()/MPID_VCRT_Release(), also I add a free of MPI_COMM_WORLD and MPI_COMM_SELF at mpid_finalize() time. The patch is being tested and reviewed.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-15 12:12:01 -0500

So, with an enable-g=all build, what else should I be looking for besides "leaked context IDs detected"?

mpichbot commented 7 years ago

Originally by robl on 2013-10-15 13:06:24 -0500

Replying to bobc:

So, with an enable-g=all build, what else should I be looking for besides "leaked context IDs detected"?

You're looking for any output that didn't come from your application. Sometimes the message is quite clear, as when it mentions COMM, INFO, TYPE, or ATTR references.

I pasted a big wall of output a few replies back. Here's one of the less-clear lines:

[0] 80 at [0x00000019c58253f8], pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]

Valgrind it aint! Those numbers are

Process rank, size of allocation, address of allocation, file where allocation was made, line number.

mpichbot commented 7 years ago

Originally by blocksom on 2013-10-16 13:27:29 -0500

A commit has been pushed to the ticket-1947 branch in the mpich-ibm git repository. This commit fixes the leaks associated with dynamic tasking.

The remaining leaks are still being worked, but may require code changes to the BGQ and/or PE pami library which will cause problems for an "open source build" of mpich as the pami code is not yet publicly available.

So ... still working on it, but the dynamic tasking problem has been fixed.

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 14:03:42 -0500

I cherry-picked [5735cd270] onto mpich master and ran the leak testcase on vesta with one process. Got a seg fault:

% bgq_stack --demangle leak-testcase core.0
------------------------------------------------------------------------
Program   : /gpfs/vesta-home/robl/src/mpich/./leak-testcase
------------------------------------------------------------------------
+++ID Rank: 0, TGID: 1, Core: 0, HWTID:0 TID: 1 State: RUN

0000000001396bb8
operator delete(void*)
/bgsys/drivers/V1R2M1/ppc64/toolchain/gnu/gcc-4.4.6/libstdc++-v3/libsupc++/del_op.cc:44

00000000011046d8
PAMI::Geometry::Common::freeAllocations_impl()
/bgsys/source/srcV1R2M1.24367/comm/sys/buildtools/pami/algorithms/geometry/Geometry.h:859

0000000001016d68
geom_destroy_wrapper
/home/robl/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c:211

00000000010178a4
MPIDI_Coll_comm_destroy
/home/robl/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c:428

0000000001017a00
MPIDI_Comm_destroy
/home/robl/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c:51

0000000001008cd4
MPIR_Comm_delete_internal  
/home/robl/src/mpich/src/mpi/comm/commutil.c:1779

000000000101eb40
MPIR_Comm_release
/home/robl/src/mpich/src/include/mpiimpl.h:1284

000000000101ebe8
MPID_Finalize
/home/robl/src/mpich/src/mpid/pamid/src/mpid_finalize.c:137

00000000010028d4
PMPI_Finalize
/home/robl/src/mpich/src/mpi/init/finalize.c:211
000000000100089c
00000012.long_branch_r2off.__libc_start_main+0
:0

00000000013ba928
generic_start_main
/bgsys/drivers/V1R2M1/ppc64/toolchain/gnu/glibc-2.12.2/csu/../csu/libc-start.c:226

00000000013bac24
__libc_start_main
/bgsys/drivers/V1R2M1/ppc64/toolchain/gnu/glibc-2.12.2/csu/../sysdeps/unix/sysv/linux/powerpc/libc-start.c:194

0000000000000000
??
??:0

So, I'd like to know what's going on.

mpichbot commented 7 years ago

Originally by blocksom on 2013-10-16 14:21:22 -0500

uhg .. sorry about that. The code that depends on pami slipped in there on accident - totally my bad.

Here's a new commit -> branch ticket-1947-1 in the mpich-ibm git repository.

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 14:48:56 -0500

no more segfault, but i still see 80 leaked memory regions (that's actually up two from before the patch!); that's 3552 bytes if I sum it all up, more than the 3536 bytes from before the patch.

Are you guys testing against the test cases in this bug report or are you using something else? Because whatever else you're using isn't cutting it....

mpichbot commented 7 years ago

Originally by haizhu on 2013-10-16 15:18:09 -0500

hmm, I am curious why the patch could introduce more leak. I didn't use the leak tool attached to the bug report, Bob may have run it, but I used the full debug library with -enable-g=full, which reports some (not all) of the leak was fixed. Can you share your leak report?

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 15:32:01 -0500

I hope that is just a typo. --enable-g=full doesn't do anything except cause configure to emit an easy to overlook warning "Unknown value $option for enable-g"

the option you should use is --enable-g=all

==rob

mpichbot commented 7 years ago

Originally by haizhu on 2013-10-16 16:07:10 -0500

Yes, sorry, it is a typo. I used -enable-g=all for leak report versus -enable-g=none for regular build.

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 16:16:53 -0500

Replying to haizhu:

I didn't use the leak tool attached to the bug report,

Just to be clear, the codes attached to this bug are not tools, but rather 4 line test cases.

Bob may have run it, but I used the full debug library with -enable-g=full, which reports some (not all) of the leak was fixed. Can you share your leak report?

my leak report? it's unchanged from earlier. The memory allocations in MPIDI_Comm_coll_query and MPID_VCRT_Create are not cleaned up on program exit. Do you really need to see the output? It's essentially what's listed in comment 5.

mpichbot commented 7 years ago

Originally by blocksom on 2013-10-16 16:26:51 -0500

I think the difference here is that Haizhu is probably testing on PE and Rob is probably testing on BGQ.

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 16:31:02 -0500

thanks mike. that's exactly what I'm doing. Did not realize the code paths would be so different. Glad PE is clean!

mpichbot commented 7 years ago

Originally by haizhu on 2013-10-16 16:37:54 -0500

With MPI_COMM_WORLD not being freed, you'll still see some leak report in mpid_vc.c because that memory associated with MPI_COMM_WORLD is not freed. In your leak report, do you see the number bumped up for mpid_vc.c (MPID_VCR_Create)?

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 16:52:51 -0500

Both before and after the ticket-1947-1 patch, I'm seeing mpid_vc.c leaks. Let's try this: I sorted the output by file name, cut out the memory addresses then diffed the two experiments. So yeah, there is now a new mpid_vc.c leak. 

% diff -burpN pre-patch-leaks.out post-patch-leaks.out 
--- pre-patch-leaks.out 2013-10-16 21:50:14.994135000 +0000
+++ post-patch-leaks.out        2013-10-16 21:50:20.962969000 +0000
@@ -1,9 +1,11 @@
 504 /home/robl/src/mpich/src/mpi/comm/commutil.c[260]
 504 l/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c[232]
-24 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[66]
-24 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[66]
-8 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[68]
-8 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[68]
+24 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[70]
+24 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[70]
+8 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[71]
+8 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[71]
+8 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[74]
+8 ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[74]
 16 pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
 16 pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
 16 pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
mpichbot commented 7 years ago

Originally by bobc on 2013-10-16 17:08:42 -0500

We have a variety of logistical problems here. I'm testing on released BGQ branches, NOT master branch. It's different code than either Haizhu or Rob are using. But's it the supported BGQ V1R2M1 code.

So I don't really have the code to test Haizhu's dynamic tasking/vcrt changes.

I tested freeing comm world, self, and (conditionally) the internal copy of comm world. With those fixes, which Mike did not check in, and with a fix to PAMI I see two remaining leaks but I didn't get to them today.

stderr[0]: /dev/mpich2/src/mpid/pamid/src/comm/mpid_comm.c[306]
stderr[0]: bgq/comm/lib/dev/mpich2/src/mpi/comm/commutil.c[248]

That appears to be:

  geom_destroy_post = MPIU_Calloc0(1, MPIDI_Post_geom_destroy_t);

and

        MPIU_CHKPMEM_CALLOC(ops, struct MPID_Collops *, sizeof(struct MPID_Collops), mpi_errno, "default intracomm collops");

I don't know what the latter is offhand.

mpichbot commented 7 years ago

Originally by haizhu on 2013-10-16 19:38:45 -0500

Bob, did you include my change to MPID_VCRT_Create/MPID_VCRT_Free in your testing?

mpichbot commented 7 years ago

Originally by robl on 2013-10-16 22:00:43 -0500

Replying to bobc:

We have a variety of logistical problems here. I'm testing on released BGQ branches, NOT master branch. It's different code than either Haizhu or Rob are using. But's it the supported BGQ V1R2M1 code.

I'd be happy to get on the same tree/branch/whatever as either BobC or Haizhu. The branch mpich-ibm/BGQ/IBM_V1R2M1 hasn't seen any changes since 7 August.

mpichbot commented 7 years ago

Originally by bobc on 2013-10-17 08:05:13 -0500

Replying to haizhu:

Bob, did you include my change to MPID_VCRT_Create/MPID_VCRT_Free in your testing?

No. I don't have the underlying dynamic tasking updates that you're fixing, so there's really nothing to fix/test in BGQ.

For example, the BGQ branch version of mpid_vc.c in my branch hasn't changed (except copyrights/doxygen) since:

commit 636ebb9c8847701fd9b4a64124cba11615c16f0d
Author: Joe Ratterman <jratt@us.ibm.com>
Date:   Wed Jul 7 17:55:30 2010 -0500
mpichbot commented 7 years ago

Originally by bobc on 2013-10-17 09:33:15 -0500

Replying to robl:

I'd be happy to get on the same tree/branch/whatever as either BobC or Haizhu. The branch mpich-ibm/BGQ/IBM_V1R2M1 hasn't seen any changes since 7 August.

Looks like you have efix04, which is good enough. We're preparing the next efix but it's not checked in. Can you build that branch? Mike did work to make master build, so I don't know if v1r2m1 is buildable outside IBM.

mpichbot commented 7 years ago

Originally by robl on 2013-10-17 12:26:46 -0500

Replying to bobc:

Replying to robl:

I'd be happy to get on the same tree/branch/whatever as either BobC or Haizhu. The branch mpich-ibm/BGQ/IBM_V1R2M1 hasn't seen any changes since 7 August.

Looks like you have efix04, which is good enough. We're preparing the next efix but it's not checked in. Can you build that branch? Mike did work to make master build, so I don't know if v1r2m1 is buildable outside IBM.

Configures ok if you use automake-1.13.1

Needed to tell configure where to find pami.h

I get a build error with anything BG specific that looks like this:

  CC       adio/ad_bg/ad_bg_pset.lo
In file included from /home/robl/src/mpich/mpich2/src/mpid/pamid/include/../src/pt2pt/mpid_irecv.h:26,
                 from /home/robl/src/mpich/mpich2/src/mpid/pamid/include/mpidpost.h:38,
                 from /home/robl/src/mpich/mpich2/src/include/mpiimpl.h:3776,
                 from /home/robl/src/mpich/mpich2/src/mpid/pamid/include/mpidimpl.h:26,
                 from /home/robl/src/mpich/mpich2/src/mpi/romio/adio/ad_bg/ad_bg_pset.c:19:
/home/robl/src/mpich/mpich2/src/mpid/pamid/include/../src/pt2pt/mpidi_recv.h: In function 'MPIDI_Recv':
/home/robl/src/mpich/mpich2/src/mpid/pamid/include/../src/pt2pt/mpidi_recv.h:205: error: 'TOKEN_FLOW_CONTROL' undeclared (first use in this function)

Tried to track it down a little bit: TOKEN_FLOW_CONTROL_ON expands as it should but for some reason the header file that defines TOKEN_FLOW_CONTROL is not getting included? Something isn't defining BGQ ?

I just jammed a bare #define TOKEN_FLOW_CONTROL 0 in that file, and kept hacking at include files until it would build. then I got bored of doing that so I just jammed all the .h files from mpich-3.1b1, and when that didn't work i said "what the heck have I been doing for the last two hours".

Uh, so the short answer is "yeah, it's not buildable outside IBM" ...

mpichbot commented 7 years ago

Originally by robl on 2013-11-11 20:09:01 -0600

I applied Bob's change [984acc33d] from the mpich-ibm branch to "whatever we have today" master (roughly mpich-3.1rc1).

Now, leak-testcase is a lot cleaner with the "efix15" memory leak patches (thanks!), but it's not 100% clean. 

[0] 504 at [0x00000019c5832378], ../src/mpi/comm/commutil.c[260]
[0] 8 at [0x00000019c58322b8], ../src/mpid/pamid/src/mpid_vc.c[68]
[0] 8 at [0x00000019c5832138], ../src/mpid/pamid/src/mpid_vc.c[68]

Peanuts in the big scheme of things, but if every process reports 3 lines of ignorable errors, that's going to make memory debugging at a quarter million processes a real headache :>

BobC already identified that 504 one, from the function init_default_collops(). That function is called as part of MPIR_Comm_commit(). Why is not-pami stuff leaking for IBM, but we don't see this in ch3 ?

The 8 bytes come from MPID_VCRT_Create()

mpichbot commented 7 years ago

Originally by robl on 2013-11-12 09:43:04 -0600

Ah, I think mpich-ibm change [25693b4b] might clean this up? it modifies some non-pami bits of mpich, though...

mpichbot commented 7 years ago

Originally by robl on 2013-11-12 11:00:48 -0600

Huh. that made it worse... "Issue 9645: Make comm destroy non-blocking and use pami callback to complete it" introduces more leaks, and a "leaked context id". I even un-commented the call to "MPID_VCR_FREE_LPIDS".

==rob

mpichbot commented 7 years ago

Originally by Rob Latham robl@mcs.anl.gov on 2013-11-14 16:00:16 -0600

In a4c8f44235de1923ceb0cf113b17fd3bc9d4e33a: check for size!=0 when erroring out on new==NULL

per http://stackoverflow.com/questions/2132273/what-does-malloc0-return, malloc(0) may or may not return NULL. it is implementation defined in C99. therefore, it is incorrect to throw an OoM error on !new when size=0.

closes #1947

Signed-off-by: Rob Latham robl@mcs.anl.gov

mpichbot commented 7 years ago

Originally by robl on 2013-11-14 16:02:40 -0600

Uh, that was a mistake. Meant to close #1943

mpichbot commented 7 years ago

Originally by blocksom on 2013-12-04 14:45:29 -0600

I rebased the latest review branch (mpich-ibm/ticket-1947-1) to master and pushed this branch to mpich-ibm/ticket-1947-new.

I'm not quite sure where we stand on this memory leak code. Rob Latham, does this help? Should this be pushed to master?

mpichbot commented 7 years ago

Originally by robl on 2013-12-04 15:15:52 -0600

Replying to blocksom:

I rebased the latest review branch (mpich-ibm/ticket-1947-1) to master and pushed this branch to mpich-ibm/ticket-1947-new.

I'm not quite sure where we stand on this memory leak code. Rob Latham, does this help? Should this be pushed to master?

I don't understand where the confusion lies. Run the attached 'leak-testcase' against an MPICH configured with --enable-g=all and if you get output, the problem is still there!

Ok, you're short handed and this ticket is super-low priority, so I'm not actually upset.

This rebased and updated branch is still leaky: about 3.5 k of dangling resources. Here are the locations of the allocations that are not accounted for at exit. 

/home/robl/src/mpich/src/mpi/comm/commutil.c[260]
l/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c[232]
ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[70]
ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[71]
ome/robl/src/mpich/src/mpid/pamid/src/mpid_vc.c[74]
pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[620]
pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[622]
pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[624]
pich/src/mpid/pamid/src/comm/mpid_selectcolls.c[626]
mpichbot commented 7 years ago

Originally by blocksom on 2013-12-04 15:23:49 -0600

ok, ok .. touche. I was being lazy and didn't parse the history of this ticket. There may be other pending commits that help with the remaining leaks - but I'm not sure yet. I'm still doing commit cleanup work. :)

Thanks for the update.

mpichbot commented 7 years ago

Originally by blocksom on 2013-12-04 15:36:41 -0600

I found another commit from Bob before he left IBM and pushed it to the top of the mpich-ibm/ticket-1947-new branch.

commit cf9eb8658cd411ec69ee8d1c08eaf5a1f6571885
Author: Bob Cernohous <bobc@us.ibm.com>
Date:   Tue Oct 15 10:39:32 2013 -0500

    Free the builtin communicators (world/self/iworld)

    Also, do not reserve the internal-only copy of comm world context id when
    it isn't being used (MPID_NEEDS_ICOMM_WORLD)

    See mpich.org ticket #1947

    (ibm) 984acc33d6c8149987016b62da5afb36450be94e

    Signed-off-by: Michael Blocksome <blocksom@us.ibm.com>
mpichbot commented 7 years ago

Originally by robl on 2013-12-04 15:38:42 -0600

yeah, on the one hand we're talking about a tiny amount of memory -- it's crazy we've spent 44 comments and counting on it :> on the other hand, at scale it ends up being megabytes of output...

BobC's commit [984acc33d] went a long way, but not all the way, towards cleaning up the memory. Ken committed something similar (based on a suggestion by charles archer oddly enough): [dcf00b35], but his change only touches the context_mask, and makes no changes to pamid's MPID_Finalize().

mpichbot commented 7 years ago

Originally by robl on 2013-12-04 16:08:37 -0600

bleah, I applied BobC's commit but something inside pami doesn't like that. Here, I cleaned up the output of bgq_stack a bit. looks like a classic double-free.

__libc_free /bgsys/drivers/V1R2M0/ppc64/toolchain/gnu/glibc-2.12.2/malloc/malloc.c:3779

000034d7.long_branch_r2off._fini+0 /bgsys/source/srcV1R2M0.14091/comm/sys/buildtools/pami/components/memory/heap/HeapMemoryManager.h:158  

PAMI::Client::geometry_destroy_impl(void*, void*, void (*)(void*, void*, pami_result_t), void*) /bgsys/source/srcV1R2M0.14091/comm/sys/buildtools/pami/common/bgq/Client.h:1384

geom_destroy_wrapper /home/robl/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c:211

MPIDI_Coll_comm_destroy /home/robl/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c:428

MPIDI_Comm_destroy /home/robl/src/mpich/src/mpid/pamid/src/comm/mpid_comm.c:51

MPIR_Comm_delete_internal /home/robl/src/mpich/src/mpi/comm/commutil.c:1806

MPIR_Comm_release_always /home/robl/src/mpich/src/mpi/comm/commutil.c:1920

MPID_Finalize /home/robl/src/mpich/src/mpid/pamid/src/mpid_finalize.c:100

PMPI_Finalize /home/robl/src/mpich/src/mpi/init/finalize.c:211

main /home/robl/src/mpich/leak-testcase.c:6

You want me to push my change into mpich-ibm/ticket-1947-new ? It's broken, as you can see, but it'll save someone from doing the format-patch/am/apply dance.

mpichbot commented 7 years ago

Originally by robl on 2013-12-04 16:20:37 -0600

It's got to be some kind of bad interaction with this change: I don't see any other change that would be mucking around with freeing memory

commit 506aff3f44a076523ddedf3a50a5ff0877f2f179
Author: Haizhu Liu <haizhu@us.ibm.com>
Date:   Fri Oct 11 10:48:18 2013 -0400

    Fix Memory leak due to dynamic task change

    Signed-off-by: Michael Blocksome <blocksom@us.ibm.com>
mpichbot commented 7 years ago

Originally by blocksom on 2013-12-05 08:47:41 -0600

Hmm .. maybe try just Bob's commit and drop the one from Haizhu? Or it could be that we need a change in pami - I seem to recall Bob saying something like that.

I'll check to see if he added anything to pami. If so, then we'll have to wait until April or so to get a released version of pami that contains the change. We are working on doing a proper open source of pami, but it is taking a looong time and doesn't have an ETA.

mpichbot commented 7 years ago

Originally by robl on 2013-12-05 11:50:38 -0600

Ok, I reverted Haizhu's patch, keeping only BobC's, but the explosion still happens in basically the same path -- ending with PAMI::Client::geometry_destroy_impl triggering a free()-related crash.

Given the choice of leaking a few bytes or crashing, I'll take the leak, of course :>