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 <[email protected]>
---
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(-)
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);
};
/*
--
2.34.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 <[email protected]>
This looks like a really nice conversion. I haven't run-tested this, but
it reads correct to me.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Mon, Dec 4, 2023 at 2:20 PM Kees Cook <[email protected]> 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 <[email protected]>
>
> 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
On Thu, Dec 07, 2023 at 09:33:17PM -0800, Song Liu wrote:
> On Mon, Dec 4, 2023 at 2:20 PM Kees Cook <[email protected]> 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 <[email protected]>
> >
> > 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...)
--
Kees Cook
On Fri, Dec 8, 2023 at 9:27 AM Kees Cook <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > 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
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.
Hi Christoph,
On Mon, Dec 11, 2023 at 9:46 PM Christoph Hellwig <[email protected]> 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