linux-kernel - Re: [PATCH for -mm] getrusage: fill ru

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090103175913.GA21180@redhat.com>
Date:	Sat, 3 Jan 2009 18:59:13 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	Jiri Pirko <jpirko@...hat.com>, linux-kernel@...r.kernel.org,
	Hugh Dickins <hugh@...itas.com>, linux-mm <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH for -mm] getrusage: fill ru_maxrss value

sorry for delay!

On 12/31, KOSAKI Motohiro wrote:
>
> Jiri's resend3 -> v1
>  - At wait_task_zombie(), parent process doesn't only collect child maxrss,
>    but also cmaxrss.

Ah yes, this looks very right to me.

>  - ru_maxrss inherit at exec()

I must admit, I hate this ;)

That said, I agree with you point about compatibility. So I have to
agree with this change.

Still, I'd like to know what other people think ;)

And I also agree that xacct is linux specific feature, but I still
I dislike the fact that xacct and getrusage report different numbers.
Perhaps we should change xacct as well?

> --- a/kernel/exit.c	2008-12-29 23:27:59.000000000 +0900
> +++ b/kernel/exit.c	2008-12-31 21:08:08.000000000 +0900
> @@ -1053,6 +1053,12 @@ NORET_TYPE void do_exit(long code)
>  	if (group_dead) {
>  		hrtimer_cancel(&tsk->signal->real_timer);
>  		exit_itimers(tsk->signal);
> +		if (tsk->mm) {
> +			unsigned long hiwater_rss = get_mm_hiwater_rss(tsk->mm);
> +
> +			if (tsk->signal->maxrss < hiwater_rss)
> +				tsk->signal->maxrss = hiwater_rss;
> +		}
[...snip...]
> --- a/fs/exec.c	2008-12-25 08:26:37.000000000 +0900
> +++ b/fs/exec.c	2008-12-31 21:11:28.000000000 +0900
> @@ -870,6 +870,13 @@ static int de_thread(struct task_struct 
>  	sig->notify_count = 0;
>
>  no_thread_group:
> +	if (current->mm) {
> +		unsigned long hiwater_rss = get_mm_hiwater_rss(current->mm);
> +
> +		if (sig->maxrss < hiwater_rss)
> +			sig->maxrss = hiwater_rss;
> +	}

Perhaps it makes sense to factor out this code and make a helper?

Unfortunately, exit_mm() and exec_mmap() do not have the common
path which can update sig->maxrss, mm_release() can't do this...

> +	if (who != RUSAGE_CHILDREN) {
> +		struct mm_struct *mm = get_task_mm(p);
> +		if (mm) {
> +			unsigned long hiwater_rss = get_mm_hiwater_rss(mm);
> +
> +			if (maxrss < hiwater_rss)
> +				maxrss = hiwater_rss;
> +			mmput(mm);
> +		}
> +	}
> +	r->ru_maxrss = maxrss * (PAGE_SIZE / 1024); /* convert pages to KBs */

Hmm... So, RUSAGE_THREAD always report maxrss == get_mm_hiwater_rss(mm)
and ignores signal->maxrss. Doesn't look right to me...

Unless I missed something, Jiris's patch was fine, but given that now
we inherit maxrss at exec(), signal->maxrss can have the "inherited"
value?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/