Ocfs2 devel - Jun 2023 - [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays

On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula
wrote:
> On Wed, 21 Jun 2023, Joel Granados <j.granados at samsung.com> wrote:
> > Remove the empty end element from all the arrays that are passed to
the
> > register sysctl calls. In some files this means reducing the explicit
> > array size by one. Also make sure that we are using the size in
> > ctl_table_header instead of evaluating the .procname element.
> 
> Where's the harm in removing the end elements driver by driver? This is
> an unwieldy patch to handle.
I totally agree. Its a big one!!! but I'm concerned of breaking
bisectibility:
* I could for example separate all the removes into separate commits and
  then have a final commit that removes the check for the empty element.
  But this will leave the tree in a state where the for loop will have
  undefined behavior when it looks for the empty end element. It might
  or might not work (probably not :) until the final commit where I fix
  that.

* I could also change the logic that looks for the final element,
  commit that first and then remove the empty element one commit per
  driver after that. But then for all the arrays that still have an
  empty element, there would again be undefined behavior as it would
  think that the last element is valid (when it is really the sentinel).

Any ideas on how to get around these?
> 
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c
b/drivers/gpu/drm/i915/i915_perf.c
> > index f43950219ffc..e4d7372afb10 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct
drm_device *dev, void *data,
> >  
> >  static struct ctl_table oa_table[] = {
> >  	{
> > -	 .procname = "perf_stream_paranoid",
> > -	 .data = &i915_perf_stream_paranoid,
> > -	 .maxlen = sizeof(i915_perf_stream_paranoid),
> > -	 .mode = 0644,
> > -	 .proc_handler = proc_dointvec_minmax,
> > -	 .extra1 = SYSCTL_ZERO,
> > -	 .extra2 = SYSCTL_ONE,
> > -	 },
> > +		.procname = "perf_stream_paranoid",
> > +		.data = &i915_perf_stream_paranoid,
> > +		.maxlen = sizeof(i915_perf_stream_paranoid),
> > +		.mode = 0644,
> > +		.proc_handler = proc_dointvec_minmax,
> > +		.extra1 = SYSCTL_ZERO,
> > +		.extra2 = SYSCTL_ONE,
> > +	},
> >  	{
> > -	 .procname = "oa_max_sample_rate",
> > -	 .data = &i915_oa_max_sample_rate,
> > -	 .maxlen = sizeof(i915_oa_max_sample_rate),
> > -	 .mode = 0644,
> > -	 .proc_handler = proc_dointvec_minmax,
> > -	 .extra1 = SYSCTL_ZERO,
> > -	 .extra2 = &oa_sample_rate_hard_limit,
> > -	 },
> > -	{}
> > +		.procname = "oa_max_sample_rate",
> > +		.data = &i915_oa_max_sample_rate,
> > +		.maxlen = sizeof(i915_oa_max_sample_rate),
> > +		.mode = 0644,
> > +		.proc_handler = proc_dointvec_minmax,
> > +		.extra1 = SYSCTL_ZERO,
> > +		.extra2 = &oa_sample_rate_hard_limit,
> > +	}
> >  };
> 
> The existing indentation is off, but fixing it doesn't really belong in
> this patch.
Agreed. But I actually was trying to fix something that checkpatch
flagged. I'll change these back (which will cause this patch to be
flagged).

An alternative solution would be to fix the indentation as part of the
preparation patches. Tell me what you think.

Thx
> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
-- 

Joel Granados
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL:
<http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20230621/c98d19bf/attachment-0001.sig>

