diff mbox series

md/md-multipath: Convert "struct mpconf" to flexible array

Message ID 03dd7de1cecdb7084814f2fab300c9bc716aff3e.1701632867.git.christophe.jaillet@wanadoo.fr (mailing list archive)
State Handled Elsewhere
Headers show
Series md/md-multipath: Convert "struct mpconf" to flexible array | expand

Commit Message

Christophe JAILLET Dec. 3, 2023, 7:48 p.m. UTC
The 'multipaths' field of 'struct mpconf' can be declared as a flexible
array.

The advantages are:
   - 1 less indirection when accessing to the 'multipaths' array
   - save 1 pointer in the structure
   - improve memory usage
   - give the opportunity to use __counted_by() for additional safety

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
On my x86_64 system, with configured with allmodconfig, I have:

Before the change:
=================
struct mpconf {
	struct mddev *             mddev;                /*     0     8 */
	struct multipath_info *    multipaths;           /*     8     8 */
	int                        raid_disks;           /*    16     4 */

	/* XXX 4 bytes hole, try to pack */

	spinlock_t                 device_lock;          /*    24    72 */
	/* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
	struct list_head           retry_list;           /*    96    16 */
	mempool_t                  pool;                 /*   112   200 */

	/* size: 312, cachelines: 5, members: 6 */
	/* sum members: 308, holes: 1, sum holes: 4 */
	/* last cacheline: 56 bytes */
};

struct multipath_info {
	struct md_rdev *           rdev;                 /*     0     8 */

	/* size: 8, cachelines: 1, members: 1 */
	/* last cacheline: 8 bytes */
};

size drivers/md/md-multipath.o
   text	   data	    bss	    dec	    hex	filename
  12863	   1041	     16	  13920	   3660	drivers/md/md-multipath.o


After the change:
================
struct mpconf {
	struct mddev *             mddev;                /*     0     8 */
	int                        raid_disks;           /*     8     4 */

	/* XXX 4 bytes hole, try to pack */

	spinlock_t                 device_lock;          /*    16    72 */
	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
	struct list_head           retry_list;           /*    88    16 */
	mempool_t                  pool;                 /*   104   200 */
	/* --- cacheline 4 boundary (256 bytes) was 48 bytes ago --- */
	struct multipath_info      multipaths[];         /*   304     0 */

	/* size: 304, cachelines: 5, members: 6 */
	/* sum members: 300, holes: 1, sum holes: 4 */
	/* last cacheline: 48 bytes */
};

struct multipath_info {
	struct md_rdev *           rdev;                 /*     0     8 */

	/* size: 8, cachelines: 1, members: 1 */
	/* last cacheline: 8 bytes */
};

size drivers/md/md-multipath.o
   text	   data	    bss	    dec	    hex	filename
  12470	   1041	     16	  13527	   34d7	drivers/md/md-multipath.o


So:
  - about 400 bytes of code are saved.
  - because of the way memory allocation works, 'struct mpconf' really
    uses 512 bytes of memory when allocated. So the "extra" memory that is
    allocated (512-304 = 208) can be used to store up to 26 multipaths,
    for free.

