[libc] Fix sched_getscheduler return value

[mikhail.ramalho]

Summary

This patch fixes the return time for sched_getscheduler which was set to
always zero. The syscall documentation, however, defines:

On success, sched_getscheduler() returns the policy for the thread (a
nonnegative integer).

I also changed the return type for sched_setscheduler, but this change
didn't impact and test case.

This patch also removes the duplicated code from param_and_scheduler_test.cpp
and adds SCHED_BATCH and SCHED_IDLE to the tests.

[sivachandra]

Overall LGTM from my side but please wait for @michaelrj's review.

libc/test/src/sched/param_and_scheduler_test.cpp

40

I think the duplicated code is present to help with debugging.

[michaelrj]

libc/test/src/sched/param_and_scheduler_test.cpp

40

the reason these tests were separated to begin with is the ASSERT macro only reports the line its on. This makes it very hard to debug test failures where the error doesn't obviously map to a specific input to your test function. In this case I'd recommend making testSched a proper function, then making each of the calls its own TEST to solve that problem.

[goldstein.w.n]

Minus test changes LGTM. But I am not a maintainer for this project so please wait on michael or sivachandras approval.

[mikhail.ramalho]

• Changed test case to should sched policy on failure, it should look like:

[ RUN      ] LlvmLibcSchedTest.Sched_SCHED_FIFO
/home/mgadelha/tools/llvm-project/libc/test/src/sched/param_and_scheduler_test.cpp:135: FAILURE
      Expected: __llvm_libc::sched_setscheduler(0, policy, nullptr)
      Which is: -1
To be equal to: 0
      Which is: 0
[  FAILED  ] LlvmLibcSchedTest.Sched_SCHED_FIFO

In this case, SCHED_FIFO was policy used.

• Rebased

[michaelrj]

LGTM

libc/src/sched/linux/sched_getscheduler.cpp

 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(int, sched_getscheduler, (pid_t tid)) {
   long ret = __llvm_libc::syscall_impl(SYS_sched_getscheduler, tid);
   if (ret < 0) {
     libc_errno = -ret;
     return -1;
   }
-  return 0;
+  return ret;
 }
 
 } // namespace __llvm_libc

libc/src/sched/linux/sched_setscheduler.cpp

 LLVM_LIBC_FUNCTION(int, sched_setscheduler,
                    (pid_t tid, int policy, const struct sched_param *param)) {
   long ret =
       __llvm_libc::syscall_impl(SYS_sched_setscheduler, tid, policy, param);
   if (ret < 0) {
     libc_errno = -ret;
     return -1;
   }
-  return 0;
+  return ret;
 }
 
 } // namespace __llvm_libc

libc/test/src/sched/param_and_scheduler_test.cpp

 //          seems risky. Maybe something with fork/clone would work.
 //
 //       2) Unkown pid -> ESRCH. Probably safe to choose a large range
 //          number or scanning current pids and getting one that doesn't
 //          exist, but again seems like it may risk actually changing
 //          sched policy on a running task.
 //
 //       Linux specific test could also include:
-//          SCHED_BATCH, SCHED_ISO, SCHED_IDLE, SCHED_DEADLINE
+//          SCHED_ISO, SCHED_DEADLINE
 
