| 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
| ||
|
Message-ID: <4c8da281-fb6a-423a-bf8f-56c9ee45514f@intel.com>
Date: Fri, 18 Apr 2025 16:02:08 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>
CC: <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v3 11/26] fs/resctrl: Add support for additional monitor
event display formats
Hi Tony,
Please add context.
On 4/7/25 4:40 PM, Tony Luck wrote:
> Add a type field to both the mon_evt and mon_data structures.
This is getting too much? How about mon_data pointing to mon_evt?
>
> Legacy monitor events are still all displayed as an unsigned decimal
> 64-bit integer.
>
> Add an additional format of fixed-point with 18 binary places displayed
> as a floating point value with six decimal places.
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> include/linux/resctrl_types.h | 8 ++++++++
> fs/resctrl/internal.h | 4 ++++
> fs/resctrl/ctrlmondata.c | 23 ++++++++++++++++++++++-
> fs/resctrl/monitor.c | 3 +++
> fs/resctrl/rdtgroup.c | 1 +
> 5 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index 898068a99ef7..fbd4b55c41aa 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
Why is this exposed to architecture? There are no users in arch code.
changelog gives no hints on how to interpret this.
> @@ -58,6 +58,14 @@ enum resctrl_event_id {
> #define QOS_NUM_MBM_EVENTS (QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
> #define MBM_EVENT_IDX(evt) ((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
>
> +/*
> + * Event value display types
How about "Event value display formats"?
> + */
> +enum resctrl_event_type {
resctrl_event_format/resctrl_event_fmt?
> + EVT_TYPE_U64,
EVT_FMT_U64/EVT_FORMAT_U64?
> + EVT_TYPE_U46_18,
> +};
> +
> static inline bool resctrl_is_mbm_event(int e)
> {
> return (e >= QOS_L3_MBM_TOTAL_EVENT_ID &&
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 74a77794364d..4a840e683e96 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -71,6 +71,7 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
> * struct mon_evt - Entry in the event list of a resource
> * @evtid: event id
> * @name: name of the event
> + * @type: format for display to user
> * @configurable: true if the event is configurable
> * @any_cpu: true if this event can be read from any CPU
> * @list: entry in &rdt_resource->evt_list
> @@ -79,6 +80,7 @@ struct mon_evt {
> enum resctrl_event_id evtid;
> enum resctrl_res_level rid;
> char *name;
> + enum resctrl_event_type type;
> bool configurable;
> bool any_cpu;
> struct list_head list;
> @@ -89,6 +91,7 @@ struct mon_evt {
> * @list: List of all allocated structures.
> * @rid: Resource id associated with the event file.
> * @evtid: Event id associated with the event file.
> + * @type: Format for display to user
> * @sum: Set when event must be summed across multiple
> * domains.
> * @domid: When @sum is zero this is the domain to which
> @@ -104,6 +107,7 @@ struct mon_data {
> struct list_head list;
> unsigned int rid;
> enum resctrl_event_id evtid;
> + enum resctrl_event_type type;
> unsigned int sum;
> unsigned int domid;
> bool any_cpu;
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index cd77960657f0..5ea8113c96ac 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -562,6 +562,27 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
> }
>
> +#define NUM_FRAC_BITS 18
> +#define FRAC_MASK GENMASK(NUM_FRAC_BITS - 1, 0)
> +
> +static void show_value(struct seq_file *m, enum resctrl_event_type type, u64 val)
show_value() is a bit vague ... print_event_value() ?
> +{
> + u64 frac;
> +
> + switch (type) {
> + case EVT_TYPE_U64:
> + seq_printf(m, "%llu\n", val);
> + break;
> + case EVT_TYPE_U46_18:
> + frac = val & FRAC_MASK;
> + frac = frac * 1000000;
> + frac += 1ul << (NUM_FRAC_BITS - 1);
Could you please help me understand why above line is needed? Seems like
shift below will just undo it?
> + frac >>= NUM_FRAC_BITS;
> + seq_printf(m, "%llu.%06llu\n", val >> NUM_FRAC_BITS, frac);
> + break;
> + }
> +}
> +
> int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> {
> struct kernfs_open_file *of = m->private;
> @@ -633,7 +654,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> else if (rr.err == -EINVAL)
> seq_puts(m, "Unavailable\n");
> else
> - seq_printf(m, "%llu\n", rr.val);
> + show_value(m, md->type, rr.val);
>
> out:
> rdtgroup_kn_unlock(of->kn);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 1cf0b085e07a..1efad57d1d85 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -847,16 +847,19 @@ static struct mon_evt all_events[QOS_NUM_EVENTS] = {
> .name = "llc_occupancy",
> .evtid = QOS_L3_OCCUP_EVENT_ID,
> .rid = RDT_RESOURCE_L3,
> + .type = EVT_TYPE_U64,
> },
> [QOS_L3_MBM_TOTAL_EVENT_ID] = {
> .name = "mbm_total_bytes",
> .evtid = QOS_L3_MBM_TOTAL_EVENT_ID,
> .rid = RDT_RESOURCE_L3,
> + .type = EVT_TYPE_U64,
> },
> [QOS_L3_MBM_LOCAL_EVENT_ID] = {
> .name = "mbm_local_bytes",
> .evtid = QOS_L3_MBM_LOCAL_EVENT_ID,
> .rid = RDT_RESOURCE_L3,
> + .type = EVT_TYPE_U64,
> },
> };
>
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 97c2ba8af930..bd41f7a0f416 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2927,6 +2927,7 @@ static struct mon_data *mon_get_kn_priv(int rid, int domid, struct mon_evt *mevt
> priv->sum = do_sum;
> priv->evtid = mevt->evtid;
> priv->any_cpu = mevt->any_cpu;
> + priv->type = mevt->type;
> list_add_tail(&priv->list, &kn_priv_list);
>
> return priv;
Reinette
Powered by blists - more mailing lists