Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4



On Fri, 17 Apr 2009, Ingo Molnar wrote:

Could we perhaps round up to 1MB in this case too?

(The below 1MB one).

I'd argue against it, at least in this incarnation. I can well imagine
somebody wanting to do resource management in the 640k-1M window, so..

Would it make sense to round up everything that is listed in the
E820 map? Just in case the BIOS is not entirely honest about the
true extent of that area.

Well, it would probably work, but on the other hand, when we see
"E820_RAM", that means that we really _can_ trust that that E820 entry is
right, since we're going to use it as RAM (and Windows would too), and if
it wasn't RAM, really bad things would happen.

So E820_RAM is a _lot_ more trustworthy than the other cases. If we're
rounding up by reasonably large amounts like 32MB or even more, I really
think we should do so for the things we really know are there, and that we
really fundamentally know come in big granularities.

The other entries in the e820 map can reasonably be 4kB or something,
because they are an IO-APIC or whatever. I can't say that I'd feel happy
putting a guard area around something like that. But RAM? Sure, it can
come in 384kB chunks (think RAM remapping for the low 1MB area), but it
doesn't tend to happen when we're talking gigs any more.

+ start = entry->addr + entry->size;
+ end = round_up(start, ram_alignment(start));
+ if (start == end)
+ continue;

Hm, indeed, the continue is needed - reserve_region_with_split()
lets zero-sized resources be inserted silently. I'd have missed this
case. Do zero-sized memory resources have a special role somewhere?

No. But it wouldn't be a zero-size region, it would be a one-byte sized
region. It's just that my patch was missing the "-1" from the end that I
meant to put there:

+ reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");

That 'end' there should be 'end-1', and that also explains why "start ==
end" must have a continue.

The 'end' in a resource region is the last byte, not the 'byte after'.

So there was a small buglet in the patch, but as I mentioned, using
"reserve_region_with_split()" is really wrong anyway, because we do not
want to recurse into existing regions, just split _around_ them. So the
patch was meant as

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [ckrm-tech] [RFC][PATCH 2/7] UBC: core (structures, API)
    ... Matt Helsley wrote: ... +struct user_beancounter ub0; ... You haven't explained what an luid is at this point in the patch series. ... So we create one beancounter object for every resource the user's tasks ...
    (Linux-Kernel)
  • Re: Garbage Collection and Library Deployment
    ... However, looking at Task Manager, the RAM is ... Just because you're done with the memory, that doesn't mean .NET releases the OS allocation it made in order to satisfy your need. ... Just because RAM is "a valuable resource for your low-end system", that doesn't mean your program should be explicitly trying to manage it. ...
    (microsoft.public.dotnet.languages.csharp)
  • Re: [PATCH 0/1] Recurse when searching for empty slots in resources trees
    ... find_resource() found room for a resource. ... But the reason I don't like your patch is that I think the _deeper_ ... the ambiguity doesn't exist any more (because we'd explicitly take the ... I just looked at our PCI bus resource allocation code, ...
    (Linux-Kernel)
  • Re: [PATCH 0/1] Recurse when searching for empty slots in resources trees
    ... parent and needs to insert the child's resource inside the parent's ... Well, just deep enough. ... Hence this patch. ... the ambiguity doesn't exist any more (because we'd explicitly take the ...
    (Linux-Kernel)
  • Re: more on #loadWithContext:forEdit:
    ... three packages and a resource folder are created. ... Looks like the beta patch has broken the upgrade - ironically if you'd tried ... ^accessor loadWithContext: context forEdit: aBoolean! ...
    (comp.lang.smalltalk.dolphin)