-TEST(LlvmLibcSchedParamAndSchedulerTest, SchedOtherTest) {
+TEST(LlvmLibcSchedParamAndSchedulerTest, SchedTest) {
   libc_errno = 0;
 
-  int policy = SCHED_OTHER;
-  bool can_set = true;
-
-  int init_policy = __llvm_libc::sched_getscheduler(0);
-  ASSERT_GE(init_policy, 0);
-  ASSERT_EQ(libc_errno, 0);
-
-  int max_priority = __llvm_libc::sched_get_priority_max(policy);
-  ASSERT_GE(max_priority, 0);
-  ASSERT_EQ(libc_errno, 0);
-  int min_priority = __llvm_libc::sched_get_priority_min(policy);
-  ASSERT_GE(min_priority, 0);
-  ASSERT_EQ(libc_errno, 0);
-
-  struct sched_param param = {min_priority};
-
-  // Negative pid
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(-1, policy, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(-1), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  // Invalid Policy
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy | 128, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  // Out of bounds priority
-  param.sched_priority = min_priority - 1;
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  param.sched_priority = max_priority + 1;
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
-  // A bit hard to test as depending if we are root or not we can run into
-  // different issues.
-  ASSERT_TRUE(libc_errno == EINVAL || libc_errno == EPERM);
-  libc_errno = 0;
-
-  // Some sched policies require permissions, so skip
-  param.sched_priority = min_priority;
-  // Success / missing permissions.
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param),
-            can_set ? 0 : -1);
-  ASSERT_TRUE(can_set ? (libc_errno == 0)
-                      : (libc_errno == EINVAL || libc_errno == EPERM));
-  libc_errno = 0;
-
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(0), can_set ? policy : init_policy);
-  ASSERT_EQ(libc_errno, 0);
-
-  // Out of bounds priority
-  param.sched_priority = -1;
-  ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  param.sched_priority = max_priority + 1;
-  ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  for (int priority = min_priority; priority <= max_priority; ++priority) {
-    ASSERT_EQ(__llvm_libc::sched_getparam(0, &param), 0);
-    ASSERT_EQ(libc_errno, 0);
-    int init_priority = param.sched_priority;
-
-    param.sched_priority = priority;
-
-    // Negative pid
-    ASSERT_EQ(__llvm_libc::sched_setparam(-1, &param), -1);
-    ASSERT_EQ(libc_errno, EINVAL);
-    libc_errno = 0;
-
-    ASSERT_EQ(__llvm_libc::sched_getparam(-1, &param), -1);
-    ASSERT_EQ(libc_errno, EINVAL);
-    libc_errno = 0;
-
-    // Success / missing permissions
-    ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), can_set ? 0 : -1);
-    ASSERT_TRUE(can_set ? (libc_errno == 0)
-                        : (libc_errno == EINVAL || libc_errno == EPERM));
-    libc_errno = 0;
-
-    ASSERT_EQ(__llvm_libc::sched_getparam(0, &param), 0);
-    ASSERT_EQ(libc_errno, 0);
-
-    ASSERT_EQ(param.sched_priority, can_set ? priority : init_priority);
-  }
-  // Null test
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, nullptr), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-}
-
-TEST(LlvmLibcSchedParamAndSchedulerTest, SchedFIFOTest) {
-  libc_errno = 0;
-
-  int policy = SCHED_FIFO;
-  bool can_set = __llvm_libc::getuid() == 0;
-
+  auto testSched = [&](int policy, bool can_set) -> void {
     int init_policy = __llvm_libc::sched_getscheduler(0);
     ASSERT_GE(init_policy, 0);
     ASSERT_EQ(libc_errno, 0);
 
     int max_priority = __llvm_libc::sched_get_priority_max(policy);
     ASSERT_GE(max_priority, 0);
     ASSERT_EQ(libc_errno, 0);
     int min_priority = __llvm_libc::sched_get_priority_min(policy);
     ASSERT_GE(min_priority, 0);
     ASSERT_EQ(libc_errno, 0);
 
     struct sched_param param = {min_priority};
 
     // Negative pid
     ASSERT_EQ(__llvm_libc::sched_setscheduler(-1, policy, &param), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
 
     ASSERT_EQ(__llvm_libc::sched_getscheduler(-1), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
 
     // Invalid Policy
     ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy | 128, &param), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
 
     // Out of bounds priority
     param.sched_priority = min_priority - 1;
     ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
 
     param.sched_priority = max_priority + 1;
     ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
     // A bit hard to test as depending if we are root or not we can run into
     // different issues.
     ASSERT_TRUE(libc_errno == EINVAL || libc_errno == EPERM);
     libc_errno = 0;
 
     // Some sched policies require permissions, so skip
     param.sched_priority = min_priority;
     // Success / missing permissions.
     ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param),
               can_set ? 0 : -1);
     ASSERT_TRUE(can_set ? (libc_errno == 0)
                         : (libc_errno == EINVAL || libc_errno == EPERM));
     libc_errno = 0;
 
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(0), can_set ? policy : init_policy);
+    ASSERT_EQ(__llvm_libc::sched_getscheduler(0),
+              can_set ? policy : init_policy);
     ASSERT_EQ(libc_errno, 0);
 
     // Out of bounds priority
     param.sched_priority = -1;
     ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
 
     param.sched_priority = max_priority + 1;
     ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
 
     for (int priority = min_priority; priority <= max_priority; ++priority) {
       ASSERT_EQ(__llvm_libc::sched_getparam(0, &param), 0);
       ASSERT_EQ(libc_errno, 0);
       int init_priority = param.sched_priority;
 
       param.sched_priority = priority;
 
       // Negative pid
       ASSERT_EQ(__llvm_libc::sched_setparam(-1, &param), -1);
       ASSERT_EQ(libc_errno, EINVAL);
       libc_errno = 0;
 
       ASSERT_EQ(__llvm_libc::sched_getparam(-1, &param), -1);
       ASSERT_EQ(libc_errno, EINVAL);
       libc_errno = 0;
 
       // Success / missing permissions
       ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), can_set ? 0 : -1);
       ASSERT_TRUE(can_set ? (libc_errno == 0)
                           : (libc_errno == EINVAL || libc_errno == EPERM));
       libc_errno = 0;
 
       ASSERT_EQ(__llvm_libc::sched_getparam(0, &param), 0);
       ASSERT_EQ(libc_errno, 0);
 
       ASSERT_EQ(param.sched_priority, can_set ? priority : init_priority);
     }
     // Null test
     ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, nullptr), -1);
     ASSERT_EQ(libc_errno, EINVAL);
     libc_errno = 0;
