Re: [Xen-devel] [PATCH v3 4/4] xen: add per-cpu buffer option to debugtrace

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@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 4/4] xen: add per-cpu buffer option to debugtrace
Date: Tue, 3 Sep 2019 14:01:59 +0200	[thread overview]
Message-ID: <1acbfafa-0ca7-05d4-393a-040f54245fa5@suse.com> (raw)
In-Reply-To: <556c3cce-90aa-6ce3-4ebf-4f97252aeb38@suse.com>

On 03.09.2019 13:10, Juergen Gross wrote:
> On 03.09.19 12:51, Jan Beulich wrote:
>> On 28.08.2019 10:00, Juergen Gross wrote:
>>> +static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
>>> +                                   const char *which)
>>>   {
>>> -    if ( !debtr_data || !debugtrace_used )
>>> +    if ( !data )
>>>           return;
>>>   
>>> -    printk("debugtrace_dump() starting\n");
>>> +    printk("debugtrace_dump() %s buffer starting\n", which);
>>>   
>>>       /* Print oldest portion of the ring. */
>>> -    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
>>> -    if ( debtr_data->buf[debtr_data->prd] != '\0' )
>>> -        console_serial_puts(&debtr_data->buf[debtr_data->prd],
>>> -                            debtr_data->bytes - debtr_data->prd - 1);
>>> +    ASSERT(data->buf[data->bytes - 1] == 0);
>>> +    if ( data->buf[data->prd] != '\0' )
>>> +        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 1);
>>
>> Seeing this getting changed yet another time I now really wonder if
>> this nul termination is really still needed now that a size is being
>> passed into the actual output function. If you got rid of this in an
>> early prereq patch, the subsequent (to that one) ones would shrink.
> 
> Yes.
> 
>>
>> Furthermore I can't help thinking that said change to pass the size
>> into the actual output functions actually broke the logic here: By
>> memset()-ing the buffer to zero, output on a subsequent invocation
>> would have been suitably truncated (in fact, until prd had wrapped,
>> I think it would have got truncated more than intended). Julien,
>> can you please look into this apparent regression?
> 
> I can do that. Resetting prd to 0 when clearing the buffer is
> required here.

I'm afraid it's not this simple: Doing so will confuse
debugtrace_printk() - consider the function then storing the
previously latched last_prd into debugtrace_prd.

>>> -    order = get_order_from_bytes(bytes);
>>> +    order = get_order_from_bytes(debugtrace_bytes);
>>>       data = alloc_xenheap_pages(order, 0);
>>>       if ( !data )
>>> -        return -ENOMEM;
>>> +    {
>>> +        printk("failed to allocate debugtrace buffer\n");
>>
>> Perhaps better to also indicate which/whose buffer?
> 
> Hmm, I'm not sure this is really required. I can add it if you want, but
> as a user of debugtrace I'd be only interested in the information
> whether I can expect all trace entries to be seen or not.

Well, if the allocation fails for a CPU, it's not impossible for
the CPU bringup to then also fail. Subsequent to this the system
would then still provide an almost complete set of debugtrace
entries (ones issued by subsequent bringup actions would be
missing), _despite_ this log message.

Jan

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