Save and propagate si_pid for async signals, update omrsig_info() #7963

pshipton · 2025-09-30T17:26:57Z

pshipton · 2025-09-30T17:27:29Z

@keithc-ca can you pls do an initial review.

keithc-ca · 2025-09-30T19:11:33Z

include_core/omrport.h

+#define OMRPORT_SIG_SIGNAL_PID   -63
+#define OMRPORT_SIG_SIGNAL_PID_NAME -64


Perhaps "sender" is a better than "signal":

#define OMRPORT_SIG_SENDER_PID -63 #define OMRPORT_SIG_SENDER_NAME -64

keithc-ca · 2025-09-30T19:23:07Z

port/aix/omrsignal_context.c

+		if ((NULL != info) && (NULL != info->sigInfo)) {
+			*name = "Sending Process";
+			*value = &info->sigInfo->si_pid;
+			return OMRPORT_SIG_VALUE_32;


Perhaps things like this should be conditional:

return (sizeof(info->sigInfo->si_pid) == sizeof(uint32_t)) ? OMRPORT_SIG_VALUE_32 : OMRPORT_SIG_VALUE_64;

keithc-ca · 2025-09-30T19:23:39Z

port/aix/omrsignal_context.c

+	case OMRPORT_SIG_SIGNAL_PID:
+	case 6:
+		if ((NULL != info) && (NULL != info->sigInfo)) {
+			*name = "Sending Process";


No other names have spaces; suggest:

*name = "Sending_Process";

keithc-ca · 2025-09-30T19:41:29Z

port/linux386/omrsignal_context.c

+		if ((NULL != info) && (NULL != info->sigInfo)) {
+			char exebuf[32];
+			char linkbuf[PATH_MAX];
+			snprintf(exebuf, sizeof(exebuf), "/proc/%d/exe", info->sigInfo->si_pid);


I think %d should be %u.

keithc-ca · 2025-09-30T19:42:49Z

port/linuxaarch64/omrsignal_context.c

+		if ((NULL != info) && (NULL != info->sigInfo)) {
+			char exebuf[32];
+			char linkbuf[PATH_MAX];
+			snprintf(exebuf, sizeof(exebuf), "/proc/%d/exe", info->sigInfo->si_pid);


In case process ids are wider than int, this should cast to 64 bits:

snprintf(exebuf, sizeof(exebuf), "/proc/%" OMR_PRIu64 "/exe", (uint64_t)info->sigInfo->si_pid);

keithc-ca · 2025-09-30T19:46:37Z

port/unix/omrsignal.c

+static volatile pid_t signalPids[ARRAY_SIZE_SIGNALS][NUM_SIGNAL_PIDS] = {0};
+static volatile uintptr_t signalPidHeads[ARRAY_SIZE_SIGNALS] = {0};
+static volatile uintptr_t signalPidTails[ARRAY_SIZE_SIGNALS] = {0};
+


Please don't create two or more consecutive blank lines.

keithc-ca · 2025-09-30T19:52:47Z

port/unix/omrsignal.c

 		}
 		cursor = cursor->next;
 	}
+	portLibrary->mem_free_memory(portLibrary, sigInfo);


Perhaps the caller should do this?

keithc-ca · 2025-09-30T19:53:19Z

port/unix/omrsignal.c

 *
 * @return void


Missing @param sigInfo; unwanted @return void.

keithc-ca · 2025-09-30T20:03:59Z

port/unix/omrsignal.c

+					/* Ensure the -1 is written before updating the head. */
+					issueWriteBarrier();
+					signalPidHeads[unixSignal] = nextHead;
+					fprintf(stderr, "si_pid %d head %" PRIuPTR " tail %" PRIuPTR "\n", siPid, head, tail);


I assume this is temporary.

keithc-ca · 2025-09-30T20:05:36Z

port/unix/omrsignal.c

+		uintptr_t head = signalPidHeads[signal];
+		/* Check if there is room. */
+		if (nextTail != head) {
+			if ((pid_t)-1 == compareAndSwapU32((uint32_t *)&signalPids[signal][tail], (pid_t)-1, sigInfo->si_pid)) {


I don't think it's safe to assume sizeof(pid_t) == sizeof(uint32_t).

pshipton · 2025-09-30T21:56:55Z

Addressed the comments.

keithc-ca · 2025-10-01T14:49:19Z

include_core/omrport.h

 #define OMRPORT_SIG_GPR_ARM_R9   -61
 #define OMRPORT_SIG_GPR_ARM_R10  -62
+#define OMRPORT_SIG_SENDER_PID   -63
+#define OMRPORT_SIG_SENDER_PID_NAME -64


It isn't a "PID" name that's returned; if you don't like just OMRPORT_SIG_SENDER_NAME, perhaps OMRPORT_SIG_SENDER_IMAGE_NAME?

keithc-ca · 2025-10-01T14:52:58Z

port/linux386/omrsignal_context.c

+		if ((NULL != info) && (NULL != info->sigInfo)) {
+			char exebuf[32];
+			char linkbuf[PATH_MAX];
+			snprintf(exebuf, sizeof(exebuf), "/proc/%u/exe", (uint64_t)info->sigInfo->si_pid);


This is for 32-bit Linux; it's hard to imagine PIDs are 64 bits, but if you want to be consistent with other platforms the matching format (OMR_PRIu64) must be used.

keithc-ca · 2025-10-01T15:11:48Z

port/unix/omrsignal.c

+			if (sizeof(sigInfo->si_pid) == sizeof(uint32_t)) {
+				if ((uint32_t)-1 == compareAndSwapU32((uint32_t *)&signalPids[signal][tail], (uint32_t)-1, sigInfo->si_pid)) {
+					/* The pid is written before the tail is incremented. */
+					signalPidTails[signal] = nextTail;
+					break;
+				}
+			} else {
+				if ((uint64_t)-1 == compareAndSwapUDATA((uint64_t *)&signalPids[signal][tail], (uint64_t)-1, sigInfo->si_pid)) {
+					/* The pid is written before the tail is incremented. */
+					signalPidTails[signal] = nextTail;
+					break;
+				}
+			}


UDATA and uint64_t aren't necessarily the same size without a check; suggest inverting this:

if (sizeof(sigInfo->si_pid) == sizeof(uintptr_t)) { if ((uintptr_t)-1 == compareAndSwapUDATA((uintptr_t *)&signalPids[signal][tail], (uintptr_t)-1, sigInfo->si_pid)) { /* The pid is written before the tail is incremented. */ signalPidTails[signal] = nextTail; break; } } else { if ((uint32_t)-1 == compareAndSwapU32((uint32_t *)&signalPids[signal][tail], (uint32_t)-1, sigInfo->si_pid)) { /* The pid is written before the tail is incremented. */ signalPidTails[signal] = nextTail; break; } }

The if on line 1261 could also be inverted to reduce the indentation here.

pshipton · 2025-10-02T01:50:26Z

Updated.

keithc-ca · 2025-10-02T13:38:15Z

include_core/omrport.h

 #define OMRPORT_SIG_GPR_ARM_R9   -61
 #define OMRPORT_SIG_GPR_ARM_R10  -62
+#define OMRPORT_SIG_SENDER_PID   -63
+#define OMRPORT_SIG_SENDER_NAME -64


An extra space so the values line up would be welcome.

keithc-ca · 2025-10-02T13:41:27Z

port/unix/omrsignal.c

+		uintptr_t nextTail = (tail + 1) % NUM_SIGNAL_PIDS;
+		uintptr_t head = signalPidHeads[signal];
+		if (nextTail == head) {
+			/* Stop if there is no room. */


nit: "if" is not appropriate here, if we reach this line, there is no room:

if (nextTail == head) { /* Stop: there is no room. */ break; }

pshipton · 2025-10-02T15:33:58Z

Updated. What do you think about the algorithm to keep the pids. There can be multiple signals/threads adding, but only one thread removing.

keithc-ca · 2025-10-02T20:15:55Z

What do you think about the algorithm to keep the pids

I haven't looked at that part closely enough to comment yet: I take a closer look tomorrow.

keithc-ca · 2025-10-03T19:19:09Z

I see OMRPORT_SIG_SENDER_NAME has been removed: Why?

keithc-ca · 2025-10-03T19:23:16Z

include_core/omrport.h

 	/** see @ref omrsysinfo.c::omrsysinfo_get_processes "omrsysinfo_get_processes" */
 	uintptr_t (*sysinfo_get_processes)(struct OMRPortLibrary *portLibrary, OMRProcessInfoCallback callback, void *userData);
+	/** see @ref omrsysinfo.c::omrsysinfo_get_process_id_name "omrsysinfo_get_process_id_name"*/
+	const char *(*sysinfo_get_process_id_name)(struct OMRPortLibrary *portLibrary, uintptr_t pid) ;


The return type should be char * because the caller is expected to free that memory.
Please remove the space before ;.
As before, I don't think _id_ makes sense here processes run programs, but IDs don't have names.

keithc-ca · 2025-10-03T19:23:31Z

include_core/omrport.h

 	intptr_t (*sysinfo_get_CPU_load)(struct OMRPortLibrary *portLibrary, double *load) ;
 	/** see @ref omrsysinfo.c::omrsysinfo_get_processes "omrsysinfo_get_processes" */
 	uintptr_t (*sysinfo_get_processes)(struct OMRPortLibrary *portLibrary, OMRProcessInfoCallback callback, void *userData);
+	/** see @ref omrsysinfo.c::omrsysinfo_get_process_id_name "omrsysinfo_get_process_id_name"*/


Space before */, please.

keithc-ca · 2025-10-03T19:24:25Z

port/common/omrsysinfo.c

+ * Get the name of the specified process.
+ * @param[in] portLibrary The port library.
+ * @param[in] pid The process ID.
+ * @return The process name string (NULL terminated) on success, which must be freed, NULL on error or not supported.


"or not" -> "or if not"

pshipton · 2025-10-03T19:27:53Z

I moved the code to omrsysinfo_get_process_id_name(). For one thing the code doesn't need to be duplicated a number of times, and the sysinfo function can be used outside of signals. To handle SIGABRT, for which the handler is in openj9, it doesn't work to call omrsig_info() because it takes a private omr OMRUnixSignalInfo structure.

keithc-ca · 2025-10-03T19:31:16Z

I see OMRPORT_SIG_SENDER_NAME has been removed: Why?

I assume it is replaced by sysinfo_get_process_id_name?

keithc-ca · 2025-10-03T19:51:14Z

port/unix/omrsysinfo.c

+	char exebuf[32];
+	char linkbuf[PATH_MAX];
+	snprintf(exebuf, sizeof(exebuf), "/proc/%" OMR_PRIuPTR "/exe", pid);
+	ssize_t rc = readlink(exebuf, linkbuf, sizeof(linkbuf));


This is not legal C code (rc must be declared before line 7905).

keithc-ca · 2025-10-03T19:51:50Z

port/unix/omrsysinfo.c

+	char linkbuf[PATH_MAX];
+	snprintf(exebuf, sizeof(exebuf), "/proc/%" OMR_PRIuPTR "/exe", pid);
+	ssize_t rc = readlink(exebuf, linkbuf, sizeof(linkbuf));
+	if ((-1 != rc) && rc < sizeof(linkbuf)) {


Missing parentheses:

if ((-1 != rc) && (rc < sizeof(linkbuf))) {

pshipton · 2025-10-03T20:12:39Z

Updated.

keithc-ca · 2025-10-03T21:05:15Z

port/common/omrsysinfo.c

+ * Get the name of the specified process.
+ * @param[in] portLibrary The port library.
+ * @param[in] pid The process ID.
+ * @return The process name string (NULL terminated) on success, which must be freed, NULL on error or if not supported.


I think this line is a little long.

Signed-off-by: Peter Shipton <[email protected]>

keithc-ca · 2025-10-08T22:18:28Z

What do you think about the algorithm to keep the pids

I think there's a real danger of dropping signals. Maybe we can just drop the sending pid rather than drop the signal entirely?

pshipton · 2025-10-08T23:32:10Z

I think there's a real danger of dropping signals. Maybe we can just drop the sending pid rather than drop the signal entirely?

That's how it already works, I didn't change anything to do with counting signals and waking up asynchSignalReporter to process them.

keithc-ca · 2025-10-09T15:44:38Z

That's how it already works.

Ok, you're right; we just might not have the sending PID for some signals when a queue overflows. The PIDs can also be "wrong" in that they may be consumed by a signal occurrence that wasn't queued in the same order (I don't think this is a serious issue).

pshipton · 2025-10-17T14:04:00Z

@babsingh pls review when you get a chance.

babsingh · 2025-10-17T19:09:02Z

jenkins build all

keithc-ca suggested changes Sep 30, 2025

View reviewed changes

pshipton force-pushed the sipid2 branch 2 times, most recently from 1cc3395 to 0c589af Compare September 30, 2025 21:56

keithc-ca suggested changes Oct 1, 2025

View reviewed changes

0xdaryl added the comp:port label Oct 1, 2025

pshipton force-pushed the sipid2 branch from 0c589af to 5beff2c Compare October 2, 2025 01:46

keithc-ca suggested changes Oct 2, 2025

View reviewed changes

pshipton force-pushed the sipid2 branch from 5beff2c to 17c9e43 Compare October 2, 2025 15:32

pshipton force-pushed the sipid2 branch 2 times, most recently from efff2c6 to 2514074 Compare October 3, 2025 17:06

keithc-ca suggested changes Oct 3, 2025

View reviewed changes

pshipton force-pushed the sipid2 branch from 2514074 to d2367be Compare October 3, 2025 20:11

keithc-ca reviewed Oct 3, 2025

View reviewed changes

pshipton force-pushed the sipid2 branch 3 times, most recently from 9d5aef1 to f2fe8d0 Compare October 3, 2025 21:35

pshipton marked this pull request as ready for review October 3, 2025 21:35

pshipton requested review from 0xdaryl, babsingh and mstoodle as code owners October 3, 2025 21:35

pshipton force-pushed the sipid2 branch from f2fe8d0 to 71b8da1 Compare October 3, 2025 22:31

Save and propagate si_pid for async signals, update omrsig_info()

eaf96f2

Signed-off-by: Peter Shipton <[email protected]>

pshipton force-pushed the sipid2 branch from 71b8da1 to eaf96f2 Compare October 3, 2025 22:36

		#define OMRPORT_SIG_SIGNAL_PID -63
		#define OMRPORT_SIG_SIGNAL_PID_NAME -64

Save and propagate si_pid for async signals, update omrsig_info() #7963

Are you sure you want to change the base?

Save and propagate si_pid for async signals, update omrsig_info() #7963

Uh oh!

Conversation

pshipton commented Sep 30, 2025

Uh oh!

pshipton commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton commented Oct 2, 2025

Uh oh!

keithc-ca commented Oct 2, 2025

Uh oh!

keithc-ca commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithc-ca commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshipton commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithc-ca commented Oct 8, 2025

Uh oh!

pshipton commented Oct 8, 2025

Uh oh!

keithc-ca commented Oct 9, 2025

Uh oh!

pshipton commented Oct 17, 2025

Uh oh!

babsingh commented Oct 17, 2025

Uh oh!

pshipton commented Oct 3, 2025 •

edited

Loading