Messages in this thread | | | Date | Tue, 19 Dec 2017 07:40:04 -0700 | From | "Jan Beulich" <> | Subject | Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE |
| |
>>> On 19.12.17 at 15:25, <boris.ostrovsky@oracle.com> wrote: > On 12/19/2017 03:23 AM, Jan Beulich wrote: >>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote: >>> + if (!xen_e820_table) >>> + return; >> Not saying "out of memory" here is certainly fine, but shouldn't >> there nevertheless be a warning, as failure to go through the >> rest of the function will impact overall functionality? > > Commit ebfdc40969f claims that these types of messages are unnecessary > because allocation failures are signalled by the memory subsystem.
But the memory subsystem can't possibly provide an indication of what will not work because of the failed allocation.
>>> + memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries); >> Is it really reasonable to have a static upper bound here? As we >> know especially EFI systems can come with a pretty scattered >> (pseudo) E820 table. Even if (iirc) this has a static upper bound >> right now in the hypervisor too, it would be nice if the kernel >> didn't need further changes once the hypervisor is being made >> more flexible. > > This is how we obtain the map in xen_memory_setup(). Are you suggesting > that we should query for the size first?
That would be better, I think.
>>> + /* Mark non-RAM regions as not available. */ >>> + for (; i < memmap.nr_entries; i++) { >>> + entry = &xen_e820_table->entries[i]; >>> + >>> + if (entry->type == E820_TYPE_RAM) >>> + continue; >> I can't seem to match up this with ... >> >>> + if (entry->addr >= hostmem_resource->end) >>> + break; >>> + >>> + res = kzalloc(sizeof(*res), GFP_KERNEL); >>> + if (!res) >>> + goto out; >>> + >>> + res->name = "Host memory"; >> ... this. Do you mean != instead (with the comment ahead of the >> loop also clarified, saying something like "host RAM regions which >> aren't RAM for us")? And perhaps better "Host RAM"? > > Right, this is not memory but rather something else (and so "!=" is > correct). "Unavailable host RAM"?
If you like to be even more specific than what I had suggested - sure.
Jan
| |