Debug ibm stability - #1792
Conversation
…handoff, which ic clearly the broken function
…memory writes which created particle drift
|
Claude Code Review Head SHA: 88877c1 Files changed:
Findings:
|
|
This is not ready for PR, and I will @ maintainers when this is ready. |
|
Merged master to resolve the conflict from #1762. Beyond the marked hunk, three calls in the new stability-report code in |
Conflict in m_data_output.fpp: this branch guards the stability scan so immersed-boundary cells cannot trip a violation, master added the alpha_rho argument for per-phase EOS evaluation and the Mie-Gruneisen Hugoniot-limit reporting. Kept the guard and moved master's work inside it. s_report_icfl_violation, new on this branch, merged without conflict but still called s_compute_cell_state and s_compute_speed_of_sound on the old signature; updated those three calls and declared alpha_rho for them. Also corrected a comment typo that fails the spell-check gate.
|
The two red Only the
The window is only available where --- a/src/common/m_finite_differences.fpp
+++ b/src/common/m_finite_differences.fpp
@@ -25,6 +25,7 @@
integer :: lB, lE !< loop bounds
+ integer :: fd_margin !< usable ghost width
integer, intent(in) :: q
@@ -33,16 +34,17 @@
- ! Coefficients always extend at least fd_number_in beyond the interior on each side, so a stencil centered on a
- ! ghost-adjacent cell (e.g. an immersed boundary near a domain boundary) has a real coefficient to read instead of
- ! reading past the caller's allocation. offset_s, when given, widens this further (never narrows it) for callers
- ! that need more than fd_number_in of margin.
-
- if (present(offset_s)) then
- lB = -max(fd_number_in, offset_s%beg)
- lE = q + max(fd_number_in, offset_s%end)
- else
- lB = -fd_number_in
- lE = q + fd_number_in
- end if
+ ! A centered stencil at cell i reads s_cc(i - fd_number_in : i + fd_number_in), so coefficients exist only where
+ ! that window stays inside s_cc, which carries local_buff_size ghost cells. That leaves fd_margin cells past the
+ ! interior on each side: zero where buff_size equals fd_number, and positive only where buff_size has been floored
+ ! higher, which is the immersed-boundary case these ghost-adjacent coefficients are for.
+ fd_margin = max(0, local_buff_size - fd_number_in)
+
+ if (present(offset_s)) then
+ lB = -min(offset_s%beg, fd_margin)
+ lE = q + min(offset_s%end, fd_margin)
+ else
+ lB = -fd_margin
+ lE = q + fd_margin
+ end if
--- a/src/simulation/m_hypoelastic.fpp
+++ b/src/simulation/m_hypoelastic.fpp
@@ -43,6 +43,7 @@
integer :: i
+ integer :: fd_margin_hypo !< usable ghost width for the coefficient range
@@ -62,13 +63,15 @@
- ! s_compute_finite_difference_coefficients always extends fd_number beyond the interior on each side
- @:ALLOCATE(fd_coeff_x_hypo(-fd_number:fd_number,-fd_number:m + fd_number))
+ ! Match the range s_compute_finite_difference_coefficients can actually fill: it stops where the stencil would
+ ! leave x_cc's ghost region, so the margin is what buff_size has over fd_number and is zero when they are equal.
+ fd_margin_hypo = max(0, buff_size - fd_number)
+ @:ALLOCATE(fd_coeff_x_hypo(-fd_number:fd_number,-fd_margin_hypo:m + fd_margin_hypo))
if (n > 0) then
- @:ALLOCATE(fd_coeff_y_hypo(-fd_number:fd_number,-fd_number:n + fd_number))
+ @:ALLOCATE(fd_coeff_y_hypo(-fd_number:fd_number,-fd_margin_hypo:n + fd_margin_hypo))
end if
if (p > 0) then
- @:ALLOCATE(fd_coeff_z_hypo(-fd_number:fd_number,-fd_number:p + fd_number))
+ @:ALLOCATE(fd_coeff_z_hypo(-fd_number:fd_number,-fd_margin_hypo:p + fd_margin_hypo))
end ifThe allocation has to move with it, or the margin the routine fills and the margin the caller reserved disagree in the other direction. Checked on your head One thing to weigh: nothing reads outside the interior today. Every consumer in |
|
Following up on the finite-difference diff above - a fuller read of the branch turned up something more serious than that one, plus a few smaller things. Out-of-bounds writes in
|
| radius | num_dims | neighbours | vs dimension(26) |
|---|---|---|---|
| 1 | 3 | 26 | fits exactly |
| 2 | 3 | 124 | overflow |
| 3 | 2 | 48 | overflow |
| 3 | 3 | 342 | overflow |
So recv_neighbor_list(nbr_idx) and requests(nreqs) are written past the end of two stack arrays, and MPI_IRECV(recv_bufs(:, nbr_idx), ...) posts receives past the end of a heap allocation that is (buf_size, 26) (:1528). Stack corruption plus MPI writing into memory it does not own - a crash or a silent wrong answer depending on what the stack happens to hold.
This is reachable rather than hypothetical. ib_neighborhood_radius is a case parameter with {"min": 0} (toolchain/mfc/params/definitions.py:381), and when left at 0 it is auto-computed at src/simulation/m_start_up.fpp:1606:
ib_neighborhood_radius = max(1, ceiling(1.1_wp*max_ib_bound/(min_rank_width)))which exceeds 1 exactly when a body is large relative to a rank's width - the regime this PR exists to handle. There is also now an example in the tree that sets ib_neighborhood_radius: 3 explicitly.
Everything else on the radius-aware path is sized correctly - ib_neighbor_ranks is allocated (-ax:ax) for ax = ib_neighborhood_radius at m_start_up.fpp:1427 - so it is just these three arrays that were missed. Deriving max_nbrs from the radius (and allocating recv_bufs and requests from it) rather than hardcoding 26 would close it.
Two smaller things in the same hunks:
- The tag formula
tag = 200 + (dx+1)*9 + (dy+1)*3 + (dz+1)(:1563,:1580) is a base-3 encoding, injective only for offsets in-1..1. At radius 2 distinct offsets collide -(0,2,-1)and(1,-1,-1)both give 205. Harmless today because everyMPI_ISENDsends the samesend_buf, so a mismatched pair is indistinguishable, but it becomes a real bug the moment the payload is per-neighbour. - The unpack bound
((2*ib_neighborhood_radius + 1)**num_dims) - 1(:1592) disagrees with the enumeration in 1D: thedyloop runs-R..Rregardless of dimensionality, sonbr_idxreaches 8 at radius 1 while the unpack loop stops at 2. Harmless now (those entries areMPI_PROC_NULL) but the two should come from one expression.
Worth a look, lower severity
s_ibm_correct_statenow allocates and frees 13 device arrays per RK stage (:231-234,:507-508).@:ALLOCATEexpands toallocate+GPU_ENTER_DATA(create=...), and device allocation synchronises on both CUDA and HIP, so at ~3 stages per step this is likely the dominant new cost for a PR aimed at throughput. Hoisting them to module scope sized atmax_num_gps, next toghost_points, would remove it. Relatedly, the phase-1 copy-out and phase-2 copy-in ofr_IP/v_IP/pb_IP/mv_IP/nmom_IP/presb_IP/massv_IP/Ys_IPare unconditional while the interpolation that fills them is guarded bybubbles_euler/qbmm/chemistry- about 20 reals per ghost point per stage of uninitialised data copied in every non-bubble non-chemistry case.- The two-kernel split itself is right: phase 2 reads no cell of
q_prim_vf/pb_in/mv_inother than its own(j,k,l), so the read-after-write hazard is genuinely removed, and the race was real (theeta/sum(eta)fallback ins_compute_interpolation_coeffsfires exactly when an image point's stencil lies inside a neighbouring particle). One behavioural note: on CPU the old serial loop let ghost point i see corrections from 1..i-1 in that fallback, and it no longer does, so overlapping-particle cases can shift. if (ib)moved inside the CFL kernel (src/simulation/m_data_output.fpp:202-203). Every otherib_markersreference in the tree hoists the flag outside the compute construct (m_rhs.fpp:813). Whenibis false,ib_markers%sfis never allocated and@:ACC_SETUP_SFsnever ran, so the region references an allocatable component absent from the device present table underdefault(present). nvfortran will likely tolerate the untaken branch; CCE's lookup is stricter. Also worth noting the mask excludes ghost-point cells as well as deep-interior ones, which is wider than "values interior to the IBs" and means a diverging ghost state no longer trips the stability guard.s_report_icfl_violation(m_data_output.fpp:333-457) is called unconditionally from the per-time-step path whenever a rank's local ICFL exceeds 1: ~130 lines ofprintfrom every offending rank, a fullGPU_UPDATE(host=...)of everyq_prim_vfplusib_markers, and a second global barrier per step. It also readspatch_ib%force/torque/velon the host with noGPU_UPDATE(host='[patch_ib]'), so for moving IBs the particle state it reports can be a step stale and actively mislead. The PR description says this part is a work in progress - gating it behindMFC_DEBUGor dropping it before merge would be my suggestion.s_read_ib_restart_data: nothing checksnum_gbl_ibs <= num_ib_patches_max_namelistbeforepatch_ib(i)is written (m_start_up.fpp:1230-1240); the@:PROHIBITthat enforces it lives ins_reduce_ib_patch_array, which runs after. And the broadcast loop issues sixMPI_BCASTcalls per global IB - 3600 tiny collectives for a 600-particle bed. The underlying fix is right and valuable, though: the olddo i = 1, num_ibsread only namelist patches, so restart with particle clouds was silently dropping every particle.- Pre-existing but in a block this PR rewrote:
ris missing from the phase-2privatelist (:275-278, used at:486for QBMM non-polytropic). OpenACC predetermines compute-region scalars private; OpenMP offload does not - the documented trap in.claude/rules/common-pitfalls.md. The list was copied from master, so the PR did not introduce it, but the rewrite is the natural moment. ib_gbl_idx_lookup(tmp_patch%gbl_patch_id) = num_ibsat:1604looks redundant next tos_update_ib_lookup()two lines later, but it is what letss_get_neighborhood_idxsee a patch added earlier in the same unpack loop, de-duplicating one that arrives from two neighbours - which becomes possible as soon as radius > 1 aliases two offsets onto one rank. Worth a comment so it survives a cleanup.num_ib_patches_max_namelist54000 -> 216000 andnum_local_ibs_max2000 -> 8000 are not free:ib_patch_parametersmeasures 512 bytes, sopatch_ibgoes from 26 MB to 105 MB in host BSS and as a static device allocation, for every run including a single sphere. Several per-step transfers are sized by the constant rather than bynum_ibs-m_ibm.fpp:1496/1521/1608do whole-arrayGPU_UPDATEs where other sites already usepatch_ib(1:num_ibs), and:1527-1528allocates ~110 MB of send/recv buffers every step. Sizing those by the actual count is the change that would let the ceiling rise cheaply.
Verified clean, for what it is worth: all four fd_coeff_* allocation sites match the new coefficient bounds including the offset_s path, @:ALLOCATE/@:DEALLOCATE pairing in s_ibm_correct_state is balanced, no wp/stp mixing is introduced, the shell_axis plumbing is complete and self-consistent end to end, s_restart_particle_clouds's id accounting matches both packers, and $:GPU_UPDATE(device='[num_gps]') at :1017 is a genuine fix for a stale device loop bound.
|
The two failures on this branch look like they are not yours. Both hypoelasticity cases die the same way, and it is a hard crash rather than a tolerance miss: That is the out-of-bounds finite-difference read tracked in #1856 and #1860: So the order is #1878 -> #1859 -> here. #1859 is currently red for an unrelated reason, the 23 Worth confirming rather than assuming, though: if the crash persists after #1859 lands, it is a second site and I would want to see it. |
|
Correcting my earlier comment: I said #1859 would fix these two failures. #1859 was closed without merging, so that fix is not coming from there, and you should not wait on it. The diagnosis itself still holds. Both cases die the same way: and the underlying bug is still open as #1856 and #1860. So the two failures here are still not caused by this branch, but they will not clear on their own either. Either #1856 gets a new fix, or this branch needs to work around it. Sorry for the bad steer — I should have checked that #1859 had actually landed before pointing you at it. |
|
Following up properly, because my last two comments were both partly wrong and this branch is where the fix actually lives. #1859 was closed in favour of the fix on this branch, so "wait for #1859" was exactly backwards. Your fix is here, and the widening in The loop bounds were widened: lB = -max(fd_number_in, offset_s%beg)
lE = q + max(fd_number_in, offset_s%end)but real(wp), dimension(-local_buff_size:q + local_buff_size), intent(in) :: s_ccand the 4th-order stencil reaches fd_coeff_s(-2, i) = 1._wp/(s_cc(i - 2) - 8._wp*s_cc(i - 1) - s_cc(i + 2) + 8._wp*s_cc(i + 1))With So the coefficient array got wider but the coordinate array it reads did not, and the loop now runs where the stencil has nothing to stand on. The safe start is Two ways out, and the choice is yours because they mean different things physically:
Both examples in #1856 are still worth running against whichever you pick: rank-invariance of |
The widened range let the loop start at -fd_number_in, but s_cc only
exists over the caller's buffer and the stencil reaches fd_number_in
cells either side of i. A caller whose buffer is narrower than
2*fd_number_in therefore reads off the front of it: a 4th-order
hypoelastic case with buff_size = 2 starts at i = -2 and immediately
asks for s_cc(-4), which is
At line 65 of file src/common/m_finite_differences.fpp
Fortran runtime error: Index '-4' of dimension 1 of array 's_cc'
below lower bound of -2
on the five 2D/3D hypoelastic cases in the suite.
Clamping the loop to where the stencil has data leaves the widening
itself intact where it matters. Immersed-boundary cases get
buff_size >= 10 from s_mfc_buff_size against fd_number = 2, so the
clamp never binds for them and they still receive coefficients out to
-fd_number_in, which is the point of widening the range. Non-IB
callers fall back to the interior-only range they had before, which is
safe because the out-of-interior read comes from s_compute_ib_forces
and does not run without IB.
Verified on a bounds-checked debug build: the five hypoelastic cases
that aborted now pass, and the IBM suite runs clean -- 37 of 58 in one
pass and 32 of 58 in another, no failures and no bounds errors in
either, covering the 3D sphere, cylinder, cuboid and particle-cloud
geometries where a clamp that bound too early would show up.
|
Pushed The widened range let the loop start at lB = max(lB, -local_buff_size + fd_number_in)
lE = min(lE, q + local_buff_size - fd_number_in)The clamp never binds for the cases this was written for. Verified on a bounds-checked debug build. The five hypoelastic cases that aborted now pass, and the IBM suite runs clean -- 37 of 58 in one pass and 32 of 58 in another, no failures and no bounds errors in either, across the 3D sphere, cylinder, cuboid and particle-cloud geometries where a clamp binding too early would show up. Both IBM passes were cut short by my own timeout rather than by any failure; a full pass is still running and I will say so here if anything turns up. That should be the last thing standing between this branch and green. Worth re-running the rank-invariance check from #1856 before merge, though: |
Lines of Code
|
There has been a growing amount of technical debt on the immersed boundary code for multi-rank cases since the introduction of the IB neighborhoods. This has led to a host of new potential issues that threaten the stability of simulations being run. As I recently began scaling a relatively-difficult case, in terms of opportunities for instability, I made multiple bugfixes that were latent and untested. Some were relatively innocuous and others were extremely problematic, but all obvious bugs. And explanation of the changes are as follows
The most impactful bug fixes were the update of num_ibs and num_gps, and the checking of interior GP points with a mask during CFL. Current tests show total stability on 8 ranks with 600 IBs that are very light compared to the ambient fluid in a mach 10 shock. Assuming these results hold, then we should have much higher stability, even in non-physically significant regimes. I am currently working on extending this result to higher numbers of IBs and ranks.
Acknowledgement
PR template credit: junegunn