| 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 |
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>
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
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...)
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
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 <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 --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); }; /*
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(-)