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

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>, Julien Grall <julien.grall@arm.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>,
	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 13:10:48 +0200	[thread overview]
Message-ID: <556c3cce-90aa-6ce3-4ebf-4f97252aeb38@suse.com> (raw)
In-Reply-To: <1c59fe9b-299f-61b9-bd9c-35930a759728@suse.com>

On 03.09.19 12:51, Jan Beulich wrote:
> On 28.08.2019 10:00, Juergen Gross wrote:
>> @@ -24,32 +25,62 @@ struct debugtrace_data_s {
>>   };
>>   
>>   static struct debugtrace_data_s *debtr_data;
>> +static DEFINE_PER_CPU(struct debugtrace_data_s *, debtr_cpu_data);
>>   
>> -static unsigned int debugtrace_kilobytes = 128;
>> +static unsigned int debugtrace_bytes = 128 << 10;
> 
> And after patch 3 this is now left as "unsigned int"?

Good catch. :-)

> 
>> +static bool debugtrace_per_cpu;
>>   static bool debugtrace_used;
>>   static DEFINE_SPINLOCK(debugtrace_lock);
>> -integer_param("debugtrace", debugtrace_kilobytes);
>>   
>> -static void debugtrace_dump_worker(void)
>> +static int __init debugtrace_parse_param(const char *s)
>> +{
>> +    if ( !strncmp(s, "cpu:", 4) )
>> +    {
>> +        debugtrace_per_cpu = true;
>> +        s += 4;
>> +    }
>> +    debugtrace_bytes =  parse_size_and_unit(s, NULL);
> 
> Stray double blank.

Yes. Will remove one.

> 
>> +    return 0;
>> +}
>> +custom_param("debugtrace", debugtrace_parse_param);
>> +
>> +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.

> 
>> @@ -156,33 +195,70 @@ static void debugtrace_key(unsigned char key)
>>       debugtrace_toggle();
>>   }
>>   
>> -static int __init debugtrace_init(void)
>> +static void debugtrace_alloc_buffer(struct debugtrace_data_s **ptr)
>>   {
>>       int order;
>> -    unsigned long kbytes, bytes;
>>       struct debugtrace_data_s *data;
>>   
>> -    /* Round size down to next power of two. */
>> -    while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
>> -        debugtrace_kilobytes = kbytes;
>> -
>> -    bytes = debugtrace_kilobytes << 10;
>> -    if ( bytes == 0 )
>> -        return 0;
>> +    if ( debugtrace_bytes < PAGE_SIZE || *ptr )
> 
> Why the check against PAGE_SIZE?

With recaclulating debugtrace_bytes this can be dropped.

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

> 
>> +        return;
>> +    }
>> +
>> +    memset(data, '\0', debugtrace_bytes);
>> +    data->bytes = debugtrace_bytes - sizeof(*data);
>> +
>> +    *ptr = data;
>> +}
>> +
>> +static int debugtrace_cpu_callback(struct notifier_block *nfb,
>> +                                   unsigned long action, void *hcpu)
>> +{
>> +    unsigned int cpu = (unsigned long)hcpu;
>> +
>> +    /* Buffers are only ever allocated, never freed. */
>> +    switch ( action )
>> +    {
>> +    case CPU_UP_PREPARE:
>> +        debugtrace_alloc_buffer(&per_cpu(debtr_cpu_data, cpu));
>> +        break;
>> +    default:
>> +        break;
> 
> There no apparent need for "default:" here; quite possibly this
> could be if() instead of switch() anyway.

Fine with me.

Juergen

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