Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Mon, Jan 21, 2013 at 03:24:40PM +, Russell King - ARM Linux wrote: > On Fri, Jan 18, 2013 at 11:37:25PM -0500, Konrad Rzeszutek Wilk wrote: > > On Fri, Jan 18, 2013 at 01:45:27PM -0800, Greg Kroah-Hartman wrote: > > > On Fri, Jan 18, 2013 at 09:08:59PM +, 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. > > > > > > > > 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. > > > > I thought the code had per_cpu for it - so that you wouldn't do that unless > > you really went out the way to do it. > > Actually, yes, you're right - that negates point (4) and possibly (2), > but (3) is still a concern. (3) shouldn't be that much of an issue > _provided_ that the virtual addresses aren't explicitly made use of by > other CPUs. Is that guaranteed by the zsmalloc code? (IOW, does it > own the virtual region it places these mappings in?) It does own them but it does also hand them off. So the users of it might be put on a different CPU. I think, I need to trace the call-chain. > > What is the performance difference between having and not having this > optimization? Can you provide some measurements please? Oh boy, there were somewhere. > > Lastly, as you hold per_cpu stuff across this, that means preemption > is disabled - and any kind of scheduling is also a bug. Is there > any reason the kmap stuff can't be used? Has this been tried? How > does it compare numerically with the existing solutions? It was really dependent on the architecture. On x86 the copying was superior, but on ARM it was sllow. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
> > 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
Thank you. I am sorry for butchering your name.
>
> >
> > 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.
Not sure either.
>
> > > #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 majord...@vger.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
Hi Matt,
On Mon, Jan 21, 2013 at 10:00:17AM -0600, Matt Sealey wrote:
> Hi Minchan,
>
> On Sun, Jan 20, 2013 at 11:55 PM, Minchan Kim wrote:
> > 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:
> >> >
> >> > 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.
>
> It prevents no problems, but if that isn't there, kernels build
> without SMP support (i.e. specifically uniprocessor kernels) will fail
> at the linker stage.
>
> That's not desirable.
>
> We have 3 problems here, this solves the first of them, and creates
> the third. The second is constant regardless..
>
> 1) zsmalloc will not build on ARM without CONFIG_SMP because on UP
> local_tlb_flush_kern_range uses a function which uses an export which
> isn't required on SMP
>
> Basically, with CONFIG_SMP (and CONFIG_UP_ON_SMP),
> local_tlb_flush_kern_range is calling functions by dereference from
> the per-cpu global cpu_tlb structure.
>
> On UP (!CONFIG_SMP), it is calling functions directly (in my case,
> v7wbi_local_tlb_flush_kern_range or whatever, but on v6, v5, v4 ARM
> processor kernel builds it may be different) which need to be exported
> outside of the MM core.
>
> If this difference is going to stick around - Russell is refusing here
> to export that/those direct functions - then the optimized vm mapping
> code simply should never be allowed to run on non-SMP systems to keep
> it building for everyone.
>
> The patch above is simply a build fix for !CONFIG_SMP in this case to
> force it to use the slow path for systems where the above missing
> export problem will cause the linker failure.
>
> 2) the optimized vm mapping isn't per-cpu aware as per Russell's
> arguments. I'll let you guys discuss that as I have no idea what the
> real implications are for SMP systems (and my current testing is only
> on a non-SMP CPU, I will have to go grab a couple boards from the lab
> for SMP)
>
> 3) it somewhat defeats the purpose of the optimization if UP systems
> (which tend to have less memory and might benefit from things like
> zsmalloc/zram more) cannot use it, but SMP systems which tend to have
> more memory (unless we're talking about a frugal configuration of a
> virtual machine, perhaps). Given the myriad use cases of zram that is
> not a pervasive or persuasive argument, I know, but it does seem
> slightly backwards.
>
> > 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.
>
> In the meantime please take into account building a non-SMP kernel for
> this board and testing that; if there is a way to do the flush without
> using the particular function which uses the particular export that
> Russell will not export, then that would be better. Maybe for
> !CONFIG_SMP using flush_tlb_kernel_range is doing the exact same job
> and the real patch is not to disable the optimization with
> !CONFIG_SMP, but to additionally #if defined(CONFIG_SMP) around
> local_flush_tlb_kernel_range and alternatively for UP use
> flush_tlb_kernel_range which.. probably.. doesn't use that contentious
> export?
>
> This is far beyond the level I want to be digging around in the Linux
> kernel so I am not comfortable even trying that to find out.
>
How about this? Could you confirm it? If you confirm, I will send it
to stable, too.
Thanks!
>From 7e1f315bef3e1956a32665335c3f86321c069947 Mon Sep 17 00:00:00 2001
From: Minchan Kim
Date: Wed, 23 Jan 2013 00:28:02 +0900
Subject: [PATCH] Fix build error in !CONFIG_SMP and CONFIG_ARM
Matt Sealey reported he fail to build zsmalloc due to use of
local_flush_tlb_kernel_range which are architecture dependent
function so !CONFIG_SMP in ARM couldn't implement it so it ends up
build error.
MODPOST 216 modules
LZMAarch/arm/boot/compressed/piggy.lzma
AS arch/arm/boot/compressed/lib1funcs.o
ERROR: "v7wbi_flush_kern_tlb_range"
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
Hi Matt,
On Mon, Jan 21, 2013 at 10:00:17AM -0600, Matt Sealey wrote:
Hi Minchan,
On Sun, Jan 20, 2013 at 11:55 PM, Minchan Kim minc...@kernel.org wrote:
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:
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.
It prevents no problems, but if that isn't there, kernels build
without SMP support (i.e. specifically uniprocessor kernels) will fail
at the linker stage.
That's not desirable.
We have 3 problems here, this solves the first of them, and creates
the third. The second is constant regardless..
1) zsmalloc will not build on ARM without CONFIG_SMP because on UP
local_tlb_flush_kern_range uses a function which uses an export which
isn't required on SMP
Basically, with CONFIG_SMP (and CONFIG_UP_ON_SMP),
local_tlb_flush_kern_range is calling functions by dereference from
the per-cpu global cpu_tlb structure.
On UP (!CONFIG_SMP), it is calling functions directly (in my case,
v7wbi_local_tlb_flush_kern_range or whatever, but on v6, v5, v4 ARM
processor kernel builds it may be different) which need to be exported
outside of the MM core.
If this difference is going to stick around - Russell is refusing here
to export that/those direct functions - then the optimized vm mapping
code simply should never be allowed to run on non-SMP systems to keep
it building for everyone.
The patch above is simply a build fix for !CONFIG_SMP in this case to
force it to use the slow path for systems where the above missing
export problem will cause the linker failure.
2) the optimized vm mapping isn't per-cpu aware as per Russell's
arguments. I'll let you guys discuss that as I have no idea what the
real implications are for SMP systems (and my current testing is only
on a non-SMP CPU, I will have to go grab a couple boards from the lab
for SMP)
3) it somewhat defeats the purpose of the optimization if UP systems
(which tend to have less memory and might benefit from things like
zsmalloc/zram more) cannot use it, but SMP systems which tend to have
more memory (unless we're talking about a frugal configuration of a
virtual machine, perhaps). Given the myriad use cases of zram that is
not a pervasive or persuasive argument, I know, but it does seem
slightly backwards.
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.
In the meantime please take into account building a non-SMP kernel for
this board and testing that; if there is a way to do the flush without
using the particular function which uses the particular export that
Russell will not export, then that would be better. Maybe for
!CONFIG_SMP using flush_tlb_kernel_range is doing the exact same job
and the real patch is not to disable the optimization with
!CONFIG_SMP, but to additionally #if defined(CONFIG_SMP) around
local_flush_tlb_kernel_range and alternatively for UP use
flush_tlb_kernel_range which.. probably.. doesn't use that contentious
export?
This is far beyond the level I want to be digging around in the Linux
kernel so I am not comfortable even trying that to find out.
How about this? Could you confirm it? If you confirm, I will send it
to stable, too.
Thanks!
From 7e1f315bef3e1956a32665335c3f86321c069947 Mon Sep 17 00:00:00 2001
From: Minchan Kim minc...@kernel.org
Date: Wed, 23 Jan 2013 00:28:02 +0900
Subject: [PATCH] Fix build error in !CONFIG_SMP and CONFIG_ARM
Matt Sealey reported he fail to build zsmalloc due to use of
local_flush_tlb_kernel_range which are architecture dependent
function so !CONFIG_SMP in ARM couldn't implement it so it ends up
build error.
MODPOST 216 modules
LZMAarch/arm/boot/compressed/piggy.lzma
AS arch/arm/boot/compressed/lib1funcs.o
ERROR: v7wbi_flush_kern_tlb_range
[drivers/staging/zsmalloc/zsmalloc.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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
Thank you. I am sorry for butchering your name.
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.
nods Not sure either.
#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 majord...@vger.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Mon, Jan 21, 2013 at 03:24:40PM +, Russell King - ARM Linux wrote: On Fri, Jan 18, 2013 at 11:37:25PM -0500, Konrad Rzeszutek Wilk wrote: On Fri, Jan 18, 2013 at 01:45:27PM -0800, Greg Kroah-Hartman wrote: On Fri, Jan 18, 2013 at 09:08:59PM +, 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. 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. I thought the code had per_cpu for it - so that you wouldn't do that unless you really went out the way to do it. Actually, yes, you're right - that negates point (4) and possibly (2), but (3) is still a concern. (3) shouldn't be that much of an issue _provided_ that the virtual addresses aren't explicitly made use of by other CPUs. Is that guaranteed by the zsmalloc code? (IOW, does it own the virtual region it places these mappings in?) It does own them but it does also hand them off. So the users of it might be put on a different CPU. I think, I need to trace the call-chain. What is the performance difference between having and not having this optimization? Can you provide some measurements please? Oh boy, there were somewhere. Lastly, as you hold per_cpu stuff across this, that means preemption is disabled - and any kind of scheduling is also a bug. Is there any reason the kmap stuff can't be used? Has this been tried? How does it compare numerically with the existing solutions? It was really dependent on the architecture. On x86 the copying was superior, but on ARM it was sllow. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Mon, Jan 21, 2013 at 10:20:38AM -0600, Matt Sealey wrote:
> See previous mail to Minchan; local_tlb_flush_kernel_range calls
> cpu_tlb.flush_kernel_range on SMP, but a direct function call
> ("glue(_TLB, flush_kernel_range)" which resolves to
> v7wbi_flush_kernel_range etc. etc.) without CONFIG_SMP.
Actually, that's wrong - it's got nothing to do with SMP vs non-SMP.
It's more to do with which CPUs are being supported. If they all use one
single cache maintanence implementation, then direct calls are used as an
optimization. If they require more than one cache maintanence
implementation, they are indirect calls. SMP really doesn't come into
that decision.
So:
> >> 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)
Would be wrong.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 10:46 PM, 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
>>
>> 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?
Yes, just back out the USE_PGTABLE_MAPPING code. But as I just replied
with Minchan, maybe there is a better way.
The only real problem here apart from the non-per-cpu usage Russell
describes (which does not affect UP systems anyway) is that without
CONFIG_SMP we have a FTBFS.
However I am sure you agree on the odd fix of enabling pagetable
mapping optimization only on "high end" systems and leaving the low
end using the slow path. It *is* odd. Also, my rudimentary patch for
disabling the code on !CONFIG_SMP is just *hiding* a misuse of a
non-exported mm function...
The FTBFS showed the problem, I don't want the fix to be to hide it,
which is why I brought it up.
>> * 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?
See previous mail to Minchan; local_tlb_flush_kernel_range calls
cpu_tlb.flush_kernel_range on SMP, but a direct function call
("glue(_TLB, flush_kernel_range)" which resolves to
v7wbi_flush_kernel_range etc. etc.) without CONFIG_SMP. That direct
function call it resolves to is not an export and Russell just said he
won't accept a patch to export it.
> 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
>> #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?
Nope, sorry. It would rely on knowing the precise configuration *and*
the user intent of using zram which is far beyond the scope of zram or
zsmalloc itself. It could be a VM, a phone with only 256MB RAM and
slow storage, it could be a device with 2TB RAM but no fast local
storage.. whether using it as a compressed block device or a swap
device that is compressed, you can never know inside the driver what
the real use it except the kernel build time config.
All we can know in zram, at build time, is whether we're configuring
for SMP, UP-on-SMP, or UP (!SMP) and which code path makes more sense
to build (ideally, SMP and UP would run the pagetable mapping code
alike). If we can make the pagetable mapping code compile on UP
without the above patch being required, and it has the same effect,
then this would be the best solution. Then the code needs to be fixed
for proper operation on SMP anyway.
If there are still a bunch of export problems with an alternate method
of flushing the tlb for a range of kernel memory exposed by trying a
different way around, this is just proving an issue here that the ARM
guys disagree that things that can be built as modules should be doing
such things, or that the cpu_tlb.flush_kernel_range vs.
v7wbi_tlb_flush_kernel_range export thing is confusing as crap at the
very least in that the CPU topology model the kernel lives by and is
compiled for at build time causes build breakages if you don't want
(or have) your driver to be knowledgable of the differences :)
>> 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.
Somewhat. It is not urgent, since I fixed it "for now" in my tree, I
am just worried that if that "fix" goes upstream it hides a
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
Hi Minchan,
On Sun, Jan 20, 2013 at 11:55 PM, Minchan Kim wrote:
> 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:
>> >
>> > 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.
It prevents no problems, but if that isn't there, kernels build
without SMP support (i.e. specifically uniprocessor kernels) will fail
at the linker stage.
That's not desirable.
We have 3 problems here, this solves the first of them, and creates
the third. The second is constant regardless..
1) zsmalloc will not build on ARM without CONFIG_SMP because on UP
local_tlb_flush_kern_range uses a function which uses an export which
isn't required on SMP
Basically, with CONFIG_SMP (and CONFIG_UP_ON_SMP),
local_tlb_flush_kern_range is calling functions by dereference from
the per-cpu global cpu_tlb structure.
On UP (!CONFIG_SMP), it is calling functions directly (in my case,
v7wbi_local_tlb_flush_kern_range or whatever, but on v6, v5, v4 ARM
processor kernel builds it may be different) which need to be exported
outside of the MM core.
If this difference is going to stick around - Russell is refusing here
to export that/those direct functions - then the optimized vm mapping
code simply should never be allowed to run on non-SMP systems to keep
it building for everyone.
The patch above is simply a build fix for !CONFIG_SMP in this case to
force it to use the slow path for systems where the above missing
export problem will cause the linker failure.
2) the optimized vm mapping isn't per-cpu aware as per Russell's
arguments. I'll let you guys discuss that as I have no idea what the
real implications are for SMP systems (and my current testing is only
on a non-SMP CPU, I will have to go grab a couple boards from the lab
for SMP)
3) it somewhat defeats the purpose of the optimization if UP systems
(which tend to have less memory and might benefit from things like
zsmalloc/zram more) cannot use it, but SMP systems which tend to have
more memory (unless we're talking about a frugal configuration of a
virtual machine, perhaps). Given the myriad use cases of zram that is
not a pervasive or persuasive argument, I know, but it does seem
slightly backwards.
> 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.
In the meantime please take into account building a non-SMP kernel for
this board and testing that; if there is a way to do the flush without
using the particular function which uses the particular export that
Russell will not export, then that would be better. Maybe for
!CONFIG_SMP using flush_tlb_kernel_range is doing the exact same job
and the real patch is not to disable the optimization with
!CONFIG_SMP, but to additionally #if defined(CONFIG_SMP) around
local_flush_tlb_kernel_range and alternatively for UP use
flush_tlb_kernel_range which.. probably.. doesn't use that contentious
export?
This is far beyond the level I want to be digging around in the Linux
kernel so I am not comfortable even trying that to find out.
--
Matt Sealey
Product Development Analyst, Genesi USA, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 11:37:25PM -0500, Konrad Rzeszutek Wilk wrote: > On Fri, Jan 18, 2013 at 01:45:27PM -0800, Greg Kroah-Hartman wrote: > > On Fri, Jan 18, 2013 at 09:08:59PM +, 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. > > > > > > 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. > > I thought the code had per_cpu for it - so that you wouldn't do that unless > you really went out the way to do it. Actually, yes, you're right - that negates point (4) and possibly (2), but (3) is still a concern. (3) shouldn't be that much of an issue _provided_ that the virtual addresses aren't explicitly made use of by other CPUs. Is that guaranteed by the zsmalloc code? (IOW, does it own the virtual region it places these mappings in?) What is the performance difference between having and not having this optimization? Can you provide some measurements please? Lastly, as you hold per_cpu stuff across this, that means preemption is disabled - and any kind of scheduling is also a bug. Is there any reason the kmap stuff can't be used? Has this been tried? How does it compare numerically with the existing solutions? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 11:37:25PM -0500, Konrad Rzeszutek Wilk wrote: On Fri, Jan 18, 2013 at 01:45:27PM -0800, Greg Kroah-Hartman wrote: On Fri, Jan 18, 2013 at 09:08:59PM +, 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. 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. I thought the code had per_cpu for it - so that you wouldn't do that unless you really went out the way to do it. Actually, yes, you're right - that negates point (4) and possibly (2), but (3) is still a concern. (3) shouldn't be that much of an issue _provided_ that the virtual addresses aren't explicitly made use of by other CPUs. Is that guaranteed by the zsmalloc code? (IOW, does it own the virtual region it places these mappings in?) What is the performance difference between having and not having this optimization? Can you provide some measurements please? Lastly, as you hold per_cpu stuff across this, that means preemption is disabled - and any kind of scheduling is also a bug. Is there any reason the kmap stuff can't be used? Has this been tried? How does it compare numerically with the existing solutions? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
Hi Minchan,
On Sun, Jan 20, 2013 at 11:55 PM, Minchan Kim minc...@kernel.org wrote:
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:
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.
It prevents no problems, but if that isn't there, kernels build
without SMP support (i.e. specifically uniprocessor kernels) will fail
at the linker stage.
That's not desirable.
We have 3 problems here, this solves the first of them, and creates
the third. The second is constant regardless..
1) zsmalloc will not build on ARM without CONFIG_SMP because on UP
local_tlb_flush_kern_range uses a function which uses an export which
isn't required on SMP
Basically, with CONFIG_SMP (and CONFIG_UP_ON_SMP),
local_tlb_flush_kern_range is calling functions by dereference from
the per-cpu global cpu_tlb structure.
On UP (!CONFIG_SMP), it is calling functions directly (in my case,
v7wbi_local_tlb_flush_kern_range or whatever, but on v6, v5, v4 ARM
processor kernel builds it may be different) which need to be exported
outside of the MM core.
If this difference is going to stick around - Russell is refusing here
to export that/those direct functions - then the optimized vm mapping
code simply should never be allowed to run on non-SMP systems to keep
it building for everyone.
The patch above is simply a build fix for !CONFIG_SMP in this case to
force it to use the slow path for systems where the above missing
export problem will cause the linker failure.
2) the optimized vm mapping isn't per-cpu aware as per Russell's
arguments. I'll let you guys discuss that as I have no idea what the
real implications are for SMP systems (and my current testing is only
on a non-SMP CPU, I will have to go grab a couple boards from the lab
for SMP)
3) it somewhat defeats the purpose of the optimization if UP systems
(which tend to have less memory and might benefit from things like
zsmalloc/zram more) cannot use it, but SMP systems which tend to have
more memory (unless we're talking about a frugal configuration of a
virtual machine, perhaps). Given the myriad use cases of zram that is
not a pervasive or persuasive argument, I know, but it does seem
slightly backwards.
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.
In the meantime please take into account building a non-SMP kernel for
this board and testing that; if there is a way to do the flush without
using the particular function which uses the particular export that
Russell will not export, then that would be better. Maybe for
!CONFIG_SMP using flush_tlb_kernel_range is doing the exact same job
and the real patch is not to disable the optimization with
!CONFIG_SMP, but to additionally #if defined(CONFIG_SMP) around
local_flush_tlb_kernel_range and alternatively for UP use
flush_tlb_kernel_range which.. probably.. doesn't use that contentious
export?
This is far beyond the level I want to be digging around in the Linux
kernel so I am not comfortable even trying that to find out.
--
Matt Sealey m...@genesi-usa.com
Product Development Analyst, Genesi USA, Inc.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 10:46 PM, Konrad Rzeszutek Wilk
konrad.w...@oracle.com 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
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?
Yes, just back out the USE_PGTABLE_MAPPING code. But as I just replied
with Minchan, maybe there is a better way.
The only real problem here apart from the non-per-cpu usage Russell
describes (which does not affect UP systems anyway) is that without
CONFIG_SMP we have a FTBFS.
However I am sure you agree on the odd fix of enabling pagetable
mapping optimization only on high end systems and leaving the low
end using the slow path. It *is* odd. Also, my rudimentary patch for
disabling the code on !CONFIG_SMP is just *hiding* a misuse of a
non-exported mm function...
The FTBFS showed the problem, I don't want the fix to be to hide it,
which is why I brought it up.
* 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?
See previous mail to Minchan; local_tlb_flush_kernel_range calls
cpu_tlb.flush_kernel_range on SMP, but a direct function call
(glue(_TLB, flush_kernel_range) which resolves to
v7wbi_flush_kernel_range etc. etc.) without CONFIG_SMP. That direct
function call it resolves to is not an export and Russell just said he
won't accept a patch to export it.
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
#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?
Nope, sorry. It would rely on knowing the precise configuration *and*
the user intent of using zram which is far beyond the scope of zram or
zsmalloc itself. It could be a VM, a phone with only 256MB RAM and
slow storage, it could be a device with 2TB RAM but no fast local
storage.. whether using it as a compressed block device or a swap
device that is compressed, you can never know inside the driver what
the real use it except the kernel build time config.
All we can know in zram, at build time, is whether we're configuring
for SMP, UP-on-SMP, or UP (!SMP) and which code path makes more sense
to build (ideally, SMP and UP would run the pagetable mapping code
alike). If we can make the pagetable mapping code compile on UP
without the above patch being required, and it has the same effect,
then this would be the best solution. Then the code needs to be fixed
for proper operation on SMP anyway.
If there are still a bunch of export problems with an alternate method
of flushing the tlb for a range of kernel memory exposed by trying a
different way around, this is just proving an issue here that the ARM
guys disagree that things that can be built as modules should be doing
such things, or that the cpu_tlb.flush_kernel_range vs.
v7wbi_tlb_flush_kernel_range export thing is confusing as crap at the
very least in that the CPU topology model the kernel lives by and is
compiled for at build time causes build breakages if you don't want
(or have) your driver to be knowledgable of the differences :)
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.
Somewhat. It is not urgent, since I fixed it for now in my tree, I
am just worried that if that fix goes upstream it hides a real, more
important issue here.
I am mostly only concerned about whether zram
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Mon, Jan 21, 2013 at 10:20:38AM -0600, Matt Sealey wrote:
See previous mail to Minchan; local_tlb_flush_kernel_range calls
cpu_tlb.flush_kernel_range on SMP, but a direct function call
(glue(_TLB, flush_kernel_range) which resolves to
v7wbi_flush_kernel_range etc. etc.) without CONFIG_SMP.
Actually, that's wrong - it's got nothing to do with SMP vs non-SMP.
It's more to do with which CPUs are being supported. If they all use one
single cache maintanence implementation, then direct calls are used as an
optimization. If they require more than one cache maintanence
implementation, they are indirect calls. SMP really doesn't come into
that decision.
So:
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)
Would be wrong.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
> > 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
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
li...@arm.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
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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
> 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.
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
> #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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 01:45:27PM -0800, Greg Kroah-Hartman wrote: > On Fri, Jan 18, 2013 at 09:08:59PM +, 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. > > > > 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. I thought the code had per_cpu for it - so that you wouldn't do that unless you really went out the way to do it. > > 4. when the mapping is torn down, no guarantee that it's the same CPU that > >used the happing. With per_cpu that actually would be the case. > > > > So, the use of the local TLB flush leaves all the other CPUs potentially > > containing TLB entries for this mapping. Right. That is the point of a local TLB flush. > > > > Finally, there is no TODO file for this driver, which I believe is a > > requirement for anything to be in stable. So as far as I can see, it > > should be deleted or a TODO file added. I'm not sure why Greg decided > > to add it without a TODO file. A TODO file can certainly be added and it is welcome. > > I don't know, I'm cursing the day I took the whole zsmalloc, zcache, > zram mess that we have in the staging tree now. People are working to > get them out of staging, which is good, but the churn involved is > driving me crazy. Oh oh. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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..
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
.. 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 09:08:59PM +, 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. > > 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. > > Finally, there is no TODO file for this driver, which I believe is a > requirement for anything to be in stable. So as far as I can see, it > should be deleted or a TODO file added. I'm not sure why Greg decided > to add it without a TODO file. I don't know, I'm cursing the day I took the whole zsmalloc, zcache, zram mess that we have in the staging tree now. People are working to get them out of staging, which is good, but the churn involved is driving me crazy. I wouldn't worry about it, anyone who uses this code is really on their own. Matt, best of luck. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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. 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. Finally, there is no TODO file for this driver, which I believe is a requirement for anything to be in stable. So as far as I can see, it should be deleted or a TODO file added. I'm not sure why Greg decided to add it without a TODO file. (If there was such a file, I'd have added the above to it. As it is, I've just decided to disable the thing in my randconfig builds.) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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: MODPOST 216 modules LZMAarch/arm/boot/compressed/piggy.lzma AS arch/arm/boot/compressed/lib1funcs.o ERROR: "v7wbi_flush_kern_tlb_range" [drivers/staging/zsmalloc/zsmalloc.ko] undefined! make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2 make: *** Waiting for unfinished jobs I am wondering if someone who is far better versed in the mm code can tell me exactly what would end up calling this function and why it would only be in the kernel at all if SMP was enabled - all I can see right now is that it is PROBABLY something resulting from this code and comment inside the zsmalloc driver: /* * By default, zsmalloc uses a copy-based object mapping method to access * allocations that span two pages. However, if a particular architecture * 1) Implements local_flush_tlb_kernel_range() and 2) 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 And of course, once USE_PGTABLE_MAPPING is enabled, local_flush_tlb_kernel_range being used is the actual culprit here. But the question is, for the ARM guys, shouldn't local_flush_tlb_kernel_range actually be defined in the kernel build, even without SMP, even if it would be architecturally a no-op on UP systems, and then CONFIG_SMP_ON_UP would catch this case? If not, then the fix is obvious, the check inside zsmalloc for CONFIG_ARM should be fixed to check specifically for local_flush_tlb_kernel_range definition as well, or for CONFIG_SMP as well, or the non-presence of CONFIG_SMP_ON_UP or something? The build cases I have tested are basically CONFIG_SMP + CONFIG_SMP_ON_UP (since it's a single-core Cortex-A8) and without CONFIG_SMP (since it's a Cortex-A8..) using imx_v7_defconfig and imx_v6_v7_defconfig alike and making the appropriate adjustments. Trying to compile it with CONFIG_SMP without CONFIG_SMP_ON_UP makes no sense on my target system - but essentially it only links with CONFIG_SMP. I got no clue what I am looking at in arch/arm/mm and related, so I am unsure precisely how I should proceed in patching it with the intent it goes upstream.. or what the real implication of this kind of memory management actually is on SMP vs. UP systems, but the intended functionality of local_flush_tlb_kernel_range seems like it should exist even on UP, to me. -- Matt Sealey Product Development Analyst, Genesi USA, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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: MODPOST 216 modules LZMAarch/arm/boot/compressed/piggy.lzma AS arch/arm/boot/compressed/lib1funcs.o ERROR: v7wbi_flush_kern_tlb_range [drivers/staging/zsmalloc/zsmalloc.ko] undefined! make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2 make: *** Waiting for unfinished jobs I am wondering if someone who is far better versed in the mm code can tell me exactly what would end up calling this function and why it would only be in the kernel at all if SMP was enabled - all I can see right now is that it is PROBABLY something resulting from this code and comment inside the zsmalloc driver: /* * By default, zsmalloc uses a copy-based object mapping method to access * allocations that span two pages. However, if a particular architecture * 1) Implements local_flush_tlb_kernel_range() and 2) 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 And of course, once USE_PGTABLE_MAPPING is enabled, local_flush_tlb_kernel_range being used is the actual culprit here. But the question is, for the ARM guys, shouldn't local_flush_tlb_kernel_range actually be defined in the kernel build, even without SMP, even if it would be architecturally a no-op on UP systems, and then CONFIG_SMP_ON_UP would catch this case? If not, then the fix is obvious, the check inside zsmalloc for CONFIG_ARM should be fixed to check specifically for local_flush_tlb_kernel_range definition as well, or for CONFIG_SMP as well, or the non-presence of CONFIG_SMP_ON_UP or something? The build cases I have tested are basically CONFIG_SMP + CONFIG_SMP_ON_UP (since it's a single-core Cortex-A8) and without CONFIG_SMP (since it's a Cortex-A8..) using imx_v7_defconfig and imx_v6_v7_defconfig alike and making the appropriate adjustments. Trying to compile it with CONFIG_SMP without CONFIG_SMP_ON_UP makes no sense on my target system - but essentially it only links with CONFIG_SMP. I got no clue what I am looking at in arch/arm/mm and related, so I am unsure precisely how I should proceed in patching it with the intent it goes upstream.. or what the real implication of this kind of memory management actually is on SMP vs. UP systems, but the intended functionality of local_flush_tlb_kernel_range seems like it should exist even on UP, to me. -- Matt Sealey m...@genesi-usa.com Product Development Analyst, Genesi USA, Inc. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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. 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. Finally, there is no TODO file for this driver, which I believe is a requirement for anything to be in stable. So as far as I can see, it should be deleted or a TODO file added. I'm not sure why Greg decided to add it without a TODO file. (If there was such a file, I'd have added the above to it. As it is, I've just decided to disable the thing in my randconfig builds.) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 09:08:59PM +, 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. 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. Finally, there is no TODO file for this driver, which I believe is a requirement for anything to be in stable. So as far as I can see, it should be deleted or a TODO file added. I'm not sure why Greg decided to add it without a TODO file. I don't know, I'm cursing the day I took the whole zsmalloc, zcache, zram mess that we have in the staging tree now. People are working to get them out of staging, which is good, but the churn involved is driving me crazy. I wouldn't worry about it, anyone who uses this code is really on their own. Matt, best of luck. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 3:08 PM, Russell King - ARM Linux
li...@arm.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:
* 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..
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
.. 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 m...@genesi-usa.com
Product Development Analyst, Genesi USA, Inc.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
On Fri, Jan 18, 2013 at 01:45:27PM -0800, Greg Kroah-Hartman wrote: On Fri, Jan 18, 2013 at 09:08:59PM +, 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. 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. I thought the code had per_cpu for it - so that you wouldn't do that unless you really went out the way to do it. 4. when the mapping is torn down, no guarantee that it's the same CPU that used the happing. With per_cpu that actually would be the case. So, the use of the local TLB flush leaves all the other CPUs potentially containing TLB entries for this mapping. Right. That is the point of a local TLB flush. Finally, there is no TODO file for this driver, which I believe is a requirement for anything to be in stable. So as far as I can see, it should be deleted or a TODO file added. I'm not sure why Greg decided to add it without a TODO file. A TODO file can certainly be added and it is welcome. I don't know, I'm cursing the day I took the whole zsmalloc, zcache, zram mess that we have in the staging tree now. People are working to get them out of staging, which is good, but the churn involved is driving me crazy. Oh oh. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Compilation problem with drivers/staging/zsmalloc when !SMP on ARM
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
li...@arm.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.
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
#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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
