On Mon, 24 Jan 2011, saeed bishara wrote:

quoted

quoted

quoted

quoted

quoted

I've port 2.6.35 to SMP system that runs in V6 mode, this system
doesn't support TLB operations broadcasting by hw, so it uses IPI
messages for that. ?when enabling DEBUG_LOCKDEP, I got the following
error message while booting the system from NFS:

You've bypassed this check:

? ? ? ? ? ? ? ?if (is_smp() && tlb_ops_need_broadcast()) {
? ? ? ? ? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? ? ? ? ? * kmap_high needs to occasionally flush TLB entries,
? ? ? ? ? ? ? ? ? ? ? ? * however, if the TLB entries need to be broadcast
? ? ? ? ? ? ? ? ? ? ? ? * we may deadlock:
? ? ? ? ? ? ? ? ? ? ? ? * ?kmap_high(irqs off)->flush_all_zero_pkmaps->
? ? ? ? ? ? ? ? ? ? ? ? * ?flush_tlb_kernel_range->smp_call_function_many
? ? ? ? ? ? ? ? ? ? ? ? * ? (must not be called with irqs off)
? ? ? ? ? ? ? ? ? ? ? ? */
? ? ? ? ? ? ? ? ? ? ? ?reason = "without hardware TLB ops broadcasting";
? ? ? ? ? ? ? ?}

so you lose. ?There's reasons why such checks are put in. ?We can not
support SMP and highmem on systems which do not have TLB broadcasting.
That's not because the code doesn't support it, it's because there are
deadlocks which will occur.

thanks, I missed that

quoted

The fact is that it is unsafe to send IPIs with IRQs disabled, which
means you can't IPI a TLB operation and wait for it to complete with IRQs
disabled.

as I understand it, the lock_kmap() started to disable IRQs in order
to support the vivt and vipt caches, but in SMP (at least in my case),
the caches are PIPT, so I think I can do the following:
1. undef ?the ?ARCH_NEEDS_KMAP_HIGH_GET
2. use page_address instead of kmap_high_get()
do you think it will work?

Definitely not. ?We use kmap_high_get() so that we can ensure that we've
flushed data out of the PIPT cache for highmem pages. ?highmem pages
which are unmapped do not have a valid page_address() but may have PIPT
cache lines associated with them.

So no, I don't think it'll be safe.

ok, what about the following patch, the idea is to use only the
kmap_high_l1_vipt when doing cache maintenance.

This looks like it should work.

The reason for kmap_high_get() is to ensure that the currently kmap'd
page usage count does not decrease to zero while we're using its
existing virtual mapping in an atomic context. ?With a VIVT cache this
is essential to do, but with a VIPT cache this is only an optimization
so not to pay the price of establishing a second mapping if an existing
one can be used.

However your patch is ugly. ?I'd suggest the following instead:

diff --git a/arch/arm/include/asm/highmem.h b/arch/arm/include/asm/highmem.h
index 7080e2c..be535ad 100644
--- a/arch/arm/include/asm/highmem.h
+++ b/arch/arm/include/asm/highmem.h

@@ -19,13 +19,37 @@

?extern pte_t *pkmap_page_table;

-#define ARCH_NEEDS_KMAP_HIGH_GET
-
?extern void *kmap_high(struct page *page);
-extern void *kmap_high_get(struct page *page);
?extern void kunmap_high(struct page *page);

?/*
+ * The reason for kmap_high_get() is to ensure that the currently kmap'd
+ * page usage count does not decrease to zero while we're using its
+ * existing virtual mapping in an atomic context. ?With a VIVT cache this
+ * is essential to do, but with a VIPT cache this is only an optimization
+ * so not to pay the price of establishing a second mapping if an existing
+ * one can be used. ?However, on platforms without hardware TLB maintainence
+ * broadcast, we simply cannot use ARCH_NEEDS_KMAP_HIGH_GET at all since
+ * the locking involved must also disable IRQs which is incompatible with
+ * the IPI mechanism used by global TLB operations.
+ */
+#define ARCH_NEEDS_KMAP_HIGH_GET
+#if defined(CONFIG_SMP) && defined(CONFIG_CPU_TLB_V6)
+#undef ARCH_NEEDS_KMAP_HIGH_GET
+#if defined(CONFIG_HIGHMEM) && defined(CONFIG_CPU_CACHE_VIVT)
+#error "The sum of feature in your kernel config cannot be supported together"
+#endif

+
+#ifdef ARCH_NEEDS_KMAP_HIGH_GET
+extern void *kmap_high_get(struct page *page);
+#else
+static inline void *kmap_high_get(struct page *page)
+{
+ ? ? ? return NULL;
+}
+#endif
+
+/*
?* The following functions are already defined by <linux/highmem.h>
?* when CONFIG_HIGHMEM is not set.
?*/

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 3c67e92..ff7b43b 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c

@@ -827,16 +827,6 @@ static void __init sanity_check_meminfo(void)

? ? ? ? ? ? ? ? ? ? ? ? * rather difficult.
? ? ? ? ? ? ? ? ? ? ? ? */
? ? ? ? ? ? ? ? ? ? ? ?reason = "with VIPT aliasing cache";
- ? ? ? ? ? ? ? } else if (is_smp() && tlb_ops_need_broadcast()) {
- ? ? ? ? ? ? ? ? ? ? ? /*
- ? ? ? ? ? ? ? ? ? ? ? ?* kmap_high needs to occasionally flush TLB entries,
- ? ? ? ? ? ? ? ? ? ? ? ?* however, if the TLB entries need to be broadcast
- ? ? ? ? ? ? ? ? ? ? ? ?* we may deadlock:
- ? ? ? ? ? ? ? ? ? ? ? ?* ?kmap_high(irqs off)->flush_all_zero_pkmaps->
- ? ? ? ? ? ? ? ? ? ? ? ?* ?flush_tlb_kernel_range->smp_call_function_many
- ? ? ? ? ? ? ? ? ? ? ? ?* ? (must not be called with irqs off)
- ? ? ? ? ? ? ? ? ? ? ? ?*/
- ? ? ? ? ? ? ? ? ? ? ? reason = "without hardware TLB ops broadcasting";
? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?if (reason) {
? ? ? ? ? ? ? ? ? ? ? ?printk(KERN_CRIT "HIGHMEM is not supported %s, ignoring high memory\n",