Finally, several places use pointer arithmetic to access the desired
structure, such as:
	for (i = 0; i < conf->raid_disks; i++) {
		tmp = conf->multipaths + i;
		if (tmp->rdev)

Should this be rewritten as:
	for (i = 0; i < conf->raid_disks; i++) {
		if (tmpconf->multipaths[i]->rdev)
in order to have the compiler be able to check boundaries defined by
__counted_by()?
---
 drivers/md/md-multipath.c | 12 +++---------
 drivers/md/md-multipath.h |  3 ++-
 2 files changed, 5 insertions(+), 10 deletions(-)

Comments

Kees Cook Dec. 4, 2023, 10:20 p.m. UTC | #1
On Sun, Dec 03, 2023 at 08:48:06PM +0100, Christophe JAILLET wrote:
> The 'multipaths' field of 'struct mpconf' can be declared as a flexible
> array.
> 
> The advantages are:
>    - 1 less indirection when accessing to the 'multipaths' array
>    - save 1 pointer in the structure
>    - improve memory usage
>    - give the opportunity to use __counted_by() for additional safety
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

This looks like a really nice conversion. I haven't run-tested this, but
it reads correct to me.

Reviewed-by: Kees Cook <keescook@chromium.org>
Song Liu Dec. 8, 2023, 5:33 a.m. UTC | #2
On Mon, Dec 4, 2023 at 2:20 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Dec 03, 2023 at 08:48:06PM +0100, Christophe JAILLET wrote:
> > The 'multipaths' field of 'struct mpconf' can be declared as a flexible
> > array.
> >
> > The advantages are:
> >    - 1 less indirection when accessing to the 'multipaths' array
> >    - save 1 pointer in the structure
> >    - improve memory usage
> >    - give the opportunity to use __counted_by() for additional safety
> >
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> This looks like a really nice conversion. I haven't run-tested this, but
> it reads correct to me.

Agreed this is a good optimization. However, since MD_MULTIPATH is
already marked as deprecated. I don't think we should ship further
changes to it.

Thanks,
Song
Kees Cook Dec. 8, 2023, 5:27 p.m. UTC | #3
On Thu, Dec 07, 2023 at 09:33:17PM -0800, Song Liu wrote:
> On Mon, Dec 4, 2023 at 2:20 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Sun, Dec 03, 2023 at 08:48:06PM +0100, Christophe JAILLET wrote:
> > > The 'multipaths' field of 'struct mpconf' can be declared as a flexible
> > > array.
> > >
> > > The advantages are:
> > >    - 1 less indirection when accessing to the 'multipaths' array
> > >    - save 1 pointer in the structure
> > >    - improve memory usage
> > >    - give the opportunity to use __counted_by() for additional safety
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >
> > This looks like a really nice conversion. I haven't run-tested this, but
> > it reads correct to me.
> 
> Agreed this is a good optimization. However, since MD_MULTIPATH is
> already marked as deprecated. I don't think we should ship further
> changes to it.

Hm, that seems like a weird catch-22 to me. I would say we should
continue to improve any code in the kernel that people spend time to
work on, or we should remove that code entirely. Should MD_MULTIPATH be
removed? How long has it been deprecated? (We just had an LTS release,
so doing removal now is a good time...)
Song Liu Dec. 8, 2023, 6:11 p.m. UTC | #4
On Fri, Dec 8, 2023 at 9:27 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Dec 07, 2023 at 09:33:17PM -0800, Song Liu wrote:
> > On Mon, Dec 4, 2023 at 2:20 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Sun, Dec 03, 2023 at 08:48:06PM +0100, Christophe JAILLET wrote:
> > > > The 'multipaths' field of 'struct mpconf' can be declared as a flexible
> > > > array.
> > > >
> > > > The advantages are:
> > > >    - 1 less indirection when accessing to the 'multipaths' array
> > > >    - save 1 pointer in the structure
> > > >    - improve memory usage
> > > >    - give the opportunity to use __counted_by() for additional safety
> > > >
> > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > >
> > > This looks like a really nice conversion. I haven't run-tested this, but
> > > it reads correct to me.
> >
> > Agreed this is a good optimization. However, since MD_MULTIPATH is
> > already marked as deprecated. I don't think we should ship further
> > changes to it.
>
> Hm, that seems like a weird catch-22 to me. I would say we should
> continue to improve any code in the kernel that people spend time to
> work on, or we should remove that code entirely. Should MD_MULTIPATH be
> removed? How long has it been deprecated? (We just had an LTS release,
> so doing removal now is a good time...)

We marked it as deprecated about 2.5 years ago. But to be honest,
I currently don't have a plan to remove it. I guess I should start thinking
about it.

Thanks,
Song
Christoph Hellwig Dec. 12, 2023, 5:46 a.m. UTC | #5
On Fri, Dec 08, 2023 at 10:11:10AM -0800, Song Liu wrote:
> We marked it as deprecated about 2.5 years ago. But to be honest,
> I currently don't have a plan to remove it. I guess I should start thinking
> about it.

Let's just kill it off ASAP.  It never had a large user base and based
by dm-multipath not long after it has been added.  It also doesn't
support any uniqueue hardware and has no on-disk format.

If you want any blame deflected from you I'd be happy to send the patch
to remove it.
Song Liu Dec. 12, 2023, 7:46 a.m. UTC | #6
Hi Christoph,

On Mon, Dec 11, 2023 at 9:46 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Dec 08, 2023 at 10:11:10AM -0800, Song Liu wrote:
> > We marked it as deprecated about 2.5 years ago. But to be honest,
> > I currently don't have a plan to remove it. I guess I should start thinking
> > about it.
>
> Let's just kill it off ASAP.  It never had a large user base and based
> by dm-multipath not long after it has been added.  It also doesn't
> support any uniqueue hardware and has no on-disk format.

Thanks for the suggestion.

> If you want any blame deflected from you I'd be happy to send the patch
> to remove it.

Let me give it a try. I am kinda curious what gonna happen. :)

Thanks,
Song
diff mbox series

Patch

diff --git a/drivers/md/md-multipath.c b/drivers/md/md-multipath.c
index d22276870283..6a23065a65f7 100644
--- a/drivers/md/md-multipath.c
+++ b/drivers/md/md-multipath.c
@@ -357,16 +357,13 @@  static int multipath_run (struct mddev *mddev)
 	 * should be freed in multipath_free()]
 	 */
 
