Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-18 Thread Stanislav Kinsburskiy 



18.11.2015 14:46, Konstantin Khorenko пишет:

On 11/17/2015 06:31 PM, Stanislav Kinsburskiy wrote:



17.11.2015 16:08, Vladimir Davydov пишет:

On Tue, Nov 17, 2015 at 03:42:46PM +0100, Stanislav Kinsburskiy wrote:


17.11.2015 15:20, Vladimir Davydov пишет:
On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy 
wrote:



@@ -0,0 +1,23 @@
+#ifndef _ASM_X86_FREE_PAGE_H
+#define _ASM_X86_FREE_PAGE_H
+
+#ifdef __KERNEL__
+
+#ifndef __ASSEMBLY__
+
+#include 
+
+#define HAVE_ARCH_FREE_PAGE
+
+extern struct static_key zero_free_pages;
+extern void do_zero_pages(struct page *page, int order);
+
+static __always_inline void arch_free_page(struct page *page, 
int order)

+{
+if (static_key_false(_free_pages))
+do_zero_pages(page, order);
+}
There is no point in making this feature arch-dependant now, as 
you use

jump labels, which are arch-independent. The only reason why I had to
overwrite arch_free_page in PCS6 is that I had to write some asm 
code to
implement functionality close to that provided by jump labels, 
which are

absent in RH6-based kernels. Please move it to mm/page_alloc.c.

Thanks.

If we will make this function generic, then we have to declare
"zero_free_pages" as generic (which is more or less ok)  and define
do_zero_pages for the case, when HAVE_ARCH_FREE_PAGE is not 
defined. Does

the goal worth it?
If yes, then why?


I don't think I get your point. All I want you to do is something like
this:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f70c5f4da2a2..12126f212f3b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -730,8 +730,11 @@ static bool free_pages_prepare(struct page 
*page, unsigned int order)


   if (PageAnon(page))
   page->mapping = NULL;
-for (i = 0; i < (1 << order); i++)
+for (i = 0; i < (1 << order); i++) {
   bad += free_pages_check(page + i);
+if (static_key_false(_free_pages))
+clear_highpage(page + i);
+}
   if (bad)
   return false;

IMO this is better than messing around arch code.


Ok, I got your point.
I'm trying to follow a little bit different strategy, which is aimed to
impact generic code as less, as possible. And from this point of view
using already defined arch_free_page callback looks better to me. But I
don't insist and can make it your way.
Should I?


Stas, i like the idea not touching generic code if possible,
but don't like the idea of using arch_free_page() for zeroing 
purposes, because

* it's really arch independent
* does not suit the logic of name (arch_free_page() <-> zero pages)
* it can be defined one day for x86 as well and is already defined for
  powerpc which we might want to support one day.

=> please rework the patch. If you wish to leave your idea of heaving 
in different files - i'm ok,
but create them in arch independent dirs and call another (new) 
function near arch_free_page().


Or go with Vladimir's solution - if you got persuaded. :)

Thank you.

Ok, sure.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-18 Thread Konstantin Khorenko 

On 11/17/2015 06:31 PM, Stanislav Kinsburskiy wrote:



17.11.2015 16:08, Vladimir Davydov пишет:

On Tue, Nov 17, 2015 at 03:42:46PM +0100, Stanislav Kinsburskiy wrote:


17.11.2015 15:20, Vladimir Davydov пишет:

On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy wrote:


@@ -0,0 +1,23 @@
+#ifndef _ASM_X86_FREE_PAGE_H
+#define _ASM_X86_FREE_PAGE_H
+
+#ifdef __KERNEL__
+
+#ifndef __ASSEMBLY__
+
+#include 
+
+#define HAVE_ARCH_FREE_PAGE
+
+extern struct static_key zero_free_pages;
+extern void do_zero_pages(struct page *page, int order);
+
+static __always_inline void arch_free_page(struct page *page, int order)
+{
+   if (static_key_false(_free_pages))
+   do_zero_pages(page, order);
+}

There is no point in making this feature arch-dependant now, as you use
jump labels, which are arch-independent. The only reason why I had to
overwrite arch_free_page in PCS6 is that I had to write some asm code to
implement functionality close to that provided by jump labels, which are
absent in RH6-based kernels. Please move it to mm/page_alloc.c.

Thanks.

If we will make this function generic, then we have to declare
"zero_free_pages" as generic (which is more or less ok)  and define
do_zero_pages for the case, when HAVE_ARCH_FREE_PAGE is not defined. Does
the goal worth it?
If yes, then why?


I don't think I get your point. All I want you to do is something like
this:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f70c5f4da2a2..12126f212f3b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -730,8 +730,11 @@ static bool free_pages_prepare(struct page *page, unsigned 
int order)

if (PageAnon(page))
page->mapping = NULL;
-   for (i = 0; i < (1 << order); i++)
+   for (i = 0; i < (1 << order); i++) {
bad += free_pages_check(page + i);
+   if (static_key_false(_free_pages))
+   clear_highpage(page + i);
+   }
if (bad)
return false;

IMO this is better than messing around arch code.


Ok, I got your point.
I'm trying to follow a little bit different strategy, which is aimed to
impact generic code as less, as possible. And from this point of view
using already defined arch_free_page callback looks better to me. But I
don't insist and can make it your way.
Should I?


Stas, i like the idea not touching generic code if possible,
but don't like the idea of using arch_free_page() for zeroing purposes, because
* it's really arch independent
* does not suit the logic of name (arch_free_page() <-> zero pages)
* it can be defined one day for x86 as well and is already defined for
  powerpc which we might want to support one day.

=> please rework the patch. If you wish to leave your idea of heaving in 
different files - i'm ok,
but create them in arch independent dirs and call another (new) function near 
arch_free_page().

Or go with Vladimir's solution - if you got persuaded. :)

Thank you.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-17 Thread Vladimir Davydov 
On Tue, Nov 17, 2015 at 03:42:46PM +0100, Stanislav Kinsburskiy wrote:
> 
> 
> 17.11.2015 15:20, Vladimir Davydov пишет:
> >On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy wrote:
> >
> >>@@ -0,0 +1,23 @@
> >>+#ifndef _ASM_X86_FREE_PAGE_H
> >>+#define _ASM_X86_FREE_PAGE_H
> >>+
> >>+#ifdef __KERNEL__
> >>+
> >>+#ifndef __ASSEMBLY__
> >>+
> >>+#include 
> >>+
> >>+#define HAVE_ARCH_FREE_PAGE
> >>+
> >>+extern struct static_key zero_free_pages;
> >>+extern void do_zero_pages(struct page *page, int order);
> >>+
> >>+static __always_inline void arch_free_page(struct page *page, int order)
> >>+{
> >>+   if (static_key_false(_free_pages))
> >>+   do_zero_pages(page, order);
> >>+}
> >There is no point in making this feature arch-dependant now, as you use
> >jump labels, which are arch-independent. The only reason why I had to
> >overwrite arch_free_page in PCS6 is that I had to write some asm code to
> >implement functionality close to that provided by jump labels, which are
> >absent in RH6-based kernels. Please move it to mm/page_alloc.c.
> >
> >Thanks.
> 
> If we will make this function generic, then we have to declare
> "zero_free_pages" as generic (which is more or less ok)  and define
> do_zero_pages for the case, when HAVE_ARCH_FREE_PAGE is not defined. Does
> the goal worth it?
> If yes, then why?
> 

I don't think I get your point. All I want you to do is something like
this:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f70c5f4da2a2..12126f212f3b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -730,8 +730,11 @@ static bool free_pages_prepare(struct page *page, unsigned 
int order)
 
if (PageAnon(page))
page->mapping = NULL;
-   for (i = 0; i < (1 << order); i++)
+   for (i = 0; i < (1 << order); i++) {
bad += free_pages_check(page + i);
+   if (static_key_false(_free_pages))
+   clear_highpage(page + i);
+   }
if (bad)
return false;

IMO this is better than messing around arch code.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-17 Thread Stanislav Kinsburskiy 



17.11.2015 16:08, Vladimir Davydov пишет:

On Tue, Nov 17, 2015 at 03:42:46PM +0100, Stanislav Kinsburskiy wrote:


17.11.2015 15:20, Vladimir Davydov пишет:

On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy wrote:


@@ -0,0 +1,23 @@
+#ifndef _ASM_X86_FREE_PAGE_H
+#define _ASM_X86_FREE_PAGE_H
+
+#ifdef __KERNEL__
+
+#ifndef __ASSEMBLY__
+
+#include 
+
+#define HAVE_ARCH_FREE_PAGE
+
+extern struct static_key zero_free_pages;
+extern void do_zero_pages(struct page *page, int order);
+
+static __always_inline void arch_free_page(struct page *page, int order)
+{
+   if (static_key_false(_free_pages))
+   do_zero_pages(page, order);
+}

There is no point in making this feature arch-dependant now, as you use
jump labels, which are arch-independent. The only reason why I had to
overwrite arch_free_page in PCS6 is that I had to write some asm code to
implement functionality close to that provided by jump labels, which are
absent in RH6-based kernels. Please move it to mm/page_alloc.c.

Thanks.

If we will make this function generic, then we have to declare
"zero_free_pages" as generic (which is more or less ok)  and define
do_zero_pages for the case, when HAVE_ARCH_FREE_PAGE is not defined. Does
the goal worth it?
If yes, then why?


I don't think I get your point. All I want you to do is something like
this:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f70c5f4da2a2..12126f212f3b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -730,8 +730,11 @@ static bool free_pages_prepare(struct page *page, unsigned 
int order)
  
  	if (PageAnon(page))

page->mapping = NULL;
-   for (i = 0; i < (1 << order); i++)
+   for (i = 0; i < (1 << order); i++) {
bad += free_pages_check(page + i);
+   if (static_key_false(_free_pages))
+   clear_highpage(page + i);
+   }
if (bad)
return false;

IMO this is better than messing around arch code.


Ok, I got your point.
I'm trying to follow a little bit different strategy, which is aimed to 
impact generic code as less, as possible. And from this point of view 
using already defined arch_free_page callback looks better to me. But I 
don't insist and can make it your way.

Should I?


___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-17 Thread Stanislav Kinsburskiy 



17.11.2015 15:20, Vladimir Davydov пишет:

On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy wrote:


@@ -0,0 +1,23 @@
+#ifndef _ASM_X86_FREE_PAGE_H
+#define _ASM_X86_FREE_PAGE_H
+
+#ifdef __KERNEL__
+
+#ifndef __ASSEMBLY__
+
+#include 
+
+#define HAVE_ARCH_FREE_PAGE
+
+extern struct static_key zero_free_pages;
+extern void do_zero_pages(struct page *page, int order);
+
+static __always_inline void arch_free_page(struct page *page, int order)
+{
+   if (static_key_false(_free_pages))
+   do_zero_pages(page, order);
+}

There is no point in making this feature arch-dependant now, as you use
jump labels, which are arch-independent. The only reason why I had to
overwrite arch_free_page in PCS6 is that I had to write some asm code to
implement functionality close to that provided by jump labels, which are
absent in RH6-based kernels. Please move it to mm/page_alloc.c.

Thanks.


If we will make this function generic, then we have to declare 
"zero_free_pages" as generic (which is more or less ok)  and define 
do_zero_pages for the case, when HAVE_ARCH_FREE_PAGE is not defined. 
Does the goal worth it?

If yes, then why?
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-17 Thread Vladimir Davydov 
On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy wrote:

> @@ -0,0 +1,23 @@
> +#ifndef _ASM_X86_FREE_PAGE_H
> +#define _ASM_X86_FREE_PAGE_H
> +
> +#ifdef __KERNEL__
> +
> +#ifndef __ASSEMBLY__
> +
> +#include 
> +
> +#define HAVE_ARCH_FREE_PAGE
> +
> +extern struct static_key zero_free_pages;
> +extern void do_zero_pages(struct page *page, int order);
> +
> +static __always_inline void arch_free_page(struct page *page, int order)
> +{
> + if (static_key_false(_free_pages))
> + do_zero_pages(page, order);
> +}

There is no point in making this feature arch-dependant now, as you use
jump labels, which are arch-independent. The only reason why I had to
overwrite arch_free_page in PCS6 is that I had to write some asm code to
implement functionality close to that provided by jump labels, which are
absent in RH6-based kernels. Please move it to mm/page_alloc.c.

Thanks.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH rh7] x86: arch_free_page() introduced

2015-11-13 Thread Konstantin Khorenko 

Stas, please put reviewer to CC.

Volodya, please review.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 11/12/2015 09:01 PM, Stanislav Kinsburskiy wrote:

This patch defines arch_free_page() callback to x86 architecture. The only
thing it does is pages contents zeroing.
It's disabled by default. To enable it one should add "zero-free-pages"
kernel option to kernel boot parameters.
Note, that kernel boot option handling and enabling of pages zeroing are
divided. The reason for this is that static keys initialization can't be
performed during kernel parameters parsing (can't specify, what is the exact
reason, but kernel just doesn't boot, if static key is incremented on
parameter callback). Because of this split, actual zeroing is enabled later
during kernel boot, but still early enough.
Another thing to notice, is that code is moved to a separate source and header
files. The reason for source file is all other are not really suitable. The
reason for header is that asm/page.h cna't be used for this (compile
dependences).

https://jira.sw.ru/browse/PSBM-33071

Signed-off-by: Stanislav Kinsburskiy 
---
  arch/x86/include/asm/free_page.h   |   23 +++
  arch/x86/include/asm/thread_info.h |1 +
  arch/x86/mm/Makefile   |2 +-
  arch/x86/mm/free_page.c|   28 
  4 files changed, 53 insertions(+), 1 deletion(-)
  create mode 100644 arch/x86/include/asm/free_page.h
  create mode 100644 arch/x86/mm/free_page.c

diff --git a/arch/x86/include/asm/free_page.h b/arch/x86/include/asm/free_page.h
new file mode 100644
index 000..f1be780
--- /dev/null
+++ b/arch/x86/include/asm/free_page.h
@@ -0,0 +1,23 @@
+#ifndef _ASM_X86_FREE_PAGE_H
+#define _ASM_X86_FREE_PAGE_H
+
+#ifdef __KERNEL__
+
+#ifndef __ASSEMBLY__
+
+#include 
+
+#define HAVE_ARCH_FREE_PAGE
+
+extern struct static_key zero_free_pages;
+extern void do_zero_pages(struct page *page, int order);
+
+static __always_inline void arch_free_page(struct page *page, int order)
+{
+   if (static_key_false(_free_pages))
+   do_zero_pages(page, order);
+}
+
+#endif // __ASSEMBLY__
+#endif // __KERNEL__
+#endif // _ASM_X86_FREE_PAGE_H
diff --git a/arch/x86/include/asm/thread_info.h 
b/arch/x86/include/asm/thread_info.h
index f9513ef..4275318 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -9,6 +9,7 @@

  #include 
  #include 
+#include 
  #include 

  /*
diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index ac9ca65..5527581 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -1,5 +1,5 @@
  obj-y :=  init.o init_$(BITS).o fault.o ioremap.o extable.o pageattr.o mmap.o 
\
-   pat.o pgtable.o physaddr.o gup.o setup_nx.o
+   pat.o pgtable.o physaddr.o gup.o setup_nx.o free_page.o

  # Make sure __phys_addr has no stackprotector
  nostackp := $(call cc-option, -fno-stack-protector)
diff --git a/arch/x86/mm/free_page.c b/arch/x86/mm/free_page.c
new file mode 100644
index 000..d5d7e53
--- /dev/null
+++ b/arch/x86/mm/free_page.c
@@ -0,0 +1,28 @@
+#include 
+#include 
+
+static int zero_data_pages_enabled;
+struct static_key __initdata zero_free_pages = STATIC_KEY_INIT_FALSE;
+
+void do_zero_pages(struct page *page, int order)
+{
+   int i;
+
+   for (i = 0; i < (1 << order); i++)
+   clear_highpage(page + i);
+}
+
+static int __init enable_zero_free_pages(char *__unused)
+{
+   zero_data_pages_enabled = 1;
+   return 1;
+}
+__setup("zero-free-pages", enable_zero_free_pages);
+
+static int __init setup_zero_free_pages(void)
+{
+   if (zero_data_pages_enabled)
+   static_key_slow_inc(_free_pages);
+   return 0;
+}
+early_initcall(setup_zero_free_pages);

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel


___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel