All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Palethorpe <rpalethorpe@suse.de>
To: Joerg Vehlow <lkml@jv-coder.de>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH 0/3] cpuset_regression_test: convert and improve
Date: Mon, 15 Nov 2021 09:19:52 +0000	[thread overview]
Message-ID: <871r3heps5.fsf@suse.de> (raw)
In-Reply-To: <3a6d7a45-2205-34f9-aaab-2d039d132456@jv-coder.de>

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi Richard,
>
> On 6/23/2021 1:11 PM, Richard Palethorpe wrote:
>> Hello Joerg,
>>
>> Joerg Vehlow <lkml@jv-coder.de> writes:
>>
>>> Hi,
>>>
>>> this is more or less a v2 of a patch I send previously (patch 3).
>>> I know that richard is not entirely happy with this patch, I will
>>> give it another try anyway...
>>> It is either this patch or another patch, that has to look through
>>> the cgroup hierarchy, to check if there is any group,that explicitely
>>> uses cpu 0.
>> If it is already being used then can you set it?
> The test can use any cpu, that is not explicitly assigned to a group
> already.
> What I mean by "either this or another patch" (and forgot to type),
> the alternative
> patch has to check if anything is using cpu 0 explicitly and then fail
> with TCONF.
> Or it has to look for an used cpu core. That would be another possibility...
>
>>
>>> To me, it is a better solution to just change groups for a short time,
>>> and check if the bug exists. If ltp tests are running, the chance, that
>>> there is anything running, that really needs a correct cpuset is very low.
>>> I am comming from a system, where cgroups are setup by a container launcher,
>>> that just happens to assign cpus to the containers - not even exclusively.
>>> LTP tests are used as some part of the testsuite, to test as close to a
>>> production system as possible.
>> I was thinking that if you are already using CPU sets then you either
>> don't have the bug or you won't hit it on your setup(s)? If so then the
>> test is redundant.
> True about the "don't hit it part", at least with the setup, but I
> guess the reason for a regression test,
> is to prevent regressions. This was clearly a bug in the kernel and
> not only an inconvenience. And since
> there  is not "the one kernel source", I think it is important to run
> tests like this for as many different
> kernels as possible. Apart from the already setup cgroups, there may
> be other uses of cgroups as well,
> that try to set them up the other way around (first exclusive, then cpus).
>>
>>> The only way I could think of a process misbehaving by disabeling cpu pinning,
>>> would be a badly written multithread application, that cannot correctly run,
>>> if threads are really running in parallel, but this would also require a scheduling
>>> policy, that makes scheduling points predicatable. While I know that software like
>>> that exists (in fact I was working on something like that in the past), I think it
>>> is highly unlikely, that it is running parallel to ltp.
>>> And even then, this could be mitigated by not just setting cpu binding to undefined,
>>> but to one fixed core. But with the changes in patch 2, this is not
>>> possible.
>>>
>>> But anyhow ltp fiddles with lots of critical system parameters during it's runtime,
>>> there is no guarantee, that an application that requires some very specific kernel
>>> runtime settings survives this. That's why I would still vote for this patch.
>>>
>>> Jörg
>> I still think it has a small chance of causing problems for us. There
>> are some heterogeneous CPU systems where control software should run on
>> a given CPU. I don't know whether CGroups are used to control that or if
>> it would matter if the process is moved temporarily. It's just a small
>> risk I would avoid if the test is not really worth it.
> I get that, but these systems may have to opt-out of some tests anyway.
>>
>> OTOH the patch looks good otherwise, so it should be merged if no one
>> else agrees with me.
> Ok, lets see what the others have to say :)
>
> Jörg

So a few months later there are no comments. The patch-set as a whole
looks a like an improvement. So let's merge it.


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

      reply	other threads:[~2021-11-15  9:23 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23  7:15 [LTP] [PATCH 0/3] cpuset_regression_test: convert and improve Joerg Vehlow
2021-06-23  7:15 ` [LTP] [PATCH 1/3] cpuset_regression_test: Convert to new api Joerg Vehlow
2021-11-15  9:23   ` Richard Palethorpe
2021-11-15 10:36   ` Richard Palethorpe
2021-06-23  7:15 ` [LTP] [PATCH 2/3] cpuset_regression_test: Drop min cpu requirement Joerg Vehlow
2021-11-15  9:23   ` Richard Palethorpe
2021-06-23  7:15 ` [LTP] [PATCH 3/3] cpuset_regression_test: Allow running, if groups exist Joerg Vehlow
2021-11-15  9:24   ` Richard Palethorpe
2021-06-23 11:11 ` [LTP] [PATCH 0/3] cpuset_regression_test: convert and improve Richard Palethorpe
2021-06-23 11:20   ` Joerg Vehlow
2021-11-15  9:19     ` Richard Palethorpe [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871r3heps5.fsf@suse.de \
    --to=rpalethorpe@suse.de \
    --cc=lkml@jv-coder.de \
    --cc=ltp@lists.linux.it \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.