[SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Franck Bui-Huu]

Is it me or the use of CONFIG_SPARSEMEM_EXTREME is really confusing in
mm/sparce.c ? Shouldn't we use CONFIG_SPARSEMEM_STATIC instead like
the following patch suggests ?

-- >8 --
Subject: [PATCH] Remove confusing uses of SPARSEMEM_EXTREME

CONFIG_SPARSEMEM_EXTREME is used in sparce.c whereas
CONFIG_SPARSEMEM_STATIC seems to be more appropriate.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com> 

---

include/linux/mmzone.h |    2 +-
mm/sparse.c            |    6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index ebfc238..35f38b0 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -551,7 +551,7 @@ #define SECTION_NR_TO_ROOT(sec)	((sec) /
 #define NR_SECTION_ROOTS	(NR_MEM_SECTIONS / SECTIONS_PER_ROOT)
 #define SECTION_ROOT_MASK	(SECTIONS_PER_ROOT - 1)
 
-#ifdef CONFIG_SPARSEMEM_EXTREME
+#ifndef CONFIG_SPARSEMEM_STATIC
 extern struct mem_section *mem_section[NR_SECTION_ROOTS];
 #else
 extern struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
diff --git a/mm/sparse.c b/mm/sparse.c
index 0a51f36..341d935 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -16,7 +16,7 @@ #include <asm/dma.h>
  *
  * 1) mem_section	- memory sections, mem_map's for valid memory
  */
-#ifdef CONFIG_SPARSEMEM_EXTREME
+#ifndef CONFIG_SPARSEMEM_STATIC
 struct mem_section *mem_section[NR_SECTION_ROOTS]
 	____cacheline_internodealigned_in_smp;
 #else
@@ -25,7 +25,7 @@ struct mem_section mem_section[NR_SECTIO
 #endif
 EXPORT_SYMBOL(mem_section);
 
-#ifdef CONFIG_SPARSEMEM_EXTREME
+#ifndef CONFIG_SPARSEMEM_STATIC
 static struct mem_section *sparse_index_alloc(int nid)
 {
 	struct mem_section *section = NULL;
@@ -67,7 +67,7 @@ out:
 	spin_unlock(&index_init_lock);
 	return ret;
 }
-#else /* !SPARSEMEM_EXTREME */
+#else /* SPARSEMEM_STATIC */
 static inline int sparse_index_init(unsigned long section_nr, int nid)
 {
 	return 0;
-- 
1.3.3.g8701
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@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: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Dave Hansen]

On Mon, 2006-06-12 at 09:00 +0200, Franck Bui-Huu wrote:
> Is it me or the use of CONFIG_SPARSEMEM_EXTREME is really confusing in
> mm/sparce.c ? Shouldn't we use CONFIG_SPARSEMEM_STATIC instead like
> the following patch suggests ? 

I'll take positive config options over negative ones any day.  I find it
easier to read things that say what they *are* rather than what they are
*not*.

In any case, STATIC is really there as an override for architectures to
say, "I know what I am doing, I use gcc 3.4 and above, or, I don't want
to use bootmem".  Extreme is really there to say, "I want two-level
lookups because my memory is extremely sparse."

Make sense?

-- Dave

Re: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Andy Whitcroft]

Franck Bui-Huu wrote:
> Is it me or the use of CONFIG_SPARSEMEM_EXTREME is really confusing in
> mm/sparce.c ? Shouldn't we use CONFIG_SPARSEMEM_STATIC instead like
> the following patch suggests ?
> 
> -- >8 --
> Subject: [PATCH] Remove confusing uses of SPARSEMEM_EXTREME
> 
> CONFIG_SPARSEMEM_EXTREME is used in sparce.c whereas
> CONFIG_SPARSEMEM_STATIC seems to be more appropriate.
> 
> Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com> 

In my mind the positive option is selecting for code supporting EXTREME
so it seems to make sense to use that option.  Perhaps the confusion
comes from a lack of comments there to say that the else case is STATIC.

-apw

Re: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Franck Bui-Huu]

Hi Andy

2006/6/12, Andy Whitcroft <apw@shadowen.org>:
>
> In my mind the positive option is selecting for code supporting EXTREME
> so it seems to make sense to use that option.

well I find it confusing because in my mind, something like this seems
more logical.

#ifndef CONFIG_SPARSEMEM_STATIC
static struct mem_section *sparse_index_alloc(int nid)
{
        return alloc_bootmem_node(...);
}
#else
static struct mem_section *sparse_index_alloc(int nid)
{
        /* nothing to do here, since it has been statically allocated */
        return 0;
}
#endif

This code only deals with section allocation and the way it's achieved
depends only if the section array has been statically allocated.
There's nothing related on the two-level lookups here, is there ?

And use SPARSEMEM_EXTREME when it deals only with two-level lookups:

#ifdef CONFIG_SPARSEMEM_EXTREME
#define SECTIONS_PER_ROOT       (PAGE_SIZE / sizeof (struct mem_section))
#else
#define SECTIONS_PER_ROOT       1
#endif

> Perhaps the confusion
> comes from a lack of comments there to say that the else case is STATIC.
>

nope but maybe a comment to explain why i386 use SPARSEMEM_STATIC
option could be useful. At least for someone who is not working on
this arch...

Thanks
-- 
               Franck

Re: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Andy Whitcroft]

Franck Bui-Huu wrote:
> Hi Andy
> 
> 2006/6/12, Andy Whitcroft <apw@shadowen.org>:
> 
>>
>> In my mind the positive option is selecting for code supporting EXTREME
>> so it seems to make sense to use that option.
> 
> 
> well I find it confusing because in my mind, something like this seems
> more logical.
> 
> #ifndef CONFIG_SPARSEMEM_STATIC
> static struct mem_section *sparse_index_alloc(int nid)
> {
>        return alloc_bootmem_node(...);
> }
> #else
> static struct mem_section *sparse_index_alloc(int nid)
> {
>        /* nothing to do here, since it has been statically allocated */
>        return 0;
> }
> #endif

But also in this case the code in the first stanza is only applicable to
SPARSEMEM EXTREME, therefore its also logical to say

#ifdef CONFIG_SPARSEMEM_EXTREME
special handling for that mode
#else
normal handling
#endif

Which is what the code currently says right?

-apw

Re: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Franck Bui-Huu]

2006/6/12, Andy Whitcroft <apw@shadowen.org>:
> Franck Bui-Huu wrote:
> > Hi Andy
> >
> > 2006/6/12, Andy Whitcroft <apw@shadowen.org>:
> >
> >>
> >> In my mind the positive option is selecting for code supporting EXTREME
> >> so it seems to make sense to use that option.
> >
> >
> > well I find it confusing because in my mind, something like this seems
> > more logical.
> >
> > #ifndef CONFIG_SPARSEMEM_STATIC
> > static struct mem_section *sparse_index_alloc(int nid)
> > {
> >        return alloc_bootmem_node(...);
> > }
> > #else
> > static struct mem_section *sparse_index_alloc(int nid)
> > {
> >        /* nothing to do here, since it has been statically allocated */
> >        return 0;
> > }
> > #endif
>
> But also in this case the code in the first stanza is only applicable to
> SPARSEMEM EXTREME, therefore its also logical to say
>

Well I don't think so. Please show me which part of this code is
_only_ applicable to EXTREME.

The only thing that makes it applicable to EXTREME is not in the code
but rather in the Kconfig script:

        config SPARSEMEM_EXTREME
                def_bool y
                depends on SPARSEMEM && !SPARSEMEM_STATIC

-- 
               Franck

Re: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Franck Bui-Huu]

Hi Dave,

2006/6/12, Dave Hansen <haveblue@us.ibm.com>:
> On Mon, 2006-06-12 at 09:00 +0200, Franck Bui-Huu wrote:
> > Is it me or the use of CONFIG_SPARSEMEM_EXTREME is really confusing in
> > mm/sparce.c ? Shouldn't we use CONFIG_SPARSEMEM_STATIC instead like
> > the following patch suggests ?
>
> I'll take positive config options over negative ones any day.  I find it
> easier to read things that say what they *are* rather than what they are
> *not*.
>
> In any case, STATIC is really there as an override for architectures to
> say, "I know what I am doing, I use gcc 3.4 and above, or, I don't want
> to use bootmem".  Extreme is really there to say, "I want two-level
> lookups because my memory is extremely sparse."
>
> Make sense?

yes and that's what the patch is trying to show...please take a look
to it and show me what part of the code, used by SPARSEMEM_STATIC
config, is dealing with the two-level lookups.

thanks
-- 
               Franck

Re: [SPARSEMEM] confusing uses of SPARSEM_EXTREME (try #2)

[Andy Whitcroft]

Franck Bui-Huu wrote:
> 2006/6/12, Andy Whitcroft <apw@shadowen.org>:
> 
>> Franck Bui-Huu wrote:
>> > Hi Andy
>> >
>> > 2006/6/12, Andy Whitcroft <apw@shadowen.org>:
>> >
>> >>
>> >> In my mind the positive option is selecting for code supporting
>> EXTREME
>> >> so it seems to make sense to use that option.
>> >
>> >
>> > well I find it confusing because in my mind, something like this seems
>> > more logical.
>> >
>> > #ifndef CONFIG_SPARSEMEM_STATIC
>> > static struct mem_section *sparse_index_alloc(int nid)
>> > {
>> >        return alloc_bootmem_node(...);
>> > }
>> > #else
>> > static struct mem_section *sparse_index_alloc(int nid)
>> > {
>> >        /* nothing to do here, since it has been statically allocated */
>> >        return 0;
>> > }
>> > #endif
>>
>> But also in this case the code in the first stanza is only applicable to
>> SPARSEMEM EXTREME, therefore its also logical to say
>>
> 
> Well I don't think so. Please show me which part of this code is
> _only_ applicable to EXTREME.
> 
> The only thing that makes it applicable to EXTREME is not in the code
> but rather in the Kconfig script:
> 
>        config SPARSEMEM_EXTREME
>                def_bool y
>                depends on SPARSEMEM && !SPARSEMEM_STATIC
> 

I think you have missed the point of the code here.  There are two
SPARSEMEM variants; STATIC and EXTREME.  The key point is that STATIC
was the original and only variant.  EXTREME is a later variant.

In the static case the section map is defined as:

  struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]

Where the NR_SECTION_ROOTS is always 1 allowing greater code sharing
between the two modes.  This the the primary and default variant.

In the extreme case it is defined as:

  struct mem_section *mem_section[NR_SECTION_ROOTS]

The key difference is that in EXTREME the second level is not allocated
statically, they are allocated as sections are discovered and
initialised.  The sparse_index_alloc routine is a SPARSEMEM hook to give
 variants the oppotunity to prepare for an area of sections to be used.
The EXTREME variant is using this hook to allocate the second levels as
it goes thus only populating those it is using; which is its key feature.

If we look at the definition in question, this is describing this
relationship exactly.  If we have EXTREME variant turned on then use the
alloc_bootmem_node to init the section root, else use the default
behaviour from the default variant STATIC.

  #ifdef CONFIG_SPARSEMEM_EXTREME
  static struct mem_section *sparse_index_alloc(int nid)
  {
         return alloc_bootmem_node(...);
  }
  #else
  static struct mem_section *sparse_index_alloc(int nid)
  {
         return 0;
  }
  #endif

This would be even more clear should a third variant MAGIC be added
where we would have:

  #ifdef CONFIG_SPARSEMEM_MAGIC
  static struct mem_section *sparse_index_alloc(int nid)
  {
         return wave_wand();
  }
  #elif defined(CONFIG_SPARSEMEM_EXTREME)
  static struct mem_section *sparse_index_alloc(int nid)
  {
         return alloc_bootmem_node(...);
  }
  #else
  static struct mem_section *sparse_index_alloc(int nid)
  {
         return 0;
  }
  #endif

If this was changed to an #ifndef we would be talking about something like:

  #if !defined(SPARSEMEM_EXTREME) && !defined(SPARSEMEM_STATIC)
  ... magic case
  #elif !defined(SPARSMEM_MAGIC) && !defined(SPARSMEM_STATIC)
  ... extreme case
  #else
  ... default case
  #endif

I think we can agree this is not clearer.

-apw