Linux-rt-users Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rt-tests: Drop use_current_cpuset() check
@ 2021-03-15 19:37 Peter Xu
  2021-03-16  8:18 ` Daniel Wagner
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2021-03-15 19:37 UTC (permalink / raw)
  To: linux-rt-users; +Cc: John Kacur, peterx, Clark Williams, Daniel Wagner

CPU list should allow to be any list rather than only the cores that are
allowed to schedule for the current process.

Before this patch, cyclictest will fail with below condition:

$ taskset -pc $$
pid 2316's current affinity list: 0,2,4,6,8
$ sudo cyclictest -m -N -p 1 -a 1,3,5,7 -t 4
WARN: Couldn't setaffinity in main thread: Invalid argument

After this patch, it'll be allowed to run.

Cc: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 src/lib/rt-numa.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
index babcc63..f581020 100644
--- a/src/lib/rt-numa.c
+++ b/src/lib/rt-numa.c
@@ -93,32 +93,6 @@ int cpu_for_thread_ua(int thread_num, int max_cpus)
 	return 0;
 }
 
-/*
- * After this function is called, affinity_mask is the intersection of
- * the user supplied affinity mask and the affinity mask from the run
- * time environment
- */
-static void use_current_cpuset(int max_cpus, struct bitmask *cpumask)
-{
-	struct bitmask *curmask;
-	int i;
-
-	curmask = numa_allocate_cpumask();
-	numa_sched_getaffinity(getpid(), curmask);
-
-	/*
-	 * Clear bits that are not set in both the cpuset from the
-	 * environment, and in the user specified affinity.
-	 */
-	for (i = 0; i < max_cpus; i++) {
-		if ((!numa_bitmask_isbitset(cpumask, i)) ||
-		    (!numa_bitmask_isbitset(curmask, i)))
-			numa_bitmask_clearbit(cpumask, i);
-	}
-
-	numa_bitmask_free(curmask);
-}
-
 int parse_cpumask(char *str, int max_cpus, struct bitmask **cpumask)
 {
 	struct bitmask *mask;
@@ -133,7 +107,6 @@ int parse_cpumask(char *str, int max_cpus, struct bitmask **cpumask)
 		return 0;
 	}
 
-	use_current_cpuset(max_cpus, mask);
 	*cpumask = mask;
 
 	return 0;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-15 19:37 [PATCH] rt-tests: Drop use_current_cpuset() check Peter Xu
@ 2021-03-16  8:18 ` Daniel Wagner
  2021-03-16 20:07   ` Peter Xu
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Wagner @ 2021-03-16  8:18 UTC (permalink / raw)
  To: Peter Xu; +Cc: linux-rt-users, John Kacur, Clark Williams, Daniel Wagner

Hi Peter,

On Mon, Mar 15, 2021 at 03:37:07PM -0400, Peter Xu wrote:
> CPU list should allow to be any list rather than only the cores that are
> allowed to schedule for the current process.

See commit aaa57168dfd3 ("rt-tests: cyclictest: Only run on runtime
affinity and user supplied affinity")

> Before this patch, cyclictest will fail with below condition:
> 
> $ taskset -pc $$
> pid 2316's current affinity list: 0,2,4,6,8
> $ sudo cyclictest -m -N -p 1 -a 1,3,5,7 -t 4
> WARN: Couldn't setaffinity in main thread: Invalid argument
> 
> After this patch, it'll be allowed to run.

As John writes in the commit above message I think cyclictest should
honor to the runtime settings. The warning above could be extended and
telling you what's the problem is.

Your commit message should contain the argument why it's better not to
check the environment settings. So the question is what is the least
surprise for the user?

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-16  8:18 ` Daniel Wagner
@ 2021-03-16 20:07   ` Peter Xu
  2021-03-17  7:49     ` Daniel Wagner
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2021-03-16 20:07 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: linux-rt-users, John Kacur, Clark Williams, Daniel Wagner

On Tue, Mar 16, 2021 at 09:18:26AM +0100, Daniel Wagner wrote:
> Hi Peter,

Daniel,

