the ntpd bug

Andrew Morton
2002-08-01T04:10:21Z

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Rik,

the bug which Peter Vendrovec reported appears to have snuck in
with the pte.direct optimisation.

- boot with mem=512m
- service ntpd start
- usemem 800 megs (a few times)
- service ntpd stop (goes bang)

I've attached the original diff. A patch -R of this still applies,
and it makes the problem go away. Have you time to see if you
can debug this problem?

I saw one rather strange looking thing which appears to add
two pte_chains for a single pte. But this patch:

--- 2.5.29/mm/rmap.c~rmap-fixes Wed Jul 31 20:59:34 2002
+++ 2.5.29-akpm/mm/rmap.c Wed Jul 31 20:59:53 2002
@@ -135,16 +135,17 @@ void page_add_rmap(struct page * page, p
pte_chain->next = NULL;
page->pte.chain = pte_chain;
ClearPageDirect(page);
- }
- if (page->pte.chain) {
- /* Hook up the pte_chain to the page. */
- pte_chain = pte_chain_alloc();
- pte_chain->ptep = ptep;
- pte_chain->next = page->pte.chain;
- page->pte.chain = pte_chain;
} else {
- page->pte.direct = ptep;
- SetPageDirect(page);
+ if (page->pte.chain) {
+ /* Hook up the pte_chain to the page. */
+ pte_chain = pte_chain_alloc();
+ pte_chain->ptep = ptep;
+ pte_chain->next = page->pte.chain;
+ page->pte.chain = pte_chain;
+ } else {
+ page->pte.direct = ptep;
+ SetPageDirect(page);
+ }
}
pte_chain_unlock(page);
}

doesn't help :(

