Re: [PATCH 02/17] drm/xe/perf/uapi: Add perf_stream_paranoid sysctl

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 19 Dec 2023 12:28:51 -0800, Dixit, Ashutosh wrote:
>
> On Thu, 07 Dec 2023 22:43:14 -0800, Ashutosh Dixit wrote:
> >
> > +static struct ctl_table perf_ctl_table[] = {
> > +	{
> > +	 .procname = "perf_stream_paranoid",
> > +	 .data = &xe_perf_stream_paranoid,
> > +	 .maxlen = sizeof(xe_perf_stream_paranoid),
> > +	 .mode = 0644,
> > +	 .proc_handler = proc_dointvec_minmax,
> > +	 .extra1 = SYSCTL_ZERO,
> > +	 .extra2 = SYSCTL_ONE,
> > +	 },
> > +	{}
> > +};
> > +
> > +int xe_perf_sysctl_register(void)
> > +{
> > +	sysctl_header = register_sysctl("dev/xe", perf_ctl_table);
> > +	return 0;
> > +}
>
> Any idea why this (and xe_oa_max_sample_rate) is create in /proc, rather
> than something attached to the module?
>
> We would want it to be per module, rather than per device, so that's one
> reason. What are the options for creating per module params? One is
> module_param itself. In that case this would appear in
> "/sys/module/xe/parameters/perf_stream_paranoid" rather than in
> "/proc/sys/dev/xe/perf_stream_paranoid".
>
> Module params are slightly simpler to manage than /proc stuff I think. Any
> other reason to prefer one over the other?
>
> Comments?

Looking into i915 history, seems these were created in /proc to follow what
the kernel perf subsystem does. However, considering there is nothing
common between kernel perf and xe/i915 perf subsystems, there seems to be
little reason to have these files where the kernel subsystem places similar
files (if the files are are indeed similar). These files might as well
created in xe module params or device level sysfs. Module param is probably
the least lines of code.

Anyway I've left them in /proc for now and probably doesn't matter a lot,
but I think we should think a little bit if we should move them to module
params or sysfs.

Thanks.
--
Ashutosh