2.6.5-rc2-aa1

2.6.5-rc2-aa1

[Andrea Arcangeli]

This fixes MAP_SHARED|MAP_ANONYMOUS spotted by Jens and Martin, plus it
avoids vmware to crash (but it only warns about the missing VM_RESERVED
bitflag).

URL:

	http://www.us.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.6/2.6.5-rc2-aa1.gz
	http://www.us.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.6/2.6.5-rc2-aa1/

Changelog diff between 2.6.5-rc1-aa3 and 2.6.5-rc2-aa1:

Binary files 2.6.5-rc1-aa3/anon_vma.gz and 2.6.5-rc2-aa1/anon_vma.gz differ

	Fixed silly bug in mmap(MAP_SHARED|MAP_ANONYMOUS) and converted a
	BUG_ON to a WARN_ON if some device driver passes non-pageable pages
	to the VM via ->nopage. VM_RESERVED it's not required with current
	VM, so even if the WARN_ON triggers the machine remains completely
	stable.

Only in 2.6.5-rc1-aa3: linus.patch.gz

	Updated to 2.6.5-rc2 so not needed anymore.

Re: 2.6.5-rc2-aa1

[Christoph Hellwig]

A bunch of rather cosmetic comments while looking over the patch:

 - all the binfmt handler seems to copy exactly the same vma setup,
   maybe we should add a helper for that?
 - the struct anon_vma_s / anon_vma_t naming is awkward, why not just
   struct anon_vma *insert reference to Documentation/CodingStyle here*
 - the inclusion guards in objrmap.h are wrong
 - why is PG_swapcache bit 21?  You removed PG_direct so bit 16 is free now
 - I don't really like the swapper space special case in ___add_to_page_cache,
   IIRC there's only one caller of it for the swapper space and IMHO you're
   better off opencoding it
 - dito for __remove_from_page_cache 
 - is renaming rmap.c to objrmap.c really nessecary?  It contains about
   the same functions, and keeping the old, implementation-agnostic name
   makes it easiert to follow the radical changes..

Re: 2.6.5-rc2-aa1

[Andrea Arcangeli]

On Sun, Mar 21, 2004 at 04:24:01PM +0000, Christoph Hellwig wrote:
> A bunch of rather cosmetic comments while looking over the patch:
> 
>  - all the binfmt handler seems to copy exactly the same vma setup,
>    maybe we should add a helper for that?

yes, this isn't related to my patch btw (my patch just shows the
duplication since I had to fixup many places).

>  - the struct anon_vma_s / anon_vma_t naming is awkward, why not just
>    struct anon_vma *insert reference to Documentation/CodingStyle here*

Andrew already complained about that, I don't mind either ways now that
it's implemented, it never needs forward declaration so it's not
required to be a struct and I don't see why we should restrict us to a
subset of the C language when a typedef can save characters. While
coding I want to be efficient so I want to save characters, typedefs
help in saving my time so I would been less efficient in not taking
advantage of them. if there's any religious issue against typedefs it will
be faster anyways to do a search and replace on the patch itself,
instead of doing it while coding where it's me typing chars.

just run a s/anon_vma_t/struct anon_vma/ on the patch and then apply it,
it should just work and it didn't waste my time while coding, this will
be fine with me of course.

>  - the inclusion guards in objrmap.h are wrong

can you elaborate? I hate including garbage inclusions especially now
that we've a reliable buildsystem (with the previous buildsystem no
matter how smart you coded the inclusions, I had to make distclean
anyways after every include change). I like strict always, if something
can be avoided I avoid it, the shorter the inclusion the more correct it
is to me (again, especially true with a reliable buildsystem like in 2.6).

>  - why is PG_swapcache bit 21?  You removed PG_direct so bit 16 is free now

indeed, I will cleanup this bit, it didn't make any difference in
practice due the fact 24bit are reserved anyways, so you could use bit
16 instead of bit 21 for the future bitflags but it's cleaner to follow
your suggestion of course. I will change it in the next version.

>  - I don't really like the swapper space special case in ___add_to_page_cache,
>    IIRC there's only one caller of it for the swapper space and IMHO you're
>    better off opencoding it
>  - dito for __remove_from_page_cache 

agreed, but those are microoptimizations, I agree they can be done, but
it's nothing I worry about right now. At the moment from my point of
view the most obvious and the less modifications the better, at least
it's included in some more official kernel like -mm or mainline.
Furthermore I much prefer keeping to cover only the entry/exit points
than having to deal with PageSwapCache checks in all generic pagecache
operations that the swapcache may be sharing at runtime (like sync_page
for example).  (again, at least until this is in some more official
kernel where it's not only me having to fixup and rediff when something
change in the pagecache layer in mainline and/or -mm)

