Le 06/10/2026 à 02:58, Jerry D a écrit :
On 10/5/26 10:04 AM, Jerry D wrote:
See the attached patch. In my campaign to identify caf related bugs
I discovered this one.
Regression tested under heavy loads.
OK for mainline?
Regards,
Jerry
I am working on ways to create LLM assisted patches more human
readable. Using Claude, and I suspect other LLMs, one can set up
rules and guidelines to follow. The attached V2 of the subject patch
is essentially identical with an improved commit message that better
explanations of the problem and the fix. There is a bit of a guide
here on how to review all this as well.
How the hang happens: image 1 waits in SYNC IMAGES ([2, 3]) when image
3 stops. Image 2 then executes SYNC IMAGES ([1, 3]), finds image 3
stopped and returns with STAT_STOPPED_IMAGE without synchronizing, so
image 1 is not released. FAIL IMAGE leads to the same hang.
The point to check is the order in sync_table_terminated: under the
table lock it releases the waiting images and takes back their
unmatched counts, and only then stores the new status. Outside the
WIN32 supervisor, which this patch does not change, every change of an
image's status to stopped or failed goes through it.
Reading guide: start with sync.h, the new waiting and aborted fields
and what sync_table and sync_table_terminated promise. Then those two
functions in sync.c. In shmem.c, _gfortran_caf_sync_images drops its
early return for a terminated image and sets STAT= only when sync_table
reports a terminated image. The other shmem.c and supervisor.c hunks
route the status changes through sync_table_terminated, which replaces
the supervisor's wake-up of all waiting images. set_table_parts and
the sync_init changes only lay out the enlarged table.
Regression tested on x86_64.
OK for mainline?
Regards,
Jerry
PS Feedback on the format of this email is greatly appreciated.
---
libgfortran: [PR127684] caf_shmem: SYNC IMAGES hang on
terminated image
SYNC IMAGES hung when an image of the image set stopped or failed while
another image of the set was waiting in the statement.
An image that found a stopped or failed image in its set returned at
once
with STAT=, without synchronizing, so an image already waiting for it
was
never released. The supervisor also woke the waiting images without
holding the table lock, so a wakeup could be lost.
Each image now records the set it waits for. Images that stop or fail,
and the POSIX supervisor when it reaps one, set the status through the
new sync_table_terminated, which holds the table lock. For a stopped
image it releases the images waiting for it and takes back the counts
they raised that no partner has matched, before it stores the status,
so an image that sees the stop cannot pair with the aborted statement.
A failed image is skipped and the other images of the set synchronize
(F2023 11.7.11).
Assisted-by: Claude Opus 5.5
PR libfortran/127684
libgfortran/ChangeLog:
* caf/shmem.c (_gfortran_caf_sync_images): Do not return early for
a stopped or failed image. Check the images only when one of them
terminated.
(mark_stopped): Use sync_table_terminated.
(_gfortran_caf_fail_image): Likewise.
* caf/shmem/supervisor.c (supervisor_main_loop): Likewise. Do not
signal all images waiting in sync_table.
* caf/shmem/sync.c (set_table_parts): New function.
(sync_init): Use it.
(sync_init_supervisor): Allocate the image sets of the waiting
images and the aborted flags.
(stopped_image_in): New function.
(sync_table): Return whether an image of the set terminated. Only
synchronize memory when an image of the set stopped. Record the
image set and end the wait when aborted.
(sync_table_terminated): New function.
* caf/shmem/sync.h (sync_t): Add waiting and aborted.
(sync_table): Return bool.
(sync_table_terminated): Declare.
gcc/testsuite/ChangeLog:
* gfortran.dg/coarray/sync_images_failed_1.f90: New test.
* gfortran.dg/coarray/sync_images_stopped_2.f90: New test.
---
+void
+sync_table_terminated (sync_t *si, int image, bool stopped)
As far as I can see, the function doesn't impact only sync_table as it
also updates the supervisor tables, so please remove sync_table from
the name (notify_image_terminated, image_terminated,
acknowledge_image_terminated or similar).
+{
+ volatile int *table = si->table;
+ const size_t img_c = local->total_num_images;
+
+ lock_table (si);
+ for (size_t j = 0; j < img_c; ++j)
+ {
+ if (!si->waiting[image + img_c * j])
+ continue;
+ if (stopped)
+ {
+ /* Take back the counts of J that its partners have not matched.
+ IMAGE is marked stopped only below, so no image that sees the
+ stop can pair with J's aborted SYNC IMAGES. */
+ for (size_t k = 0; k < img_c; ++k)
+ if (si->waiting[k + img_c * j]
+ && table[k + img_c * j] > table[j + img_c * k])
+ --table[k + img_c * j];
It took me some time to understand why this loop was necessary. I
finally convinced myself that it makes sense, but I think we can do
without it if...
+ si->aborted[j] = 1;
+ }
+ caf_shmem_cond_signal (&si->triggers[j]);
+ }
+ /* The images woken above need the table lock to return from their
wait,> + so they see this count and the new status. */
+ atomic_fetch_add (stopped ? &this_image.supervisor->finished_images
+ : &this_image.supervisor->failed_images, 1);
+ this_image.supervisor->images[image].status
+ = stopped ? IMAGE_SUCCESS : IMAGE_FAILED;
unlock_table (si);
}
@@ -106,6 +131,16 @@ sync_table (sync_t *si, int *images, int size)
}
lock_table (si);
+ /* With a stopped image in the set this only has the effect of SYNC
+ MEMORY; failed images are skipped (F2023 11.7.11). */
+ if (stopped_image_in (images, size))
+ {
+ unlock_table (si);
+ return true;
+ }
... this is moved further down...
+ for (i = 0; i < size; ++i)
+ si->waiting[images[i] + img_c * this_image.image_num] = 1;
+ si->aborted[this_image.image_num] = 0;
for (i = 0; i < size; ++i)
{
if (this_image.supervisor->images[images[i]].status != IMAGE_OK)
... after this loop. Instead of decreasing the count of already
arrived images, we would not skip the increase of the count as images
arrive to the barrier.
@@ -115,6 +150,9 @@ sync_table (sync_t *si, int *images, int size)
}
for (;;)
{
+ /* An image of the set stopped, see sync_table_terminated. */
+ if (si->aborted[this_image.image_num])
+ break;
The information present in the aborted array seems to be redundant
with the status of images shared with the supervisor.
for (i = 0; i < size; ++i)
if (this_image.supervisor->images[images[i]].status == IMAGE_OK
As the status is already queried in the main loop, we can just as well
handle status different from IMAGE_OK here, so that the aborted array
is no longer necessary.
&& table[images[i] + img_c * this_image.image_num]
Next there is the waiting array that I would like to remove as well.
As far as I can see, with the count decrease loop removed and the
aborted array removed, the waiting array remains useful to selectively
wake the needed images in sync_table_terminated. But it requires
updating it in every sync image, just to support a premature image
stop. Can we just wake every image in the team on image stop and
remove the waiting array and the associated book keeping in the main
synchronisation code?
The explaining text is an improvement I think, but I had already a
clear picture of the patch in mind when I started reading it. Let's
see how it helps with a completely unknown patch.
Thanks for your work.
