Re: [PATCH 13/12] ksm: fix munlock during exit_mmap deadlock

On Tue, 25 Aug 2009, Andrea Arcangeli wrote:
On Tue, Aug 25, 2009 at 06:49:09PM +0100, Hugh Dickins wrote:
Looking ksm.c it should have been down_write indeed...

Nor do we want to change your down_read here to down_write, that will
just reintroduce the OOM deadlock that 9/12 was about solving.

I'm not sure anymore I get what this fix is about...

Yes, it's easy to drop one end of the string while picking up the other ;)

And it wouldn't be exactly the same deadlock, but similar.
The original deadlock that 9/12 was about was:
There's a now-obvious deadlock in KSM's out-of-memory handling:
imagine ksmd or KSM_RUN_UNMERGE handling, holding ksm_thread_mutex,
trying to allocate a page to break KSM in an mm which becomes the
OOM victim (quite likely in the unmerge case): it's killed and goes
to exit, and hangs there waiting to acquire ksm_thread_mutex.

Whereas with down_write(&mm->mmap_sem); up_write(&mm->mmap_sem)
just before calling exit_mmap(), the deadlock comes on mmap_sem
instead: the exiting OOM-killed task waiting there (for break_cow
or the like to up_read mmap_sem), before it has freed any memory
to allow break_cow etc. to proceed.

mm_users is
allowed to go to 0. If mm_users is allowed to go to 0, it's up to ksm
to check inside its inner loops that mm_users is 0 and bail
out. Bailing out it will unblock exit so that exit_mmap can run.

Yes, but one of those checks that mm_users is 0 has to be lie below
handle_mm_fault, because mm_users may go to 0 and exit_mmap proceed
while one of handle_pte_fault's helpers is waiting to allocate a page
(for example; but SMP could race anywhere). Hence ksm_test_exit()s
in mm/memory.c.

(And as I remarked in the 9/12 comments, it's no use bumping up
mm_users in break_ksm, say, though that would be a normal thing to
do: that just ensures the memory we'd be waiting for cannot be freed.)

What exactly is the unfixable issue?

Oh, there's no unfixable issue,
just an issue we've not yet found the right fix for ;)

The idea I'm currently playing with, would fix one of your objections
but violate another, is to remove the ksm_test_exit()s from mm/memory.c,
allow KSM to racily fault in too late, but observe mm_users 0 afterwards
and zap it then.

I agree with you that it seems _wrong_ for KSM to fault into an area
being exited, which was why the ksm_test_exit()s; but the neatest
answer might turn out to be to allow it to do so after all.

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

Relevant Pages

  • Re: Am I using ThreadPool the right way?
    ... It is possible to debug deadlocks and other threading issues in the Express version, but it's not something I'd recommend for someone unfamiliar with the general techniques of dealing with thread issues in the first place, since Express doesn't provide any direct way to get at the individual threads in the debugger. ... You'll be looking for threads that are stopped on a statement that waits for some resource, to identify which threads are involved in the deadlock and why they are waiting. ...
    (microsoft.public.dotnet.languages.csharp)
  • Re: Deadlock resolution
    ... > another thread (in the same workstation). ... > kernel performs a traceback on the chain of threads it is waiting ... If the chain doesn't break before the original thread is ... > reached again, deadlock is detected. ...
    (comp.lang.ada)
  • Re: Locking
    ... When this happens sql server automatically detects it and will ... So if your app is slow and waiting it is not ... > At the same time another stored proc is inserting data into the table,> updated fields, and deleting records. ... A deadlock> occurs, an the SQL box begins to get extremely slow. ...
    (microsoft.public.sqlserver.programming)
  • Re: Critical section ?
    ... James wrote: ... A deadlock is caused by thread A waiting for thread B to do something ... first is to send one thread to sleep on return, ...
    (alt.comp.lang.learn.c-cpp)