Re: [PATCH] introduce get_mm_hiwater_xxx(), fix taskstats->hiwater_xxx accounting

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* Oleg Nesterov <oleg@redhat.com> [2008-12-07 17:17:50]:

> (sorry for delay, I am travelling till 11 Dec)
> 
> On 12/06, Balbir Singh wrote:
> >
> > * Hugh Dickins <hugh@veritas.com> [2008-12-06 09:56:19]:
> >
> > > On Sat, 6 Dec 2008, Balbir Singh wrote:
> > > >
> > > > Yes, true and getdelays can display all the exported information.
> > > >
> > > > The race does seem concerning, I would vote for keeping the update in
> > > > there and disabling preemption around the update, so that hiwater
> > > > cannot swing back and forth.
> > >
> > > ??  Oleg is _fixing_ a race by removing the update from do_exit();
> > > and he is fixing the way the hiwater info was collected in tsacct.c.
> >
> > I see that change and the reasoning seems accurate that we can query
> > the task at anytime, but I worry that if taskstats is not enabled, we'll
> > never call update_hiwater.* on the exiting task.
> 
> With this patch, even if taskstats _is_ enabled, we never call update_
> on do_exit() path. Because there is no point to do this.
> 

Hmmm.. I thought the rules were to update it when RSS/total_vm is
decreasing. taskstats_exit() calls fill_pid(), which in turn calls
xacct_add_tsk().

> > I wonder if a thread came in and like Oleg said, did (without taskstats
> > enabled)
> >
> > free(malloc(some size)), followed by exit()
> >
> > whether task_mem() would show the correct results for hiwater.*.
> 
> unlike taskstats, task_mem() doesn't rely on update_hiwater_xxx(),
> it reads the current values and calculates the maximum. And this is
> the "right thing".
> 
> update_hiwater_xxx() is only needed when we are going to decrease
> the current value, so we can lose the info if we don't calculate
> the maximum right now.
> 

This is a bit confusing, look at strerror_l.c in libc. It frees the
last strerror value on exit of the thread. If a thread did strerror()
followed by exit(). If free() and malloc() map to mmap() and munmap(), 
do_exit() will affect RSS and total_vm... no?

> We can disable preemption around update_ in do_exit(), but this
> doesn't close the race. We can even disable irqs but this (in
> theory) is not enough either. But the main point we do not need
> to update.
> 

See above.

> And please note that taskstats was wrong even if update_ was not
> racy. Exactly because it relies on update_ in do_exit(), but it
> should not.
> 

This is because you believe we should do the comparison like
task_mem()? task_mem() does no updates of hi_water.*.

> As for ru_maxrss accounting, we can keep these update_hiwater_xxx()
> calls in do_exit() and then use mm->hiwater_xxx directly, but we
> should check group_dead in that case. I don't really think this
> would be cleaner/better, and then we have the similar problems with
> CLONE_VM tasks.

CLONE_VM without thread groups is sort of annoying and hopefully dead
:) mm_owner had a lot of complexity due to that

-- 
	Balbir