Re: [PATCH v3 1/2] mm/highmem: make kmap cache coloring aware

[Max Filippov]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/