linux-kernel - Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM

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]
Date:	Mon, 21 Jan 2013 14:55:41 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Cc:	Matt Sealey <matt@...esi-usa.com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Linux ARM Kernel ML <linux-arm-kernel@...ts.infradead.org>,
	devel <devel@...verdev.osuosl.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nitin Gupta <ngupta@...are.org>,
	Seth Jennings <sjenning@...ux.vnet.ibm.com>
Subject: Re: Compilation problem with drivers/staging/zsmalloc when !SMP on
 ARM

On Fri, Jan 18, 2013 at 11:46:02PM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 18, 2013 at 07:11:32PM -0600, Matt Sealey wrote:
> > On Fri, Jan 18, 2013 at 3:08 PM, Russell King - ARM Linux
> > <linux@....linux.org.uk> wrote:
> > > On Fri, Jan 18, 2013 at 02:24:15PM -0600, Matt Sealey wrote:
> > >> Hello all,
> > >>
> > >> I wonder if anyone can shed some light on this linking problem I have
> > >> right now. If I configure my kernel without SMP support (it is a very
> > >> lean config for i.MX51 with device tree support only) I hit this error
> > >> on linking:
> > >
> > > Yes, I looked at this, and I've decided that I will _not_ fix this export,
> > > neither will I accept a patch to add an export.
> > 
> > Understood..
> > 
> > > As far as I can see, this code is buggy in a SMP environment.  There's
> > > apparantly no guarantee that:
> > >
> > > 1. the mapping will be created on a particular CPU.
> > > 2. the mapping will then be used only on this specific CPU.
> > > 3. no guarantee that another CPU won't speculatively prefetch from this
> > >    region.
> > > 4. when the mapping is torn down, no guarantee that it's the same CPU that
> > >    used the happing.
> > >
> > > So, the use of the local TLB flush leaves all the other CPUs potentially
> > > containing TLB entries for this mapping.
> > 
> > I'm gonna put this out to the maintainers (Konrad, and Seth since he
> > committed it) that if this code is buggy it gets taken back out, even
> > if it makes zsmalloc "slow" on ARM, for the following reasons:
> 
> Just to make sure I understand, you mean don't use page table
> mapping but instead use copying?
> 
> > 
> > * It's buggy on SMP as Russell describes above
> > * It might not be buggy on UP (opposite to Russell's description above
> > as the restrictions he states do not exist), but that would imply an
> > export for a really core internal MM function nobody should be using
> > anyway
> > * By that assessment, using that core internal MM function on SMP is
> > also bad voodoo that zsmalloc should not be doing
> 
>  'local_tlb_flush' is bad voodoo?
> 
> > 
> > It also either smacks of a lack of comprehensive testing or defiance
> > of logic that nobody ever built the code without CONFIG_SMP, which
> > means it was only tested on a bunch of SMP ARM systems (I'm guessing..
> > Pandaboard? :) or UP systems with SMP/SMP_ON_UP enabled (to expand on
> > that guess, maybe Beagleboard in some multiplatform Beagle/Panda
> > hybrid kernel). I am sure I was reading the mailing lists when that
> > patch was discussed, coded and committed and my guess is correct. In
> > this case, what we have here anyway is code which when PROPERLY
> > configured as so..
> 
> The initial patch were done on x86. Then Seth did the work to make sure
> it worked on PPC. Munchin looked on ARM and that is it.

s/Munchin/Minchan

> 
> If you have an ARM server that you would be willing to part with I would
> be thrilled to look at it.
> 
> > 
> > diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
> > b/drivers/staging/zsmalloc/zsmalloc-main.c
> > index 09a9d35..ecf75fb 100644
> 	> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> > +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> > @@ -228,7 +228,7 @@ struct zs_pool {
> >   * mapping rather than copying
> >   * for object mapping.
> >  */
> > -#if defined(CONFIG_ARM)
> > +#if defined(CONFIG_ARM) && defined(CONFIG_SMP)
> >  #define USE_PGTABLE_MAPPING

I don't get it. How to prevent the problem Russel described?
The problem is that other CPU can prefetch _speculatively_ under us.

> >  #endif
> > 
> > .. such that it even compiles in both "guess" configurations, the
> > slower Cortex-A8 600MHz single core system gets to use the slow copy
> > path and the dual-core 1GHz+ Cortex-A9 (with twice the RAM..) gets to
> > use the fast mapping path. Essentially all the patch does is "improve
> > performance" on the fastest, best-configured, large-amounts-of-RAM,
> > lots-of-CPU-performance ARM systems (OMAP4+, Snapdragon, Exynos4+,
> > marvell armada, i.MX6..) while introducing the problems Russell
> > describes, and leave performance exactly the same and potentially far
> > more stable on the slower, memory-limited ARM machines.
> 
> Any ideas on how to detect that?
> > 
> > Given the purpose of zsmalloc, zram, zcache etc. this somewhat defies
> > logic. If it's not making the memory-limited, slow ARM systems run
> > better, what's the point?
> > 
> > So in summary I suggest "we" (Greg? or is it Seth's responsibility?)
> > should just back out that whole USE_PGTABLE_MAPPING chunk of code
> > introduced with f553646. Then Russell can carry on randconfiging and I
> > can build for SMP and UP and get the same code.. with less bugs.
> 
> I get that you want to have this fixed right now. I think having it
> fixed the right way is a better choice. Lets discuss that first
> before we start tossing patches to disable parts of it.

If I don't miss something, we could have 2 choice.

1) use flush_tlb_kernel_range instead of local_flush_tlb_kernel_range
Or
2) use only memory copy

I guess everybody want 2 because it makes code very simple and maintainable.
Even, rencently Joonsoo sent optimize patch.
Look at https://lkml.org/lkml/2013/1/16/68 so zram/zcache effect caused by 2
would be minimized.

But please give me the time and I will borrow quad-core embedded target board
and test 1 on the phone with Seth's benchmark.

> 
> --
> 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/

-- 
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/