> 
> On Mon, Mar 15, 2021 at 03:37:07PM -0400, Peter Xu wrote:
> > CPU list should allow to be any list rather than only the cores that are
> > allowed to schedule for the current process.
> 
> See commit aaa57168dfd3 ("rt-tests: cyclictest: Only run on runtime
> affinity and user supplied affinity")

(Thanks for the commit ID)

> 
> > Before this patch, cyclictest will fail with below condition:
> > 
> > $ taskset -pc $$
> > pid 2316's current affinity list: 0,2,4,6,8
> > $ sudo cyclictest -m -N -p 1 -a 1,3,5,7 -t 4
> > WARN: Couldn't setaffinity in main thread: Invalid argument
> > 
> > After this patch, it'll be allowed to run.
> 
> As John writes in the commit above message I think cyclictest should
> honor to the runtime settings. The warning above could be extended and
> telling you what's the problem is.
> 
> Your commit message should contain the argument why it's better not to
> check the environment settings. So the question is what is the least
> surprise for the user?

I think what I'm missing is why we had such a restriction.  Quotting from the
commit ID:

    Currently if the user passes the affinity to cyclictest, threads are run
    there even if they should be excluded according to the affinity of the
    runtime environment.

So I'm not sure I understand the word "runtime environment".

If it's defined as "the set of cores that this process is allowed to run", I
don't understand why it's not allowed to schedule things outside of this set of
cores, especially if sched_setaffinity() syscall would succeed.  IOW, I'm
afraid we got the idea slightly mixed up on "cores allowed to schedule this
process" with "cores allowed to be set as schedule target by this process", and
IMHO we should simply rely on sched_setaffinity() syscall to decide whether the
affinity setting is legal or not as a single checkpoint.

John and I had some discussion offlist about this last time on oslat, it should
be the same thing here I think. But I'd like John to help confirm too since I
could be missing something.

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-16 20:07   ` Peter Xu
@ 2021-03-17  7:49     ` Daniel Wagner
  2021-03-17 12:51       ` Peter Xu
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Wagner @ 2021-03-17  7:49 UTC (permalink / raw)
  To: Peter Xu; +Cc: linux-rt-users, John Kacur, Clark Williams, Daniel Wagner

On Tue, Mar 16, 2021 at 04:07:05PM -0400, Peter Xu wrote:
> I think what I'm missing is why we had such a restriction.  Quotting from the
> commit ID:

IIRC, the current behavior allows the process to be placed into a cgroup
with a subset of CPUs and you just can do 'cyclictest -a -t'. Process
should not ignore external configuration. That's my whole point here.

> So I'm not sure I understand the word "runtime environment".
>
> If it's defined as "the set of cores that this process is allowed to
> run",

I am trying to say is, the tests should not assume they have the full
control of the placement as this is not what I would expect. If I as
'admin' limits the sched mask then cyclictest should not overwrite it.

If you insist removing this code, please add a section in the
documentation explaining why the tools ignore it.

> John and I had some discussion offlist about this last time on oslat,

As I said, I think this behavior is wrong. Anyway, I stopped caring.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17  7:49     ` Daniel Wagner
@ 2021-03-17 12:51       ` Peter Xu
  2021-03-17 15:08         ` John Kacur
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2021-03-17 12:51 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: linux-rt-users, John Kacur, Clark Williams, Daniel Wagner

Hi, Daniel,

On Wed, Mar 17, 2021 at 08:49:03AM +0100, Daniel Wagner wrote:
> On Tue, Mar 16, 2021 at 04:07:05PM -0400, Peter Xu wrote:
> > I think what I'm missing is why we had such a restriction.  Quotting from the
> > commit ID:
> 
> IIRC, the current behavior allows the process to be placed into a cgroup
> with a subset of CPUs and you just can do 'cyclictest -a -t'. Process
> should not ignore external configuration. That's my whole point here.

In that case again I think a sane solution is not to check the cpu list in
every single tool we use, because even if we do that for all tools in rt-teets
repo, we can't guarantee to have this check for the rest tools to not ignore
this restriction.

A simple example is: what if the user specified "taskset -c $CPU cyclictest -a
$CPU -t 1 ..." where $CPU is not in the allowed list of current bash?  As long
as the taskset would work the so-called "environment" will be changed before
even loading cyclictest.

If you see that's the point I said we should fail at the same check point of
sched_setaffinity() rather than checking it explicitly in the tool, because
if we want a real-world restriction that's the only place I think it's possible..

But I'm not a cgroup/container guy, please correct me if I understood.

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17 12:51       ` Peter Xu
@ 2021-03-17 15:08         ` John Kacur
  2021-03-17 15:21           ` Peter Xu
  2021-03-17 16:40           ` Ahmed S. Darwish
  0 siblings, 2 replies; 11+ messages in thread
From: John Kacur @ 2021-03-17 15:08 UTC (permalink / raw)
  To: Peter Xu; +Cc: Daniel Wagner, linux-rt-users, Clark Williams, Daniel Wagner



On Wed, 17 Mar 2021, Peter Xu wrote:

> Hi, Daniel,
> 
> On Wed, Mar 17, 2021 at 08:49:03AM +0100, Daniel Wagner wrote:
> > On Tue, Mar 16, 2021 at 04:07:05PM -0400, Peter Xu wrote:
> > > I think what I'm missing is why we had such a restriction.  Quotting from the
> > > commit ID:
> > 
> > IIRC, the current behavior allows the process to be placed into a cgroup
> > with a subset of CPUs and you just can do 'cyclictest -a -t'. Process
> > should not ignore external configuration. That's my whole point here.
> 
> In that case again I think a sane solution is not to check the cpu list in
> every single tool we use, because even if we do that for all tools in rt-teets
> repo, we can't guarantee to have this check for the rest tools to not ignore
> this restriction.
> 
> A simple example is: what if the user specified "taskset -c $CPU cyclictest -a
> $CPU -t 1 ..." where $CPU is not in the allowed list of current bash?  As long
> as the taskset would work the so-called "environment" will be changed before
> even loading cyclictest.
> 
> If you see that's the point I said we should fail at the same check point of
> sched_setaffinity() rather than checking it explicitly in the tool, because
> if we want a real-world restriction that's the only place I think it's possible..
> 
> But I'm not a cgroup/container guy, please correct me if I understood.
> 
> -- 
> Peter Xu
> 
> 

When cyclictest and friends were originally written, we had this view 
point that we "owned" the whole machine, and didn't have any restrictions 
on where to schedule. As machines grew in size, and we added numa 
awareness, and cgroups became more prominent we added this code that tried 
to schedule according to the ill-defined environment that we found 
ourselves in.

As Peter points out we may have restricted ourselves more than is 
necessary, and can rely a bit more on the operating system to restrict us. 
On the otherhand using taskset is an easy workaround if the current code 
is to restrictive.

Because we can use taskset and things are working well otherwise I don't 
see this as super urgent, but I am willing to revisit this code and make 
it less restrictive if that makes sense.

I also am not a cgroup / container person, and would like to play around 
with this a bit more before we make some decisions on which direction to 
go in.

Does that make sense to everyone?

John

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17 15:08         ` John Kacur
@ 2021-03-17 15:21           ` Peter Xu
  2021-03-17 15:47             ` John Kacur
  2021-03-17 16:40           ` Ahmed S. Darwish
  1 sibling, 1 reply; 11+ messages in thread
From: Peter Xu @ 2021-03-17 15:21 UTC (permalink / raw)
  To: John Kacur; +Cc: Daniel Wagner, linux-rt-users, Clark Williams, Daniel Wagner

On Wed, Mar 17, 2021 at 11:08:31AM -0400, John Kacur wrote:
> 
> 
> On Wed, 17 Mar 2021, Peter Xu wrote:
> 
> > Hi, Daniel,
> > 
> > On Wed, Mar 17, 2021 at 08:49:03AM +0100, Daniel Wagner wrote:
> > > On Tue, Mar 16, 2021 at 04:07:05PM -0400, Peter Xu wrote:
> > > > I think what I'm missing is why we had such a restriction.  Quotting from the
> > > > commit ID:
> > > 
> > > IIRC, the current behavior allows the process to be placed into a cgroup
> > > with a subset of CPUs and you just can do 'cyclictest -a -t'. Process
> > > should not ignore external configuration. That's my whole point here.
> > 
> > In that case again I think a sane solution is not to check the cpu list in
> > every single tool we use, because even if we do that for all tools in rt-teets
> > repo, we can't guarantee to have this check for the rest tools to not ignore
> > this restriction.
> > 
> > A simple example is: what if the user specified "taskset -c $CPU cyclictest -a
> > $CPU -t 1 ..." where $CPU is not in the allowed list of current bash?  As long
> > as the taskset would work the so-called "environment" will be changed before
> > even loading cyclictest.
> > 
> > If you see that's the point I said we should fail at the same check point of
> > sched_setaffinity() rather than checking it explicitly in the tool, because
> > if we want a real-world restriction that's the only place I think it's possible..
> > 
> > But I'm not a cgroup/container guy, please correct me if I understood.
> > 
> > -- 
> > Peter Xu
> > 
> > 
> 
> When cyclictest and friends were originally written, we had this view 
> point that we "owned" the whole machine, and didn't have any restrictions 
> on where to schedule. As machines grew in size, and we added numa 
> awareness, and cgroups became more prominent we added this code that tried 
> to schedule according to the ill-defined environment that we found 
> ourselves in.
> 
> As Peter points out we may have restricted ourselves more than is 
> necessary, and can rely a bit more on the operating system to restrict us. 
> On the otherhand using taskset is an easy workaround if the current code 
> is to restrictive.
> 
> Because we can use taskset and things are working well otherwise I don't 
> see this as super urgent, but I am willing to revisit this code and make 
> it less restrictive if that makes sense.
> 
> I also am not a cgroup / container person, and would like to play around 
> with this a bit more before we make some decisions on which direction to 
> go in.
> 
> Does that make sense to everyone?

Sure thing on my side.  No bug reported so far this time, so I'll wait at least
until then :) I just don't know why it's not hit just like oslat since I don't
see a difference.  When I fixed the oslat thing, I thought cyclictest didn't
have such issue for some reason so I didn't consider to touch it at all.  But
when yesterday I rerun some tests I see this issue on rhel8, hence this patch.

