darwin: not every process should be in running state (#1611)

[aestriplex]

This PR follows from the discussion in #1611

Summary:

• removed the static function ProcessTable_mapDarwinProcessState;
• added proc->super.state = pti.pti_numrunning > 0 ? RUNNING : SLEEPING; in DarwinProcess_setFromLibprocPidinfo;
• refactored DarwinProcess_setFromLibprocPidinfo:
  • added early exit;
  • introduced XNU constant PROC_PIDTASKINFO_SIZE instead of sizeof.
• removed a redundant system call in a closed scope

[fasterit]

Linting complains as follows:
darwin/DarwinProcess.c:393: trailing whitespace.
darwin/DarwinProcess.c:450: trailing whitespace.

[aestriplex]

@fasterit Thanks, fixed

[fasterit]

This works as intended. Good work!
The native top also shows "stuck" tasks quite often, do we have an idea what that is and do we want to represent it?

[Explorer09]

Just tested this PR and it seems to work great.

I would argue it shows more status than macOS's top does, especially on the Stopped ("T") state.

There is another "stuck" state that are shown in macOS's top, which display as Sleeping ("S") in htop instead with this PR. Although I didn't have the need to distinguish this "stuck" state from "sleeping" right now, it's still a room for a future enhancement.

(I tested this with a little program called stress that can generate workloads, and I can occasionally see "stuck" states with stress. See the screenshot below.)

[aestriplex]

@fasterit

The native top also shows "stuck" tasks quite often, do we have an idea what that is and do we want to represent it?

@Explorer09

There is another "stuck" state that are shown in macOS's top, which display as Sleeping ("S") in htop instead with this PR. Although I didn't have the need to distinguish this "stuck" state from "sleeping" right now, it's still a room for a future enhancement.

I also think that it could be a good future enhancement

[aestriplex]

Thank you @BenBE

[fasterit]

@fasterit

The native top also shows "stuck" tasks quite often, do we have an idea what that is and do we want to represent it?

@Explorer09

There is another "stuck" state that are shown in macOS's top, which display as Sleeping ("S") in htop instead with this PR. Although I didn't have the need to distinguish this "stuck" state from "sleeping" right now, it's still a room for a future enhancement.

I also think that it could be a good future enhancement

Please get an understanding what "stuck" tasks are and how to identify them. This is so we don't have to completely rewrite the logic yet again. I'd like to be sure this can be an incremental change.
/DLange

[aestriplex]

@fasterit in top, a stuck task is a sleeping task that is waiting for resources. Usually it's waiting for a I/O resource that is currently blocked, or for a mutex tat lock some global resource. In top's man page, stuck is mentioned as "stuck" (i.e. uninterruptible sleep).

Anyway, by using public APIs (like the one we are using from sysctl or libproc) it's not so straightforward how to get this information. Probably we can get it by adding another syscall that shows us if a process is waiting for resources, and then we could do something like

if (ps->state == SLEEPING && isWaitingForResources(ps)) {
   ps->state = STUCK;
}

Although it's not clear to me which syscall shows the most reliable info in this respect, as far as I know, it should be an incremental change, because the set of stuck processes is a subset of sleeping processes.

[fasterit]

https://github.com/apple-oss-distributions/top/blob/47e40b78502828778d0632952e193442f4d24c74/libtop.c#L644 looks useful

[aestriplex]

@fasterit Yes, it seems useful, especially the way it's used here. Should I open a new pr for this change?

[BenBE]

If the change logically still belongs to this issue, you can just add additional commits to this PR.

[aestriplex]

Thank you @BenBE.
For what concerns stuck processes, simply adding the condition extended_info.pth_run_state == TH_STATE_UNINTERRUPTIBLE in DarwinProcess_scanThreads does not work. The threads I see in stuck state in top do not meet this condition in htop. I am currently working to see which syscalls return reliable information about the threads, process by process. For this reason, if it's ok with you, I would open a separate issue (with separate pr). Instances of stuck processes are quite rare (on my mac I only see them when I use the stress utility), so I think you might consider a small additional feature.

[BenBE]

As this PR and #1622 are currently blocking for the next release of htop, I'd appreciate, if we could avoid feature-creeping the changes in the first iteration and where sensible postpone these nice-to-have extensions to 3.4.1. As it is rn we already improved handling of process state quite a bit over the previous release, thus there's no need to hurry things along more than necessary.

If you're okay with this, I'd suggest we put this PR and #1622 for the current release and do all further enhancements in the next (minor) release.

[aestriplex]

@BenBE Sounds good to me