linux-kernel - Re: [PATCH 2.6.21] Return available first timeslice to the creator, not parent

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: <20070830094931.GB316@tv-sign.ru>
Date:	Thu, 30 Aug 2007 13:49:31 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Vitaly Mayatskikh <vmayatsk@...hat.com>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Pavel Emelianov <xemul@...nvz.org>
Subject: Re: [PATCH 2.6.21] Return available first timeslice to the creator, not parent

On 08/30, Peter Zijlstra wrote:
>
> On Thu, 2007-08-30 at 09:50 +0200, Vitaly Mayatskikh wrote:
> > Short-living process returns its timeslice to the parent, this affects
> > process that creates a lot of such short-living threads, because its
> > not a parent for new threads. Patch fixes this issue and doesn't break
> > kabi as does the patch from reporter: http://lkml.org/lkml/2007/4/7/21
> 
> > plain text document attachment (2.6.21-timeslice.patch), "proposed
> > patch"
> > diff -up -bB ./include/linux/sched.h.orig ./include/linux/sched.h
> > --- ./include/linux/sched.h.orig	2007-08-21 09:20:22.000000000 +0200
> > +++ ./include/linux/sched.h	2007-08-27 10:14:06.000000000 +0200
> > @@ -827,7 +827,9 @@ struct task_struct {
> >  
> >  	unsigned long policy;
> >  	cpumask_t cpus_allowed;
> > -	unsigned int time_slice, first_time_slice;
> > +	unsigned int time_slice;
> > +	/* Pid of creator */
> > +	unsigned int cpid;
> 
> might as well make that pid_t, or maybe even a struct pid* and keep a
> reference on it - the struct pid police might have an opinion.

I agree, "struct pid*" is better, because

	1. we don't need a costly find_pid() in sched_exit()
	2. we don't suffer from pid re-use problem

> >  void fastcall sched_exit(struct task_struct *p)
> >  {
> >  	unsigned long flags;
> >  	struct rq *rq;
> > -
> > +	struct task_struct* creator = NULL;
> >  	/*
> >  	 * If the child was a (relative-) CPU hog then decrease
> > -	 * the sleep_avg of the parent as well.
> > +	 * the sleep_avg of the creator as well.
> >  	 */
> > -	rq = task_rq_lock(p->parent, &flags);
> > -	if (p->first_time_slice && task_cpu(p) == task_cpu(p->parent)) {
> > -		p->parent->time_slice += p->time_slice;
> > -		if (unlikely(p->parent->time_slice > task_timeslice(p)))
> > -			p->parent->time_slice = task_timeslice(p);
> > +	if (p->cpid) {
> > +		struct pid *pid = find_get_pid((pid_t)p->cpid);
> > +		if (pid) {
> > +			creator = get_pid_task(pid, PIDTYPE_PID);
> > +			put_pid(pid);

sched_exit() was removed in 2.6.23-rc.

If you are going to re-introduce this logic, please don't do sched_exit()
from release_task(). It was done this way just because we can't access
->parent after release_task(). But release_task() is called either too
early, or too late for timeslice accounting, depending on ->exit_signal == -1.

I'd suggest to do this in do_exit(), before the last schedule(). Without
write_unlock_irq() the code above needs a couple of rcu_read_lock()'s.

I am not sure Ingo will like this change though...

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/