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>
 <063fdabc-5d9a-5137-9f27-3289488a3e44@oracle.com>
 <5A3932D402000078001987C5@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: <78016d07-d4dc-e6d1-fba2-995d2299bd8f@oracle.com>
Date: Tue, 19 Dec 2017 10:03:06 -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: <5A3932D402000078001987C5@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: 2623
Lines: 68

On 12/19/2017 09:40 AM, Jan Beulich wrote:
>>>> 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.


There should be a stack dump which will make it clear which routine failed.


>
>>>> +	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.


I think we will first need to fix xen_memory_setup() to do that too and
that would be a separate patch.

I am also not clear how this will work on earlier version of the
hypervisor that didn't support querying for size. From what I am seeing
in 4.4 we will get -EFAULT if the buffer is NULL.


>
>>>> +	/* 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.

But did you want to have some changes in the preceding comment? Not sure
I read your comment correctly.

-boris