| 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: <CAF6AEGtueEoHCT1rg6G3BPAMaFMidr0un1i6X1VFMxq_xeUq7w@mail.gmail.com>
Date: Tue, 5 Sep 2023 15:23:31 -0700
From: Rob Clark <robdclark@...il.com>
To: Adrián Larumbe <adrian.larumbe@...labora.com>
Cc: maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
quic_abhinavk@...cinc.com, dmitry.baryshkov@...aro.org,
sean@...rly.run, marijn.suijten@...ainline.org, robh@...nel.org,
steven.price@....com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org, healych@...zon.com,
kernel@...labora.com, Tvrtko Ursulin <tvrtko.ursulin@...el.com>,
Rob Clark <robdclark@...omium.org>
Subject: Re: [PATCH v2 6/6] drm/drm-file: Allow size unit selection in drm_show_memory_stats
On Wed, Aug 30, 2023 at 8:51 AM Adrián Larumbe
<adrian.larumbe@...labora.com> wrote:
>
> >> The current implementation will try to pick the highest available
> >> unit. This is rather unflexible, and allowing drivers to display BO size
> >> statistics through fdinfo in units of their choice might be desirable.
> >>
> >> The new argument to drm_show_memory_stats is to be interpreted as the
> >> integer multiplier of a 10-power of 2, so 1 would give us size in Kib and 2
> >> in Mib. If we want drm-file functions to pick the highest unit, then 0
> >> should be passed.
> >>
> >> Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
> >> ---
> >> drivers/gpu/drm/drm_file.c | 22 +++++++++++++---------
> >> drivers/gpu/drm/msm/msm_drv.c | 2 +-
> >> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> >> include/drm/drm_file.h | 5 +++--
> >> 4 files changed, 18 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> >> index 762965e3d503..517e1fb8072a 100644
> >> --- a/drivers/gpu/drm/drm_file.c
> >> +++ b/drivers/gpu/drm/drm_file.c
> >> @@ -873,7 +873,7 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
> >> EXPORT_SYMBOL(drm_send_event);
> >>
> >> static void print_size(struct drm_printer *p, const char *stat,
> >> - const char *region, u64 sz)
> >> + const char *region, u64 sz, unsigned int unit)
> >> {
> >> const char *units[] = {"", " KiB", " MiB"};
> >> unsigned u;
> >> @@ -881,6 +881,8 @@ static void print_size(struct drm_printer *p, const char *stat,
> >> for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> >> if (sz < SZ_1K)
> >> break;
> >> + if (unit > 0 && unit == u)
> >> + break;
> >> sz = div_u64(sz, SZ_1K);
> >> }
> >>
> >> @@ -898,17 +900,18 @@ static void print_size(struct drm_printer *p, const char *stat,
> >> void drm_print_memory_stats(struct drm_printer *p,
> >> const struct drm_memory_stats *stats,
> >> enum drm_gem_object_status supported_status,
> >> - const char *region)
> >> + const char *region,
> >> + unsigned int unit)
> >
> >I'm not really adverse to changing what units we use.. or perhaps
> >changing the threshold to go to higher units to be 10000x or 100000x
> >of the previous unit. But I'm less excited about having different
> >drivers using different units.
> >
> >BR,
> >-R
>
> Would it be alright if I left it set to the default unit, and allow changing it
> at runtime with a debugfs file?
I suppose we could, but it does seem a bit like overkill. OTOH I
think it would make sense to increase the threshold, ie. switch to MiB
after 10MiB instead of 1MiB.. at that point the fractional component
is less significant..
BR,
-R
> >> {
> >> - print_size(p, "total", region, stats->private + stats->shared);
> >> - print_size(p, "shared", region, stats->shared);
> >> - print_size(p, "active", region, stats->active);
> >> + print_size(p, "total", region, stats->private + stats->shared, unit);
> >> + print_size(p, "shared", region, stats->shared, unit);
> >> + print_size(p, "active", region, stats->active, unit);
> >>
> >> if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> >> - print_size(p, "resident", region, stats->resident);
> >> + print_size(p, "resident", region, stats->resident, unit);
> >>
> >> if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
> >> - print_size(p, "purgeable", region, stats->purgeable);
> >> + print_size(p, "purgeable", region, stats->purgeable, unit);
> >> }
> >> EXPORT_SYMBOL(drm_print_memory_stats);
> >>
> >> @@ -916,11 +919,12 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> >> * drm_show_memory_stats - Helper to collect and show standard fdinfo memory stats
> >> * @p: the printer to print output to
> >> * @file: the DRM file
> >> + * @unit: multipliyer of power of two exponent of desired unit
> >> *
> >> * Helper to iterate over GEM objects with a handle allocated in the specified
> >> * file.
> >> */
> >> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> >> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, unsigned int unit)
> >> {
> >> struct drm_gem_object *obj;
> >> struct drm_memory_stats status = {};
> >> @@ -967,7 +971,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> >> }
> >> spin_unlock(&file->table_lock);
> >>
> >> - drm_print_memory_stats(p, &status, supported_status, "memory");
> >> + drm_print_memory_stats(p, &status, supported_status, "memory", unit);
> >> }
> >> EXPORT_SYMBOL(drm_show_memory_stats);
> >>
> >> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> >> index 2a0e3529598b..cd1198151744 100644
> >> --- a/drivers/gpu/drm/msm/msm_drv.c
> >> +++ b/drivers/gpu/drm/msm/msm_drv.c
> >> @@ -1067,7 +1067,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >>
> >> msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
> >>
> >> - drm_show_memory_stats(p, file);
> >> + drm_show_memory_stats(p, file, 0);
> >> }
> >>
> >> static const struct file_operations fops = {
> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> >> index 93d5f5538c0b..79c08cee3e9d 100644
> >> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> >> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> >> @@ -563,7 +563,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >>
> >> panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> >>
> >> - drm_show_memory_stats(p, file);
> >> + drm_show_memory_stats(p, file, 1);
> >> }
> >>
> >> static const struct file_operations panfrost_drm_driver_fops = {
> >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> >> index 010239392adf..21a3b022dd63 100644
> >> --- a/include/drm/drm_file.h
> >> +++ b/include/drm/drm_file.h
> >> @@ -466,9 +466,10 @@ enum drm_gem_object_status;
> >> void drm_print_memory_stats(struct drm_printer *p,
> >> const struct drm_memory_stats *stats,
> >> enum drm_gem_object_status supported_status,
> >> - const char *region);
> >> + const char *region,
> >> + unsigned int unit);
> >>
> >> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
> >> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, unsigned int unit);
> >> void drm_show_fdinfo(struct seq_file *m, struct file *f);
> >>
> >> struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);
> >> --
> >> 2.42.0
> >>
Powered by blists - more mailing lists