| Message ID | 1598273705-69124-7-git-send-email-alex.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | per memcg lru_lock | expand |
On Mon, Aug 24, 2020 at 08:54:39PM +0800, Alex Shi wrote: > lru_lock and page cache xa_lock have no reason with current sequence, > put them together isn't necessary. let's narrow the lru locking, but > left the local_irq_disable to block interrupt re-entry and statistic update. What stats are you talking about here? > +++ b/mm/huge_memory.c > @@ -2397,7 +2397,7 @@ static void __split_huge_page_tail(struct page *head, int tail, > } > > static void __split_huge_page(struct page *page, struct list_head *list, > - pgoff_t end, unsigned long flags) > + pgoff_t end) Please don't change this whitespace. It's really annoying having to adjust the whitespace when renaming a function. Just two tabs indentation to give a clear separation of arguments from code is fine. How about this patch instead? It occurred to me we already have perfectly good infrastructure to track whether or not interrupts are already disabled, and so we should use that instead of ensuring that interrupts are disabled, or tracking that ourselves. But I may have missed something else that's relying on having interrupts disabled. Please check carefully. diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 2ccff8472cd4..74cae6c032f9 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2376,17 +2376,16 @@ static void __split_huge_page_tail(struct page *head, int tail, } static void __split_huge_page(struct page *page, struct list_head *list, - pgoff_t end, unsigned long flags) + pgoff_t end) { struct page *head = compound_head(page); pg_data_t *pgdat = page_pgdat(head); struct lruvec *lruvec; struct address_space *swap_cache = NULL; unsigned long offset = 0; + unsigned long flags; int i; - lruvec = mem_cgroup_page_lruvec(head, pgdat); - /* complete memcg works before add pages to LRU */ mem_cgroup_split_huge_fixup(head); @@ -2395,9 +2394,13 @@ static void __split_huge_page(struct page *page, struct list_head *list, offset = swp_offset(entry); swap_cache = swap_address_space(entry); - xa_lock(&swap_cache->i_pages); + xa_lock_irq(&swap_cache->i_pages); } + /* prevent PageLRU to go away from under us, and freeze lru stats */ + spin_lock_irqsave(&pgdat->lru_lock, flags); + lruvec = mem_cgroup_page_lruvec(head, pgdat); + for (i = HPAGE_PMD_NR - 1; i >= 1; i--) { __split_huge_page_tail(head, i, lruvec, list); /* Some pages can be beyond i_size: drop them from page cache */ @@ -2417,6 +2420,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, } ClearPageCompound(head); + spin_unlock_irqrestore(&pgdat->lru_lock, flags); split_page_owner(head, HPAGE_PMD_ORDER); @@ -2425,18 +2429,16 @@ static void __split_huge_page(struct page *page, struct list_head *list, /* Additional pin to swap cache */ if (PageSwapCache(head)) { page_ref_add(head, 2); - xa_unlock(&swap_cache->i_pages); + xa_unlock_irq(&swap_cache->i_pages); } else { page_ref_inc(head); } } else { /* Additional pin to page cache */ page_ref_add(head, 2); - xa_unlock(&head->mapping->i_pages); + xa_unlock_irq(&head->mapping->i_pages); } - spin_unlock_irqrestore(&pgdat->lru_lock, flags); - remap_page(head); for (i = 0; i < HPAGE_PMD_NR; i++) { @@ -2574,7 +2576,6 @@ bool can_split_huge_page(struct page *page, int *pextra_pins) int split_huge_page_to_list(struct page *page, struct list_head *list) { struct page *head = compound_head(page); - struct pglist_data *pgdata = NODE_DATA(page_to_nid(head)); struct deferred_split *ds_queue = get_deferred_split_queue(head); struct anon_vma *anon_vma = NULL; struct address_space *mapping = NULL; @@ -2640,9 +2641,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) unmap_page(head); VM_BUG_ON_PAGE(compound_mapcount(head), head); - /* prevent PageLRU to go away from under us, and freeze lru stats */ - spin_lock_irqsave(&pgdata->lru_lock, flags); - if (mapping) { XA_STATE(xas, &mapping->i_pages, page_index(head)); @@ -2650,13 +2648,13 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) * Check if the head page is present in page cache. * We assume all tail are present too, if head is there. */ - xa_lock(&mapping->i_pages); + xa_lock_irq(&mapping->i_pages); if (xas_load(&xas) != head) goto fail; } /* Prevent deferred_split_scan() touching ->_refcount */ - spin_lock(&ds_queue->split_queue_lock); + spin_lock_irqsave(&ds_queue->split_queue_lock, flags); count = page_count(head); mapcount = total_mapcount(head); if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) { @@ -2664,7 +2662,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) ds_queue->split_queue_len--; list_del(page_deferred_list(head)); } - spin_unlock(&ds_queue->split_queue_lock); + spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); if (mapping) { if (PageSwapBacked(head)) __dec_node_page_state(head, NR_SHMEM_THPS); @@ -2672,7 +2670,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) __dec_node_page_state(head, NR_FILE_THPS); } - __split_huge_page(page, list, end, flags); + __split_huge_page(page, list, end); if (PageSwapCache(head)) { swp_entry_t entry = { .val = page_private(head) }; @@ -2688,10 +2686,9 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) dump_page(page, "total_mapcount(head) > 0"); BUG(); } - spin_unlock(&ds_queue->split_queue_lock); + spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); fail: if (mapping) - xa_unlock(&mapping->i_pages); - spin_unlock_irqrestore(&pgdata->lru_lock, flags); + xa_unlock_irq(&mapping->i_pages); remap_page(head); ret = -EBUSY; }
在 2020/9/10 下午9:49, Matthew Wilcox 写道: > On Mon, Aug 24, 2020 at 08:54:39PM +0800, Alex Shi wrote: >> lru_lock and page cache xa_lock have no reason with current sequence, >> put them together isn't necessary. let's narrow the lru locking, but >> left the local_irq_disable to block interrupt re-entry and statistic update. > > What stats are you talking about here? Hi Matthew, Thanks for comments! like __dec_node_page_state(head, NR_SHMEM_THPS); will have preemptive warning... > >> +++ b/mm/huge_memory.c >> @@ -2397,7 +2397,7 @@ static void __split_huge_page_tail(struct page *head, int tail, >> } >> >> static void __split_huge_page(struct page *page, struct list_head *list, >> - pgoff_t end, unsigned long flags) >> + pgoff_t end) > > Please don't change this whitespace. It's really annoying having to > adjust the whitespace when renaming a function. Just two tabs indentation > to give a clear separation of arguments from code is fine. > > > How about this patch instead? It occurred to me we already have > perfectly good infrastructure to track whether or not interrupts are > already disabled, and so we should use that instead of ensuring that > interrupts are disabled, or tracking that ourselves. So your proposal looks like; 1, xa_lock_irq(&mapping->i_pages); (optional) 2, spin_lock_irqsave(&ds_queue->split_queue_lock, flags); 3, spin_lock_irqsave(&pgdat->lru_lock, flags); Is there meaningful for the 2nd and 3rd flags? IIRC, I had a similar proposal as your, the flags used in xa_lock_irqsave(), but objected by Hugh. Thanks Alex > > But I may have missed something else that's relying on having > interrupts disabled. Please check carefully. > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 2ccff8472cd4..74cae6c032f9 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2376,17 +2376,16 @@ static void __split_huge_page_tail(struct page *head, int tail, > } > > static void __split_huge_page(struct page *page, struct list_head *list, > - pgoff_t end, unsigned long flags) > + pgoff_t end) > { > struct page *head = compound_head(page); > pg_data_t *pgdat = page_pgdat(head); > struct lruvec *lruvec; > struct address_space *swap_cache = NULL; > unsigned long offset = 0; > + unsigned long flags; > int i; > > - lruvec = mem_cgroup_page_lruvec(head, pgdat); > - > /* complete memcg works before add pages to LRU */ > mem_cgroup_split_huge_fixup(head); > > @@ -2395,9 +2394,13 @@ static void __split_huge_page(struct page *page, struct list_head *list, > > offset = swp_offset(entry); > swap_cache = swap_address_space(entry); > - xa_lock(&swap_cache->i_pages); > + xa_lock_irq(&swap_cache->i_pages); > } > > + /* prevent PageLRU to go away from under us, and freeze lru stats */ > + spin_lock_irqsave(&pgdat->lru_lock, flags); > + lruvec = mem_cgroup_page_lruvec(head, pgdat); > + > for (i = HPAGE_PMD_NR - 1; i >= 1; i--) { > __split_huge_page_tail(head, i, lruvec, list); > /* Some pages can be beyond i_size: drop them from page cache */ > @@ -2417,6 +2420,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > } > > ClearPageCompound(head); > + spin_unlock_irqrestore(&pgdat->lru_lock, flags); > > split_page_owner(head, HPAGE_PMD_ORDER); > > @@ -2425,18 +2429,16 @@ static void __split_huge_page(struct page *page, struct list_head *list, > /* Additional pin to swap cache */ > if (PageSwapCache(head)) { > page_ref_add(head, 2); > - xa_unlock(&swap_cache->i_pages); > + xa_unlock_irq(&swap_cache->i_pages); > } else { > page_ref_inc(head); > } > } else { > /* Additional pin to page cache */ > page_ref_add(head, 2); > - xa_unlock(&head->mapping->i_pages); > + xa_unlock_irq(&head->mapping->i_pages); > } > > - spin_unlock_irqrestore(&pgdat->lru_lock, flags); > - > remap_page(head); > > for (i = 0; i < HPAGE_PMD_NR; i++) { > @@ -2574,7 +2576,6 @@ bool can_split_huge_page(struct page *page, int *pextra_pins) > int split_huge_page_to_list(struct page *page, struct list_head *list) > { > struct page *head = compound_head(page); > - struct pglist_data *pgdata = NODE_DATA(page_to_nid(head)); > struct deferred_split *ds_queue = get_deferred_split_queue(head); > struct anon_vma *anon_vma = NULL; > struct address_space *mapping = NULL; > @@ -2640,9 +2641,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > unmap_page(head); > VM_BUG_ON_PAGE(compound_mapcount(head), head); > > - /* prevent PageLRU to go away from under us, and freeze lru stats */ > - spin_lock_irqsave(&pgdata->lru_lock, flags); > - > if (mapping) { > XA_STATE(xas, &mapping->i_pages, page_index(head)); > > @@ -2650,13 +2648,13 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > * Check if the head page is present in page cache. > * We assume all tail are present too, if head is there. > */ > - xa_lock(&mapping->i_pages); > + xa_lock_irq(&mapping->i_pages); > if (xas_load(&xas) != head) > goto fail; > } > > /* Prevent deferred_split_scan() touching ->_refcount */ > - spin_lock(&ds_queue->split_queue_lock); > + spin_lock_irqsave(&ds_queue->split_queue_lock, flags); > count = page_count(head); > mapcount = total_mapcount(head); > if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) { > @@ -2664,7 +2662,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > ds_queue->split_queue_len--; > list_del(page_deferred_list(head)); > } > - spin_unlock(&ds_queue->split_queue_lock); > + spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); > if (mapping) { > if (PageSwapBacked(head)) > __dec_node_page_state(head, NR_SHMEM_THPS); > @@ -2672,7 +2670,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > __dec_node_page_state(head, NR_FILE_THPS); > } > > - __split_huge_page(page, list, end, flags); > + __split_huge_page(page, list, end); > if (PageSwapCache(head)) { > swp_entry_t entry = { .val = page_private(head) }; > > @@ -2688,10 +2686,9 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > dump_page(page, "total_mapcount(head) > 0"); > BUG(); > } > - spin_unlock(&ds_queue->split_queue_lock); > + spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); > fail: if (mapping) > - xa_unlock(&mapping->i_pages); > - spin_unlock_irqrestore(&pgdata->lru_lock, flags); > + xa_unlock_irq(&mapping->i_pages); > remap_page(head); > ret = -EBUSY; > } >
On Fri, Sep 11, 2020 at 11:37:50AM +0800, Alex Shi wrote: > > > 在 2020/9/10 下午9:49, Matthew Wilcox 写道: > > On Mon, Aug 24, 2020 at 08:54:39PM +0800, Alex Shi wrote: > >> lru_lock and page cache xa_lock have no reason with current sequence, > >> put them together isn't necessary. let's narrow the lru locking, but > >> left the local_irq_disable to block interrupt re-entry and statistic update. > > > > What stats are you talking about here? > > Hi Matthew, > > Thanks for comments! > > like __dec_node_page_state(head, NR_SHMEM_THPS); will have preemptive warning... OK, but those stats are guarded by 'if (mapping)', so this patch doesn't produce that warning because we'll have taken the xarray lock and disabled interrupts. > > How about this patch instead? It occurred to me we already have > > perfectly good infrastructure to track whether or not interrupts are > > already disabled, and so we should use that instead of ensuring that > > interrupts are disabled, or tracking that ourselves. > > So your proposal looks like; > 1, xa_lock_irq(&mapping->i_pages); (optional) > 2, spin_lock_irqsave(&ds_queue->split_queue_lock, flags); > 3, spin_lock_irqsave(&pgdat->lru_lock, flags); > > Is there meaningful for the 2nd and 3rd flags? Yes. We want to avoid doing: if (mapping) spin_lock(&ds_queue->split_queue_lock); else spin_lock_irq(&ds_queue->split_queue_lock); ... if (mapping) spin_unlock(&ds_queue->split_queue_lock); else spin_unlock_irq(&ds_queue->split_queue_lock); Just using _irqsave has the same effect and is easier to reason about. > IIRC, I had a similar proposal as your, the flags used in xa_lock_irqsave(), > but objected by Hugh. I imagine Hugh's objection was that we know it's safe to disable/enable interrupts here because we're in a sleepable context. But for the other two locks, we'd rather not track whether we've already disabled interrupts or not. Maybe you could dig up the email from Hugh? I can't find it.
On Sun, 13 Sep 2020, Matthew Wilcox wrote: > On Fri, Sep 11, 2020 at 11:37:50AM +0800, Alex Shi wrote: > > 在 2020/9/10 下午9:49, Matthew Wilcox 写道: > > > On Mon, Aug 24, 2020 at 08:54:39PM +0800, Alex Shi wrote: > > >> lru_lock and page cache xa_lock have no reason with current sequence, > > >> put them together isn't necessary. let's narrow the lru locking, but > > >> left the local_irq_disable to block interrupt re-entry and statistic update. > > > > > > What stats are you talking about here? > > > > Hi Matthew, > > > > Thanks for comments! > > > > like __dec_node_page_state(head, NR_SHMEM_THPS); will have preemptive warning... > > OK, but those stats are guarded by 'if (mapping)', so this patch doesn't > produce that warning because we'll have taken the xarray lock and disabled > interrupts. > > > > How about this patch instead? It occurred to me we already have > > > perfectly good infrastructure to track whether or not interrupts are > > > already disabled, and so we should use that instead of ensuring that > > > interrupts are disabled, or tracking that ourselves. > > > > So your proposal looks like; > > 1, xa_lock_irq(&mapping->i_pages); (optional) > > 2, spin_lock_irqsave(&ds_queue->split_queue_lock, flags); > > 3, spin_lock_irqsave(&pgdat->lru_lock, flags); > > > > Is there meaningful for the 2nd and 3rd flags? > > Yes. We want to avoid doing: > > if (mapping) > spin_lock(&ds_queue->split_queue_lock); > else > spin_lock_irq(&ds_queue->split_queue_lock); > ... > if (mapping) > spin_unlock(&ds_queue->split_queue_lock); > else > spin_unlock_irq(&ds_queue->split_queue_lock); > > Just using _irqsave has the same effect and is easier to reason about. > > > IIRC, I had a similar proposal as your, the flags used in xa_lock_irqsave(), > > but objected by Hugh. > > I imagine Hugh's objection was that we know it's safe to disable/enable > interrupts here because we're in a sleepable context. But for the > other two locks, we'd rather not track whether we've already disabled > interrupts or not. > > Maybe you could dig up the email from Hugh? I can't find it. I did not find exactly the objection Alex seems to be remembering, but I have certainly expressed frustration with the lack of a reason for the THP split lock reordering, and in private mail in June while I was testing and sending back fixes: "I'd prefer that you never got into this: it looks like an unrelated and debatable cleanup, and I can see more such cleanup to make there, that we'd better not get into right now." I've several times toyed with just leaving this patch out of the series: but each time ended up, for better or worse, deciding we'd better keep it in - partly because we've never tested without it, and it cannot be dropped without making some other change (to stabilize the memcg in the !list case) - easily doable, but already done by this patch. Alex asked me to improve his commit message to satisfy my objections, here's what I sent him last night: === lru_lock and page cache xa_lock have no obvious reason to be taken one way round or the other: until now, lru_lock has been taken before page cache xa_lock, when splitting a THP; but nothing else takes them together. Reverse that ordering: let's narrow the lru locking - but leave local_irq_disable to block interrupts throughout, like before. Hugh Dickins point: split_huge_page_to_list() was already silly, to be using the _irqsave variant: it's just been taking sleeping locks, so would already be broken if entered with interrupts enabled. So we can save passing flags argument down to __split_huge_page(). Why change the lock ordering here? That was hard to decide. One reason: when this series reaches per-memcg lru locking, it relies on the THP's memcg to be stable when taking the lru_lock: that is now done after the THP's refcount has been frozen, which ensures page memcg cannot change. Another reason: previously, lock_page_memcg()'s move_lock was presumed to nest inside lru_lock; but now lru_lock must nest inside (page cache lock inside) move_lock, so it becomes possible to use lock_page_memcg() to stabilize page memcg before taking its lru_lock. That is not the mechanism used in this series, but it is an option we want to keep open. === It's still the case that I want to avoid further cleanups and bikeshedding here for now. I took an open-minded look at Alex's patch versus Matthew's patch, and do prefer Alex's: largely because it's simple and explicit about where the irq disabling and enabling is done (exactly where it was done before), and doesn't need irqsave clutter in between. If this were to be the only local_irq_disable() in mm I'd NAK it, but that's not so - and as I said before, I don't take the RT THP case very seriously anyway. One slight worry in Matthew's version: spin_lock_irqsave(&ds_queue->split_queue_lock, flags); count = page_count(head); mapcount = total_mapcount(head); if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) { if (!list_empty(page_deferred_list(head))) { ds_queue->split_queue_len--; list_del(page_deferred_list(head)); } spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); if (mapping) { if (PageSwapBacked(head)) __dec_node_page_state(head, NR_SHMEM_THPS); else __dec_node_page_state(head, NR_FILE_THPS); } __split_huge_page(page, list, end); In the Anon case, interrupts are enabled when calling __split_huge_page() there, but head's refcount is frozen: I'm uneasy about preemption when a refcount is frozen. But I'd worry much more if it were the mapping case: no, that has interrupts safely disabled at that point (as does Anon in the current kernel, and with Alex's patch). Hugh
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 247f53def87b..0132d363253e 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2397,7 +2397,7 @@ static void __split_huge_page_tail(struct page *head, int tail, } static void __split_huge_page(struct page *page, struct list_head *list, - pgoff_t end, unsigned long flags) + pgoff_t end) { struct page *head = compound_head(page); pg_data_t *pgdat = page_pgdat(head); @@ -2406,8 +2406,6 @@ static void __split_huge_page(struct page *page, struct list_head *list, unsigned long offset = 0; int i; - lruvec = mem_cgroup_page_lruvec(head, pgdat); - /* complete memcg works before add pages to LRU */ mem_cgroup_split_huge_fixup(head); @@ -2419,6 +2417,11 @@ static void __split_huge_page(struct page *page, struct list_head *list, xa_lock(&swap_cache->i_pages); } + /* prevent PageLRU to go away from under us, and freeze lru stats */ + spin_lock(&pgdat->lru_lock); + + lruvec = mem_cgroup_page_lruvec(head, pgdat); + for (i = HPAGE_PMD_NR - 1; i >= 1; i--) { __split_huge_page_tail(head, i, lruvec, list); /* Some pages can be beyond i_size: drop them from page cache */ @@ -2438,6 +2441,8 @@ static void __split_huge_page(struct page *page, struct list_head *list, } ClearPageCompound(head); + spin_unlock(&pgdat->lru_lock); + /* Caller disabled irqs, so they are still disabled here */ split_page_owner(head, HPAGE_PMD_ORDER); @@ -2455,8 +2460,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, page_ref_add(head, 2); xa_unlock(&head->mapping->i_pages); } - - spin_unlock_irqrestore(&pgdat->lru_lock, flags); + local_irq_enable(); remap_page(head); @@ -2595,12 +2599,10 @@ bool can_split_huge_page(struct page *page, int *pextra_pins) int split_huge_page_to_list(struct page *page, struct list_head *list) { struct page *head = compound_head(page); - struct pglist_data *pgdata = NODE_DATA(page_to_nid(head)); struct deferred_split *ds_queue = get_deferred_split_queue(head); struct anon_vma *anon_vma = NULL; struct address_space *mapping = NULL; int count, mapcount, extra_pins, ret; - unsigned long flags; pgoff_t end; VM_BUG_ON_PAGE(is_huge_zero_page(head), head); @@ -2661,9 +2663,8 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) unmap_page(head); VM_BUG_ON_PAGE(compound_mapcount(head), head); - /* prevent PageLRU to go away from under us, and freeze lru stats */ - spin_lock_irqsave(&pgdata->lru_lock, flags); - + /* block interrupt reentry in xa_lock and spinlock */ + local_irq_disable(); if (mapping) { XA_STATE(xas, &mapping->i_pages, page_index(head)); @@ -2693,7 +2694,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) __dec_node_page_state(head, NR_FILE_THPS); } - __split_huge_page(page, list, end, flags); + __split_huge_page(page, list, end); if (PageSwapCache(head)) { swp_entry_t entry = { .val = page_private(head) }; @@ -2712,7 +2713,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) spin_unlock(&ds_queue->split_queue_lock); fail: if (mapping) xa_unlock(&mapping->i_pages); - spin_unlock_irqrestore(&pgdata->lru_lock, flags); + local_irq_enable(); remap_page(head); ret = -EBUSY; }