Subject: Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory
 as UNUSABLE
To: Jan Beulich <JBeulich@suse.com>
References: <1513635771-11779-1-git-send-email-boris.ostrovsky@oracle.com>
 <5A38DA840200007800198579@prv-mh.provo.novell.com>
Cc: christian.koenig@amd.com, helgaas@kernel.org, xen-devel@lists.xen.org,
        Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <063fdabc-5d9a-5137-9f27-3289488a3e44@oracle.com>
Date: Tue, 19 Dec 2017 09:25:55 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <5A38DA840200007800198579@prv-mh.provo.novell.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2649
Lines: 85

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