Thanks,

-- 
Peter Xu


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17 15:21           ` Peter Xu
@ 2021-03-17 15:47             ` John Kacur
  2021-03-17 17:20               ` Daniel Wagner
  0 siblings, 1 reply; 11+ messages in thread
From: John Kacur @ 2021-03-17 15:47 UTC (permalink / raw)
  To: Peter Xu; +Cc: Daniel Wagner, linux-rt-users, Clark Williams, Daniel Wagner



On Wed, 17 Mar 2021, Peter Xu wrote:

> On Wed, Mar 17, 2021 at 11:08:31AM -0400, John Kacur wrote:
> > 
> > 
> > On Wed, 17 Mar 2021, Peter Xu wrote:
> > 
> > > Hi, Daniel,
> > > 
> > > On Wed, Mar 17, 2021 at 08:49:03AM +0100, Daniel Wagner wrote:
> > > > On Tue, Mar 16, 2021 at 04:07:05PM -0400, Peter Xu wrote:
> > > > > I think what I'm missing is why we had such a restriction.  Quotting from the
> > > > > commit ID:
> > > > 
> > > > IIRC, the current behavior allows the process to be placed into a cgroup
> > > > with a subset of CPUs and you just can do 'cyclictest -a -t'. Process
> > > > should not ignore external configuration. That's my whole point here.
> > > 
> > > In that case again I think a sane solution is not to check the cpu list in
> > > every single tool we use, because even if we do that for all tools in rt-teets
> > > repo, we can't guarantee to have this check for the rest tools to not ignore
> > > this restriction.
> > > 
> > > A simple example is: what if the user specified "taskset -c $CPU cyclictest -a
> > > $CPU -t 1 ..." where $CPU is not in the allowed list of current bash?  As long
> > > as the taskset would work the so-called "environment" will be changed before
> > > even loading cyclictest.
> > > 
> > > If you see that's the point I said we should fail at the same check point of
> > > sched_setaffinity() rather than checking it explicitly in the tool, because
> > > if we want a real-world restriction that's the only place I think it's possible..
> > > 
> > > But I'm not a cgroup/container guy, please correct me if I understood.
> > > 
> > > -- 
> > > Peter Xu
> > > 
> > > 
> > 
> > When cyclictest and friends were originally written, we had this view 
> > point that we "owned" the whole machine, and didn't have any restrictions 
> > on where to schedule. As machines grew in size, and we added numa 
> > awareness, and cgroups became more prominent we added this code that tried 
> > to schedule according to the ill-defined environment that we found 
> > ourselves in.
> > 
> > As Peter points out we may have restricted ourselves more than is 
> > necessary, and can rely a bit more on the operating system to restrict us. 
> > On the otherhand using taskset is an easy workaround if the current code 
> > is to restrictive.
> > 
> > Because we can use taskset and things are working well otherwise I don't 
> > see this as super urgent, but I am willing to revisit this code and make 
> > it less restrictive if that makes sense.
> > 
> > I also am not a cgroup / container person, and would like to play around 
> > with this a bit more before we make some decisions on which direction to 
> > go in.
> > 
> > Does that make sense to everyone?
> 
> Sure thing on my side.  No bug reported so far this time, so I'll wait at least
> until then :) I just don't know why it's not hit just like oslat since I don't
> see a difference.  When I fixed the oslat thing, I thought cyclictest didn't
> have such issue for some reason so I didn't consider to touch it at all.  But
> when yesterday I rerun some tests I see this issue on rhel8, hence this patch.
> 
> Thanks,
> 
> -- 
> Peter Xu
> 
> 

Thanks Peter - I appreciate it!

John

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17 15:08         ` John Kacur
  2021-03-17 15:21           ` Peter Xu
@ 2021-03-17 16:40           ` Ahmed S. Darwish
  2021-03-17 17:15             ` Daniel Wagner
  1 sibling, 1 reply; 11+ messages in thread
From: Ahmed S. Darwish @ 2021-03-17 16:40 UTC (permalink / raw)
  To: John Kacur
  Cc: Peter Xu, Daniel Wagner, linux-rt-users, Clark Williams, Daniel Wagner

On Wed, Mar 17, 2021 at 11:08:31AM -0400, John Kacur wrote:
>
> I also am not a cgroup / container person, and would like to play around
> with this a bit more before we make some decisions on which direction to
> go in.
>

Pardon my ignorance, but (scheduling wise) what does an RT task inside a
container/cgroup mean?

AFAIK, the RT task will still command the CPU as it pleases, no matter
the cgroup's cpu.shares configuration.

Thanks,

--
Ahmed S. Darwish

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17 16:40           ` Ahmed S. Darwish
@ 2021-03-17 17:15             ` Daniel Wagner
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Wagner @ 2021-03-17 17:15 UTC (permalink / raw)
  To: Ahmed S. Darwish
  Cc: John Kacur, Peter Xu, Daniel Wagner, linux-rt-users, Clark Williams

On Wed, Mar 17, 2021 at 05:40:56PM +0100, Ahmed S. Darwish wrote:
> Pardon my ignorance, but (scheduling wise) what does an RT task inside a
> container/cgroup mean?
> 
> AFAIK, the RT task will still command the CPU as it pleases, no matter
> the cgroup's cpu.shares configuration.

It's not about scheduling it's about affinity only.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rt-tests: Drop use_current_cpuset() check
  2021-03-17 15:47             ` John Kacur
