Re: PATCH? introduce get_compound_page (Was: process 'stuck' at exit)

[Oleg Nesterov <oleg@redhat.com>]

Andrea. Thanks a lot for the detailed reply.

On 12/13, Andrea Arcangeli wrote:
>
> On Wed, Dec 11, 2013 at 08:18:55PM +0100, Oleg Nesterov wrote:
>
> get_huge_page_tail checks different invariants in the VM_BUG_ON and is
> only used by gup.c not sure why to call that here.

Compared to __get_page_tail_foll(get_page_head => F) get_huge_page_tail()
lacks  the ->first_page->_count, but it is called right after
get_page_unless_zero(page_head) so to me get_huge_page_tail() looks a bit
better.

Personally, I think that even this change

	--- x/kernel/events/../../mm/internal.h
	+++ x/kernel/events/../../mm/internal.h
	@@ -47,11 +47,9 @@ static inline void __get_page_tail_foll(
		 * page_cache_get_speculative()) on tail pages.
		 */
		VM_BUG_ON(atomic_read(&page->first_page->_count) <= 0);
	-	VM_BUG_ON(atomic_read(&page->_count) != 0);
	-	VM_BUG_ON(page_mapcount(page) < 0);
		if (get_page_head)
			atomic_inc(&page->first_page->_count);
	-	atomic_inc(&page->_mapcount);
	+	get_huge_page_tail(page);
	 }
	 
	 /*

makes sense. But this is minor and subjective, I am not going to argue.

And I agree with other comments, the patch I sent only tried to explain
what I meant.

> If we've to mess with the compound lock for this, considering
> get_compound_head would have only this user anyway, we could do:
>
>      page_head = put_compound_tail(page);
>      if (!page_head)
>      	page_head = page;
>
> Implemented as:

OK, this looks better, I agree.

I'll try to make v2 based on -mm and your suggestions.

> Considering this is going incremental with -mm, and that the -mm
> previous patches the above would depend on are not upstream yet, I'd
> suggest to fix the bug in futex with Linus patch now (s/1/!ro/).

Yes, yes, sure. I didn't mean that this (potential) cleanup can replace
the simple fix from Linus.

Thanks!

Oleg.