Re: Updated rusage patch

Ok, hopefully we're in the home stretch here. Patch is at the same location. I did the following:

1) Renamed p_ru back. I erroneously thought some code was accessing pstats indirectly through here. That's why I changed the name.
2) Cleaned up some more comments, casts, style bugs, etc.
3) Fixed a problem where we migh've called callout_init_mtx() multiple times.
4) Renamed ruxcollect since it doesn't do the same thing as rucollect().
5) Resorted lim_cb() which now holds the sched_lock over much less code but is certainly safe as is.

Hopefully you will find this to your liking. I intend to fix some more of the races in a follow-up commit when I change the scheduler locking.

Sorry for the top post.

Jeff

On Wed, 30 May 2007, Bruce Evans wrote:

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

_______________________________________________
freebsd-arch@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "freebsd-arch-unsubscribe@xxxxxxxxxxx"

Relevant Pages

  • Re: Updated rusage patch
    ... % retrieving revision 1.298 ... Unnecessary cast. ... happens but it gives simpler code as well as saving time when it ... the tick rate conversion bug gives more than wrong stats. ...
    (freebsd-arch)
  • Re: Updated rusage patch
    ... % retrieving revision 1.89 ... acct_processis called from exit1() eve before the premature ... The locking annotation doesn't work well for pointers. ... the same races as in RELENG_4: ...
    (freebsd-arch)
  • Re: FreeBSD 5.5 persistent crashing
    ... I do not know right locking ... retrieving revision 1.11.2.1 ... Thanks Kostik, ...
    (freebsd-hackers)