linux-kernel - Re: [PATCH] drm/v3d: Add DRM_IOCTL_V3D_PERFMON_SET

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH9NwWd8iWALZbVkcPUsMGWNZSgh-8ARgyHSTULJpOqVj+88zw@mail.gmail.com>
Date: Thu, 31 Oct 2024 21:57:13 +0100
From: Christian Gmeiner <christian.gmeiner@...il.com>
To: Maíra Canal <mcanal@...lia.com>
Cc: Melissa Wen <mwen@...lia.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, kernel-dev@...lia.com, 
	Christian Gmeiner <cgmeiner@...lia.com>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/v3d: Add DRM_IOCTL_V3D_PERFMON_SET_GLOBAL

Hi Maíra,

>
> I have one major issue with this approach: I don't think we should
> introduce a `global_perfmon` in `struct v3d_perfmon_info`. `struct
> v3d_perfmon_info` was created to store information about the counters,
> such as total number of perfcnts supported and the description of the
> counters.
>

Ah okay.. got the idea of global_perfmon.

>
> I believe you should use `active_perfmon` in your implementation and
> don't create `global_perfmon`. This is going to make the code less
> tricky to understand and it's going to make sure that the hardware inner
> working is transparent in software.
>
>
> Only one perfmon can be active in a given moment of time, therefore,
> let's use `active_perfmon` to represent it.
>

Relying solely on active_perfmon makes the code hard to follow. I need at
least a flag to indicate whether we are in global perfmon mode.

> I couple more things came to my attention. First, I don't think we need
> to limit the creation of other perfmons. We can create perfmons and
> don't use it for a while. We only need to make sure that the user can't
> attach perfmons to jobs, when the global perfmon is enabled.
>
> For sure, if we go through this strategy, there is no need to have a
> count of all the perfmons that V3D has.
>

That is a fantastic idea.

> I would prefer to treat the global perfmon as a state. Ideally, we would
> enable and disable this state through the IOCTL.
>
> One last thing is: don't forget to stop the perfmons when you don't use
> it anymore :)
>

I've sent v2 of this patch and hope I've addressed all your comments.

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy