From: Hugh Dickins <hughd@google.com> To: Christoph Hellwig <hch@lst.de> Cc: Linus Torvalds <torvalds@linux-foundation.org>, Andrew Morton <akpm@linux-foundation.org>, Dan Williams <dan.j.williams@intel.com>, Eric Dumazet <edumazet@google.com>, Hugh Dickins <hughd@google.com>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: [PATCH] dma-debug: fix debug_dma_assert_idle(), use rcu_read_lock() Date: Wed, 12 Aug 2020 20:17:00 -0700 (PDT) [thread overview] Message-ID: <alpine.LSU.2.11.2008122005240.11996@eggly.anvils> (raw) Since commit 2a9127fcf229 ("mm: rewrite wait_on_page_bit_common() logic") improved unlock_page(), it has become more noticeable how cow_user_page() in a kernel with CONFIG_DMA_API_DEBUG=y can create and suffer from heavy contention on DMA debug's radix_lock in debug_dma_assert_idle(). It is only doing a lookup: use rcu_read_lock() and rcu_read_unlock() instead; though that does require the static ents[] to be moved onstack... ...but, hold on, isn't that radix_tree_gang_lookup() and loop doing quite the wrong thing: searching CACHELINES_PER_PAGE entries for an exact match with the first cacheline of the page in question? radix_tree_gang_lookup() is the right tool for the job, but we need nothing more than to check the first entry it can find, reporting if that falls anywhere within the page. (Is RCU safe here? As safe as using the spinlock was. The entries are never freed, so don't need to be freed by RCU. They may be reused, and there is a faint chance of a race, with an offending entry reused while printing its error info; but the spinlock did not prevent that either, and I agree that it's not worth worrying about.) Fixes: 3b7a6418c749 ("dma debug: account for cachelines and read-only mappings in overlap tracking") Signed-off-by: Hugh Dickins <hughd@google.com> --- kernel/dma/debug.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) --- v5.9-rc/kernel/dma/debug.c 2020-08-05 18:17:57.544203766 -0700 +++ linux/kernel/dma/debug.c 2020-08-12 19:53:33.159070245 -0700 @@ -565,11 +565,8 @@ static void active_cacheline_remove(stru */ void debug_dma_assert_idle(struct page *page) { - static struct dma_debug_entry *ents[CACHELINES_PER_PAGE]; - struct dma_debug_entry *entry = NULL; - void **results = (void **) &ents; - unsigned int nents, i; - unsigned long flags; + struct dma_debug_entry *entry; + unsigned long pfn; phys_addr_t cln; if (dma_debug_disabled()) @@ -578,20 +575,14 @@ void debug_dma_assert_idle(struct page * if (!page) return; - cln = (phys_addr_t) page_to_pfn(page) << CACHELINE_PER_PAGE_SHIFT; - spin_lock_irqsave(&radix_lock, flags); - nents = radix_tree_gang_lookup(&dma_active_cacheline, results, cln, - CACHELINES_PER_PAGE); - for (i = 0; i < nents; i++) { - phys_addr_t ent_cln = to_cacheline_number(ents[i]); + pfn = page_to_pfn(page); + cln = (phys_addr_t) pfn << CACHELINE_PER_PAGE_SHIFT; - if (ent_cln == cln) { - entry = ents[i]; - break; - } else if (ent_cln >= cln + CACHELINES_PER_PAGE) - break; - } - spin_unlock_irqrestore(&radix_lock, flags); + rcu_read_lock(); + if (!radix_tree_gang_lookup(&dma_active_cacheline, (void **) &entry, + cln, 1) || entry->pfn != pfn) + entry = NULL; + rcu_read_unlock(); if (!entry) return;
WARNING: multiple messages have this Message-ID (diff)
From: Hugh Dickins via iommu <iommu@lists.linux-foundation.org> To: Christoph Hellwig <hch@lst.de> Cc: Hugh Dickins <hughd@google.com>, Eric Dumazet <edumazet@google.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, iommu@lists.linux-foundation.org, Andrew Morton <akpm@linux-foundation.org>, Linus Torvalds <torvalds@linux-foundation.org>, Dan Williams <dan.j.williams@intel.com> Subject: [PATCH] dma-debug: fix debug_dma_assert_idle(), use rcu_read_lock() Date: Wed, 12 Aug 2020 20:17:00 -0700 (PDT) [thread overview] Message-ID: <alpine.LSU.2.11.2008122005240.11996@eggly.anvils> (raw) Since commit 2a9127fcf229 ("mm: rewrite wait_on_page_bit_common() logic") improved unlock_page(), it has become more noticeable how cow_user_page() in a kernel with CONFIG_DMA_API_DEBUG=y can create and suffer from heavy contention on DMA debug's radix_lock in debug_dma_assert_idle(). It is only doing a lookup: use rcu_read_lock() and rcu_read_unlock() instead; though that does require the static ents[] to be moved onstack... ...but, hold on, isn't that radix_tree_gang_lookup() and loop doing quite the wrong thing: searching CACHELINES_PER_PAGE entries for an exact match with the first cacheline of the page in question? radix_tree_gang_lookup() is the right tool for the job, but we need nothing more than to check the first entry it can find, reporting if that falls anywhere within the page. (Is RCU safe here? As safe as using the spinlock was. The entries are never freed, so don't need to be freed by RCU. They may be reused, and there is a faint chance of a race, with an offending entry reused while printing its error info; but the spinlock did not prevent that either, and I agree that it's not worth worrying about.) Fixes: 3b7a6418c749 ("dma debug: account for cachelines and read-only mappings in overlap tracking") Signed-off-by: Hugh Dickins <hughd@google.com> --- kernel/dma/debug.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) --- v5.9-rc/kernel/dma/debug.c 2020-08-05 18:17:57.544203766 -0700 +++ linux/kernel/dma/debug.c 2020-08-12 19:53:33.159070245 -0700 @@ -565,11 +565,8 @@ static void active_cacheline_remove(stru */ void debug_dma_assert_idle(struct page *page) { - static struct dma_debug_entry *ents[CACHELINES_PER_PAGE]; - struct dma_debug_entry *entry = NULL; - void **results = (void **) &ents; - unsigned int nents, i; - unsigned long flags; + struct dma_debug_entry *entry; + unsigned long pfn; phys_addr_t cln; if (dma_debug_disabled()) @@ -578,20 +575,14 @@ void debug_dma_assert_idle(struct page * if (!page) return; - cln = (phys_addr_t) page_to_pfn(page) << CACHELINE_PER_PAGE_SHIFT; - spin_lock_irqsave(&radix_lock, flags); - nents = radix_tree_gang_lookup(&dma_active_cacheline, results, cln, - CACHELINES_PER_PAGE); - for (i = 0; i < nents; i++) { - phys_addr_t ent_cln = to_cacheline_number(ents[i]); + pfn = page_to_pfn(page); + cln = (phys_addr_t) pfn << CACHELINE_PER_PAGE_SHIFT; - if (ent_cln == cln) { - entry = ents[i]; - break; - } else if (ent_cln >= cln + CACHELINES_PER_PAGE) - break; - } - spin_unlock_irqrestore(&radix_lock, flags); + rcu_read_lock(); + if (!radix_tree_gang_lookup(&dma_active_cacheline, (void **) &entry, + cln, 1) || entry->pfn != pfn) + entry = NULL; + rcu_read_unlock(); if (!entry) return; _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next reply other threads:[~2020-08-13 3:17 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-13 3:17 Hugh Dickins [this message] 2020-08-13 3:17 ` [PATCH] dma-debug: fix debug_dma_assert_idle(), use rcu_read_lock() Hugh Dickins via iommu 2020-08-13 3:17 ` Hugh Dickins 2020-08-13 19:02 ` Linus Torvalds 2020-08-13 19:02 ` Linus Torvalds 2020-08-13 19:02 ` Linus Torvalds 2020-08-13 23:37 ` Dan Williams 2020-08-13 23:37 ` Dan Williams 2020-08-13 23:37 ` Dan Williams 2020-08-14 5:42 ` Christoph Hellwig 2020-08-14 5:42 ` Christoph Hellwig 2020-08-14 22:40 ` Linus Torvalds 2020-08-14 22:40 ` Linus Torvalds 2020-08-14 22:40 ` Linus Torvalds 2020-08-15 0:26 ` Hugh Dickins 2020-08-15 0:26 ` Hugh Dickins via iommu 2020-08-15 0:26 ` Hugh Dickins 2020-08-15 0:59 ` Linus Torvalds 2020-08-15 0:59 ` Linus Torvalds 2020-08-15 0:59 ` Linus Torvalds
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.LSU.2.11.2008122005240.11996@eggly.anvils \ --to=hughd@google.com \ --cc=akpm@linux-foundation.org \ --cc=dan.j.williams@intel.com \ --cc=edumazet@google.com \ --cc=hch@lst.de \ --cc=iommu@lists.linux-foundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.