Re: [Xen-devel] [PATCH v3 3/4] xen: refactor debugtrace data

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v3 3/4] xen: refactor debugtrace data
Date: Tue, 3 Sep 2019 15:26:39 +0200	[thread overview]
Message-ID: <439c045b-127b-1944-a47d-4d33a0e42773@suse.com> (raw)
In-Reply-To: <8c915d3c-c022-a397-bf43-db98111ffd0a@suse.com>

On 03.09.19 13:50, Jan Beulich wrote:
> On 03.09.2019 12:31, Juergen Gross wrote:
>> On 03.09.19 12:16, Jan Beulich wrote:
>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>> +static unsigned int debugtrace_kilobytes = 128;
>>>
>>> Since you touch this anyway, add __initdata? Maybe also move it
>>> next to its integer_param()?
>>
>> This is modified in patch 4 again, and there I need it in the running
>> system for support of cpu hotplug with per-cpu buffers.
> 
> Right, I've meanwhile noticed this. Hence it's fine to keep as is.
> 
>>>> @@ -165,12 +171,14 @@ static int __init debugtrace_init(void)
>>>>            return 0;
>>>>    
>>>>        order = get_order_from_bytes(bytes);
>>>> -    debugtrace_buf = alloc_xenheap_pages(order, 0);
>>>> -    ASSERT(debugtrace_buf != NULL);
>>>> +    data = alloc_xenheap_pages(order, 0);
>>>> +    if ( !data )
>>>> +        return -ENOMEM;
>>>>    
>>>> -    memset(debugtrace_buf, '\0', bytes);
>>>> +    memset(data, '\0', bytes);
>>>>    
>>>> -    debugtrace_bytes = bytes;
>>>> +    data->bytes = bytes - sizeof(*data);
>>>> +    debtr_data = data;
>>>
>>> The allocation may end up being almost twice as big as what gets
>>> actually used this way. Wouldn't it make sense to re-calculate
>>> "bytes" from "order"?
>>
>> Yes, you are right.
> 
> Actually I wasn't, which I did notice seeing the relevant piece
> of code getting touched in patch 4:
> 
>      while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
>          debugtrace_kilobytes = kbytes;

For kbytes < 4 you still were right.

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel