On Thu, 27 Jun 2002, Andrea Arcangeli wrote:
> I will make a full vm update for 2.5 [in small pieces based on post-Andrew
> split of the monolithic patch] in the next days anyways):
Great!
> diff -urNp 2.4.19rc1/mm/page_alloc.c 2.4.19rc1aa1/mm/page_alloc.c
> --- 2.4.19rc1/mm/page_alloc.c Tue Jun 25 23:56:23 2002
> +++ 2.4.19rc1aa1/mm/page_alloc.c Wed Jun 26 00:55:35 2002
> @@ -82,8 +82,11 @@ static void __free_pages_ok (struct page
> /* Yes, think what happens when other parts of the kernel take
> * a reference to a page in order to pin it for io. -ben
> */
> - if (PageLRU(page))
> + if (PageLRU(page)) {
> + if (unlikely(in_interrupt()))
> + BUG();
> lru_cache_del(page);
> + }
>
> this adds a debugging check just in case.
Well, of course I like this patch, having proposed it months ago.
But the evidence for needing it seems to have died down, and I
don't think -rc is the right time for adding a BUG() where you
can almost always get away with such bad behaviour - I intended
it for a diagnostic aid in the early stages of a release -
but I'm happy to go whichever way Marcelo decides on it.
> @@ -91,6 +94,8 @@ static void __free_pages_ok (struct page
> BUG();
> if (!VALID_PAGE(page))
> BUG();
> + if (PageSwapCache(page))
> + BUG();
>
> This resurrects a valid debugging check dropped in rc1.
Sorry, Andrea, perhaps I should have copied you explicitly on my
patch which dropped that check. It is a valid check, yes, but it's
a waste of space and time: remember that PageSwapCache(page) just
checks whether page->mapping == &swapper_space (used to be a bitflag,
yes, but not since last Autumn), and the BUG() you can just see above
was if page->mapping was set at all. One day, yes, PageSwapCache may
become bitflag test again: then would be the time to add the test back.
> @@ -280,10 +285,12 @@ static struct page * balance_classzone(z
> BUG();
> if (!VALID_PAGE(page))
> BUG();
> + if (PageSwapCache(page))
> + BUG();
>
> same as above. a page with page->mapping set cannot be freed, if it
> happens it's a bug that we want to trap.
As you say, same as above: again the BUG() you can just see at the top
was on a test for whether page->mapping is set, so PageSwapCache test
again redundant. If compiler optimized it away, I'd leave it: but no.
> if (PageLocked(page))
> BUG();
> if (PageLRU(page))
> - BUG();
> + lru_cache_del(page);
I was going to query this, but see later mail from you withdrawing it.
> diff -urNp 2.4.19rc1/mm/swap_state.c 2.4.19rc1aa1/mm/swap_state.c
> --- 2.4.19rc1/mm/swap_state.c Tue Jan 22 12:55:27 2002
> +++ 2.4.19rc1aa1/mm/swap_state.c Wed Jun 26 00:48:13 2002
> @@ -95,11 +95,8 @@ int add_to_swap_cache(struct page *page,
> */
> void __delete_from_swap_cache(struct page *page)
> {
> - if (!PageLocked(page))
> - BUG();
> if (!PageSwapCache(page))
> BUG();
> - ClearPageDirty(page);
> __remove_inode_page(page);
> INC_CACHE_INFO(del_total);
> }
>
> Here I play risky for a rc1, but it made perfect sense to me,
Yes and yes. My comment when I weakened the __remove_inode_page BUG
was "Remove the prior ClearPageDirty? maybe but not without deeper
thought: let stay for now." I've not given it deep enough thought,
and I'm not convinced you have yet: need to consider the peculiarities
of mm/shmem.c, and the way clearing the flag lets the page be dirtied
on to a list later. My _guess_ is that it's fine to remove that
ClearPageDirty in 2.4, but 2.5 a rather different case; but it
seems to me a risky cleanup, not -rc material. (But, sorry,
maybe I'm just advertizing the shallowness of _my_ thought.)
> @@ -114,9 +111,6 @@ void delete_from_swap_cache(struct page
> {
> swp_entry_t entry;
>
> - if (!PageLocked(page))
> - BUG();
> -
> block_flushpage(page, 0);
>
> entry.val = page->index;
>
> dropped also two pagelocked checks because there execute unconditionally
> paths that checks the pagelocked bit at the lower layer.
I don't mind that removal if you leave the !PageLocked BUG test in
__delete_from_swap_cache, but you've proposed to remove that one too?
Hugh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Jun 30 2002 - 22:00:12 EST