lkml.org 
[lkml]   [2006]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] simplify/improve rcu batch tuning
On Fri, Sep 08, 2006 at 03:39:30PM +0400, Oleg Nesterov wrote:
> On 09/07, Paul E. McKenney wrote:
> >
> > Some thoughts for testing...
> >
> > 1. Modify rcutorture.c to keep all the rcutorture kernel threads
> > off of at least one CPU. Run a CPU-bound user process on that
> > CPU. Compare the rate a which grace periods progress in
> > the following three situations:
> >
> > a. With your patch.
> >
> > b. With stock kernel.
> >
> > c. With the function disabled (e.g., use the
> > not-CONFIG_SMP version of force_quiescent_state()).
> >
> > You would expect to see fast grace-period progress for (a) and
> > (b), slow for (c).
> >
> > 2. As above, but have another process generating lots of
> > RCU callbacks, for example, by opening and closing lots
> > of files, creating and deleting lots of files with long
> > randomly selected names, thrashing the route cache, or
> > whatever.
>
> Thanks for review and suggestions. I'll try to run these tests next week.
> Afaics, it is enough to just do
>
> for (;;) close(open(...))
>
> for '2.'.

That code sequence certainly has been known to generate very large
numbers of RCU callbacks very quickly in the past... ;-)

> > > @@ -86,8 +83,8 @@ static void force_quiescent_state(struct
> > > int cpu;
> > > cpumask_t cpumask;
> > > set_need_resched();
> >
> > Not that it makes a big difference, but why is the above
> > set_need_resched() not in the body of the following "if" statement?
> > It used to be important, because it could prevent additional IPIs in
> > the same grace period, but since the current code will only send one
> > IPI per grace period, it seems like it can safely be tucked under the
> > "if" statement.
>
> I think there was another reason to do set_need_resched() unconditionally,
> but this is only my guess. We are sending IPIs to speedup the flashing of
> callbacks we already have in the queue. But set_need_resched() tries to
> suppress current process from adding new callbacks (not that it is perfect,
> though). Consider the 'for (;;) close(open(...))' loop.

Good point -- should get things going upon return from the next syscall.

> Actually I think it also makes sense to do tasklet_schedule(rcu_tasklet)
> in call_rcu(), this way we can detect that we need to start the next batch
> earlier.

As long as we don't do this too often... One way to prevent doing this
too often would be to check rcp->completed against rdp->batch similarly
to __rcu_process_callbacks()'s checks. In call_rcu(), perhaps something
like the following inside the ->qlen check:

if (__rcu_pending(&rcu_ctrlblk, rdp) {
tasklet_schedule(&per_cpu(rcu_tasklet, rdp->cpu));
}

with similar code in call_rcu_bh().

> > > - if (unlikely(rdp->qlen - rdp->last_rs_qlen > rsinterval)) {
> > > - rdp->last_rs_qlen = rdp->qlen;
> > > + if (unlikely(!rcp->signaled)) {
> > > + rcp->signaled = 1;
> > > /*
> > > * Don't send IPI to itself. With irqs disabled,
> > > * rdp->cpu is the current cpu.
> > > @@ -297,6 +294,7 @@ static void rcu_start_batch(struct rcu_c
> > > smp_mb();
> > > cpus_andnot(rcp->cpumask, cpu_online_map, nohz_cpu_mask);
> > >
> > > + rcp->signaled = 0;
> >
> > Would it make sense to invoke force_quiescent_state() here in the
> > case that rdp->qlen is still large? The disadvantage is that qlen
> > still counts the number of callbacks that are already slated for
> > invocation.
>
> This is not easy to do. rcu_start_batch() is "global", we need
> to scan all per-cpu 'struct rcu_data' and check it's ->qlen.

My thought was that it might make sense to check only this CPU's struct
rcu_data. But I agree that the next approach seems more promising.

> > Another approach would be to check rdp->qlen and
> > rcp->signaled in rcu_do_batch(), but only once rdp->donlist goes
> > NULL.
>
> Agree. Probably we don't need to check !rdp->donlist, it should be
> empty after rcu_do_batch() invocation when ->qlen > qhimark, because
> in that case ->blimit == INT_MAX.

And rcu_do_batch() already checks rdp->donelist anyway, so this code
could just be put into the "else" clause of the existing rdp->donelist
check at the end of rcu_do_batch(). So, yes, this does appear to be
the best approach.

> But first I'd like to do a couple of other cleanups here, I'll send
> the patches on weekend.

Look forward to seeing them!

Thanx, Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2006-09-08 18:05    [W:0.058 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site