Date: Wed, 30 May 2007 20:59:05 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jeff Roberson <jroberson@chesapeake.net> Cc: freebsd-arch@freebsd.org Subject: Re: Updated rusage patch Message-ID: <20070530201618.T13220@besplex.bde.org> In-Reply-To: <20070529220936.W661@10.0.0.1> References: <20070529105856.L661@10.0.0.1> <200705291456.38515.jhb@freebsd.org> <20070529121653.P661@10.0.0.1> <20070530065423.H93410@delplex.bde.org> <20070529141342.D661@10.0.0.1> <20070530125553.G12128@besplex.bde.org> <20070529201255.X661@10.0.0.1> <20070529220936.W661@10.0.0.1>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 29 May 2007, Jeff Roberson wrote: > I have updated the patch at: > > http://people.freebsd.org/~jeff/rusage3.diff New minor points about an even later update: % Index: kern/kern_exit.c % =================================================================== % RCS file: /usr/home/ncvs/src/sys/kern/kern_exit.c,v % retrieving revision 1.298 % diff -u -p -r1.298 kern_exit.c % --- kern/kern_exit.c 14 May 2007 22:21:58 -0000 1.298 % +++ kern/kern_exit.c 29 May 2007 21:38:21 -0000 % ... % @@ -229,7 +233,7 @@ retry: % */ % EVENTHANDLER_INVOKE(process_exit, p); % % - MALLOC(p->p_ru, struct rusage *, sizeof(struct rusage), % + MALLOC(ru, struct rusage *, sizeof(struct rusage), % M_ZOMBIE, M_WAITOK); % /* % * If parent is waiting for us to exit or exec, Perhaps this should not be micro-optimized for space (allocate it inside struct proc for the whole process lifetime). Alternatively, inherit it from the first thread in the process that exits. % Index: kern/kern_resource.c % =================================================================== % RCS file: /usr/home/ncvs/src/sys/kern/kern_resource.c,v % retrieving revision 1.171 % diff -u -p -r1.171 kern_resource.c % --- kern/kern_resource.c 27 May 2007 20:50:23 -0000 1.171 % +++ kern/kern_resource.c 29 May 2007 22:06:05 -0000 % @@ -619,6 +620,47 @@ setrlimit(td, uap) % return (error); % } % % +static void % +lim_cb(void *arg) % +{ % + struct rlimit rlim; % + struct thread *td; % + struct proc *p; Unsorted decls. % + % + p = (struct proc *)arg; Unnecessary cast. % + PROC_LOCK_ASSERT(p, MA_OWNED); % + /* % + * Check if the process exceeds its cpu resource allocation. If % + * it reaches the max, arrange to kill the process in ast(). % + */ % + mtx_lock_spin(&sched_lock); % + FOREACH_THREAD_IN_PROC(p, td) % + ruxcollect(&p->p_rux, td); % + if (p->p_cpulimit != RLIM_INFINITY && This should just retun if the limit is infinity. p_cpulimit is still marked as locked by sched_lock, but proc locking should be enough now, so sched locking (or whatever it becomes) is not needed for the early test and retrun. The early return rarely happens but it gives simpler code as well as saving time when it happens. % + p->p_rux.rux_runtime > p->p_cpulimit * cpu_tickrate()) { % + lim_rlimit(p, RLIMIT_CPU, &rlim); % + if (p->p_rux.rux_runtime >= rlim.rlim_max * cpu_tickrate()) { Hmm, the tick rate conversion bug gives more than wrong stats. Here it shoots processs. E.g., suppose you have a cpulimit of 1 hour and you throttle the CPU to 8 times slower. Processes that have run for > 7.5 minutes at the old (higher) tick rate are then killed here. % ... % + if (p->p_cpulimit != RLIM_INFINITY) % + callout_reset(&p->p_limco, hz, lim_cb, (void *)p); Unnecessary cast. % Index: kern/kern_synch.c % =================================================================== % RCS file: /usr/home/ncvs/src/sys/kern/kern_synch.c,v % retrieving revision 1.295 % diff -u -p -r1.295 kern_synch.c % --- kern/kern_synch.c 18 May 2007 07:10:45 -0000 1.295 % +++ kern/kern_synch.c 29 May 2007 21:35:40 -0000 % @@ -401,35 +401,17 @@ mi_switch(int flags, struct thread *newt % ... % /* % * Compute the amount of time during which the current % - * process was running, and add that to its total so far. % + * thread was running, and add that to its total so far. % */ % new_switchtime = cpu_ticks(); % - p->p_rux.rux_runtime += (new_switchtime - PCPU_GET(switchtime)); % - p->p_rux.rux_uticks += td->td_uticks; % - td->td_uticks = 0; % - p->p_rux.rux_iticks += td->td_iticks; % - td->td_iticks = 0; % - p->p_rux.rux_sticks += td->td_sticks; % - td->td_sticks = 0; % - % + td->td_runtime += (new_switchtime - PCPU_GET(switchtime)); Clean further by removing unnecesary parens. % td->td_generation++; /* bump preempt-detect counter */ % - % - /* % - * Check if the process exceeds its cpu resource allocation. If % - * it reaches the max, arrange to kill the process in ast(). % - */ % - if (p->p_cpulimit != RLIM_INFINITY && % - p->p_rux.rux_runtime >= p->p_cpulimit * cpu_tickrate()) { % - p->p_sflag |= PS_XCPU; % - td->td_flags |= TDF_ASTPENDING; % - } % - % /* % * Finish up stats for outgoing thread. % */ Clean further by merging the "start" and "finish" code (so that switchtime is cleared immediately after it is copied, etc). There is nothing in between any more. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070530201618.T13220>