Message-ID: <op.2llzn7wgwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Tue, 02 Apr 2024 11:20:21 -0500
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: Michal Koutný <mkoutny@...e.com>, "Jarkko Sakkinen"
 <jarkko@...nel.org>
Cc: dave.hansen@...ux.intel.com, tj@...nel.org, linux-kernel@...r.kernel.org,
 linux-sgx@...r.kernel.org, x86@...nel.org, cgroups@...r.kernel.org,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
 sohil.mehta@...el.com, tim.c.chen@...ux.intel.com, zhiquan1.li@...el.com,
 kristen@...ux.intel.com, seanjc@...gle.com, zhanb@...rosoft.com,
 anakrish@...rosoft.com, mikko.ylinen@...ux.intel.com, yangjie@...rosoft.com,
 chrisyan@...rosoft.com
Subject: Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup
 testing

On Tue, 02 Apr 2024 06:58:40 -0500, Jarkko Sakkinen <jarkko@...nel.org>  
wrote:

> On Tue Apr 2, 2024 at 2:23 PM EEST, Michal Koutný wrote:
>> Hello.
>>
>> On Sat, Mar 30, 2024 at 01:26:08PM +0200, Jarkko Sakkinen  
>> <jarkko@...nel.org> wrote:
>> > > > It'd be more complicated and less readable to do all the stuff  
>> without the
>> > > > cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only  
>> depends
>> > > > on libc so I hope this would not cause too much inconvenience.
>> > >
>> > > As per cgroup-tools, please prove this. It makes the job for more
>> > > complicated *for you* and you are making the job more  complicated
>> > > to every possible person in the planet running any kernel QA.
>> > >
>> > > I weight the latter more than the former. And it is exactly the
>> > > reason why we did custom user space kselftest in the first place.
>> > > Let's keep the tradition. All I can say is that kselftest is
>> > > unfinished in its current form.
>> > >
>> > > What is "esp cgexec"?
>> >
>> > Also in kselftest we don't drive ultimate simplicity, we drive
>> > efficient CI/QA. By open coding something like subset of
>> > cgroup-tools needed to run the test you also help us later
>> > on to backtrack the kernel changes. With cgroups-tools you
>> > would have to use strace to get the same info.
>>
>> FWIW, see also functions in
>> tools/testing/selftests/cgroup/cgroup_util.{h,c}.
>> They likely cover what you need already -- if the tests are in C.
>>
>> (I admit that stuff in tools/testing/selftests/cgroup/ is best
>> understood with strace.)
>
> Thanks!
>
> My conclusions are that:
>
> 1. We probably cannot move the test part of cgroup test itself
>    given the enclave payload dependency.
> 2. I think it makes sense to still follow the same pattern as
>    other cgroups test and re-use cgroup_util.[ch] functionaltiy.
>
> So yeah I guess we need two test programs instead of one.
>
> Something along the lines:
>
> 1. main.[ch] -> test_sgx.[ch]
> 2. introduce test_sgx_cgroup.c
>
> And test_sgx_cgroup.c would be implement similar test as the shell
> script and would follow the structure of existing cgroups tests.
>
>>
>> HTH,
>> Michal
>
> BR, Jarkko
>
Do we really want to have it implemented in c? There are much fewer lines  
of code in shell scripts. Note we are not really testing basic cgroup  
stuff. All we needed were creating/deleting cgroups and set limits which I  
think have been demonstrated feasible in the ash scripts now.

Given Dave's comments, and test scripts being working and cover the cases  
needed IMHO, I don't see much need to move to c code. I can add more cases  
if needed and fall back a c implementation later  if any case can't be  
implemented in scripts. How about that?

Haitao

Powered by blists - more mailing lists