Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 25 Apr 2010 17:59:29 +0300
From:      Kostik Belousov <kostikbel@gmail.com>
To:        Alexander Krizhanovsky <ak@natsys-lab.com>
Cc:        freebsd-hackers@freebsd.org, ur4ina@gmail.com
Subject:   Re: [PATCH] RUSAGE_THREAD
Message-ID:  <20100425145929.GJ2422@deviant.kiev.zoral.com.ua>
In-Reply-To: <4BCBA7F8.3060109@natsys-lab.com>
References:  <4BCBA7F8.3060109@natsys-lab.com>

next in thread | previous in thread | raw e-mail | index | archive | help

--eDyw1yV8HuEtd7LH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 19, 2010 at 12:46:48AM +0000, Alexander Krizhanovsky wrote:
> Hi all!
>=20
> This patch implements per-thread rusage statistic (like RUSAGE_THREAD in=
=20
> Linux and RUSAGE_LWP in OpenSolaris).
>=20
> Unfortunately, we have to acquire a number of locks to read/update=20
> system and user times for current thread rusage information because it's=
=20
> also used for whole process statistic and needs to be zeroed.
>=20
> Any comments are very appreciated.
>=20
> It's tested against 8.0-RELEASE. Appropriate PR is submitted.
>=20
> --=20
> Alexander Krizhanovsky
> NatSys Lab. (http://natsys-lab.com)
> tel: +7 (916) 717-3899, +7 (499) 747-6304
> e-mail: ak@natsys-lab.com
>=20
I think this should be somewhat modified before commit.

First, please separate patch into two pieces. One would introduce
ruxagg_tlock() helper and use it in existing locations instead of
three-liners. Possibly, add K&R->ANSI C kern_getrusage definition
change.

Second would actually add RUSAGE_THREAD. There, please follow
the style(9), in particular, do not initialize the local variables
at the declaration, instead, use explicit initialization in the code.

Should calctru() share the code with calcru1() ? I am esp. concerned
with sanity check like negative runtime etc. Possibly, this code
from calcru1() should be separated into function used from both
calcru1() and calctru().

Man page for getrusage(2) should be updated.

Thanks for the submission.

> --- sys/sys/resource.h.orig	2009-10-25 01:10:29.000000000 +0000
> +++ sys/sys/resource.h	2010-04-11 23:31:14.000000000 +0000
> @@ -56,6 +56,7 @@
> =20
>  #define	RUSAGE_SELF	0
>  #define	RUSAGE_CHILDREN	-1
> +#define	RUSAGE_THREAD	1
> =20
>  struct rusage {
>  	struct timeval ru_utime;	/* user time used */
> --- sys/kern/kern_resource.c.orig	2009-10-25 01:10:29.000000000 +0000
> +++ sys/kern/kern_resource.c	2010-04-18 23:49:04.000000000 +0000
> @@ -76,6 +76,7 @@ static void	calcru1(struct proc *p, stru
>  		    struct timeval *up, struct timeval *sp);
>  static int	donice(struct thread *td, struct proc *chgp, int n);
>  static struct uidinfo *uilookup(uid_t uid);
> +static void ruxagg_tlock(struct proc *p, struct thread *td);
> =20
>  /*
>   * Resource controls and accounting.
> @@ -623,9 +624,7 @@ lim_cb(void *arg)
>  		return;
>  	PROC_SLOCK(p);
>  	FOREACH_THREAD_IN_PROC(p, td) {
> -		thread_lock(td);
> -		ruxagg(&p->p_rux, td);
> -		thread_unlock(td);
> +		ruxagg_tlock(p, td);
>  	}
>  	PROC_SUNLOCK(p);
>  	if (p->p_rux.rux_runtime > p->p_cpulimit * cpu_tickrate()) {
> @@ -836,9 +835,7 @@ calcru(struct proc *p, struct timeval *u
>  	FOREACH_THREAD_IN_PROC(p, td) {
>  		if (td->td_incruntime =3D=3D 0)
>  			continue;
> -		thread_lock(td);
> -		ruxagg(&p->p_rux, td);
> -		thread_unlock(td);
> +		ruxagg_tlock(p, td);
>  	}
>  	calcru1(p, &p->p_rux, up, sp);
>  }
> @@ -918,6 +915,29 @@ calcru1(struct proc *p, struct rusage_ex
>  	sp->tv_usec =3D su % 1000000;
>  }
> =20
> +static void
> +calctru(struct thread *td)
> +{
> +	u_int64_t tu =3D cputick2usec(td->td_incruntime);
> +	u_int64_t ut =3D td->td_uticks;
> +	u_int64_t it =3D td->td_iticks;
> +	u_int64_t st =3D td->td_sticks;
> +	u_int64_t tt, uu, su;
> +
> +	tt =3D ut + st + it;
> +	if (!tt) {
> +		/* Avoid divide by zero */
> +		st =3D 1;
> +		tt =3D 1;
> +	}
> +	uu =3D td->td_ru.ru_utime.tv_usec + (ut * tu) / tt;
> +	su =3D td->td_ru.ru_stime.tv_usec + (st * tu) / tt;
> +	td->td_ru.ru_utime.tv_sec +=3D uu / 1000000;
> +	td->td_ru.ru_utime.tv_usec =3D uu % 1000000;
> +	td->td_ru.ru_stime.tv_sec +=3D su / 1000000;
> +	td->td_ru.ru_stime.tv_usec =3D su % 1000000;
> +}
> +
>  #ifndef _SYS_SYSPROTO_H_
>  struct getrusage_args {
>  	int	who;
> @@ -939,10 +959,7 @@ getrusage(td, uap)
>  }
> =20
>  int
> -kern_getrusage(td, who, rup)
> -	struct thread *td;
> -	int who;
> -	struct rusage *rup;
> +kern_getrusage(struct thread *td, int who, struct rusage *rup)
>  {
>  	struct proc *p;
>  	int error;
> @@ -961,6 +978,13 @@ kern_getrusage(td, who, rup)
>  		calccru(p, &rup->ru_utime, &rup->ru_stime);
>  		break;
> =20
> +	case RUSAGE_THREAD:
> +		PROC_SLOCK(p);
> +		ruxagg_tlock(p, td);
> +		PROC_SUNLOCK(p);
> +		*rup =3D td->td_ru;
> +		break;
> +
>  	default:
>  		error =3D EINVAL;
>  	}
> @@ -1010,12 +1034,24 @@ ruxagg(struct rusage_ext *rux, struct th
>  	rux->rux_uticks +=3D td->td_uticks;
>  	rux->rux_sticks +=3D td->td_sticks;
>  	rux->rux_iticks +=3D td->td_iticks;
> +
> +	/* update thread rusage before ticks counters cleaning */
> +	calctru(td);
> +
>  	td->td_incruntime =3D 0;
>  	td->td_uticks =3D 0;
>  	td->td_iticks =3D 0;
>  	td->td_sticks =3D 0;
>  }
> =20
> +static void
> +ruxagg_tlock(struct proc *p, struct thread *td)
> +{
> +	thread_lock(td);
> +	ruxagg(&p->p_rux, td);
> +	thread_unlock(td);
> +}
> +
>  /*
>   * Update the rusage_ext structure and fetch a valid aggregate rusage
>   * for proc p if storage for one is supplied.
> @@ -1030,9 +1066,7 @@ rufetch(struct proc *p, struct rusage *r
>  	*ru =3D p->p_ru;
>  	if (p->p_numthreads > 0)  {
>  		FOREACH_THREAD_IN_PROC(p, td) {
> -			thread_lock(td);
> -			ruxagg(&p->p_rux, td);
> -			thread_unlock(td);
> +			ruxagg_tlock(p, td);
>  			rucollect(ru, &td->td_ru);
>  		}
>  	}

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

--eDyw1yV8HuEtd7LH
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (FreeBSD)

iEYEARECAAYFAkvUWNEACgkQC3+MBN1Mb4jeewCfT9Wa1XBzGzGo0OwT9b5WZ5pX
f6UAnjEOf6mZt2EbxncwXCii0DdZVSAO
=zjd2
-----END PGP SIGNATURE-----

--eDyw1yV8HuEtd7LH--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100425145929.GJ2422>