On Fri, Jul 25, 2014 at 11:43 PM, Max Filippov <jcmvbkbc@xxxxxxxxx> wrote:
> VIPT cache with way size larger than MMU page size may suffer from
> aliasing problem: a single physical address accessed via different
> virtual addresses may end up in multiple locations in the cache.
> Virtual mappings of a physical address that always get cached in
> different cache locations are said to have different colors.
> L1 caching hardware usually doesn't handle this situation leaving it
> up to software. Software must avoid this situation as it leads to
> data corruption.
>
> One way to handle this is to flush and invalidate data cache every time
> page mapping changes color. The other way is to always map physical page
> at a virtual address with the same color. Low memory pages already have
> this property. Giving architecture a way to control color of high memory
> page mapping allows reusing of existing low memory cache alias handling
> code.
>
> Provide hooks that allow architectures with aliasing cache to align
> mapping address of high pages according to their color. Such architectures
> may enforce similar coloring of low- and high-memory page mappings and
> reuse existing cache management functions to support highmem.
>
> This code is based on the implementation of similar feature for MIPS by
> Leonid Yegoshin <Leonid.Yegoshin@xxxxxxxxxx>.
>
> Signed-off-by: Max Filippov <jcmvbkbc@xxxxxxxxx>
> ---
Ping? Is there anything else that can help making this patch better
and getting it merged?
> Changes v2->v3:
> - drop ARCH_PKMAP_COLORING, check gif et_pkmap_color is defined instead;
> - add comment stating that arch should place definitions into
> asm/highmem.h, include it directly to mm/highmem.c;
> - replace macros with inline functions, change set_pkmap_color to
> get_pkmap_color which better fits inline function model;
> - drop get_last_pkmap_nr;
> - replace get_next_pkmap_counter with get_pkmap_entries_count, leave
> original counting code;
> - introduce get_pkmap_wait_queue_head and make sleeping/waking dependent
> on mapping color;
> - move file-scope static variables last_pkmap_nr and pkmap_map_wait into
> get_next_pkmap_nr and get_pkmap_wait_queue_head respectively;
> - document new functions;
> - expand patch description and change authorship.
>
> Changes v1->v2:
> - define set_pkmap_color(pg, cl) as do { } while (0) instead of /* */;
> - rename is_no_more_pkmaps to no_more_pkmaps;
> - change 'if (count > 0)' to 'if (count)' to better match the original
> code behavior;
>
> mm/highmem.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 78 insertions(+), 11 deletions(-)
>
> diff --git a/mm/highmem.c b/mm/highmem.c
> index b32b70c..0d0cbbb 100644
> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -28,6 +28,9 @@
> #include <linux/highmem.h>
> #include <linux/kgdb.h>
> #include <asm/tlbflush.h>
> +#ifdef CONFIG_HIGHMEM
> +#include <asm/highmem.h>
> +#endif
>
>
> #if defined(CONFIG_HIGHMEM) || defined(CONFIG_X86_32)
> @@ -44,6 +47,66 @@ DEFINE_PER_CPU(int, __kmap_atomic_idx);
> */
> #ifdef CONFIG_HIGHMEM
>
> +/*
> + * Architecture with aliasing data cache may define the following family of
> + * helper functions in its asm/highmem.h to control cache color of virtual
> + * addresses where physical memory pages are mapped by kmap.
> + */
> +#ifndef get_pkmap_color
> +
> +/*
> + * Determine color of virtual address where the page should be mapped.
> + */
> +static inline unsigned int get_pkmap_color(struct page *page)
> +{
> + return 0;
> +}
> +#define get_pkmap_color get_pkmap_color
> +
> +/*
> + * Get next index for mapping inside PKMAP region for page with given color.
> + */
> +static inline unsigned int get_next_pkmap_nr(unsigned int color)
> +{
> + static unsigned int last_pkmap_nr;
> +
> + last_pkmap_nr = (last_pkmap_nr + 1) & LAST_PKMAP_MASK;
> + return last_pkmap_nr;
> +}
> +
> +/*
> + * Determine if page index inside PKMAP region (pkmap_nr) of given color
> + * has wrapped around PKMAP region end. When this happens an attempt to
> + * flush all unused PKMAP slots is made.
> + */
> +static inline int no_more_pkmaps(unsigned int pkmap_nr, unsigned int color)
> +{
> + return pkmap_nr == 0;
> +}
> +
> +/*
> + * Get the number of PKMAP entries of the given color. If no free slot is
> + * found after checking that much entries, kmap will sleep waiting for
> + * someone to call kunmap and free PKMAP slot.
> + */
> +static inline int get_pkmap_entries_count(unsigned int color)
> +{
> + return LAST_PKMAP;
> +}
> +
> +/*
> + * Get head of a wait queue for PKMAP entries of the given color.
> + * Wait queues for different mapping colors should be independent to avoid
> + * unnecessary wakeups caused by freeing of slots of other colors.
> + */
> +static inline wait_queue_head_t *get_pkmap_wait_queue_head(unsigned int color)
> +{
> + static DECLARE_WAIT_QUEUE_HEAD(pkmap_map_wait);
> +
> + return &pkmap_map_wait;
> +}
> +#endif
> +
> unsigned long totalhigh_pages __read_mostly;
> EXPORT_SYMBOL(totalhigh_pages);
>
> @@ -68,13 +131,10 @@ unsigned int nr_free_highpages (void)
> }
>
> static int pkmap_count[LAST_PKMAP];
> -static unsigned int last_pkmap_nr;
> static __cacheline_aligned_in_smp DEFINE_SPINLOCK(kmap_lock);
>
> pte_t * pkmap_page_table;
>
> -static DECLARE_WAIT_QUEUE_HEAD(pkmap_map_wait);
> -
> /*
> * Most architectures have no use for kmap_high_get(), so let's abstract
> * the disabling of IRQ out of the locking in that case to save on a
> @@ -161,15 +221,17 @@ static inline unsigned long map_new_virtual(struct page *page)
> {
> unsigned long vaddr;
> int count;
> + unsigned int last_pkmap_nr;
> + unsigned int color = get_pkmap_color(page);
>
> start:
> - count = LAST_PKMAP;
> + count = get_pkmap_entries_count(color);
> /* Find an empty entry */
> for (;;) {
> - last_pkmap_nr = (last_pkmap_nr + 1) & LAST_PKMAP_MASK;
> - if (!last_pkmap_nr) {
> + last_pkmap_nr = get_next_pkmap_nr(color);
> + if (no_more_pkmaps(last_pkmap_nr, color)) {
> flush_all_zero_pkmaps();
> - count = LAST_PKMAP;
> + count = get_pkmap_entries_count(color);
> }
> if (!pkmap_count[last_pkmap_nr])
> break; /* Found a usable entry */
> @@ -181,12 +243,14 @@ start:
> */
> {
> DECLARE_WAITQUEUE(wait, current);
> + wait_queue_head_t *pkmap_map_wait =
> + get_pkmap_wait_queue_head(color);
>
> __set_current_state(TASK_UNINTERRUPTIBLE);
> - add_wait_queue(&pkmap_map_wait, &wait);
> + add_wait_queue(pkmap_map_wait, &wait);
> unlock_kmap();
> schedule();
> - remove_wait_queue(&pkmap_map_wait, &wait);
> + remove_wait_queue(pkmap_map_wait, &wait);
> lock_kmap();
>
> /* Somebody else might have mapped it while we slept */
> @@ -274,6 +338,8 @@ void kunmap_high(struct page *page)
> unsigned long nr;
> unsigned long flags;
> int need_wakeup;
> + unsigned int color = get_pkmap_color(page);
> + wait_queue_head_t *pkmap_map_wait;
>
> lock_kmap_any(flags);
> vaddr = (unsigned long)page_address(page);
> @@ -299,13 +365,14 @@ void kunmap_high(struct page *page)
> * no need for the wait-queue-head's lock. Simply
> * test if the queue is empty.
> */
> - need_wakeup = waitqueue_active(&pkmap_map_wait);
> + pkmap_map_wait = get_pkmap_wait_queue_head(color);
> + need_wakeup = waitqueue_active(pkmap_map_wait);
> }
> unlock_kmap_any(flags);
>
> /* do wake-up, if needed, race-free outside of the spin lock */
> if (need_wakeup)
> - wake_up(&pkmap_map_wait);
> + wake_up(pkmap_map_wait);
> }
>
> EXPORT_SYMBOL(kunmap_high);
> --
> 1.8.1.4
>
--
Thanks.
-- Max
