Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux: support for monitoring syscall #1486

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JohnSanpe
Copy link

No description provided.

linux/LinuxProcess.c Outdated Show resolved Hide resolved
linux/LinuxProcess.h Outdated Show resolved Hide resolved
linux/LinuxProcessTable.c Outdated Show resolved Hide resolved
linux/LinuxProcessTable.c Outdated Show resolved Hide resolved
linux/LinuxProcessTable.c Outdated Show resolved Hide resolved
linux/LinuxProcess.c Outdated Show resolved Hide resolved
@BenBE
Copy link
Member

BenBE commented May 31, 2024

Any chance this kind of information is available on other platforms too?

@BenBE BenBE added Linux 🐧 Linux related issues new feature Completely new feature labels May 31, 2024
@JohnSanpe
Copy link
Author

Any chance this kind of information is available on other platforms too?

Theoretically possible, but currently only intended to support Linux.

linux/LinuxProcessTable.c Outdated Show resolved Hide resolved
linux/LinuxProcessTable.c Outdated Show resolved Hide resolved
linux/LinuxProcessTable.c Outdated Show resolved Hide resolved
@Explorer09
Copy link
Contributor

One more thing. We should allow the user to disable this "syscall" monitoring at compile time time. Since the /proc/<pid>/syscall file is only present when the kernel is compiled with CONFIG_HAVE_ARCH_TRACEHOOK, it may be useless for users without such kernel. Another reason for having this compile option is we can save code size by not compiling the syscall number-to-name mapping table into htop binary, in case we implement one in the future.

@BenBE
Copy link
Member

BenBE commented Jun 1, 2024

@Explorer09 Or disable the column, when the feature is not available at runtime.

@Explorer09
Copy link
Contributor

@ Explorer09 Or disable the column, when the feature is not available at runtime.

I wish a compile time option to disable it as well.

@BenBE
Copy link
Member

BenBE commented Jun 1, 2024

@ Explorer09 Or disable the column, when the feature is not available at runtime.

I wish a compile time option to disable it as well.

Like so?

# Check for syscall feature option
AC_ARG_ENABLE(
   [syscall],
   [
      AS_HELP_STRING(
         [--enable-syscall],
         [Enable 'syscall' feature. Possible values: off, buildtime-check, runtime-check, always-on. Default is 'off'.]
      )
   ],
   [case "${enableval}" in
      off) syscall_feature=0 ;;
      buildtime-check) syscall_feature=1 ;;
      runtime-check) syscall_feature=2 ;;
      always-on) syscall_feature=3 ;;
      *) AC_MSG_ERROR([Invalid value for --enable-syscall]) ;;
   esac],
   [syscall_feature=0]
)

# If buildtime-check is enabled, check if /proc/self/syscall exists and is non-empty
AS_IF(
   [test "x$syscall_feature" = "x1"],
   [
      AC_CHECK_FILE(
         [/proc/self/syscall],
         [syscall_exists=yes],
         [syscall_exists=no]
      )
      AS_IF(
         [test "x$syscall_exists" = "xyes" && test -s /proc/self/syscall],
         [],
         [syscall_feature=0]
      )
   ]
)

# Set the result of the check to a preprocessor directive
AC_DEFINE_UNQUOTED([SYSCALL_FEATURE], [$syscall_feature], [Define to the selected syscall feature state.])

@Explorer09
Copy link
Contributor

@BenBE

  • I merged your "runtime-check" and "always-on" into one option, because checking the thing at runtime is trivial.
  • When cross-compiling, checking /proc/self/syscall won't help. It's better just enable it in that case.
AC_ARG_ENABLE([syscall],
   [AS_HELP_STRING([--enable-syscall],
   [enable 'syscall' monitoring. @<:@default=check@:>@])],
   [],
   [enable_syscall=check]
)

if test "x$enable_syscall" = xcheck; then
   if "$cross_compiling" != no; then
      enable_syscall=yes
   elif test -f /proc/self/syscall && test -s /proc/self/syscall; then
      enable_syscall=yes
   else
      enable_syscall=no
   fi
fi
if test "x$enable_syscall" = xyes; then
   AC_DEFINE([HAVE_SYSCALL], [1], [Define to 1 is syscall monitoring is enabled.])
fi

@JohnSanpe
Copy link
Author

JohnSanpe commented Jun 1, 2024

@BenBE

* I merged your "runtime-check" and "always-on" into one option, because checking the thing at runtime is trivial.

