Re: Bug: Discontigmem virt_to_page() [Alpha,ARM,Mips64?]

[Daniel Phillips <phillips@bonn-fries.net>]

On Thursday 09 May 2002 01:11, Roman Zippel wrote:
> Hi,
> 
> Daniel Phillips wrote:
> 
> > Your patch preserves a linear relationship between physical and virtual
> > memory, because you do both the ptov and vtop lookup in the same array.  
As
> > such, you don't provide the functionality I provide of being able to fit a
> > large amount of physical memory into a small amount of virtual memory, and
> > you can't join all your separate pgdat's into one, as I do.
> 
> Read the source again. arch/m68k/mm/motorola.c:map_chunk() maps all
> memory areas into single virtual area (virtaddr is static there and only
> increased starting from PAGE_OFFSET). In paging_init() there is only a
> single call to free_area_init().

Oops, yes, I see how it works, it relies on your O(N) search for the inverse. 
(Obligatory snipe: there are almost no comments for this opaque code, I hope 
you share my feeling that needs fixing.)

Searching the table instead of doing a direct lookup allows you to eliminate 
one of my two tables.  This is not a property you'd want to tie yourself to 
though, since the cost for any large number of chunks will be excessive, and 
will show up in the page table manipulation overhead.

Now it seems our strategies are a lot more similar than different.  So what 
were we arguing about again?  I've just gone further with the generalization 
of this, and cast it into a more general form suitable for use across more 
than one arch.

Where you ignore the distinction between logical and physical, it costs you 
execution time, as where you wrote  page = virt_to_page(phys_to_virt((i << 
PAGE_SHIFT) + bdata->node_boot_start)) where formerly we just had page++.  
This in generic code too.  Unless you have an #ifdef CONFIG_SOMETHING there I 
recommend this code *not* be merged because it penalizes the common case for 
the sake of your arch.  And it's unnecessary even for your arch, as I've 
demonstrated.

Incidently, the reason I came up with the virtual/logical distinction in the 
first places is that I found myself writing such awkward constructs as you 
wrote above, and thought there must be a better way.  Indeed there is.

> > You do have a config option, it's CONFIG_SINGLE_MEMORY_CHUNK.
> 
> That was our cheap answer to avoid the loops.

My cheap answer is to turn the option off.  So why don't I need a config 
option again?

> >  You just didn't
> > attempt to create the contiguous logical address space as I did, so you
> > didn't need to go outside your arch.
> 
> I don't need that, because I create a contiguous _virtual_ address
> space.

Again, we're arguing about what?  So do I.  The relationship between virtual 
and logical, for me, is just logical = virtual - PAGE_OFFSET, a meme you'll 
find in many places in the kernel source already, often obscured by the 
impression that physical addresses are really being manipulated when in fact 
nothing of the kind is going on - the simple truth is, the arithmetic gets 
easier then you work zero-based instead of PAGE_OFFSET based.

So now that we know we're both doing the same thing, could we please stop 
doing the catholics vs the protestants thing and maybe cooperate?

-- 
Daniel