Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1635852 > unrolled thread

Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt balancing logic

Started byPeter Zijlstra <peterz@infradead.org>
First post2017-05-04 17:40 +0200
Last post2017-05-06 10:00 +0200
Articles 11 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Peter Zijlstra <peterz@infradead.org> - 2017-05-04 17:40 +0200
    Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Steven Rostedt <rostedt@goodmis.org> - 2017-05-04 19:30 +0200
      Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Peter Zijlstra <peterz@infradead.org> - 2017-05-04 20:50 +0200
        Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Steven Rostedt <rostedt@goodmis.org> - 2017-05-04 21:10 +0200
          Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Mike Galbraith <efault@gmx.de> - 2017-05-05 06:30 +0200
            Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Mike Galbraith <efault@gmx.de> - 2017-05-05 07:20 +0200
          Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Peter Zijlstra <peterz@infradead.org> - 2017-05-05 13:10 +0200
            Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Steven Rostedt <rostedt@goodmis.org> - 2017-05-05 14:10 +0200
              Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Peter Zijlstra <peterz@infradead.org> - 2017-05-05 19:50 +0200
                Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Steven Rostedt <rostedt@goodmis.org> - 2017-05-05 21:00 +0200
                  Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt  balancing logic Peter Zijlstra <peterz@infradead.org> - 2017-05-06 10:00 +0200

#1635852 — Re: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt balancing logic

FromPeter Zijlstra <peterz@infradead.org>
Date2017-05-04 17:40 +0200
SubjectRe: [PATCH tip/sched/core v2] sched/rt: Simplify the IPI rt balancing logic
Message-ID<tDr1f-5lB-13@gated-at.bofh.it>
On Mon, Apr 24, 2017 at 11:47:32AM -0400, Steven Rostedt wrote:
>  static int rto_next_cpu(struct rq *rq)
>  {
>  	int cpu;
>  
>  	/*
> +	 * When starting the IPI RT pushing, the rto_cpu is set to nr_cpu_ids
> +	 * or greater. rt_next_cpu() will simply return the first CPU found in
> +	 * the rto_mask.
> +	 *
> +	 * If rto_next_cpu() is called with rto_cpu less than nr_cpu_ids, it
> +	 * will return the next CPU found in the rto_mask.
> +	 *
> +	 * If there are no more CPUs left in the rto_mask, then a check is made
> +	 * against rto_loop and rto_loop_next. rto_loop is only updated with
> +	 * the rto_lock held, but any CPU may increment the rto_loop_next
> +	 * without any locking.
>  	 */
> +again:
> +	if (rq->rd->rto_cpu >= nr_cpu_ids) {
>  		cpu = cpumask_first(rq->rd->rto_mask);
> +		rq->rd->rto_cpu = cpu;
> +		/* If cpu is nr_cpu_ids, then there is no overloaded rqs */
> +		return cpu;
>  	}
>  
> +	cpu = cpumask_next(rq->rd->rto_cpu, rq->rd->rto_mask);
> +	rq->rd->rto_cpu = cpu;
>  
> +	if (cpu < nr_cpu_ids)
> +		return cpu;
>  
> +	if (rq->rd->rto_loop == atomic_read(&rq->rd->rto_loop_next))
> +		return cpu;
>  
> +	rq->rd->rto_loop = atomic_read(&rq->rd->rto_loop_next);
> +	goto again;
> +}

I think you want to write that as:

	struct root_domain *rd = rq->rd;
	int cpu, next;

	/* comment */
	for (;;) { 
		if (rd->rto_cpu >= nr_cpu_ids) {
			cpu = cpumask_first(rd->rto_mask);
			rd->rto_cpu = cpu;
			return cpu;
		}

		cpu = cpumask_next(rd->rto_mask);
		rd->rto_cpu = cpu;

		if (cpu < nr_cpu_ids)
			break;

//		rd->rto_cpu = -1;

		/*
		 * ACQUIRE ensures we see the @rto_mask changes
		 * made prior to the @next value observed.
		 * 
		 * Matches WMB in rt_set_overload().
		 */
		next = atomic_read_acquire(&rd->rto_loop_next);

		if (rd->rto_loop == next)
			break;

		rd->rto_loop = next;
	}

	return cpu;

And I don't fully understand the whole rto_cpu >= nr_cpus_ids thing,
can't you simply reset the thing to -1 and always use cpumask_next()?
As per the // comment above?