>  - is renaming rmap.c to objrmap.c really nessecary?  It contains >  about
>    the same functions, and keeping the old, implementation-agnostic name
>    makes it easiert to follow the radical changes..

b*tkeeper will automagically notice the rename when Linus merges (at
least this was Miles's theory in the arch lists trying to figure out how
could some of his merges with Linus's tree via patches could be tracked
right despite he didn't post them with smart metadata like arch can
produce reliably), so if this theory is right you'll see the clean diff
(and you'll see the amount of sharing too).  I renamed it primarly
because rmap is the common name for the tecnique of traking the
pagetables with pte_chains (of course rmap for 2.4 is a lot more than
that since the patch grown a lot bigger to the point of merging elevator
read latency changes, but normally in l-k when we talk about rmap we
mean the tracking of ptes with the pte_chains). The code sharing between
the rmap.c and objrmap.c is a very little percentage, and such little
percentage is only the API with the VM callers and the
pte_test_and_clear in the unmap after we reached the pagetable with the
different anon_vma/objrmap methods which is again the same of the 2.4
pagetable walking code too.

Re: 2.6.5-rc2-aa1

[Rik van Riel]

On Mon, 22 Mar 2004, Andrea Arcangeli wrote:

> >  - the struct anon_vma_s / anon_vma_t naming is awkward, why not just
> >    struct anon_vma *insert reference to Documentation/CodingStyle here*
> 
> Andrew already complained about that, I don't mind either ways now that
> it's implemented, it never needs forward declaration so it's not
> required to be a struct and I don't see why we should restrict us to a
> subset of the C language when a typedef can save characters. While
> coding I want to be efficient so I want to save characters,

You won't be spending anywhere near as much time typing the
code as you (and everybody else) will be spending _reading_
the code.

> typedefs help in saving my time

Presuming you'll never debug your code ;)

> >  - the inclusion guards in objrmap.h are wrong
> 
> can you elaborate?

They're _LINUX_RMAP_H and not _LINUX_OBJRMAP_H.  If you want
to be consistent you may want to either rename the inclusion
guards, or the file ;)

> >  - is renaming rmap.c to objrmap.c really nessecary?  It contains >  about
> >    the same functions, and keeping the old, implementation-agnostic name
> >    makes it easiert to follow the radical changes..
> 
> b*tkeeper will automagically notice the rename when Linus merges

Only if (1) you're using bitkeeper and (2) you used 'bk mv'
to move rmap.c to objrmap.c and (3) Linus pulls from your
bitkeeper tree.

Unless all 3 of these are true, you're giving bitkeeper more
credit than it deserves ;)

> I renamed it primarly because rmap is the common name for the tecnique
> of traking the pagetables with pte_chains

Funny, first thing I hear about that ;)


-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: 2.6.5-rc2-aa1

[Andrea Arcangeli]

On Sun, Mar 21, 2004 at 10:43:41PM -0500, Rik van Riel wrote:
> Presuming you'll never debug your code ;)

typedefs are more readable to my eyes, so if they're there it's more
debuggable, infact I liked when Ingo tried to inject the page_t thing
(was him Ingo or somebody else?)

> They're _LINUX_RMAP_H and not _LINUX_OBJRMAP_H.  If you want
> to be consistent you may want to either rename the inclusion
> guards, or the file ;)

I see what you mean now, I agree.

> Only if (1) you're using bitkeeper and (2) you used 'bk mv'
> to move rmap.c to objrmap.c and (3) Linus pulls from your
> bitkeeper tree.

I know this would be expected (this is what happens with arch too in the
explicit mode, the one that I prefer to be strict in the commits), but I
was just trying to say that Miles theory is that BK gets it right
automatically, either that or Linus's scripts gets it right before
injecting it into bk, that is his whole point. Miles sent to Linus
_patches_ (obviously w/o bk, AFIK Miles is not legally allowed to use bk
and Larry even refused him to sell him a commercial licence of bk so
that Miles could use bk while still working on arch) renaming files, and
he then found the renames being catched correctly in the web.

> Unless all 3 of these are true, you're giving bitkeeper more
> credit than it deserves ;)

It may not be bk catching the renames but just an external script that
Linus uses. I don't know.

