Re: [igt-dev] [PATCH i-g-t v4 3/3] tests/gem_ctx_param: Add set_priority tests for non SYS_NICE users

[Antonio Argenziano <antonio.argenziano@intel.com>]

On 25/01/18 03:37, Chris Wilson wrote:
> Quoting Antonio Argenziano (2018-01-25 01:00:03)
>> Adds tests for !SYS_NICE users trying to change the priority level of a
>> context using the provided IOCTL interface.
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>> ---
>>   README                 |  1 +
>>   configure.ac           |  8 ++++++++
>>   meson.build            |  3 +++
>>   tests/Makefile.am      |  7 +++++++
>>   tests/Makefile.sources |  1 -
>>   tests/gem_ctx_param.c  | 42 ++++++++++++++++++++++++++++++++++++------
>>   6 files changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/README b/README
>> index 7cd78e44..89c10beb 100644
>> --- a/README
>> +++ b/README
>> @@ -147,6 +147,7 @@ the default configuration (package names may vary):
>>          libkmod-dev
>>          libpciaccess-dev
>>          libprocps-dev
>> +       libcap-dev
>>          libunwind-dev
>>          python-docutils
>>          x11proto-dri2-dev
>> diff --git a/configure.ac b/configure.ac
>> index e13a3b74..2d67e3f1 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -81,6 +81,13 @@ AC_CHECK_FUNCS(timer_create, [], [
>>   ])
>>   AC_SUBST(TIMER_LIBS)
>>   
>> +AC_CHECK_HEADER(sys/capability.h, [have_libcap=yes])
>> +if test x$have_libcap = xyes; then
>> +       AC_DEFINE(HAVE_LIBCAP, 1, [Enable libcap support.])
>> +fi
>> +
>> +AM_CONDITIONAL(HAVE_LIBCAP, [test "x$have_libcap" = xyes])
>> +
>>   dnl Check for CPUID
>>   cpuid="yes"
>>   AC_TRY_LINK([
>> @@ -126,6 +133,7 @@ PKG_CHECK_MODULES(KMOD, [libkmod])
>>   PKG_CHECK_MODULES(PROCPS, [libprocps])
>>   PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
>>   PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
>> +#PKG_CHECK_MODULES(LIBCAP, [libcap-dev], [have_libcap=yes], [have_libcap=no])
>>   
>>   if test x$have_valgrind = xyes; then
>>          AC_DEFINE(HAVE_VALGRIND, 1, [Enable valgrind annotation support.])
>> diff --git a/meson.build b/meson.build
>> index 9036feb1..937edf52 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -106,6 +106,9 @@ endif
>>   if cc.has_header('sys/io.h')
>>          config.set('HAVE_SYS_IO_H', 1)
>>   endif
>> +if cc.has_header('sys/capability.h')
>> +       config.set('HAVE_LIBCAP', 1)
>> +endif
>>   if cc.has_header('cpuid.h')
>>          # FIXME: Do we need the example link test from configure.ac?
>>          config.set('HAVE_CPUID_H', 1)
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 1b9a7b0a..c482e970 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -14,6 +14,12 @@ if BUILD_VC4
>>       TESTS_progs += $(VC4_TESTS)
>>   endif
>>   
>> +if HAVE_LIBCAP
>> +TESTS_progs += \
>> +       gem_ctx_param \
>> +       $(NULL)
>> +endif
> 
> CI doesn't have libcap-dev so this nerfed the tests.
> Only one subtest requires LIBCAP...
> 
>> +#define NICE (0x1 << 4)
>>   #define ROOT (0x1 << 3)
>>   #define NEW_CTX        (0x1 << 2)
>>   #define VALID_PRIO (0x1 << 1)
>>   #define OVERFLOW_PRIO (0x1 << 0)
> 
> Hmm, this is opposite to the usual ordering :)
>>
>> -#define IS_ROOT(flags) (flags & ROOT)
>> +#define IS_ROOT_AND_NICE(flags) ((flags & ROOT) && (flags & NICE))
> 
> Nah, you meant IS_ROOT_OR_NICE in most places.

Or even just IS_NICE if I filter flags with NICE and !ROOT when creating 
sub-tests.

> 
>>   static int is_priority_valid(int64_t value, unsigned flags)
>>   {
>> -       if (IS_ROOT(flags)) {
>> +       if (IS_ROOT_AND_NICE(flags)) {
>>                  if ((value - I915_CONTEXT_MIN_USER_PRIORITY) <= PRIO_RANGE &&
>>                          (value - I915_CONTEXT_MIN_USER_PRIORITY) >= 0)
>>                          return 0;
>> @@ -67,7 +69,7 @@ get_prio_values_valid(int64_t **prio_values, unsigned *size, unsigned flags)
>>          const int64_t overflow = (flags & OVERFLOW_PRIO) ? (int64_t)1 << 32 : 0;
>>          int64_t *values;
>>   
>> -       *size = (IS_ROOT(flags) ? PRIO_RANGE : USER_PRIO_RANGE) + 1;
>> +       *size = (IS_ROOT_AND_NICE(flags) ? PRIO_RANGE : USER_PRIO_RANGE) + 1;
>>          values = (int64_t*) calloc(*size, sizeof(int64_t));
>>          igt_assert(values);
>>   
>> @@ -84,7 +86,7 @@ get_prio_values_invalid(int64_t **prio_values, unsigned *size, unsigned flags)
>>          const int64_t overflow = (flags & OVERFLOW_PRIO) ? (int64_t)1 << 32 : 0;
>>          int64_t *values;
>>   
>> -       if (IS_ROOT(flags)) {
>> +       if (IS_ROOT_AND_NICE(flags)) {
>>                  int64_t test_values[] = { /* Test space too big pick significant values */
>>                                  INT_MIN,
>>                                  I915_CONTEXT_MIN_USER_PRIORITY - 1,
>> @@ -119,6 +121,25 @@ get_prio_values(int64_t **prio_values, unsigned *size, unsigned flags)
>>          igt_permute_array(*prio_values, *size, igt_exchange_int64);
>>   }
>>   
>> +static void lower_sys_nice(void)
>> +{
> 
> #if HAVE_LIBCAP
> 
>> +       cap_t caps;
>> +       cap_value_t cap_list = CAP_SYS_NICE;
>> +       pid_t pid = getpid();
>> +       cap_flag_value_t cap_val;
>> +
>> +       caps = cap_get_pid(pid);
>> +       igt_require(caps);
>> +
>> +       cap_get_flag(caps, cap_list, CAP_EFFECTIVE, &cap_val);
>> +       if (cap_val == CAP_CLEAR)
>> +               return; /* CAP_SYS_NICE already unset */
>> +
>> +       igt_assert(cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap_list, CAP_CLEAR) == 0);
>> +       igt_assert(cap_set_proc(caps) == 0);
>> +       cap_free(caps);
> 
> #else
> 	return false;
> #endif
> 
>> +}
>> +
>>   static void
>>   set_priority(int fd, struct drm_i915_gem_context_param arg, unsigned flags)
>>   {
>> @@ -140,6 +161,11 @@ set_priority(int fd, struct drm_i915_gem_context_param arg, unsigned flags)
>>                          igt_drop_root();
>>                  }
>>   
>> +               if (!(flags & NICE)) {
>> +                       igt_debug("Dropping SYS_NICE capability\n");
>> +                       lower_sys_nice();
> 
> Rewrite as igt_require(drop_sys_nice());
> 
>> +               }
>> +
>>                  gem_context_get_param(fd, &arg);
>>                  old_value = arg.value;
>>   
>> @@ -312,9 +338,13 @@ igt_main
>>                  }
>>   
>>                  for (unsigned flags = 0;
>> -                               flags <= (ROOT | NEW_CTX | VALID_PRIO | OVERFLOW_PRIO);
>> +                               flags <= (NICE | ROOT | NEW_CTX | VALID_PRIO | OVERFLOW_PRIO);
>>                                  flags++) {
>> -                       igt_subtest_f("set-priority%s%s%s%s",
>> +                       if (!(flags & NICE) && !(flags & ROOT))
>> +                               continue; /* Needs to be rot to set properties */
> 
> Nope, just use igt_require as above.

I didn't want any "*-not-nice-user*" subtests to appear in the list at 
all, with igt_require they would just skip but still be there right?

Thanks,
Antonio

> 
>> +
>> +                       igt_subtest_f("set-priority%s%s%s%s%s",
>> +                                       (flags & NICE) ? "" : "-not-nice",
>>                                          (flags & ROOT) ? "-root" : "-user",
>>                                          (flags & NEW_CTX) ? "-new-ctx" : "-default-ctx",
>>                                          (flags & VALID_PRIO) ? "" : "-invalid",
>> -- 
>> 2.14.2
>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev