[PATCH 1/3] mm: add comment for __mod_zone_page_stat

[PATCH 1/3] mm: add comment for __mod_zone_page_stat

[Jianyu Zhan]

__mod_zone_page_stat() is not irq-safe, so it should be used carefully.
And it is not appropirately documented now. This patch adds comment for
it, and also documents for some of its call sites.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 mm/page_alloc.c |  2 ++
 mm/rmap.c       |  6 ++++++
 mm/vmstat.c     | 16 +++++++++++++++-
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5dba293..9d6f474 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -659,6 +659,8 @@ static inline int free_pages_check(struct page *page)
  *
  * And clear the zone's pages_scanned counter, to hold off the "all pages are
  * pinned" detection logic.
+ *
+ * Note: this function should be used with irq disabled.
  */
 static void free_pcppages_bulk(struct zone *zone, int count,
 					struct per_cpu_pages *pcp)
diff --git a/mm/rmap.c b/mm/rmap.c
index 9c3e773..6078a30 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -979,6 +979,8 @@ void page_add_anon_rmap(struct page *page,
 /*
  * Special version of the above for do_swap_page, which often runs
  * into pages that are exclusively owned by the current process.
+ * So we could use the irq-unsafe version __{inc|mod}_zone_page_stat
+ * here without others racing change it in between.
  * Everybody else should continue to use page_add_anon_rmap above.
  */
 void do_page_add_anon_rmap(struct page *page,
@@ -1077,6 +1079,10 @@ void page_remove_rmap(struct page *page)
 	/*
 	 * Hugepages are not counted in NR_ANON_PAGES nor NR_FILE_MAPPED
 	 * and not charged by memcg for now.
+	 *
+	 * And we are the last user of this page, so it is safe to use
+	 * the irq-unsafe version __{mod|dec}_zone_page here, since we
+	 * have no racer.
 	 */
 	if (unlikely(PageHuge(page)))
 		goto out;
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 302dd07..778f154 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -207,7 +207,21 @@ void set_pgdat_percpu_threshold(pg_data_t *pgdat,
 }
 
 /*
- * For use when we know that interrupts are disabled.
+ * Optimized modificatoin function.
+ *
+ * The code basically does the modification in two steps:
+ *
+ *  1. read the current counter based on the processor number
+ *  2. modificate the counter write it back.
+ *
+ * So this function should be used with the guarantee that
+ *
+ *  1. interrupts are disabled, or
+ *  2. interrupts are enabled, but no other sites would race to
+ *     modify this counter in between.
+ *
+ * Otherwise, an irq-safe version mod_zone_page_state() should
+ * be used instead.
  */
 void __mod_zone_page_state(struct zone *zone, enum zone_stat_item item,
 				int delta)
-- 
2.0.0-rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/3] mm: use a light-weight __mod_zone_page_state in mlocked_vma_newpage()

[Jianyu Zhan]

mlocked_vma_newpage() is only called in fault path by
page_add_new_anon_rmap(), which is called on a *new* page.
And such page is initially only visible via the pagetables, and the
pte is locked while calling page_add_new_anon_rmap(), so we need not
use an irq-safe mod_zone_page_state() here, using a light-weight version
__mod_zone_page_state() would be OK.

And as suggested by Andrew, to reduce the risks that new call sites
incorrectly using mlocked_vma_newpage() without knowing they are adding
racing, this patch also moves it from internal.h to right before its only
call site page_add_new_anon_rmap() in rmap.c, with detailed document added.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 mm/internal.h | 22 ++--------------------
 mm/rmap.c     | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 07b6736..20abafb 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -183,26 +183,8 @@ static inline void munlock_vma_pages_all(struct vm_area_struct *vma)
 	munlock_vma_pages_range(vma, vma->vm_start, vma->vm_end);
 }
 
