On Wed, 28 Feb 2024 11:47:24 +0100
Tobias Waldekranz <tobias@xxxxxxxxxxxxxx> wrote:
> >> > + TP_fast_assign(
> >> > + __entry->val = val;
> >> > + __assign_str(dev, info->dev ? netdev_name(info->dev) : "(null)");
> >> > + __entry->info = info;
> >> > + __entry->err = err;
> >> > + switchdev_notifier_str(val, info, __entry->msg, SWITCHDEV_TRACE_MSG_MAX);
> >>
> >> Is it possible to just store the information in the trace event and then
> >> call the above function in the read stage?
> >
> > I agree with Steven: it looks like that with the above code the
> > tracepoint itself will become measurably costily in terms of CPU
> > cycles: we want to avoid that.
> >
> > Perhaps using different tracepoints with different notifier_block type
> > would help? so that each trace point could just copy a few specific
> > fields.
>
> This can be done, but you will end up having to duplicate the decoding
> and formatting logic from switchdev-str.c, with the additional hurdle of
> having to figure out the sizes of all referenced objects in order to
> create flattened versions of every notification type.
Would it help if you could pass a trace_seq to it? The TP_printk() has a
"magical" trace_seq variable that trace events can use in the TP_printk()
called "p".
Look at:
include/trace/events/libata.h:
const char *libata_trace_parse_status(struct trace_seq*, unsigned char);
#define __parse_status(s) libata_trace_parse_status(p, s)
Where we have:
const char *
libata_trace_parse_status(struct trace_seq *p, unsigned char status)
{
const char *ret = trace_seq_buffer_ptr(p);
trace_seq_printf(p, "{ ");
if (status & ATA_BUSY)
trace_seq_printf(p, "BUSY ");
if (status & ATA_DRDY)
trace_seq_printf(p, "DRDY ");
if (status & ATA_DF)
trace_seq_printf(p, "DF ");
if (status & ATA_DSC)
trace_seq_printf(p, "DSC ");
if (status & ATA_DRQ)
trace_seq_printf(p, "DRQ ");
if (status & ATA_CORR)
trace_seq_printf(p, "CORR ");
if (status & ATA_SENSE)
trace_seq_printf(p, "SENSE ");
if (status & ATA_ERR)
trace_seq_printf(p, "ERR ");
trace_seq_putc(p, '}');
trace_seq_putc(p, 0);
return ret;
}
The "trace_seq p" is a pointer to trace_seq descriptor that can build
strings, and then you can use it to print a custom string in the trace
output.
>
> What I like about the current approach is that when new notification and
> object types are added, switchdev_notifier_str will automatically be
> able to decode them and give you some rough idea of what is going on,
> even if no new message specific decoding logic is added. It is also
> reusable by drivers that might want to decode notifications or objects
> in error messages.
>
> Would some variant of (how I understand) Steven's suggestion to instead
> store the formatted message in a dynamic array (__assign_str()), rather
> than in the tracepoint entry, be acceptable?
Matters if you could adapt using a trace_seq for the output. Or at least
use a seq_buf, as that's what is under the covers of trace_seq. If you
rather just use seq_buf, the above could pretty much be the same by passing
in: &p->seq.
-- Steve