[-- Attachment #2: rmap-opt.patch --]
[-- Type: text/plain, Size: 9292 bytes --]

Patch from David McCracken. It is an optimisation to the rmap
pte_chains.

In the common case where a page is mapped by only a single pte, we
don't need to allocate a pte_chain structure. Just make the page's
pte_chain pointer point straight at that pte and flag this with
PG_direct.

include/linux/mm.h | 5 +
include/linux/page-flags.h | 12 +++-
mm/page_alloc.c | 2
mm/rmap.c | 128 ++++++++++++++++++++++++++++++++-------------
mm/vmscan.c | 8 +-
5 files changed, 112 insertions(+), 43 deletions(-)

--- 2.5.26/include/linux/mm.h~rmap-opt Tue Jul 16 21:46:29 2002
+++ 2.5.26-akpm/include/linux/mm.h Tue Jul 16 21:59:38 2002
@@ -157,8 +157,11 @@ struct page {
updated asynchronously */
struct list_head lru; /* Pageout list, eg. active_list;
protected by pagemap_lru_lock !! */
- struct pte_chain * pte_chain; /* Reverse pte mapping pointer.
+ union {
+ struct pte_chain * chain; /* Reverse pte mapping pointer.
* protected by PG_chainlock */
+ pte_t * direct;
+ } pte;
unsigned long private; /* mapping-private opaque data */

/*
--- 2.5.26/include/linux/page-flags.h~rmap-opt Tue Jul 16 21:46:29 2002
+++ 2.5.26-akpm/include/linux/page-flags.h Tue Jul 16 21:59:40 2002
@@ -64,8 +64,10 @@

#define PG_private 12 /* Has something at ->private */
#define PG_writeback 13 /* Page is under writeback */
-#define PG_nosave 15 /* Used for system suspend/resume */
-#define PG_chainlock 16 /* lock bit for ->pte_chain */
+#define PG_nosave 14 /* Used for system suspend/resume */
+#define PG_chainlock 15 /* lock bit for ->pte_chain */
+
+#define PG_direct 16 /* ->pte_chain points directly at pte */

/*
* Global page accounting. One instance per CPU.
@@ -217,6 +219,12 @@ extern void get_page_state(struct page_s
#define ClearPageNosave(page) clear_bit(PG_nosave, &(page)->flags)
#define TestClearPageNosave(page) test_and_clear_bit(PG_nosave, &(page)->flags)

+#define PageDirect(page) test_bit(PG_direct, &(page)->flags)
+#define SetPageDirect(page) set_bit(PG_direct, &(page)->flags)
+#define TestSetPageDirect(page) test_and_set_bit(PG_direct, &(page)->flags)
+#define ClearPageDirect(page) clear_bit(PG_direct, &(page)->flags)
+#define TestClearPageDirect(page) test_and_clear_bit(PG_direct, &(page)->flags)
+
/*
* inlines for acquisition and release of PG_chainlock
*/
--- 2.5.26/mm/page_alloc.c~rmap-opt Tue Jul 16 21:46:29 2002
+++ 2.5.26-akpm/mm/page_alloc.c Tue Jul 16 21:59:40 2002
@@ -92,7 +92,7 @@ static void __free_pages_ok (struct page
BUG_ON(PageLRU(page));
BUG_ON(PageActive(page));
BUG_ON(PageWriteback(page));
- BUG_ON(page->pte_chain != NULL);
+ BUG_ON(page->pte.chain != NULL);
if (PageDirty(page))
ClearPageDirty(page);
BUG_ON(page_count(page) != 0);
--- 2.5.26/mm/rmap.c~rmap-opt Tue Jul 16 21:46:29 2002
+++ 2.5.26-akpm/mm/rmap.c Tue Jul 16 21:59:40 2002
@@ -13,7 +13,7 @@

/*
* Locking:
- * - the page->pte_chain is protected by the PG_chainlock bit,
+ * - the page->pte.chain is protected by the PG_chainlock bit,
* which nests within the pagemap_lru_lock, then the
* mm->page_table_lock, and then the page lock.
* - because swapout locking is opposite to the locking order
@@ -71,10 +71,15 @@ int page_referenced(struct page * page)
if (TestClearPageReferenced(page))
referenced++;

- /* Check all the page tables mapping this page. */
- for (pc = page->pte_chain; pc; pc = pc->next) {
- if (ptep_test_and_clear_young(pc->ptep))
+ if (PageDirect(page)) {
+ if (ptep_test_and_clear_young(page->pte.direct))
referenced++;
+ } else {
+ /* Check all the page tables mapping this page. */
+ for (pc = page->pte.chain; pc; pc = pc->next) {
+ if (ptep_test_and_clear_young(pc->ptep))
+ referenced++;
+ }
}
return referenced;
}
@@ -108,22 +113,39 @@ void page_add_rmap(struct page * page, p
pte_chain_lock(page);
{
struct pte_chain * pc;
- for (pc = page->pte_chain; pc; pc = pc->next) {
- if (pc->ptep == ptep)
+ if (PageDirect(page)) {
+ if (page->pte.direct == ptep)
BUG();
+ } else {
+ for (pc = page->pte.chain; pc; pc = pc->next) {
+ if (pc->ptep == ptep)
+ BUG();
+ }
}
}
pte_chain_unlock(page);
#endif

- pte_chain = pte_chain_alloc();
-
pte_chain_lock(page);

- /* Hook up the pte_chain to the page. */
- pte_chain->ptep = ptep;
- pte_chain->next = page->pte_chain;
- page->pte_chain = pte_chain;
+ if (PageDirect(page)) {
+ /* Convert a direct pointer into a pte_chain */
+ pte_chain = pte_chain_alloc();
+ pte_chain->ptep = page->pte.direct;
+ pte_chain->next = NULL;
+ page->pte.chain = pte_chain;
+ ClearPageDirect(page);
+ }
+ if (page->pte.chain) {
+ /* Hook up the pte_chain to the page. */
+ pte_chain = pte_chain_alloc();
+ pte_chain->ptep = ptep;
+ pte_chain->next = page->pte.chain;
+ page->pte.chain = pte_chain;
+ } else {
+ page->pte.direct = ptep;
+ SetPageDirect(page);
+ }

pte_chain_unlock(page);
}
@@ -149,18 +171,38 @@ void page_remove_rmap(struct page * page
return;

pte_chain_lock(page);
- for (pc = page->pte_chain; pc; prev_pc = pc, pc = pc->next) {
- if (pc->ptep == ptep) {
- pte_chain_free(pc, prev_pc, page);
+
+ if (PageDirect(page)) {
+ if (page->pte.direct == ptep) {
+ page->pte.direct = NULL;
+ ClearPageDirect(page);
goto out;
}
+ } else {
+ for (pc = page->pte.chain; pc; prev_pc = pc, pc = pc->next) {
+ if (pc->ptep == ptep) {
+ pte_chain_free(pc, prev_pc, page);
+ /* Check whether we can convert to direct */
+ pc = page->pte.chain;
+ if (!pc->next) {
+ page->pte.direct = pc->ptep;
+ SetPageDirect(page);
+ pte_chain_free(pc, NULL, NULL);
+ }
+ goto out;
+ }
+ }
}
#ifdef DEBUG_RMAP
/* Not found. This should NEVER happen! */
printk(KERN_ERR "page_remove_rmap: pte_chain %p not present.\n", ptep);
printk(KERN_ERR "page_remove_rmap: only found: ");
- for (pc = page->pte_chain; pc; pc = pc->next)
- printk("%p ", pc->ptep);
+ if (PageDirect(page)) {
+ printk("%p ", page->pte.direct);
+ } else {
+ for (pc = page->pte.chain; pc; pc = pc->next)
+ printk("%p ", pc->ptep);
+ }
printk("\n");
printk(KERN_ERR "page_remove_rmap: driver cleared PG_reserved ?\n");
#endif
@@ -270,25 +312,41 @@ int try_to_unmap(struct page * page)
if (!page->mapping)
BUG();

- for (pc = page->pte_chain; pc; pc = next_pc) {
- next_pc = pc->next;
- switch (try_to_unmap_one(page, pc->ptep)) {
- case SWAP_SUCCESS:
- /* Free the pte_chain struct. */
- pte_chain_free(pc, prev_pc, page);
- break;
- case SWAP_AGAIN:
- /* Skip this pte, remembering status. */
- prev_pc = pc;
- ret = SWAP_AGAIN;
- continue;
- case SWAP_FAIL:
- return SWAP_FAIL;
- case SWAP_ERROR:
- return SWAP_ERROR;
+ if (PageDirect(page)) {
+ ret = try_to_unmap_one(page, page->pte.direct);
+ if (ret == SWAP_SUCCESS) {
+ page->pte.direct = NULL;
+ ClearPageDirect(page);
+ }
+ } else {
+ for (pc = page->pte.chain; pc; pc = next_pc) {
+ next_pc = pc->next;
+ switch (try_to_unmap_one(page, pc->ptep)) {
+ case SWAP_SUCCESS:
+ /* Free the pte_chain struct. */
+ pte_chain_free(pc, prev_pc, page);
+ break;
+ case SWAP_AGAIN:
+ /* Skip this pte, remembering status. */
+ prev_pc = pc;
+ ret = SWAP_AGAIN;
+ continue;
+ case SWAP_FAIL:
+ ret = SWAP_FAIL;
+ break;
+ case SWAP_ERROR:
+ ret = SWAP_ERROR;
+ break;
+ }
+ }
+ /* Check whether we can convert to direct pte pointer */
+ pc = page->pte.chain;
+ if (pc && !pc->next) {
+ page->pte.direct = pc->ptep;
+ SetPageDirect(page);
+ pte_chain_free(pc, NULL, NULL);
}
}
-
return ret;
}

@@ -336,7 +394,7 @@ static inline void pte_chain_free(struct
if (prev_pte_chain)
prev_pte_chain->next = pte_chain->next;
else if (page)
- page->pte_chain = pte_chain->next;
+ page->pte.chain = pte_chain->next;

spin_lock(&pte_chain_freelist_lock);
pte_chain_push(pte_chain);
--- 2.5.26/mm/vmscan.c~rmap-opt Tue Jul 16 21:46:29 2002
+++ 2.5.26-akpm/mm/vmscan.c Tue Jul 16 21:59:40 2002
@@ -48,7 +48,7 @@ static inline int page_mapping_inuse(str
struct address_space *mapping = page->mapping;

/* Page is in somebody's page tables. */
- if (page->pte_chain)
+ if (page->pte.chain)
return 1;

/* XXX: does this happen ? */
@@ -151,7 +151,7 @@ shrink_cache(int nr_pages, zone_t *class
*
* XXX: implement swap clustering ?
*/
- if (page->pte_chain && !page->mapping && !PagePrivate(page)) {
+ if (page->pte.chain && !page->mapping && !PagePrivate(page)) {
page_cache_get(page);
pte_chain_unlock(page);
spin_unlock(&pagemap_lru_lock);
@@ -171,7 +171,7 @@ shrink_cache(int nr_pages, zone_t *class
* The page is mapped into the page tables of one or more
* processes. Try to unmap it here.
*/
- if (page->pte_chain) {
+ if (page->pte.chain) {
switch (try_to_unmap(page)) {
case SWAP_ERROR:
case SWAP_FAIL:
@@ -340,7 +340,7 @@ static void refill_inactive(int nr_pages
entry = entry->prev;

pte_chain_lock(page);
- if (page->pte_chain && page_referenced(page)) {
+ if (page->pte.chain && page_referenced(page)) {
list_del(&page->lru);
list_add(&page->lru, &active_list);
pte_chain_unlock(page);

.