All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: kanoj@google.engr.sgi.com (Kanoj Sarcar)
To: Andrea Arcangeli <andrea@suse.de>
Cc: Ben LaHaise <bcrl@redhat.com>,
	riel@nl.linux.org, Linus Torvalds <torvalds@transmeta.com>,
	linux-mm@kvack.org
Subject: Re: [patch] take 2 Re: PG_swap_entry bug in recent kernels
Date: Tue, 11 Apr 2000 11:26:39 -0700 (PDT)	[thread overview]
Message-ID: <200004111826.LAA67211@google.engr.sgi.com> (raw)
In-Reply-To: <Pine.LNX.4.21.0004111752550.19969-100000@maclaurin.suse.de> from "Andrea Arcangeli" at Apr 11, 2000 06:22:31 PM

> 
> On Mon, 10 Apr 2000, Kanoj Sarcar wrote:
> 
> >While forking, a parent might copy a swap handle into the child, but we
> 
> That's a bug in fork. Simply let fork to check if the swaphandle is SWAPOK
> or not before increasing the swap count. If it's SWAPOK then
> swap_duplicate succesfully, otherwise do the swapin using swap cache based
> locking as swapin now does in my current tree (check if the pte is changed
> before go to increase the swap side and undo the swapcache insertion in
> such case and serialize with swapoff and swap_out with page_cache_lock).

I still can't see how this will help. You still might end up having an
unreachable mm, since the task is not on the active list, and swapoff 
unconditionally decrements the swap count after printing a warning message.
Later on, the process also tries decrementing the count, or accessing the
swap handle ...

> 
> >Same problem exists in exit_mmap. In this case, one of the routines inside
> >exit_mmap() can very plausibly go to sleep. Eg: file_unmap.
> 
> exit_mmap can sleep there. But it have not to hide the mmap as said in
> earlier email. It have to zap_page_range and then unlink the vmas all bit
> by bit serializing using the vmlist_modify_lock.

Sure, write the patch. It seems to me just getting the mmap_sem once 
should be enough, instead of multiple acquire/releases of the vmlist_modify_lock,
if for no other reason than performance. But I will agree to any solution
that fixes races. Correctness first, then performance ...

> 
> >> swap_out() can't grab the mmap_sem for obvious reasons, so if you only
> >
> >Why not? Of course, not with tasklist_lock held (Hehehe, I am not that 
> >stupid :-)). But other mechanisms are possible.
> 
> Lock recursion -> deadlock.
> 
> 	userspace runs
> 	page fault
> 		down(&current->mm->mmap_sem);
> 		try_to_free_pages();
> 			swap_out();
> 			down(&current->mm->mmap_sem); <- you deadlocked			

I am not sure what you mean. Maybe we should just stop talking about this
till I get a chance to post the patch, then you can show me the holes in 
it. I am not sure if I have been able to communicate clearly what I want 
to do, and how ...

Kanoj


> 
> We are serializing swap_out/do_wp_page with the page_cache_lock (in
> swapout the page_cache_lock is implied by the vmlist_access_lock).
> 
> In the same way I'm serializing swapoff with swapin using swap cache based
> on locking and pagetable checks with page_cache_lock acquired and
> protecting swapoff with the vmlist_access_lock() that imply the
> page_cache_lock.
> 
> Using the page_cache_lock and rechecking page table looks the right way to
> go to me. It have no design problems that ends in lock recursion.
> 
> >Actually, let me put out the patch, for different reasons, IMO, it is the
> >right long term solution ...
> 
> The patch is welcome indeed. However relying on the mmap_sem looks the
> wrong way to me.
> 
> Andrea
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

  parent reply	other threads:[~2000-04-11 18:26 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2000-04-03 22:22 PG_swap_entry bug in recent kernels Ben LaHaise
2000-04-04 15:06 ` Andrea Arcangeli
2000-04-04 15:46   ` Rik van Riel
2000-04-04 16:50     ` Andrea Arcangeli
2000-04-04 17:06       ` Ben LaHaise
2000-04-04 18:03         ` Andrea Arcangeli
2000-04-06 22:11           ` [patch] take 2 " Ben LaHaise
2000-04-07 10:45             ` Andrea Arcangeli
2000-04-07 11:29               ` Rik van Riel
2000-04-07 12:00                 ` Andrea Arcangeli
2000-04-07 12:54                   ` Rik van Riel
2000-04-07 13:14                     ` Andrea Arcangeli
2000-04-07 20:12               ` Kanoj Sarcar
2000-04-07 23:26                 ` Andrea Arcangeli
2000-04-08  0:11                   ` Kanoj Sarcar
2000-04-08  0:37                     ` Kanoj Sarcar
2000-04-08 13:20                       ` Andrea Arcangeli
2000-04-08 21:39                         ` Kanoj Sarcar
2000-04-08 23:02                           ` Andrea Arcangeli
2000-04-08 23:18                             ` Kanoj Sarcar
2000-04-08 23:58                               ` Andrea Arcangeli
2000-04-08 13:30                     ` Andrea Arcangeli
2000-04-08 17:39                       ` Andrea Arcangeli
2000-04-07 23:54                 ` Andrea Arcangeli
2000-04-08  0:15                   ` Kanoj Sarcar
2000-04-08 13:14                     ` Andrea Arcangeli
2000-04-08 21:47                       ` Kanoj Sarcar
2000-04-08 23:10                         ` Andrea Arcangeli
2000-04-08 23:21                           ` Kanoj Sarcar
2000-04-08 23:39                             ` Andrea Arcangeli
2000-04-09  0:40                               ` Kanoj Sarcar
2000-04-10  8:55                                 ` andrea
2000-04-11  2:45                                   ` Kanoj Sarcar
2000-04-11 16:22                                     ` Andrea Arcangeli
2000-04-11 17:40                                       ` Rik van Riel
2000-04-11 18:20                                         ` Kanoj Sarcar
2000-04-21 18:23                                         ` Andrea Arcangeli
2000-04-21 21:00                                           ` Rik van Riel
2000-04-22  1:12                                             ` Andrea Arcangeli
2000-04-22  1:51                                               ` Linus Torvalds
2000-04-22 18:29                                                 ` Rik van Riel
2000-04-22 19:58                                                   ` Linus Torvalds
2000-04-11 18:26                                       ` Kanoj Sarcar [this message]
2000-04-10 19:10                         ` Stephen C. Tweedie
2000-04-08  0:04                 ` Andrea Arcangeli
     [not found] <yttem7xstk2.fsf@vexeta.dc.fi.udc.es>
2000-04-23  0:52 ` Andrea Arcangeli
     [not found] <yttk8ho26s8.fsf@vexeta.dc.fi.udc.es>
2000-04-23 16:07 ` Andrea Arcangeli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200004111826.LAA67211@google.engr.sgi.com \
    --to=kanoj@google.engr.sgi.com \
    --cc=andrea@suse.de \
    --cc=bcrl@redhat.com \
    --cc=linux-mm@kvack.org \
    --cc=riel@nl.linux.org \
    --cc=torvalds@transmeta.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.