-/*
- * Called only in fault path, to determine if a new page is being
- * mapped into a LOCKED vma.  If it is, mark page as mlocked.
- */
-static inline int mlocked_vma_newpage(struct vm_area_struct *vma,
-				    struct page *page)
-{
-	VM_BUG_ON_PAGE(PageLRU(page), page);
-
-	if (likely((vma->vm_flags & (VM_LOCKED | VM_SPECIAL)) != VM_LOCKED))
-		return 0;
-
-	if (!TestSetPageMlocked(page)) {
-		mod_zone_page_state(page_zone(page), NR_MLOCK,
-				    hpage_nr_pages(page));
-		count_vm_event(UNEVICTABLE_PGMLOCKED);
-	}
-	return 1;
-}
-
+extern int mlocked_vma_newpage(struct vm_area_struct *vma,
+				struct page *page);
 /*
  * must be called with vma's mmap_sem held for read or write, and page locked.
  */
diff --git a/mm/rmap.c b/mm/rmap.c
index 6078a30..a9d02ef 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1005,6 +1005,30 @@ void do_page_add_anon_rmap(struct page *page,
 		__page_check_anon_rmap(page, vma, address);
 }
 
+/*
+ * Called only in fault path, to determine if a new page is being
+ * mapped into a LOCKED vma.  If it is, mark page as mlocked.
+ * This function is only called in fault path by
+ * page_add_new_anon_rmap(), which is called on a *new* page.
+ * And such page is initially only visible via the pagetables, and the
+ * pte is locked while calling page_add_new_anon_rmap(), so using a
+ * light-weight version __mod_zone_page_state() would be OK.
+ */
+int mlocked_vma_newpage(struct vm_area_struct *vma,
+					struct page *page)
+{
+	VM_BUG_ON_PAGE(PageLRU(page), page);
+	if (likely((vma->vm_flags & (VM_LOCKED | VM_SPECIAL)) != VM_LOCKED))
+		return 0;
+
+	if (!TestSetPageMlocked(page)) {
+		__mod_zone_page_state(page_zone(page), NR_MLOCK,
+					hpage_nr_pages(page));
+		count_vm_event(UNEVICTABLE_PGMLOCKED);
+	}
+	return 1;
+}
+
 /**
  * page_add_new_anon_rmap - add pte mapping to a new anonymous page
  * @page:	the page to add the mapping to
-- 
2.0.0-rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/3] mm: use a light-weight __mod_zone_page_state in mlocked_vma_newpage()

[Hugh Dickins]

On Sat, 10 May 2014, Jianyu Zhan wrote:

> mlocked_vma_newpage() is only called in fault path by
> page_add_new_anon_rmap(), which is called on a *new* page.
> And such page is initially only visible via the pagetables, and the
> pte is locked while calling page_add_new_anon_rmap(), so we need not
> use an irq-safe mod_zone_page_state() here, using a light-weight version
> __mod_zone_page_state() would be OK.
> 
> And as suggested by Andrew, to reduce the risks that new call sites
> incorrectly using mlocked_vma_newpage() without knowing they are adding
> racing, this patch also moves it from internal.h to right before its only
> call site page_add_new_anon_rmap() in rmap.c, with detailed document added.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>

I completely agree with Andrew's suggestion that you move the code
from mm/internal.h to its sole callsite in mm/rmap.c; but I much
prefer his "probably better ... open-coding its logic into
page_add_new_anon_rmap()".

That saves you from having to dream up a satisfactory alternative name,
and a lengthy comment, and let's everybody see just what's going on.

The function-in-internal.h thing dates from an interim in which,
running short of page flags, we were not confident that we wanted
to dedicate one to PageMlocked: not all configs had it and internal.h
was somewhere to hide the #ifdefs.  Well, PageMlocked is there only
when CONFIG_MMU, but mm/rmap.c is only built for CONFIG_MMU anyway.

> ---
>  mm/internal.h | 22 ++--------------------
>  mm/rmap.c     | 24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 07b6736..20abafb 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -183,26 +183,8 @@ static inline void munlock_vma_pages_all(struct vm_area_struct *vma)
>  	munlock_vma_pages_range(vma, vma->vm_start, vma->vm_end);
>  }
>  
> -/*
> - * Called only in fault path, to determine if a new page is being
> - * mapped into a LOCKED vma.  If it is, mark page as mlocked.
> - */
> -static inline int mlocked_vma_newpage(struct vm_area_struct *vma,
> -				    struct page *page)
> -{
> -	VM_BUG_ON_PAGE(PageLRU(page), page);
> -
> -	if (likely((vma->vm_flags & (VM_LOCKED | VM_SPECIAL)) != VM_LOCKED))
> -		return 0;
> -
> -	if (!TestSetPageMlocked(page)) {
> -		mod_zone_page_state(page_zone(page), NR_MLOCK,

So there we see mod_zone_page_state...

> -				    hpage_nr_pages(page));
> -		count_vm_event(UNEVICTABLE_PGMLOCKED);
> -	}
> -	return 1;
> -}
> -
> +extern int mlocked_vma_newpage(struct vm_area_struct *vma,
> +				struct page *page);

