diff mbox series

[09/14] ax.25: Update to register_net_sysctl_sz

Message ID 20230726140635.2059334-10-j.granados@samsung.com
State New
Headers
Series [01/14] sysctl: Prefer ctl_table_header in proc_sysctl |

Commit Message

Joel Granados July 26, 2023, 2:06 p.m. UTC 
  This is part of the effort to remove the sentinel (last empty) element
from the ctl_table arrays. We update to the new function and pass it the
array size.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 net/ax25/sysctl_net_ax25.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Luis Chamberlain July 26, 2023, 6 p.m. UTC | #1 
On Wed, Jul 26, 2023 at 04:06:29PM +0200, Joel Granados wrote:
> This is part of the effort to remove the sentinel (last empty) element
> from the ctl_table arrays. We update to the new function and pass it the
> array size.

The commit log does not explain why. It also does not explain if there
is any delta on size at compile or runtime.

  Luis
Joel Granados July 27, 2023, 12:31 p.m. UTC | #2 
On Wed, Jul 26, 2023 at 11:00:37AM -0700, Luis Chamberlain wrote:
> On Wed, Jul 26, 2023 at 04:06:29PM +0200, Joel Granados wrote:
> > This is part of the effort to remove the sentinel (last empty) element
> > from the ctl_table arrays. We update to the new function and pass it the
> > array size.
> 
> The commit log does not explain why. It also does not explain if there
I'll add the "why".

> is any delta on size at compile or runtime.
There are no deltas in this patch set. We start seeing the deltas when
we start removing with the next 6 chunks. I'll try to make that more
clear in the commit message.

> 
>   Luis

thx for the feedback
Joel Granados July 27, 2023, 3:38 p.m. UTC | #3 
On Wed, Jul 26, 2023 at 11:00:37AM -0700, Luis Chamberlain wrote:
> On Wed, Jul 26, 2023 at 04:06:29PM +0200, Joel Granados wrote:
> > This is part of the effort to remove the sentinel (last empty) element
> > from the ctl_table arrays. We update to the new function and pass it the
> > array size.
> 
> The commit log does not explain why. It also does not explain if there
> is any delta on size at compile or runtime.
Just to be on the same page:
You mean the specific why for this commit. like for example:
"
We update to register_net_sysctl_sz to prepare for when the ctl_table
array sentinels are removed.
"

right?


> 
>   Luis
Luis Chamberlain July 27, 2023, 3:44 p.m. UTC | #4 
On Thu, Jul 27, 2023 at 02:31:12PM +0200, Joel Granados wrote:
> There are no deltas in this patch set. We start seeing the deltas when
> we start removing with the next 6 chunks. I'll try to make that more
> clear in the commit message.

Indeed, even if no deltas are created it is importan then to say that.
If there are no deltas the "why" becomes more important. If the why is
to make it easier to apply subsequent patches, that must be said. When
you iterate your new series try to review the patches as if you were not
the person submitting them, and try to think of ways to make it easier
for the patch reviewer to do less work. The less work and easier patch
review is the better for them.

  Luis
Luis Chamberlain July 27, 2023, 3:50 p.m. UTC | #5 
On Thu, Jul 27, 2023 at 05:38:04PM +0200, Joel Granados wrote:
> On Wed, Jul 26, 2023 at 11:00:37AM -0700, Luis Chamberlain wrote:
> > On Wed, Jul 26, 2023 at 04:06:29PM +0200, Joel Granados wrote:
> > > This is part of the effort to remove the sentinel (last empty) element
> > > from the ctl_table arrays. We update to the new function and pass it the
> > > array size.
> > 
> > The commit log does not explain why. It also does not explain if there
> > is any delta on size at compile or runtime.
> Just to be on the same page:
> You mean the specific why for this commit. like for example:
> "
> We update  to register_net_sysctl_sz

I think it is clearer to just say:

Move from register_net_sysctl() to register_net_sysctl_sz()

> to prepare for when the ctl_table
> array sentinels are removed.

That is not as clear. I think you should just spell it out.

We want to use the syctl ARRAY_SIZE() when possible, and subsequent
patches ... now the register_net_sysctl() <does something> while
register_net_sysctl_sz() <does something else> and <in this case>
this user <does something> and so we must use register_net_sysctl_sz().

> "
> 
> right?

  Luis
Joel Granados July 28, 2023, 7:35 a.m. UTC | #6 
On Thu, Jul 27, 2023 at 08:44:24AM -0700, Luis Chamberlain wrote:
> On Thu, Jul 27, 2023 at 02:31:12PM +0200, Joel Granados wrote:
> > There are no deltas in this patch set. We start seeing the deltas when
> > we start removing with the next 6 chunks. I'll try to make that more
> > clear in the commit message.
> 
> Indeed, even if no deltas are created it is importan then to say that.
> If there are no deltas the "why" becomes more important. If the why is
> to make it easier to apply subsequent patches, that must be said. When
yes. The why for this patch set in particular is to make it easier to
apply the sentinel removal patches.