> > I renamed it primarly because rmap is the common name for the tecnique
> > of traking the pagetables with pte_chains
> 
> Funny, first thing I hear about that ;)

Not sure after all those discussions how you may not have ever noticed
that people uses objrmap to mean something different than rmap, it's
really hard to believe that you never noticed.

Re: 2.6.5-rc2-aa1

[Rik van Riel]

On Mon, 22 Mar 2004, Andrea Arcangeli wrote:

> > > I renamed it primarly because rmap is the common name for the tecnique
> > > of traking the pagetables with pte_chains
> > 
> > Funny, first thing I hear about that ;)
> 
> Not sure after all those discussions how you may not have ever noticed
> that people uses objrmap to mean something different than rmap, it's
> really hard to believe that you never noticed.

Most people seem to be talking about "pte based rmap" vs
"object based rmap".  So far you're the only one who I've
seen using "rmap" to mean just "pte based rmap" and not
also "object based rmap".

Also, the BSDs seem to abbreviate "pte based reverse mapping"
to pmap, not rmap...

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: 2.6.5-rc2-aa1

[Andrea Arcangeli]

On Mon, Mar 22, 2004 at 09:07:54AM -0500, Rik van Riel wrote:
> On Mon, 22 Mar 2004, Andrea Arcangeli wrote:
> 
> > > > I renamed it primarly because rmap is the common name for the tecnique
> > > > of traking the pagetables with pte_chains
> > > 
> > > Funny, first thing I hear about that ;)
> > 
> > Not sure after all those discussions how you may not have ever noticed
> > that people uses objrmap to mean something different than rmap, it's
> > really hard to believe that you never noticed.
> 
> Most people seem to be talking about "pte based rmap" vs
> "object based rmap".  So far you're the only one who I've
> seen using "rmap" to mean just "pte based rmap" and not
> also "object based rmap".

then I'm the only one and I could have been biased because rmap.c is
including 99% of code for the pte based rmap, and my objrmap.c is including
99% of code for the objrect based _reverse_mappings_, still objrmap.c is
a more appropriate name for that stuff IMO (especially if somebody else
is mistaken as I am using the word rmap to mean the current 2.6 code in
mm/rmap.c).

Re: 2.6.5-rc2-aa1

[Hugh Dickins]

On Mon, 22 Mar 2004, Andrea Arcangeli wrote:
> On Mon, Mar 22, 2004 at 09:07:54AM -0500, Rik van Riel wrote:
> > 
> > Most people seem to be talking about "pte based rmap" vs
> > "object based rmap".  So far you're the only one who I've
> > seen using "rmap" to mean just "pte based rmap" and not
> > also "object based rmap".
> 
> then I'm the only one and I could have been biased because rmap.c is
> including 99% of code for the pte based rmap, and my objrmap.c is including
> 99% of code for the objrect based _reverse_mappings_, still objrmap.c is
> a more appropriate name for that stuff IMO (especially if somebody else
> is mistaken as I am using the word rmap to mean the current 2.6 code in
> mm/rmap.c).

I agree with Rik and Christoph (I agreed with all of Christoph's points,
but most can be left until later on): mm/rmap.c and include/linux/rmap.h
(the latter a name change from include/linux/rmap-locking.h).

objrmap is the particular implementation found within that file in your
tree, but Rik imagined right from the start that there would be various
implementations:

 * This is kept modular because we may want to experiment
 * with object-based reverse mapping schemes.

(Aaargh, now we can expect someone to propose
CONFIG_PTE_CHAIN_RMAP, CONFIG_ANON_VMA_RMAP, CONFIG_ANONMM_RMAP etc)

Hugh

Re: 2.6.5-rc2-aa1

[Andrea Arcangeli]

On Mon, Mar 22, 2004 at 02:31:15PM +0000, Hugh Dickins wrote:
> On Mon, 22 Mar 2004, Andrea Arcangeli wrote:
> > On Mon, Mar 22, 2004 at 09:07:54AM -0500, Rik van Riel wrote:
> > > 
> > > Most people seem to be talking about "pte based rmap" vs
> > > "object based rmap".  So far you're the only one who I've
> > > seen using "rmap" to mean just "pte based rmap" and not
> > > also "object based rmap".
> > 
> > then I'm the only one and I could have been biased because rmap.c is
> > including 99% of code for the pte based rmap, and my objrmap.c is including
> > 99% of code for the objrect based _reverse_mappings_, still objrmap.c is
> > a more appropriate name for that stuff IMO (especially if somebody else
> > is mistaken as I am using the word rmap to mean the current 2.6 code in
> > mm/rmap.c).
> 
> I agree with Rik and Christoph (I agreed with all of Christoph's points,
> but most can be left until later on): mm/rmap.c and include/linux/rmap.h
> (the latter a name change from include/linux/rmap-locking.h).
> 
> objrmap is the particular implementation found within that file in your
> tree, but Rik imagined right from the start that there would be various
> implementations:
> 
>  * This is kept modular because we may want to experiment
>  * with object-based reverse mapping schemes.
> 
> (Aaargh, now we can expect someone to propose
> CONFIG_PTE_CHAIN_RMAP, CONFIG_ANON_VMA_RMAP, CONFIG_ANONMM_RMAP etc)

obviously I read that comment, but I definitely hope he meant people
adding objrmap.c w/o necessairly deleting rmap.c too like me and you did
(i.e. we don't provide a CONFIG_RMAP to go back), and to have the
CONFIG_ in a Makefile _definitely_not_ in rmap.c mixing everything
unrelated in the same file.

Separating the entry points from the rest of the mm/*.c is sure a good
idea, and infact I left those separated in objrmap.c, like they were
separated in rmap.c, so you can go ahead and add an anobjrmap.c and we
can have a CONFIG_ option to select if to compile with objrmap.c or with
anobjrmap.c.

your example of having multiple methods in the same tree selectable as a
config option is the very best example where keeping the objrmap.c code
separated from the rmap.c code in two different files is a MUST (or at
the very least a SHOULD to avoid a mess of #ifdefs in the middle of C
code since there's nothing to share except the function parameters).

Re: 2.6.5-rc2-aa1

[Rik van Riel]

On Mon, 22 Mar 2004, Andrea Arcangeli wrote:

> >  * This is kept modular because we may want to experiment
> >  * with object-based reverse mapping schemes.
> 
> obviously I read that comment, but I definitely hope he meant people
> adding objrmap.c w/o necessairly deleting rmap.c too like me and you did

On the contrary.  I started out by looking at object based
rmap, but Ben and Dave told me about the worst case scenarios.
Only after that I started working on a pte based rmap scheme.

Now that the big problems with object based rmap are solved,
I'd really like to see the pte chain code go away...

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Re: 2.6.5-rc2-aa1

[Andrea Arcangeli]

On Mon, Mar 22, 2004 at 10:09:06AM -0500, Rik van Riel wrote:
> On Mon, 22 Mar 2004, Andrea Arcangeli wrote:
> 
> > >  * This is kept modular because we may want to experiment
> > >  * with object-based reverse mapping schemes.
> > 
> > obviously I read that comment, but I definitely hope he meant people
> > adding objrmap.c w/o necessairly deleting rmap.c too like me and you did
> 
> On the contrary.  I started out by looking at object based
> rmap, but Ben and Dave told me about the worst case scenarios.
> Only after that I started working on a pte based rmap scheme.
> 
> Now that the big problems with object based rmap are solved,
> I'd really like to see the pte chain code go away...

me too, that's why I deleted mm/rmap.c and I use objrmap.c instead. rmap
has always been used for years to mean your current implementation
of pte based reverse mappings in 2.6-mainline and 2.4-rmap, that's
blatantly too obvious to even make further jokes about that. If you now
me to believe that that is not true and that every single time you said
"rmap" it was a strict english abbreviation for the generic word
"reverse mappings" without any remote relation to the current
2.6-mainline and 2.4-rmap implementation of the reverse mapping methods,
then maybe you want to write some PR don't waste your time trying to
convince me. 

Re: 2.6.5-rc2-aa1

[Hugh Dickins]

On Mon, 22 Mar 2004, Andrea Arcangeli wrote:
> On Mon, Mar 22, 2004 at 02:31:15PM +0000, Hugh Dickins wrote:
> > 
> > (Aaargh, now we can expect someone to propose
> > CONFIG_PTE_CHAIN_RMAP, CONFIG_ANON_VMA_RMAP, CONFIG_ANONMM_RMAP etc)
.....
> Separating the entry points from the rest of the mm/*.c is sure a good
> idea, and infact I left those separated in objrmap.c, like they were
> separated in rmap.c, so you can go ahead and add an anobjrmap.c and we
> can have a CONFIG_ option to select if to compile with objrmap.c or with
> anobjrmap.c.

You misunderstand me.  I absolutely do not want any such CONFIG_ option.
We agree on what's best, which may well prove to be your implementation,
and stick with that.  Please.

Hugh