On 8/24/2023 6:13 AM, Ilpo Järvinen wrote:
>> Currently this feature only supports those and it also
>> relies on the TSC which is an x86 thing.
>
> I wonder why you have to rely on that. Why e.g. ktime_get_boottime() is
> not enough for this usecase?
I was motivated to make the tracing as unobtrusive as possible and it seemed
like the rdtsc instruction would be very fast and very precise. I'll have to
profile ktime_get_boottime() to see how much overhead that adds.
>>>> + ptr = uart_debug->line + uart_debug->offset;
>>>> + len = strlen(ptr);
>>> Why you need to calculate length? Shouldn't queue_remove() be able to return
>>> this information?
>>
>> Yes, we can return the string length from queue_remove() but we still need to
>> call strlen() to accommodate all code paths. The user might call read() with
>> a very small buffer and that requires us to advance ptr past the beginning of
>> the string on subsequent calls.
>
> I still find it hard to believe you could not keep track of it all
> without strlen(), snprintf() returns the number of chars it wrote to
> uart_debug->line so it should be that length - uart_debug->offset?
True. I could store the total string length and trade a little more memory and
a little more complexity for a little less CPU time. Do you think it's a good
tradeoff? The strlen() call should be pretty fast since the string is a maximum
of 27 characters. Also, this is in the trace consumer path where performance
is not as important as the trace producer path.
>>>> + static uint64_t cpu_freq; /* cycles per second */
>>>> + uint32_t h, m, s, us;
>>>> +
>>>> + if (cpu_freq == 0)
>>>> + cpu_freq = arch_freq_get_on_cpu(0) * 1000ULL;
>>>> +
>>>> + s = div64_u64_rem(cpu_cycles, cpu_freq, &cpu_cycles);
>>>> + us = div64_u64(cpu_cycles * 1000 * 1000 + 500 * 1000, cpu_freq);
>>>> +
>>>> + m = s / 60; s = s % 60;
>>>> + h = m / 60; m = m % 60;
>>>> +
>>>> + snprintf(buf, size, "%02d:%02d:%02d.%06u", h, m, s, us);
>>> seconds.us is enough. If some additional formatting is to happen, it
>>> should be done in userspace.
>>
>> I can see your point. If the user does want to reformat this it will be
>> easier to start with the format you suggested. Is this a general rule for
>> kernel space?
>
> I don't know if there's a rule. But having had to parse those :: inputs
> way too many times in the past, I have very little love for that format
> being forced on user ;-).
OK, I've changed the format from "hh:mm:ss:mmmuuu" to "ssssssss:mmmuuu".
>>>> +/*
>>>> + * Create the debugfs interface. This should be called during port
>>>> registration after
>>>> + * port->name, port->serial_in, and port->serial_out have been
>>>> initialized.
>>>> We are
>>>> + * using port->private_data to store a pointer to our data structure.
>>>> That
>>>> field appears
>>>> + * to be otherwise unused. If this is wrong we will need to create a
>>>> new
>>>> field.
>>>> + */
>>>> +void uart_debug_create(struct uart_port *port)
>>>> +{
>>>> + struct uart_debug *uart_debug;
>>>> + struct dentry *dir;
>>>> +
>>>> + uart_debug = port->private_data = kzalloc(sizeof(struct uart_debug),
>>>> GFP_KERNEL);
>>> How about the drivers which use port->private_data?
>>
>> It didn't look like this field was used. Was I wrong about this?
>
> ~/linux/uart/drivers/tty/serial/8250$ git grep 'private_data =' | wc -l
> 20
>
> There are multiple 8250 variant drivers using it.
>
> Some also come with additional registers so it's all relevant also in
> serial/8250/ domain.
OK, I've added a new field named 'debug_data' and I'll use that instead of
'private_data'. My latest changes are in the v3 patch.
