Messages in this thread | | | Date | Sun, 7 Dec 2008 22:28:28 +0530 | From | Balbir Singh <> | Subject | Re: [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
| |