Why are you adding an extern declaration for it, when it's only
used from the one source file to which you are moving it?

Why are you not removing the !CONFIG_MMU declaration?

>  /*
>   * must be called with vma's mmap_sem held for read or write, and page locked.
>   */
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 6078a30..a9d02ef 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1005,6 +1005,30 @@ void do_page_add_anon_rmap(struct page *page,
>  		__page_check_anon_rmap(page, vma, address);
>  }
>  
> +/*
> + * Called only in fault path, to determine if a new page is being
> + * mapped into a LOCKED vma.  If it is, mark page as mlocked.
> + * This function is only called in fault path by
> + * page_add_new_anon_rmap(), which is called on a *new* page.
> + * And such page is initially only visible via the pagetables, and the
> + * pte is locked while calling page_add_new_anon_rmap(), so using a
> + * light-weight version __mod_zone_page_state() would be OK.

No.  See my remarks on 1/3, that's not it at all: it's that we do
not update the NR_MLOCK count from interrupt context (I hope: check).

> + */
> +int mlocked_vma_newpage(struct vm_area_struct *vma,

I would say static, except you should just be bringing the code
into its callsite.

> +					struct page *page)
> +{
> +	VM_BUG_ON_PAGE(PageLRU(page), page);
> +	if (likely((vma->vm_flags & (VM_LOCKED | VM_SPECIAL)) != VM_LOCKED))
> +		return 0;
> +
> +	if (!TestSetPageMlocked(page)) {
> +		__mod_zone_page_state(page_zone(page), NR_MLOCK,

And here appears __mod_zone_page_state: you have a patch for moving
a function from one place to another, and  buried within that movement
you hide a "signficant" change.  No, don't do that.

Hugh

> +					hpage_nr_pages(page));
> +		count_vm_event(UNEVICTABLE_PGMLOCKED);
> +	}
> +	return 1;
> +}
> +
>  /**
>   * page_add_new_anon_rmap - add pte mapping to a new anonymous page
>   * @page:	the page to add the mapping to
> -- 
> 2.0.0-rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/3] mm: rename mlocked_vma_newpage to newpage_in_mlocked_vma

[Jianyu Zhan]

mlocked_vma_newpage is used to determine if a new page is mapped into
a *mlocked* vma. It is poorly named, so rename it to newpage_in_mlocked_vma.

Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 mm/internal.h | 4 ++--
 mm/rmap.c     | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 20abafb..35efd79 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -183,7 +183,7 @@ static inline void munlock_vma_pages_all(struct vm_area_struct *vma)
 	munlock_vma_pages_range(vma, vma->vm_start, vma->vm_end);
 }
 
-extern int mlocked_vma_newpage(struct vm_area_struct *vma,
+extern int newpage_in_mlocked_vma(struct vm_area_struct *vma,
 				struct page *page);
 /*
  * must be called with vma's mmap_sem held for read or write, and page locked.
@@ -227,7 +227,7 @@ extern unsigned long vma_address(struct page *page,
 				 struct vm_area_struct *vma);
 #endif
 #else /* !CONFIG_MMU */
-static inline int mlocked_vma_newpage(struct vm_area_struct *v, struct page *p)
+static inline int newpage_in_mlocked_vma(struct vm_area_struct *v, struct page *p)
 {
 	return 0;
 }
diff --git a/mm/rmap.c b/mm/rmap.c
index a9d02ef..9ff6915 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1014,7 +1014,7 @@ void do_page_add_anon_rmap(struct page *page,
  * pte is locked while calling page_add_new_anon_rmap(), so using a
  * light-weight version __mod_zone_page_state() would be OK.
  */
-int mlocked_vma_newpage(struct vm_area_struct *vma,
+int newpage_in_mlocked_vma(struct vm_area_struct *vma,
 					struct page *page)
 {
 	VM_BUG_ON_PAGE(PageLRU(page), page);
@@ -1050,7 +1050,7 @@ void page_add_new_anon_rmap(struct page *page,
 	__mod_zone_page_state(page_zone(page), NR_ANON_PAGES,
 			hpage_nr_pages(page));
 	__page_set_anon_rmap(page, vma, address, 1);
-	if (!mlocked_vma_newpage(vma, page)) {
+	if (!newpage_in_mlocked_vma(vma, page)) {
 		SetPageActive(page);
 		lru_cache_add(page);
 	} else
-- 
2.0.0-rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3] mm: rename mlocked_vma_newpage to newpage_in_mlocked_vma

[Hugh Dickins]

On Sat, 10 May 2014, Jianyu Zhan wrote:

> mlocked_vma_newpage is used to determine if a new page is mapped into
> a *mlocked* vma. It is poorly named, so rename it to newpage_in_mlocked_vma.
> 
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>

newpage_in_mlocked_vma() is not as bad a name as mlocked_vma_newpage(),
but it's not especially good either (sounds like it answers a question,
not like it actually does something).  Please just put it into its
sole caller and we don't need an obscuring name at all.

> ---
>  mm/internal.h | 4 ++--
>  mm/rmap.c     | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 20abafb..35efd79 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -183,7 +183,7 @@ static inline void munlock_vma_pages_all(struct vm_area_struct *vma)
>  	munlock_vma_pages_range(vma, vma->vm_start, vma->vm_end);
>  }
>  
> -extern int mlocked_vma_newpage(struct vm_area_struct *vma,
> +extern int newpage_in_mlocked_vma(struct vm_area_struct *vma,
>  				struct page *page);

This should have vanished in the previous patch.

>  /*
>   * must be called with vma's mmap_sem held for read or write, and page locked.
> @@ -227,7 +227,7 @@ extern unsigned long vma_address(struct page *page,
>  				 struct vm_area_struct *vma);
>  #endif
>  #else /* !CONFIG_MMU */
> -static inline int mlocked_vma_newpage(struct vm_area_struct *v, struct page *p)
> +static inline int newpage_in_mlocked_vma(struct vm_area_struct *v, struct page *p)

Ditto.

Hugh

>  {
>  	return 0;
>  }
> diff --git a/mm/rmap.c b/mm/rmap.c
> index a9d02ef..9ff6915 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1014,7 +1014,7 @@ void do_page_add_anon_rmap(struct page *page,
>   * pte is locked while calling page_add_new_anon_rmap(), so using a
>   * light-weight version __mod_zone_page_state() would be OK.
>   */
> -int mlocked_vma_newpage(struct vm_area_struct *vma,
> +int newpage_in_mlocked_vma(struct vm_area_struct *vma,
>  					struct page *page)
>  {
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
> @@ -1050,7 +1050,7 @@ void page_add_new_anon_rmap(struct page *page,
>  	__mod_zone_page_state(page_zone(page), NR_ANON_PAGES,
>  			hpage_nr_pages(page));
>  	__page_set_anon_rmap(page, vma, address, 1);
> -	if (!mlocked_vma_newpage(vma, page)) {
> +	if (!newpage_in_mlocked_vma(vma, page)) {
>  		SetPageActive(page);
>  		lru_cache_add(page);
>  	} else
> -- 
> 2.0.0-rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/3] mm: add comment for __mod_zone_page_stat

[Hugh Dickins]

On Sat, 10 May 2014, Jianyu Zhan wrote:

> __mod_zone_page_stat() is not irq-safe, so it should be used carefully.
> And it is not appropirately documented now. This patch adds comment for
> it, and also documents for some of its call sites.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>

Your original __mod_zone_page_state happened to be correct;
but you have no understanding of why it was correct, so its
comment was very wrong, even after you changed the irq wording.

This series just propagates your misunderstanding further,
while providing an object lesson in how not to present a series.

Sorry, you have quickly developed an unenviable reputation for
patches which waste developers' time: please consider your
patches much more carefully before posting them.

> ---
>  mm/page_alloc.c |  2 ++
>  mm/rmap.c       |  6 ++++++
>  mm/vmstat.c     | 16 +++++++++++++++-
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5dba293..9d6f474 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -659,6 +659,8 @@ static inline int free_pages_check(struct page *page)
>   *
>   * And clear the zone's pages_scanned counter, to hold off the "all pages are
>   * pinned" detection logic.
> + *
> + * Note: this function should be used with irq disabled.

Correct, but I don't see that that needed saying.  This is a static
function which is being used as intended: just because it matched your
search for "__mod_zone_page_state" is not a reason for you to add that
comment; irq disabled is only one of its prerequisites.

>   */
>  static void free_pcppages_bulk(struct zone *zone, int count,
>  					struct per_cpu_pages *pcp)
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9c3e773..6078a30 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -979,6 +979,8 @@ void page_add_anon_rmap(struct page *page,
>  /*
>   * Special version of the above for do_swap_page, which often runs
>   * into pages that are exclusively owned by the current process.
> + * So we could use the irq-unsafe version __{inc|mod}_zone_page_stat
> + * here without others racing change it in between.

And yet you can immediately see them being used without any test
for "exclusive" below: why is that?  Think about it.

>   * Everybody else should continue to use page_add_anon_rmap above.
>   */
>  void do_page_add_anon_rmap(struct page *page,
> @@ -1077,6 +1079,10 @@ void page_remove_rmap(struct page *page)
>  	/*
>  	 * Hugepages are not counted in NR_ANON_PAGES nor NR_FILE_MAPPED
>  	 * and not charged by memcg for now.
> +	 *
> +	 * And we are the last user of this page, so it is safe to use
> +	 * the irq-unsafe version __{mod|dec}_zone_page here, since we
> +	 * have no racer.

Again, the code is correct to be using the irq-unsafe version, but your
comment is doubly wrong.

We are not necessarily the last user of this page, merely the one that
just now brought the mapcount down to 0.

But think: what bearing would being the last user of this page have on
the safety of using __mod_zone_page_state to adjust per-zone counters?

None at all.  A page does not move from one zone to another (though its
contents might be migrated from one page to another when safe to do so).

Once upon a time, from 2.6.16 to 2.6.32, there was indeed a relevant
and helpful comment in __page_set_anon_rmap():
	/*
	 * nr_mapped state can be updated without turning off
	 * interrupts because it is not modified via interrupt.
	 */
	__inc_page_state(nr_mapped);

The comment survived the replacement of nr_mapped, but eventually
it got cleaned away completely.

It is safe to use the irq-unsafe __mod_zone_page_stat on counters
which are never modified via interrupt.

>  	 */
>  	if (unlikely(PageHuge(page)))
>  		goto out;
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 302dd07..778f154 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -207,7 +207,21 @@ void set_pgdat_percpu_threshold(pg_data_t *pgdat,
>  }
>  
>  /*
> - * For use when we know that interrupts are disabled.
> + * Optimized modificatoin function.
> + *
> + * The code basically does the modification in two steps:
> + *
> + *  1. read the current counter based on the processor number
> + *  2. modificate the counter write it back.
> + *
> + * So this function should be used with the guarantee that
> + *
> + *  1. interrupts are disabled, or
> + *  2. interrupts are enabled, but no other sites would race to
> + *     modify this counter in between.
> + *
> + * Otherwise, an irq-safe version mod_zone_page_state() should
> + * be used instead.

You are right that the comment is not good enough, but I don't trust
your version either.  Since percpu variables are involved, it's important
that preemption be disabled too (see comment above __inc_zone_state).

I'd prefer to let Christoph write the definitive version,
but my first stab at it would be:

/*
 * For use when we know that interrupts are disabled,
 * or when we know that preemption is disabled and that
 * particular counter cannot be updated from interrupt context.
 */

Hugh

>   */
>  void __mod_zone_page_state(struct zone *zone, enum zone_stat_item item,
>  				int delta)
> -- 
> 2.0.0-rc1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>