@ 2021-03-17 17:20               ` Daniel Wagner
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Wagner @ 2021-03-17 17:20 UTC (permalink / raw)
  To: John Kacur; +Cc: Peter Xu, Daniel Wagner, linux-rt-users, Clark Williams

On Wed, Mar 17, 2021 at 11:47:51AM -0400, John Kacur wrote:
> > > Does that make sense to everyone?
> > 
> > Sure thing on my side.  No bug reported so far this time, so I'll wait at least
> > until then :) I just don't know why it's not hit just like oslat since I don't
> > see a difference.  When I fixed the oslat thing, I thought cyclictest didn't
> > have such issue for some reason so I didn't consider to touch it at all.  But
> > when yesterday I rerun some tests I see this issue on rhel8, hence this patch.

Don't let me stop you ripping out the code. I am not agreeing with the
reasoning why it should go but it's a detail I don't care.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 19:37 [PATCH] rt-tests: Drop use_current_cpuset() check Peter Xu
2021-03-16  8:18 ` Daniel Wagner
2021-03-16 20:07   ` Peter Xu
2021-03-17  7:49     ` Daniel Wagner
2021-03-17 12:51       ` Peter Xu
2021-03-17 15:08         ` John Kacur
2021-03-17 15:21           ` Peter Xu
2021-03-17 15:47             ` John Kacur
2021-03-17 17:20               ` Daniel Wagner
2021-03-17 16:40           ` Ahmed S. Darwish
2021-03-17 17:15             ` Daniel Wagner

Linux-rt-users Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rt-users/0 linux-rt-users/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rt-users linux-rt-users/ https://lore.kernel.org/linux-rt-users \
		linux-rt-users@vger.kernel.org
	public-inbox-index linux-rt-users

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rt-users


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git