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

  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(-)
  

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