| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 17 Oct 2022 17:37:17 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ira Weiny <ira.weiny@...el.com>
CC: Dan Williams <dan.j.williams@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>,
"Ben Widawsky" <bwidawsk@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Davidlohr Bueso <dave@...olabs.net>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [RFC V2 PATCH 05/11] cxl/mem: Trace General Media Event Record
> >
> > > diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> > > index 318ba5fe046e..82a8d3b750a2 100644
> > > --- a/include/trace/events/cxl.h
> > > +++ b/include/trace/events/cxl.h
> > > @@ -122,6 +122,114 @@ TRACE_EVENT(generic_event,
> >
> >
> > > +#define CXL_GMER_VALID_CHANNEL BIT(0)
> > > +#define CXL_GMER_VALID_RANK BIT(1)
> > > +#define CXL_GMER_VALID_DEVICE BIT(2)
> > > +#define CXL_GMER_VALID_COMPONENT BIT(3)
> > > +#define show_valid_flags(flags) __print_flags(flags, "|", \
> > > + { CXL_GMER_VALID_CHANNEL, "CHANNEL" }, \
> > > + { CXL_GMER_VALID_RANK, "RANK" }, \
> > > + { CXL_GMER_VALID_DEVICE, "DEVICE" }, \
> > > + { CXL_GMER_VALID_COMPONENT, "COMPONENT" } \
> > > +)
> >
> > I'd still rather we only had the TP_printk only print those parts that are valid
> > rather than the mess of having to go check the validity bit before deciding whether
> > to take notice of the field. But meh, not that important given thats
> > not the main intended way to consume this data.
> >
>
> Ok I spent some time really thinking about this and below is an attempt at
> that.
>
> However, I must be missing something in what you are proposing because I don't
> like having extra space in the trace buffer to print into like this and I
> can't figure out where else to put a print buffer.
Putting the space in the trace buffer definitely doesn't makes sense.
Ah. Looking back the CCIX code I assumed it was serialized so could use a
static buffer.
Looking at other similar cases though and we have a lot of use
of trace_seq_printf() e.g. libata_trace_parse_status() though note
there is some magic macro stuff in include/trace/events/libata.h
to tie that together.
https://elixir.bootlin.com/linux/latest/source/drivers/ata/libata-trace.c#L14
That seems to get you access to the actual buffer we are printing into
in similar cases.
>
> Also note that this will have no impact on most of the user space tools which
> use libtracefs as they will see all the fields and will need to do a similar
> decode.
>
> Do you really think this is worth the effort?
With the allocation problem, definitely not. With that solved, it's not a huge amount
of extra code. Also rather nicely 'documents' meaning of the valid bits.
I'm a kernel hacker. I like to not need much beyond echo and cat :)
Jonathan
>
> Ira
>
>
> commit 54c4ee67bcac6a38cbc9b0ea2ef952e197356dee
> Author: Ira Weiny <ira.weiny@...el.com>
> Date: Fri Oct 14 16:15:27 2022 -0700
>
> squash: Attempt to print only valid fields per Jonathan suggestion
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2add473fc168..9e15028af79c 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -720,6 +720,28 @@ static const uuid_t gen_media_event_uuid =
> UUID_INIT(0xfbcd0a77, 0xc260, 0x417f,
> 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6);
>
> +const char *cxl_gem_print_valid(u8 *buf, int nr, u16 valid_flags, u8 channel,
> + u8 rank, u32 device, u8 *comp_id)
> +{
> + char *str = buf;
> + int rc = 0;
> +
> + if (valid_flags & CXL_GMER_VALID_CHANNEL)
> + rc = snprintf(str, nr, "channel=%u ", channel);
> +
> + if (valid_flags & CXL_GMER_VALID_RANK)
> + rc = snprintf(str + rc, nr - rc, "rank=%u ", rank);
> +
> + if (valid_flags & CXL_GMER_VALID_DEVICE)
> + rc = snprintf(str + rc, nr - rc, "device=%u ", device);
> +
> + if (valid_flags & CXL_GMER_VALID_COMPONENT)
> + rc = snprintf(str + rc, nr - rc, "comp_id=%*ph ",
> + CXL_EVENT_GEN_MED_COMP_ID_SIZE, comp_id);
> +
> + return str;
> +}
> +
> static void cxl_trace_event_record(const char *dev_name,
> enum cxl_event_log_type type,
> struct cxl_get_event_payload *payload)
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 8fbd20d8a0b2..3d3bfef69d32 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -429,6 +429,13 @@ struct cxl_event_gen_media {
> u8 reserved[0x2e];
> } __packed;
>
> +#define CXL_GMER_VALID_CHANNEL BIT(0)
> +#define CXL_GMER_VALID_RANK BIT(1)
> +#define CXL_GMER_VALID_DEVICE BIT(2)
> +#define CXL_GMER_VALID_COMPONENT BIT(3)
> +const char *cxl_gem_print_valid(u8 *buf, int nr, u16 valid_flags, u8 channel,
> + u8 rank, u32 device, u8 *comp_id);
> +
> struct cxl_mbox_get_partition_info {
> __le64 active_volatile_cap;
> __le64 active_persistent_cap;
> diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> index e3e11c9055ba..27bb790cb685 100644
> --- a/include/trace/events/cxl.h
> +++ b/include/trace/events/cxl.h
> @@ -161,16 +161,7 @@ TRACE_EVENT(generic_event,
> { CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT, "Internal Media Management" } \
> )
>
> -#define CXL_GMER_VALID_CHANNEL BIT(0)
> -#define CXL_GMER_VALID_RANK BIT(1)
> -#define CXL_GMER_VALID_DEVICE BIT(2)
> -#define CXL_GMER_VALID_COMPONENT BIT(3)
> -#define show_valid_flags(flags) __print_flags(flags, "|", \
> - { CXL_GMER_VALID_CHANNEL, "CHANNEL" }, \
> - { CXL_GMER_VALID_RANK, "RANK" }, \
> - { CXL_GMER_VALID_DEVICE, "DEVICE" }, \
> - { CXL_GMER_VALID_COMPONENT, "COMPONENT" } \
> -)
> +#define CXL_VALID_PRINT_STR_LEN 512
>
> TRACE_EVENT(general_media,
>
> @@ -191,6 +182,7 @@ TRACE_EVENT(general_media,
> __array(u8, comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE)
> __field(u16, validity_flags)
> __field(u8, rank) /* Out of order to pack trace record */
> + __array(u8, str, CXL_VALID_PRINT_STR_LEN)
> ),
>
> TP_fast_assign(
> @@ -209,17 +201,17 @@ TRACE_EVENT(general_media,
> __entry->validity_flags = get_unaligned_le16(&rec->validity_flags);
> ),
>
> - CXL_EVT_TP_printk("phys_addr=%llx volatile=%s desc='%s' type='%s' " \
> - "trans_type='%s' channel=%u rank=%u device=%x comp_id=%s " \
> - "valid_flags='%s'",
> + CXL_EVT_TP_printk("phys_addr=%llx volatile=%s desc='%s' type='%s' " \
> + "trans_type='%s' %s",
> __entry->phys_addr & CXL_GMER_PHYS_ADDR_MASK,
> (__entry->phys_addr & CXL_GMER_PHYS_ADDR_VOLATILE) ? "TRUE" : "FALSE",
> show_event_desc_flags(__entry->descriptor),
> show_mem_event_type(__entry->type),
> show_trans_type(__entry->transaction_type),
> - __entry->channel, __entry->rank, __entry->device,
> - __print_hex(__entry->comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE),
> - show_valid_flags(__entry->validity_flags)
> + cxl_gem_print_valid(__entry->str, CXL_VALID_PRINT_STR_LEN,
> + __entry->validity_flags, __entry->channel,
> + __entry->rank, __entry->device,
> + __entry->comp_id)
> )
> );
>
>
Powered by blists - more mailing lists