-}
-
-TEST(LlvmLibcSchedParamAndSchedulerTest, SchedRRTest) {
-  libc_errno = 0;
-
-  int policy = SCHED_RR;
-  bool can_set = __llvm_libc::getuid() == 0;
-
-  int init_policy = __llvm_libc::sched_getscheduler(0);
-  ASSERT_GE(init_policy, 0);
-  ASSERT_EQ(libc_errno, 0);
-
-  int max_priority = __llvm_libc::sched_get_priority_max(policy);
-  ASSERT_GE(max_priority, 0);
-  ASSERT_EQ(libc_errno, 0);
-  int min_priority = __llvm_libc::sched_get_priority_min(policy);
-  ASSERT_GE(min_priority, 0);
-  ASSERT_EQ(libc_errno, 0);
-
-  struct sched_param param = {min_priority};
-
-  // Negative pid
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(-1, policy, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(-1), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  // Invalid Policy
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy | 128, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  // Out of bounds priority
-  param.sched_priority = min_priority - 1;
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  param.sched_priority = max_priority + 1;
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
-  // A bit hard to test as depending if we are root or not we can run into
-  // different issues.
-  ASSERT_TRUE(libc_errno == EINVAL || libc_errno == EPERM);
-  libc_errno = 0;
-
-  // Some sched policies require permissions, so skip
-  param.sched_priority = min_priority;
-  // Success / missing permissions.
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param),
-            can_set ? 0 : -1);
-  ASSERT_TRUE(can_set ? (libc_errno == 0)
-                      : (libc_errno == EINVAL || libc_errno == EPERM));
-  libc_errno = 0;
-
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(0), can_set ? policy : init_policy);
-  ASSERT_EQ(libc_errno, 0);
-
-  // Out of bounds priority
-  param.sched_priority = -1;
-  ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  param.sched_priority = max_priority + 1;
-  ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  for (int priority = min_priority; priority <= max_priority; ++priority) {
-    ASSERT_EQ(__llvm_libc::sched_getparam(0, &param), 0);
-    ASSERT_EQ(libc_errno, 0);
-    int init_priority = param.sched_priority;
-
-    param.sched_priority = priority;
+  };
 
-    // Negative pid
-    ASSERT_EQ(__llvm_libc::sched_setparam(-1, &param), -1);
-    ASSERT_EQ(libc_errno, EINVAL);
-    libc_errno = 0;
-
-    ASSERT_EQ(__llvm_libc::sched_getparam(-1, &param), -1);
-    ASSERT_EQ(libc_errno, EINVAL);
-    libc_errno = 0;
-
-    // Success / missing permissions
-    ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), can_set ? 0 : -1);
-    ASSERT_TRUE(can_set ? (libc_errno == 0)
-                        : (libc_errno == EINVAL || libc_errno == EPERM));
-    libc_errno = 0;
-
-    ASSERT_EQ(__llvm_libc::sched_getparam(0, &param), 0);
-    ASSERT_EQ(libc_errno, 0);
-
-    ASSERT_EQ(param.sched_priority, can_set ? priority : init_priority);
-  }
-  // Null test
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, nullptr), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
+  constexpr int policiesRootRequired[] = {SCHED_FIFO, SCHED_RR};
+  for (auto policy : policiesRootRequired)
+    testSched(policy, /*can_set=*/__llvm_libc::getuid() == 0);
+
+  constexpr int policiesNoRootRequired[] = {SCHED_OTHER, SCHED_BATCH,
+                                            SCHED_IDLE};
+  for (auto policy : policiesNoRootRequired)
+    testSched(policy, /*can_set=*/true);
 }
 
 TEST(LlvmLibcSchedParamAndSchedulerTest, NullParamTest) {
   libc_errno = 0;
 
   ASSERT_EQ(__llvm_libc::sched_setparam(0, nullptr), -1);
   ASSERT_EQ(libc_errno, EINVAL);
   libc_errno = 0;
 
   ASSERT_EQ(__llvm_libc::sched_getparam(0, nullptr), -1);
   ASSERT_EQ(libc_errno, EINVAL);
   libc_errno = 0;
 }