I think the difficulty for me comes from having two whys: 1. The one for
this patch set which is to make it easier to apply sentinel removal patches. And 2.
The one for the "big" patch (that actually removes the sentinels) which is to
reduce build time size and run time memory bloat.

> you iterate your new series try to review the patches as if you were not
> the person submitting them, and try to think of ways to make it easier
> for the patch reviewer to do less work. The less work and easier patch
> review is the better for them.
Ack. For all these commits I'll try to weave in the two Whys to make the
review process a bit easier.

> 
>   Luis
Luis Chamberlain July 28, 2023, 6:16 p.m. UTC | #7 
On Fri, Jul 28, 2023 at 09:35:36AM +0200, Joel Granados wrote:
> On Thu, Jul 27, 2023 at 08:44:24AM -0700, Luis Chamberlain wrote:
> > On Thu, Jul 27, 2023 at 02:31:12PM +0200, Joel Granados wrote:
> > > There are no deltas in this patch set. We start seeing the deltas when
> > > we start removing with the next 6 chunks. I'll try to make that more
> > > clear in the commit message.
> > 
> > Indeed, even if no deltas are created it is importan then to say that.
> > If there are no deltas the "why" becomes more important. If the why is
> > to make it easier to apply subsequent patches, that must be said. When
> yes. The why for this patch set in particular is to make it easier to
> apply the sentinel removal patches.
> 
> I think the difficulty for me comes from having two whys: 1. The one for
> this patch set which is to make it easier to apply sentinel removal patches. And 2.
> The one for the "big" patch (that actually removes the sentinels) which is to
> reduce build time size and run time memory bloat.

The 2) is part of the real why, 1) is more of how to do 2) cleanly. But
the real why is the savings in memory because we are moving arrays out
of kernel/sysctl.c so we don't want to incur a size penalty. The
collateral to avoid increasing size in the moves also proves to save us
more memory overall, on the ballpark of about 64 bytes per array in the
kernel both at runtime and build time. The build time gain is mostly
on the __init stuff and so gets freed right away, but since sysctl
code always kzallocs the arrays passed we also save 64 bytes per array
in the end at runtime.

  Luis
Joel Granados July 31, 2023, 7:09 a.m. UTC | #8 
On Fri, Jul 28, 2023 at 11:16:06AM -0700, Luis Chamberlain wrote:
> On Fri, Jul 28, 2023 at 09:35:36AM +0200, Joel Granados wrote:
> > On Thu, Jul 27, 2023 at 08:44:24AM -0700, Luis Chamberlain wrote:
> > > On Thu, Jul 27, 2023 at 02:31:12PM +0200, Joel Granados wrote:
> > > > There are no deltas in this patch set. We start seeing the deltas when
> > > > we start removing with the next 6 chunks. I'll try to make that more
> > > > clear in the commit message.
> > > 
> > > Indeed, even if no deltas are created it is importan then to say that.
> > > If there are no deltas the "why" becomes more important. If the why is
> > > to make it easier to apply subsequent patches, that must be said. When
> > yes. The why for this patch set in particular is to make it easier to
> > apply the sentinel removal patches.
> > 
> > I think the difficulty for me comes from having two whys: 1. The one for
> > this patch set which is to make it easier to apply sentinel removal patches. And 2.
> > The one for the "big" patch (that actually removes the sentinels) which is to
> > reduce build time size and run time memory bloat.
> 
> The 2) is part of the real why, 1) is more of how to do 2) cleanly. But
> the real why is the savings in memory because we are moving arrays out
> of kernel/sysctl.c so we don't want to incur a size penalty. The
> collateral to avoid increasing size in the moves also proves to save us
> more memory overall, on the ballpark of about 64 bytes per array in the
> kernel both at runtime and build time. The build time gain is mostly
> on the __init stuff and so gets freed right away, but since sysctl
> code always kzallocs the arrays passed we also save 64 bytes per array
> in the end at runtime.
Yes. In my new version I have tried to mention both 1 and 2 and
differentiate between them. I stuck with the "why" for this patch set is
to make it easier to reach 2.

I'll send it out today.
> 
>   Luis
diff mbox series

Patch

diff --git a/net/ax25/sysctl_net_ax25.c b/net/ax25/sysctl_net_ax25.c
index 2154d004d3dc..db66e11e7fe8 100644
--- a/net/ax25/sysctl_net_ax25.c
+++ b/net/ax25/sysctl_net_ax25.c
@@ -159,7 +159,8 @@  int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
 		table[k].data = &ax25_dev->values[k];
 
 	snprintf(path, sizeof(path), "net/ax25/%s", ax25_dev->dev->name);
-	ax25_dev->sysheader = register_net_sysctl(&init_net, path, table);
+	ax25_dev->sysheader = register_net_sysctl_sz(&init_net, path, table,
+						     ARRAY_SIZE(ax25_param_table));
 	if (!ax25_dev->sysheader) {
 		kfree(table);
 		return -ENOMEM;