lkml.org 
[lkml]   [2008]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] introduce get_mm_hiwater_xxx(), fix taskstats->hiwater_xxx accounting
* 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


\
 
 \ /
  Last update: 2008-12-07 18:01    [W:0.062 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site