From patchwork Sat Jan 19 01:11:32 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matt Sealey X-Patchwork-Id: 2005091 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id C0871DF2E5 for ; Sat, 19 Jan 2013 01:15:41 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TwMyZ-0003r8-5T; Sat, 19 Jan 2013 01:12:03 +0000 Received: from mail-vc0-f181.google.com ([209.85.220.181]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TwMyS-0003qh-0J for linux-arm-kernel@lists.infradead.org; Sat, 19 Jan 2013 01:11:57 +0000 Received: by mail-vc0-f181.google.com with SMTP id d16so1918090vcd.26 for ; Fri, 18 Jan 2013 17:11:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type:x-gm-message-state; bh=1AA+GmTuHyEMw8gKnXslAGevP2WbbjumkaR6g0HEq14=; b=KBj9zsoimfsrK8zVhDwr0ifiU2QTpppc5pffJ36LS9DzK6fP+D4474Hv4cvi2ctWO/ GW97sG55re6qjDoQQiKVJtITN6g3tkqb1Wb3L/bidm+yw8yRL0/zzImA0UWGn4FIH9gz UkCO0pdNfFbQ96LTR/ZQdihEovzyENk1677xEZKnWOnxTDKxuBU9ji2/vp6VD4icKcCQ KUsjxgSiouQePOY39bMJwJeO1y0Adno5Xzx3ySaISfTGTwhIhzQNVSw1kpp9bBQwq06F 0/JkAIpvzGPnVtihQtnzpf2aILgzkonIn7eFdTTbTDC0CtTyJpD2XQyVUge0Flyi4/F+ BliA== X-Received: by 10.52.68.4 with SMTP id r4mr10327022vdt.15.1358557912684; Fri, 18 Jan 2013 17:11:52 -0800 (PST) MIME-Version: 1.0 Received: by 10.58.186.207 with HTTP; Fri, 18 Jan 2013 17:11:32 -0800 (PST) In-Reply-To: <20130118210859.GH23505@n2100.arm.linux.org.uk> References: <20130118210859.GH23505@n2100.arm.linux.org.uk> From: Matt Sealey Date: Fri, 18 Jan 2013 19:11:32 -0600 Message-ID: Subject: Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM To: Russell King - ARM Linux X-Gm-Message-State: ALoCoQkHLDZazUSfsSRO6cbumvzSZ/AuGdQ5xzkpPFX89yVCX2HhIYRZWOVIebkktiGxVIr40aYH X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130118_201156_122069_9426FD99 X-CRM114-Status: GOOD ( 24.66 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.220.181 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: devel , Seth Jennings , Konrad Rzeszutek Wilk , Greg Kroah-Hartman , LKML , Minchan Kim , Nitin Gupta , Linux ARM Kernel ML X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Fri, Jan 18, 2013 at 3:08 PM, Russell King - ARM Linux 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: * 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 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.. .. 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. 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 am sure zsmalloc/zram/zcache2 are not so awful at the end of the day despite the churn in staging.. but the amount of time I just spent today with my brain on fire because of cross-referencing mm code for a linker error, when all I wanted was a non-SMP kernel, I feel Greg's hurt a little bit. --- Matt Sealey Product Development Analyst, Genesi USA, Inc. 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 #endif