Re: [PATCH] proc: put check_mem_permission before __get_free_page in mem_read

[Hugh Dickins <hughd@google.com>]

On Sun, 17 Apr 2011, bookjovi@gmail.com wrote:
> From: Jovi Zhang <bookjovi@gmail.com>
> 
> It should be better if put check_mem_permission before __get_free_page
> in mem_read, to be same as function mem_write.
> 
> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>

Sorry to be contrary, but I disagree with this.  I'm all for consistency,
but is there a particular reason why you think the mem_write ordering is
right and mem_read wrong?

My reason for preferring the current mem_read ordering is this:

check_mem_permission gets a reference to the mm.  If we __get_free_page
after check_mem_permission, imagine what happens if the system is out
of memory, and the mm we're looking at is selected for killing by the
OOM killer: while we wait in __get_free_page for more memory, no memory
is freed from the selected mm because it cannot reach exit_mmap while
we hold that reference.

(I may be overstating the case: a little memory may be freed from the
exiting task's stack, and kswapd should still be able to pick some pages
off the mm.  But nonetheless, we would do better to let this mm go.)

No doubt there are plenty of other places in /proc which try to
allocate memory after taking a reference on an mm; but I think
we should be working to eliminate those rather than add to them.

Hugh