| 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: <e6500d5b-0151-4c31-9032-3a23157b3126@ursulin.net>
Date: Mon, 15 Apr 2024 13:50:19 +0100
From: Tvrtko Ursulin <tursulin@...ulin.net>
To: Rob Clark <robdclark@...il.com>,
Adrián Larumbe <adrian.larumbe@...labora.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Hans de Goede <hdegoede@...hat.com>, kernel@...labora.com,
Boris Brezillon <boris.brezillon@...labora.com>,
Christopher Healy <healych@...zon.com>, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/sysfs: Add drm class-wide attribute to get active
device clients
On 05/04/2024 18:59, Rob Clark wrote:
> On Wed, Apr 3, 2024 at 11:37 AM Adrián Larumbe
> <adrian.larumbe@...labora.com> wrote:
>>
>> Up to this day, all fdinfo-based GPU profilers must traverse the entire
>> /proc directory structure to find open DRM clients with fdinfo file
>> descriptors. This is inefficient and time-consuming.
>>
>> This patch adds a new device class attribute that will install a sysfs file
>> per DRM device, which can be queried by profilers to get a list of PIDs for
>> their open clients. This file isn't human-readable, and it's meant to be
>> queried only by GPU profilers like gputop and nvtop.
>>
>> Cc: Boris Brezillon <boris.brezillon@...labora.com>
>> Cc: Tvrtko Ursulin <tursulin@...ulin.net>
>> Cc: Christopher Healy <healych@...zon.com>
>> Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
>
> It does seem like a good idea.. idk if there is some precedent to
> prefer binary vs ascii in sysfs, but having a way to avoid walking
> _all_ processes is a good idea.
I naturally second that it is a needed feature, but I do not think
binary format is justified. AFAIR it should be used for things like
hw/fw standardised tables or firmware images, not when exporting a
simple list of PIDs. It also precludes easy shell/script access and the
benefit of avoiding parsing a short list is I suspect completely dwarfed
by needing to parse all the related fdinfo etc.
>> ---
>> drivers/gpu/drm/drm_internal.h | 2 +-
>> drivers/gpu/drm/drm_privacy_screen.c | 2 +-
>> drivers/gpu/drm/drm_sysfs.c | 89 ++++++++++++++++++++++------
>> 3 files changed, 74 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 2215baef9a3e..9a399b03d11c 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -145,7 +145,7 @@ bool drm_master_internal_acquire(struct drm_device *dev);
>> void drm_master_internal_release(struct drm_device *dev);
>>
>> /* drm_sysfs.c */
>> -extern struct class *drm_class;
>> +extern struct class drm_class;
>>
>> int drm_sysfs_init(void);
>> void drm_sysfs_destroy(void);
>> diff --git a/drivers/gpu/drm/drm_privacy_screen.c b/drivers/gpu/drm/drm_privacy_screen.c
>> index 6cc39e30781f..2fbd24ba5818 100644
>> --- a/drivers/gpu/drm/drm_privacy_screen.c
>> +++ b/drivers/gpu/drm/drm_privacy_screen.c
>> @@ -401,7 +401,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
>> mutex_init(&priv->lock);
>> BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
>>
>> - priv->dev.class = drm_class;
>> + priv->dev.class = &drm_class;
>> priv->dev.type = &drm_privacy_screen_type;
>> priv->dev.parent = parent;
>> priv->dev.release = drm_privacy_screen_device_release;
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index a953f69a34b6..56ca9e22c720 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -58,8 +58,6 @@ static struct device_type drm_sysfs_device_connector = {
>> .name = "drm_connector",
>> };
>>
>> -struct class *drm_class;
>> -
>> #ifdef CONFIG_ACPI
>> static bool drm_connector_acpi_bus_match(struct device *dev)
>> {
>> @@ -128,6 +126,62 @@ static const struct component_ops typec_connector_ops = {
>>
>> static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
>>
>> +static ssize_t clients_show(struct device *cd, struct device_attribute *attr, char *buf)
>> +{
>> + struct drm_minor *minor = cd->driver_data;
>> + struct drm_device *ddev = minor->dev;
>> + struct drm_file *priv;
>> + ssize_t offset = 0;
>> + void *pid_buf;
>> +
>> + if (minor->type != DRM_MINOR_RENDER)
>> + return 0;
Why this?
>> +
>> + pid_buf = kvmalloc(PAGE_SIZE, GFP_KERNEL);
I don't quite get the kvmalloc for just one page (or why even a temporay
buffer and not write into buf directly?).
>> + if (!pid_buf)
>> + return 0;
>> +
>> + mutex_lock(&ddev->filelist_mutex);
>> + list_for_each_entry_reverse(priv, &ddev->filelist, lhead) {
>> + struct pid *pid;
>> +
>> + if (drm_WARN_ON(ddev, (PAGE_SIZE - offset) < sizeof(pid_t)))
>> + break;
Feels bad.. I would suggest exploring implementing a read callback
(instead of show) and handling arbitrary size output.
>> +
>> + rcu_read_lock();
>> + pid = rcu_dereference(priv->pid);
>> + (*(pid_t *)(pid_buf + offset)) = pid_vnr(pid);
>> + rcu_read_unlock();
>> +
>> + offset += sizeof(pid_t);
>> + }
>> + mutex_unlock(&ddev->filelist_mutex);
>> +
>> + if (offset < PAGE_SIZE)
>> + (*(pid_t *)(pid_buf + offset)) = 0;
Either NULL terminated or PAGE_SIZE/sizeof(pid) entries and not NULL
terminated feels weird. If I got that right.
For me everything points towards going for text output.
>> +
>> + memcpy(buf, pid_buf, offset);
>> +
>> + kvfree(pid_buf);
>> +
>> + return offset;
>> +
>> +}
>> +static DEVICE_ATTR_RO(clients);
Shouldn't BIN_ATTR_RO be used for binary files in sysfs?
Regards,
Tvrtko
P.S. Or maybe it is time for drmfs? Where each client gets a directory
and drivers can populate files. Such as per client logging streams and
whatnot.
>> +
>> +static struct attribute *drm_device_attrs[] = {
>> + &dev_attr_clients.attr,
>> + NULL,
>> +};
>> +ATTRIBUTE_GROUPS(drm_device);
>> +
>> +struct class drm_class = {
>> + .name = "drm",
>> + .dev_groups = drm_device_groups,
>> +};
>> +
>> +static bool drm_class_initialised;
>> +
>> /**
>> * drm_sysfs_init - initialize sysfs helpers
>> *
>> @@ -142,18 +196,19 @@ int drm_sysfs_init(void)
>> {
>> int err;
>>
>> - drm_class = class_create("drm");
>> - if (IS_ERR(drm_class))
>> - return PTR_ERR(drm_class);
>> + err = class_register(&drm_class);
>> + if (err)
>> + return err;
>>
>> - err = class_create_file(drm_class, &class_attr_version.attr);
>> + err = class_create_file(&drm_class, &class_attr_version.attr);
>> if (err) {
>> - class_destroy(drm_class);
>> - drm_class = NULL;
>> + class_destroy(&drm_class);
>> return err;
>> }
>>
>> - drm_class->devnode = drm_devnode;
>> + drm_class.devnode = drm_devnode;
>> +
>> + drm_class_initialised = true;
>>
>> drm_sysfs_acpi_register();
>> return 0;
>> @@ -166,12 +221,12 @@ int drm_sysfs_init(void)
>> */
>> void drm_sysfs_destroy(void)
>> {
>> - if (IS_ERR_OR_NULL(drm_class))
>> + if (!drm_class_initialised)
>> return;
>> drm_sysfs_acpi_unregister();
>> - class_remove_file(drm_class, &class_attr_version.attr);
>> - class_destroy(drm_class);
>> - drm_class = NULL;
>> + class_remove_file(&drm_class, &class_attr_version.attr);
>> + class_destroy(&drm_class);
>> + drm_class_initialised = false;
>> }
>>
>> static void drm_sysfs_release(struct device *dev)
>> @@ -372,7 +427,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>> return -ENOMEM;
>>
>> device_initialize(kdev);
>> - kdev->class = drm_class;
>> + kdev->class = &drm_class;
>> kdev->type = &drm_sysfs_device_connector;
>> kdev->parent = dev->primary->kdev;
>> kdev->groups = connector_dev_groups;
>> @@ -550,7 +605,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>> minor_str = "card%d";
>>
>> kdev->devt = MKDEV(DRM_MAJOR, minor->index);
>> - kdev->class = drm_class;
>> + kdev->class = &drm_class;
>> kdev->type = &drm_sysfs_device_minor;
>> }
>>
>> @@ -579,10 +634,10 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>> */
>> int drm_class_device_register(struct device *dev)
>> {
>> - if (!drm_class || IS_ERR(drm_class))
>> + if (!drm_class_initialised)
>> return -ENOENT;
>>
>> - dev->class = drm_class;
>> + dev->class = &drm_class;
>> return device_register(dev);
>> }
>> EXPORT_SYMBOL_GPL(drm_class_device_register);
>>
>> base-commit: 45c734fdd43db14444025910b4c59dd2b8be714a
>> --
>> 2.44.0
>>
Powered by blists - more mailing lists