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

Daniel Phillips
2002-05-09T16:08:52Z

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