* When cross-compiling, checking `/proc/self/syscall` won't help. It's better just enable it in that case.
AC_ARG_ENABLE([syscall],
   [AS_HELP_STRING([--enable-syscall],
   [enable 'syscall' monitoring. @<:@default=check@:>@])],
   [],
   [enable_syscall=check]
)

if test "x$enable_syscall" = xcheck; then
   if "$cross_compiling" != no; then
      enable_syscall=yes
   elif test -f /proc/self/syscall && test -s /proc/self/syscall; then
      enable_syscall=yes
   else
      enable_syscall=no
   fi
fi
if test "x$enable_syscall" = xyes; then
   AC_DEFINE([HAVE_SYSCALL], [1], [Define to 1 is syscall monitoring is enabled.])
fi

Very good job, I will merge it

@JohnSanpe
Copy link
Author

@Explorer09

@JohnSanpe You should handle the running string as special and assign a placeholder number for it. The reason is it allows us to display running string with colors. This status should be special in sorting/collating anyway.

Using special placeholder number may conflict with future syscalls, so storing possible states separately may be a better choice? This is also more convenient for us to use lookup tables in the future.

@Explorer09
Copy link
Contributor

You should handle the running string as special and assign a placeholder number for it. The reason is it allows us to display running string with colors. This status should be special in sorting/collating anyway.

Using special placeholder number may conflict with future syscalls, so storing possible states separately may be a better choice? This is also more convenient for us to use lookup tables in the future.

Use a negative number. E.g. -2, or maybe -1000 which has an even less chance to cause conflict.

@JohnSanpe
Copy link
Author

You should handle the running string as special and assign a placeholder number for it. The reason is it allows us to display running string with colors. This status should be special in sorting/collating anyway.

Using special placeholder number may conflict with future syscalls, so storing possible states separately may be a better choice? This is also more convenient for us to use lookup tables in the future.

Use a negative number. E.g. -2, or maybe -1000 which has an even less chance to cause conflict.

I have already fixed it 😄

SYSCALL_STATE_RUNNING,
SYSCALL_STATE_NORMAL,
SYSCALL_STATE_NA,
} SyscallState;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are doing something wrongly here. Normally a process won't be waiting for a syscall, so the presence of a syscall number would be more of an exception rather than rule.

I won't say you can't use an enum here. But the better way is to merge this status with the syscall number, so that you don't need two member variables for recording the same thing.

Here is what I mean:

typedef enum LinuxSyscallState_ {
   SYSCALL_STATE_NO_DATA = INT_MIN,
   SYSCALL_STATE_RUNNING = INT_MIN + 1,
   SYSCALL_STATE_BLOCKED = -1
} LinuxSyscallState;

By the way, Linux syscall table for x86-64 is in syscall_64.tbl. The file is located in different directories in Linux source code throughout Linux versions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also know that the x32 system call starts from 0x40000000, and the system call allows negative numbers to exist. Using one variable is always unsafe, but using two can completely eliminate this kind of thing and is beneficial for sorting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also know that the x32 system call starts from 0x40000000, and the system call allows negative numbers to exist. Using one variable is always unsafe, but using two can completely eliminate this kind of thing and is beneficial for sorting

Nah. 0x40000000 is positive. The negative should always be unused or otherwise they won't reserve -1 for the documented reason.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, 0x40000000 is certainly not a negative number, but it is a special case. With one special case, there may be more. Also, if you do this, it will cause all N/A to be ranked at the top, which is not user-friendly. 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnSanpe With a mapping table implemented, you are not likely order the syscall by its number, but by the mapped name instead.
Linux assigns syscall number in a rather arbitrary way, so, with the table implemented, ordering by name would make more sense.

Copy link

@ffashion ffashion Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Explorer09 @JohnSanpe
First, i think use negative number to show a syscall state. it's unsafe. Although Linux may not currently use negative numbers as syscall ID.
Second, The state of syscall and the syscall ID themselves are actually two different things, and merging them would reduce readability.
Third, i think we can use a struct to wrapper syscall id and syscall state, like

typedef enum LinuxSyscallState_ {
   SYSCALL_STATE_ID,
   SYSCALL_STATE_RUNNING,
   SYSCALL_STATE_NA
} LinuxSyscallState;

typedef struct LinuxSyscall_ {
  LinuxSyscallState state;
  int  syscall_id;
} LinuxSyscall;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux 🐧 Linux related issues new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants