linux-kernel - Re: [PATCH] zsmalloc: Add Kconfig for enabling PTE method

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130205000854.GC2610@blaptop>
Date:	Tue, 5 Feb 2013 09:08:54 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Seth Jennings <sjenning@...ux.vnet.ibm.com>,
	Nitin Gupta <ngupta@...are.org>,
	Dan Magenheimer <dan.magenheimer@...cle.com>,
	Konrad Rzeszutek Wilk <konrad@...nok.org>
Subject: Re: [PATCH] zsmalloc: Add Kconfig for enabling PTE method

Hi Greg,

On Mon, Feb 04, 2013 at 10:51:46AM -0800, Greg Kroah-Hartman wrote:
> On Mon, Feb 04, 2013 at 09:23:41AM +0900, Minchan Kim wrote:
> > Zsmalloc has two methods 1) copy-based and 2) pte based to access
> > allocations that span two pages.
> > You can see history why we supported two approach from [1].
> > 
> > But it was bad choice that adding hard coding to select architecture
> > which want to use pte based method. This patch removed it and adds
> > new Kconfig to select the approach.
> > 
> > This patch is based on next-20130202.
> > 
> > [1] https://lkml.org/lkml/2012/7/11/58
> > 
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > Cc: Seth Jennings <sjenning@...ux.vnet.ibm.com>
> > Cc: Nitin Gupta <ngupta@...are.org>
> > Cc: Dan Magenheimer <dan.magenheimer@...cle.com>
> > Cc: Konrad Rzeszutek Wilk <konrad@...nok.org>
> > Signed-off-by: Minchan Kim <minchan@...nel.org>
> > ---
> >  drivers/staging/zsmalloc/Kconfig         |   12 ++++++++++++
> >  drivers/staging/zsmalloc/zsmalloc-main.c |   11 -----------
> >  2 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/zsmalloc/Kconfig b/drivers/staging/zsmalloc/Kconfig
> > index 9084565..2359123 100644
> > --- a/drivers/staging/zsmalloc/Kconfig
> > +++ b/drivers/staging/zsmalloc/Kconfig
> > @@ -8,3 +8,15 @@ config ZSMALLOC
> >  	  non-standard allocator interface where a handle, not a pointer, is
> >  	  returned by an alloc().  This handle must be mapped in order to
> >  	  access the allocated space.
> > +
> > +config ZSMALLOC_PGTABLE_MAPPING
> > +        bool "Use page table mapping to access allocations that span two pages"
> > +        depends on ZSMALLOC
> > +        default n
> > +        help
> > +	  By default, zsmalloc uses a copy-based object mapping method to access
> > +	  allocations that span two pages. However, if a particular architecture
> > +	  performs VM mapping faster than copying, then you should select this.
> > +	  This causes zsmalloc to use page table mapping rather than copying
> > +	  for object mapping. You can check speed with zsmalloc benchmark[1].
> > +	  [1] https://github.com/spartacus06/zsmalloc
> > diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> > index 06f73a9..b161ca1 100644
> > --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> > +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> > @@ -218,17 +218,6 @@ struct zs_pool {
> >  #define CLASS_IDX_MASK	((1 << CLASS_IDX_BITS) - 1)
> >  #define FULLNESS_MASK	((1 << FULLNESS_BITS) - 1)
> >  
> > -/*
> > - * By default, zsmalloc uses a copy-based object mapping method to access
> > - * allocations that span two pages. However, if a particular architecture
> > - * performs VM mapping faster than copying, then it should be added here
> > - * so that USE_PGTABLE_MAPPING is defined. This causes zsmalloc to use
> > - * page table mapping rather than copying for object mapping.
> > -*/
> > -#if defined(CONFIG_ARM)
> > -#define USE_PGTABLE_MAPPING
> > -#endif
> 
> Did you test this?  I don't see the new config value you added actually
> do anything in this code.  Also, if I select it incorrectly on ARM, or

*slaps self*

> or other platforms, what is keeping this from doing bad things?

There is no way to prevent it now.
I thought a way to detect it dynamically by testing performance
both approaches in booting/module-loading time and select the best choice.
For it, we should add benchmark code and delay booting/module-loading,
it's not good for embedded system because they are fighting with 300msec all
day long.
So I think best choice we can do is that pass the decision to user by Kconfig
which includes pointing the benchmark. I intionally removed "ARM" word in help
because we checked the performance in just three devices of all ARM CPU
so we can't make sure it does makse sense all ARM CPU.

Of course, I'm open for suggestion. Do you have better idea?


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

-- 
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/