Messages in this thread | | | Subject | Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE | From | Boris Ostrovsky <> | Date | Tue, 19 Dec 2017 09:25:55 -0500 |
| |
On 12/19/2017 03:23 AM, Jan Beulich wrote: >>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote: >>>> >>>> + >>>> + xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL); > Wouldn't kmalloc() suffice here?
Yes.
> >> + 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.
> >> + 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?
> >> + /* 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"?
> >> + rc = insert_resource(hostmem_resource, res); >> + if (rc) { >> + pr_warn("%s: Can't insert [%llx - %llx] (%d)\n", > [%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds? > (Same issues somewhere further down.)
This will not be built for non-PAE configurations because memory hotplug requires PAE.
> >> + __func__, res->start, res->end, rc); >> + kfree(res); >> + goto out; > Perhaps better not to bail out of the loop here (at least if rc is > not -ENOMEM)?
We shouldn't get -ENOMEM here since resource insertion doesn't allocate anything.
The reason I decided to bail here was because I thought that if we fail once it means there is a bug somewhere (since we shouldn't really fail) and so subsequent attempts to insert the range would fail as well.
-boris
| |