-	conf = kzalloc(sizeof(struct mpconf), GFP_KERNEL);
+	conf = kzalloc(struct_size(conf, multipaths, mddev->raid_disks),
+		       GFP_KERNEL);
 	mddev->private = conf;
 	if (!conf)
 		goto out;
 
-	conf->multipaths = kcalloc(mddev->raid_disks,
-				   sizeof(struct multipath_info),
-				   GFP_KERNEL);
-	if (!conf->multipaths)
-		goto out_free_conf;
+	conf->raid_disks = mddev->raid_disks;
 
 	working_disks = 0;
 	rdev_for_each(rdev, mddev) {
@@ -384,7 +381,6 @@  static int multipath_run (struct mddev *mddev)
 			working_disks++;
 	}
 
-	conf->raid_disks = mddev->raid_disks;
 	conf->mddev = mddev;
 	spin_lock_init(&conf->device_lock);
 	INIT_LIST_HEAD(&conf->retry_list);
@@ -421,7 +417,6 @@  static int multipath_run (struct mddev *mddev)
 
 out_free_conf:
 	mempool_exit(&conf->pool);
-	kfree(conf->multipaths);
 	kfree(conf);
 	mddev->private = NULL;
 out:
@@ -433,7 +428,6 @@  static void multipath_free(struct mddev *mddev, void *priv)
 	struct mpconf *conf = priv;
 
 	mempool_exit(&conf->pool);
-	kfree(conf->multipaths);
 	kfree(conf);
 }
 
diff --git a/drivers/md/md-multipath.h b/drivers/md/md-multipath.h
index b3099e5fc4d7..fb49e151ac94 100644
--- a/drivers/md/md-multipath.h
+++ b/drivers/md/md-multipath.h
@@ -8,12 +8,13 @@  struct multipath_info {
 
 struct mpconf {
 	struct mddev			*mddev;
-	struct multipath_info	*multipaths;
 	int			raid_disks;
 	spinlock_t		device_lock;
 	struct list_head	retry_list;
 
 	mempool_t		pool;
+
+	struct multipath_info	multipaths[] __counted_by(raid_disks);
 };
 
 /*