On Wed, 21 Jun 2023, Joel Granados <j.granados at samsung.com>
wrote:
> On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula wrote:
>> On Wed, 21 Jun 2023, Joel Granados <j.granados at samsung.com>
wrote:
>> > Remove the empty end element from all the arrays that are passed
to the
>> > register sysctl calls. In some files this means reducing the
explicit
>> > array size by one. Also make sure that we are using the size in
>> > ctl_table_header instead of evaluating the .procname element.
>> 
>> Where's the harm in removing the end elements driver by driver?
This is
>> an unwieldy patch to handle.
>
> I totally agree. Its a big one!!! but I'm concerned of breaking
bisectibility:
> * I could for example separate all the removes into separate commits and
>   then have a final commit that removes the check for the empty element.
>   But this will leave the tree in a state where the for loop will have
>   undefined behavior when it looks for the empty end element. It might
>   or might not work (probably not :) until the final commit where I fix
>   that.
>
> * I could also change the logic that looks for the final element,
>   commit that first and then remove the empty element one commit per
>   driver after that. But then for all the arrays that still have an
>   empty element, there would again be undefined behavior as it would
>   think that the last element is valid (when it is really the sentinel).
>
> Any ideas on how to get around these?
First add size to the register calls, and allow the last one to be
sentinel but do not require the sentinel.

Start removing sentinels, adjusting the size passed in.

Once enough sentinels have been removed, add warning if the final entry
is a sentinel.

Never really remove the check? (But surely you can rework the logic to
not count the number of elements up front, only while iterating.)


BR,
Jani.
>> 
>> > diff --git a/drivers/gpu/drm/i915/i915_perf.c
b/drivers/gpu/drm/i915/i915_perf.c
>> > index f43950219ffc..e4d7372afb10 100644
>> > --- a/drivers/gpu/drm/i915/i915_perf.c
>> > +++ b/drivers/gpu/drm/i915/i915_perf.c
>> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct
drm_device *dev, void *data,
>> >  
>> >  static struct ctl_table oa_table[] = {
>> >  	{
>> > -	 .procname = "perf_stream_paranoid",
>> > -	 .data = &i915_perf_stream_paranoid,
>> > -	 .maxlen = sizeof(i915_perf_stream_paranoid),
>> > -	 .mode = 0644,
>> > -	 .proc_handler = proc_dointvec_minmax,
>> > -	 .extra1 = SYSCTL_ZERO,
>> > -	 .extra2 = SYSCTL_ONE,
>> > -	 },
>> > +		.procname = "perf_stream_paranoid",
>> > +		.data = &i915_perf_stream_paranoid,
>> > +		.maxlen = sizeof(i915_perf_stream_paranoid),
>> > +		.mode = 0644,
>> > +		.proc_handler = proc_dointvec_minmax,
>> > +		.extra1 = SYSCTL_ZERO,
>> > +		.extra2 = SYSCTL_ONE,
>> > +	},
>> >  	{
>> > -	 .procname = "oa_max_sample_rate",
>> > -	 .data = &i915_oa_max_sample_rate,
>> > -	 .maxlen = sizeof(i915_oa_max_sample_rate),
>> > -	 .mode = 0644,
>> > -	 .proc_handler = proc_dointvec_minmax,
>> > -	 .extra1 = SYSCTL_ZERO,
>> > -	 .extra2 = &oa_sample_rate_hard_limit,
>> > -	 },
>> > -	{}
>> > +		.procname = "oa_max_sample_rate",
>> > +		.data = &i915_oa_max_sample_rate,
>> > +		.maxlen = sizeof(i915_oa_max_sample_rate),
>> > +		.mode = 0644,
>> > +		.proc_handler = proc_dointvec_minmax,
>> > +		.extra1 = SYSCTL_ZERO,
>> > +		.extra2 = &oa_sample_rate_hard_limit,
>> > +	}
>> >  };
>> 
>> The existing indentation is off, but fixing it doesn't really
belong in
>> this patch.
>
> Agreed. But I actually was trying to fix something that checkpatch
> flagged. I'll change these back (which will cause this patch to be
> flagged).
>
> An alternative solution would be to fix the indentation as part of the
> preparation patches. Tell me what you think.
>
> Thx
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center
-- 
Jani Nikula, Intel Open Source Graphics Center