> +static inline bool rto_start_trylock(atomic_t *v)
> +{
> +	return !atomic_cmpxchg(v, 0, 1);

Arguably this could be: !atomic_cmpxchg_acquire(v, 0, 1);

>  }
>  
> +static inline void rto_start_unlock(atomic_t *v)
> +{
> +	atomic_set_release(v, 0);
> +}
>  

>  static void tell_cpu_to_push(struct rq *rq)
>  {
> +	int cpu = nr_cpu_ids;
>  
> +	/* Keep the loop going if the IPI is currently active */
> +	atomic_inc_return(&rq->rd->rto_loop_next);

Since rt_set_overload() already provides a WMB, we don't need an
ordered primitive here and atomic_inc() is fine.

>  
> +	/* Only one CPU can initiate a loop at a time */
> +	if (!rto_start_trylock(&rq->rd->rto_loop_start))
>  		return;
>  
> +	raw_spin_lock(&rq->rd->rto_lock);
> +
> +	/*
> +	 * The rto_cpu is updated under the lock, if it has a valid cpu
> +	 * then the IPI is still running and will continue due to the
> +	 * update to loop_next, and nothing needs to be done here.
> +	 * Otherwise it is finishing up and an ipi needs to be sent.
> +	 */
> +	if (rq->rd->rto_cpu >= nr_cpu_ids)
//	if (rq->rd->rto_cpu < 0)

> +		cpu = rto_next_cpu(rq);
>  
> +	raw_spin_unlock(&rq->rd->rto_lock);
> +
> +	rto_start_unlock(&rq->rd->rto_loop_start);
> +
> +	if (cpu < nr_cpu_ids)
> +		irq_work_queue_on(&rq->rd->rto_push_work, cpu);
>  }
>  
>  /* Called from hardirq context */
> +void rto_push_irq_work_func(struct irq_work *work)
>  {
> +	struct rq *rq;
>  	int this_cpu;
>  	int cpu;
>  
> +	this_cpu = smp_processor_id();
>  	rq = cpu_rq(this_cpu);

	rq = this_rq();

>  
> +	/*
> +	 * We do not need to grab the lock to check for has_pushable_tasks.
> +	 * When it gets updated, a check is made if a push is possible.
> +	 */
>  	if (has_pushable_tasks(rq)) {
>  		raw_spin_lock(&rq->lock);
> +		push_rt_tasks(rq);
>  		raw_spin_unlock(&rq->lock);
>  	}
>  
> +	raw_spin_lock(&rq->rd->rto_lock);
>  
> +	/* Pass the IPI to the next rt overloaded queue */
> +	cpu = rto_next_cpu(rq);
>  
> +	raw_spin_unlock(&rq->rd->rto_lock);
>  
>  	if (cpu >= nr_cpu_ids)
>  		return;
>  
>  	/* Try the next RT overloaded CPU */
> +	irq_work_queue_on(&rq->rd->rto_push_work, cpu);
>  }

[toc] | [next] | [standalone]


#1635909

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-04 19:30 +0200
Message-ID<tDsJH-6Ay-1@gated-at.bofh.it>
In reply to#1635852
On Thu, 4 May 2017 17:32:56 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Apr 24, 2017 at 11:47:32AM -0400, Steven Rostedt wrote:
> >  static int rto_next_cpu(struct rq *rq)
> >  {
> >  	int cpu;
> >  
> >  	/*
> > +	 * When starting the IPI RT pushing, the rto_cpu is set to nr_cpu_ids
> > +	 * or greater. rt_next_cpu() will simply return the first CPU found in
> > +	 * the rto_mask.
> > +	 *
> > +	 * If rto_next_cpu() is called with rto_cpu less than nr_cpu_ids, it
> > +	 * will return the next CPU found in the rto_mask.
> > +	 *
> > +	 * If there are no more CPUs left in the rto_mask, then a check is made
> > +	 * against rto_loop and rto_loop_next. rto_loop is only updated with
> > +	 * the rto_lock held, but any CPU may increment the rto_loop_next
> > +	 * without any locking.
> >  	 */
> > +again:
> > +	if (rq->rd->rto_cpu >= nr_cpu_ids) {
> >  		cpu = cpumask_first(rq->rd->rto_mask);
> > +		rq->rd->rto_cpu = cpu;
> > +		/* If cpu is nr_cpu_ids, then there is no overloaded rqs */
> > +		return cpu;
> >  	}
> >  
> > +	cpu = cpumask_next(rq->rd->rto_cpu, rq->rd->rto_mask);
> > +	rq->rd->rto_cpu = cpu;
> >  
> > +	if (cpu < nr_cpu_ids)
> > +		return cpu;
> >  
> > +	if (rq->rd->rto_loop == atomic_read(&rq->rd->rto_loop_next))
> > +		return cpu;
> >  
> > +	rq->rd->rto_loop = atomic_read(&rq->rd->rto_loop_next);
> > +	goto again;
> > +}  
> 
> I think you want to write that as:
> 
> 	struct root_domain *rd = rq->rd;
> 	int cpu, next;
> 
> 	/* comment */
> 	for (;;) { 
> 		if (rd->rto_cpu >= nr_cpu_ids) {

If we go with your change, then this needs to be:

		if (rd->rto_cpu < 0) {

> 			cpu = cpumask_first(rd->rto_mask);
> 			rd->rto_cpu = cpu;
> 			return cpu;
> 		}
> 
> 		cpu = cpumask_next(rd->rto_mask);

cpumask_next() requires two parameters.

> 		rd->rto_cpu = cpu;
> 
> 		if (cpu < nr_cpu_ids)
> 			break;
> 
> //		rd->rto_cpu = -1;
> 
> 		/*
> 		 * ACQUIRE ensures we see the @rto_mask changes
> 		 * made prior to the @next value observed.
> 		 * 
> 		 * Matches WMB in rt_set_overload().
> 		 */
> 		next = atomic_read_acquire(&rd->rto_loop_next);
> 
> 		if (rd->rto_loop == next)
> 			break;
> 
> 		rd->rto_loop = next;
> 	}
> 
> 	return cpu;
> 
> And I don't fully understand the whole rto_cpu >= nr_cpus_ids thing,
> can't you simply reset the thing to -1 and always use cpumask_next()?
> As per the // comment above?
> 
> > +static inline bool rto_start_trylock(atomic_t *v)
> > +{
> > +	return !atomic_cmpxchg(v, 0, 1);  
> 
> Arguably this could be: !atomic_cmpxchg_acquire(v, 0, 1);

Yes agreed. But if you remember, at the time I was basing this off of
tip/sched/core, which didn't have atomic_cmpxchg_acquire() available.

> 
> >  }
> >  
> > +static inline void rto_start_unlock(atomic_t *v)
> > +{
> > +	atomic_set_release(v, 0);
> > +}
> >    
> 
> >  static void tell_cpu_to_push(struct rq *rq)
> >  {
> > +	int cpu = nr_cpu_ids;
> >  
> > +	/* Keep the loop going if the IPI is currently active */
> > +	atomic_inc_return(&rq->rd->rto_loop_next);  
> 
> Since rt_set_overload() already provides a WMB, we don't need an
> ordered primitive here and atomic_inc() is fine.

Agree, I mentioned this in my previous reply. It was leftover from
previous versions of the patch. I believe I also needed a memory
barrier with this and the check for rto_loop_start. Can't remember if
that was the case, but it doesn't matter now as loop_start is now
updated with a cmpxchg.

> 
> >  
> > +	/* Only one CPU can initiate a loop at a time */
> > +	if (!rto_start_trylock(&rq->rd->rto_loop_start))
> >  		return;
> >  
> > +	raw_spin_lock(&rq->rd->rto_lock);
> > +
> > +	/*
> > +	 * The rto_cpu is updated under the lock, if it has a valid cpu
> > +	 * then the IPI is still running and will continue due to the
> > +	 * update to loop_next, and nothing needs to be done here.
> > +	 * Otherwise it is finishing up and an ipi needs to be sent.
> > +	 */
> > +	if (rq->rd->rto_cpu >= nr_cpu_ids)  
> //	if (rq->rd->rto_cpu < 0)

This can be done, I was just being a bit more conservative and having
rto_cpu have less states (valid CPU or nr_cpu_ids). With a -1, we have
to manually set it to that. But I'm fine with doing it that way too.

This went through several iterations. There were times where using a -1
wasn't so simple.

> 
> > +		cpu = rto_next_cpu(rq);
> >  
> > +	raw_spin_unlock(&rq->rd->rto_lock);
> > +
> > +	rto_start_unlock(&rq->rd->rto_loop_start);
> > +
> > +	if (cpu < nr_cpu_ids)
> > +		irq_work_queue_on(&rq->rd->rto_push_work, cpu);
> >  }
> >  
> >  /* Called from hardirq context */
> > +void rto_push_irq_work_func(struct irq_work *work)
> >  {
> > +	struct rq *rq;
> >  	int this_cpu;
> >  	int cpu;
> >  
> > +	this_cpu = smp_processor_id();
> >  	rq = cpu_rq(this_cpu);  
> 
> 	rq = this_rq();

Heh, sure. I guess I was just keeping it with the previous logic.

Thanks for the review. I'll spin up a new patch. Unfortunately, I no
longer have access to the behemoth machine. I'll only be testing this
on 4 cores now, or 8 with HT.

-- Steve


> 
> >  
> > +	/*
> > +	 * We do not need to grab the lock to check for has_pushable_tasks.
> > +	 * When it gets updated, a check is made if a push is possible.
> > +	 */
> >  	if (has_pushable_tasks(rq)) {
> >  		raw_spin_lock(&rq->lock);
> > +		push_rt_tasks(rq);
> >  		raw_spin_unlock(&rq->lock);
> >  	}
> >  
> > +	raw_spin_lock(&rq->rd->rto_lock);
> >  
> > +	/* Pass the IPI to the next rt overloaded queue */
> > +	cpu = rto_next_cpu(rq);
> >  
> > +	raw_spin_unlock(&rq->rd->rto_lock);
> >  
> >  	if (cpu >= nr_cpu_ids)
> >  		return;
> >  
> >  	/* Try the next RT overloaded CPU */
> > +	irq_work_queue_on(&rq->rd->rto_push_work, cpu);
> >  }  

[toc] | [prev] | [next] | [standalone]


#1635965

FromPeter Zijlstra <peterz@infradead.org>
Date2017-05-04 20:50 +0200
Message-ID<tDtZ8-7l4-9@gated-at.bofh.it>
In reply to#1635909
On Thu, May 04, 2017 at 01:25:38PM -0400, Steven Rostedt wrote:
> > I think you want to write that as:
> > 
> > 	struct root_domain *rd = rq->rd;
> > 	int cpu, next;
> > 
> > 	/* comment */
> > 	for (;;) { 
> > 		if (rd->rto_cpu >= nr_cpu_ids) {
> 
> If we go with your change, then this needs to be:
> 
> 		if (rd->rto_cpu < 0) {
> 
> > 			cpu = cpumask_first(rd->rto_mask);
> > 			rd->rto_cpu = cpu;
> > 			return cpu;
> > 		}

No you can leave it out entirely.

> > 
> > 		cpu = cpumask_next(rd->rto_mask);
> 
> cpumask_next() requires two parameters.

Indeed it does:

		cpu = cpumask_next(rd->rto_cpu, rd->rto_mask);

will be cpumask_first() when rto_cpu == -1, see for example
for_each_cpu().

> > > +static inline bool rto_start_trylock(atomic_t *v)
> > > +{
> > > +	return !atomic_cmpxchg(v, 0, 1);  
> > 
> > Arguably this could be: !atomic_cmpxchg_acquire(v, 0, 1);
> 
> Yes agreed. But if you remember, at the time I was basing this off of
> tip/sched/core, which didn't have atomic_cmpxchg_acquire() available.

No that's the try_cmpxchg stuff, the _acquire stuff is long in.

> Thanks for the review. I'll spin up a new patch. Unfortunately, I no
> longer have access to the behemoth machine. I'll only be testing this
> on 4 cores now, or 8 with HT.

I have something with 144 CPUs in or thereabout, if you have the
testcase handy I can give it a spin.

[toc] | [prev] | [next] | [standalone]


#1635978

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-04 21:10 +0200
Message-ID<tDuiu-7H9-19@gated-at.bofh.it>
In reply to#1635965
On Thu, 4 May 2017 20:42:00 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, May 04, 2017 at 01:25:38PM -0400, Steven Rostedt wrote:
> > > I think you want to write that as:
> > > 
> > > 	struct root_domain *rd = rq->rd;
> > > 	int cpu, next;
> > > 
> > > 	/* comment */
> > > 	for (;;) { 
> > > 		if (rd->rto_cpu >= nr_cpu_ids) {  
> > 
> > If we go with your change, then this needs to be:
> > 
> > 		if (rd->rto_cpu < 0) {
> >   
> > > 			cpu = cpumask_first(rd->rto_mask);
> > > 			rd->rto_cpu = cpu;
> > > 			return cpu;
> > > 		}  
> 
> No you can leave it out entirely.
> 
> > > 
> > > 		cpu = cpumask_next(rd->rto_mask);  
> > 
> > cpumask_next() requires two parameters.  
> 
> Indeed it does:
> 
> 		cpu = cpumask_next(rd->rto_cpu, rd->rto_mask);
> 
> will be cpumask_first() when rto_cpu == -1, see for example
> for_each_cpu().

OK, I'll have to take a look.

> 
> > > > +static inline bool rto_start_trylock(atomic_t *v)
> > > > +{
> > > > +	return !atomic_cmpxchg(v, 0, 1);    
> > > 
> > > Arguably this could be: !atomic_cmpxchg_acquire(v, 0, 1);  
> > 
> > Yes agreed. But if you remember, at the time I was basing this off of
> > tip/sched/core, which didn't have atomic_cmpxchg_acquire() available.  
> 
> No that's the try_cmpxchg stuff, the _acquire stuff is long in.

OK, I was confused with the try stuff, as I remember that was what you
suggested before.

> 
> > Thanks for the review. I'll spin up a new patch. Unfortunately, I no
> > longer have access to the behemoth machine. I'll only be testing this
> > on 4 cores now, or 8 with HT.  
> 
> I have something with 144 CPUs in or thereabout, if you have the
> testcase handy I can give it a spin.

My test case is two fold. It basically just involves running rteval.

One is to run it on latest mainline to make sure it doesn't crash. The
other is to backport it to the latest -rt patch, and see how well it
helps with latency.

To get rteval:

 $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rteval.git
 $ cd rteval
 $ git checkout origin/v2/master

-- Steve

[toc] | [prev] | [next] | [standalone]


#1636154

FromMike Galbraith <efault@gmx.de>
Date2017-05-05 06:30 +0200
Message-ID<tDD2p-5aE-1@gated-at.bofh.it>
In reply to#1635978
> To get rteval:
> 
>  $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rteval.git
>  $ cd rteval
>  $ git checkout origin/v2/master

ImportError: No module named ethtool

Where does one find the ethtool bits it seems to depend upon?

	-Mike

[toc] | [prev] | [next] | [standalone]


#1636168

FromMike Galbraith <efault@gmx.de>
Date2017-05-05 07:20 +0200
Message-ID<tDDON-5Im-7@gated-at.bofh.it>
In reply to#1636154
On Fri, 2017-05-05 at 06:26 +0200, Mike Galbraith wrote:
> > To get rteval:
> > 
> >  $ git clone
> > git://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rteval.git
> >  $ cd rteval
> >  $ git checkout origin/v2/master
> 
> ImportError: No module named ethtool
> 
> Where does one find the ethtool bits it seems to depend upon?

Nevermind, google found a tarball.

	-Mike

[toc] | [prev] | [next] | [standalone]


#1636314

FromPeter Zijlstra <peterz@infradead.org>
Date2017-05-05 13:10 +0200
Message-ID<tDJhw-Vh-17@gated-at.bofh.it>
In reply to#1635978
On Thu, May 04, 2017 at 03:03:55PM -0400, Steven Rostedt wrote:
> My test case is two fold. It basically just involves running rteval.
> 
> One is to run it on latest mainline to make sure it doesn't crash. The
> other is to backport it to the latest -rt patch, and see how well it
> helps with latency.
> 
> To get rteval:
> 
>  $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rteval.git
>  $ cd rteval
>  $ git checkout origin/v2/master

Blergh, that thing wants a gazillion things installed.

Can't I run something simple like rt-migrate-test with some arguments to
stress the box out?

[toc] | [prev] | [next] | [standalone]


#1636336

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-05 14:10 +0200
Message-ID<tDKdz-1yR-5@gated-at.bofh.it>
In reply to#1636314
On Fri, 5 May 2017 13:05:29 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, May 04, 2017 at 03:03:55PM -0400, Steven Rostedt wrote:
> > My test case is two fold. It basically just involves running rteval.
> > 
> > One is to run it on latest mainline to make sure it doesn't crash. The
> > other is to backport it to the latest -rt patch, and see how well it
> > helps with latency.
> > 
> > To get rteval:
> > 
> >  $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rteval.git
> >  $ cd rteval
> >  $ git checkout origin/v2/master  
> 
> Blergh, that thing wants a gazillion things installed.

Blame Clark and friends ;-)

> 
> Can't I run something simple like rt-migrate-test with some arguments to
> stress the box out?

Actually what rteval does is basically 3 things. It runs cyclictest,
hackbench in a loop and a kernel build in a loop.

Note, rteval binds the the hackbench and kernel builds to nodes. That
is, if you have 4 nodes, it will run four instances of loops of both
hackbench and kernel builds in each of the nodes. This is because
hackbench and access to the filesystem across nodes with a stress test
can cause exorbitant latency due to cross node memory access on spin
locks.

I usually run cyclictest with:

 cyclictest -p80 -i250 -n -a -t -q -d 0

Although I think rteval does it slightly different. Like adding --numa
to it.

Clark, want to explain more?

-- Steve

[toc] | [prev] | [next] | [standalone]


#1636514

FromPeter Zijlstra <peterz@infradead.org>
Date2017-05-05 19:50 +0200
Message-ID<tDPwC-4T3-11@gated-at.bofh.it>
In reply to#1636336
On Fri, May 05, 2017 at 08:02:38AM -0400, Steven Rostedt wrote:
> Actually what rteval does is basically 3 things. It runs cyclictest,
> hackbench in a loop and a kernel build in a loop.

Of those, only cyclictest uses RT tasks and would end up poking at the
bits you just changed.

So just running cyclictest should lock up a ~120 CPU machine?

[toc] | [prev] | [next] | [standalone]


#1636645

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-05 21:00 +0200
Message-ID<tDQCm-5yw-25@gated-at.bofh.it>
In reply to#1636514
On Fri, 5 May 2017 19:39:49 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, May 05, 2017 at 08:02:38AM -0400, Steven Rostedt wrote:
> > Actually what rteval does is basically 3 things. It runs cyclictest,
> > hackbench in a loop and a kernel build in a loop.  
> 
> Of those, only cyclictest uses RT tasks and would end up poking at the
> bits you just changed.
> 
> So just running cyclictest should lock up a ~120 CPU machine?

The other tools tend to trigger RT kernel threads as well, which causes
migration. cyclictest tasks don't migrate, but they do cause other
tasks to want to move around.

You can try rt-migrate-test too, because I used that to trigger some
bugs in previous versions.

-- Steve

[toc] | [prev] | [next] | [standalone]


#1636839

FromPeter Zijlstra <peterz@infradead.org>
Date2017-05-06 10:00 +0200
Message-ID<tE2Nb-5f8-5@gated-at.bofh.it>
In reply to#1636645
On Fri, May 05, 2017 at 02:59:16PM -0400, Steven Rostedt wrote:
> On Fri, 5 May 2017 19:39:49 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, May 05, 2017 at 08:02:38AM -0400, Steven Rostedt wrote:
> > > Actually what rteval does is basically 3 things. It runs cyclictest,
> > > hackbench in a loop and a kernel build in a loop.  
> > 
> > Of those, only cyclictest uses RT tasks and would end up poking at the
> > bits you just changed.
> > 
> > So just running cyclictest should lock up a ~120 CPU machine?
> 
> The other tools tend to trigger RT kernel threads as well, which causes
> migration. cyclictest tasks don't migrate, but they do cause other
> tasks to want to move around.

There aren't that many RT threads on a !RT kernel. All my laptop has for
example are:

$ ps -faxo pid,class,comm | grep -v TS
  PID CLS COMMAND
    9 FF   \_ migration/0
   10 FF   \_ watchdog/0
   11 FF   \_ watchdog/1
   12 FF   \_ migration/1
   16 FF   \_ watchdog/2
   17 FF   \_ migration/2
   21 FF   \_ watchdog/3
   22 FF   \_ migration/3
  714 FF   \_ irq/48-iwlwifi
24254 FF   \_ irq/47-mei_me
 2444 B            \_ baloo_file

> You can try rt-migrate-test too, because I used that to trigger some
> bugs in previous versions.

Just running rt-migrate-test 'works' on the 144 CPU box. But I ran it
while it was otherwise idle. That is, it ran in reasonable time and no
lockup messages were produced.

I can try and run it together with cyclictest, but over all this sounds
like we're missing a usable test-case. If I have a spare moment (lol) I
might poke at rt-migrate-test to see if I can make it more aggressive or
something.

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web