svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys

[Ian Lepore]

On Mon, 2017-08-28 at 16:40 -0700, Maxim Sobolev wrote:
> John, well, this depends on how you look at it. The padding element size is
> "int", which when you account for the alignment has the nice property on
> both 32 and 64-bit arches that no matter what kind of element you add
> (char, short, int or void *), you only need to bring down MDNPAD by 1 to
> keep the structure size the same. It also has a second interesting property
> that number of padding elements needed stays the same no matter if
> sizeof(void *) is 64 or 32, otherwise you would have to have MDNPAD diverge
> on 32 and 64 bit arches after adding pointer which has different sizes
> (just like you suggested it should originally). I am not 100% sure if it
> was intentionally designed like this by PHK from the day one, but I found
> it quite interesting. I am not quite sure if having sizeof(md_ioctl) is
> ever been required to stay the same between 64 and 32 bit arches. I don't
> think there is any support for having 32-bit mdconfig run on 64-bit kernel.
> 
> -Max
> 

mdconfig works in 32-bit jails on a 64-bit host, and I rely on that
being the case.  With poudriere evolving to become a general image
building tool, and with it being so jail-centric, it's likely to be
another use case.

-- Ian

> On Mon, Aug 28, 2017 at 1:49 PM, John Baldwin <jhb at freebsd.org>
> wrote:
> 
> > 
> > On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote:
> > > 
> > > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev <sobomax at freebsd.
> > > org>
> > wrote:
> > > 
> > > > 
> > > > Hi John,
> > > > 
> > > > Thanks for your feedback! To address the points that you've
> > > > raised:
> > > > 
> > > > 1. I've tested on both 32 and 64 bit platforms, it seems not to
> > > > be the
> > > > case. See imp's comment and my reply here
> > > > https://reviews.freebsd.org/D10457#216855 . Did I miss
> > > > something? Can
> > you
> > > 
> > > > 
> > > > post piece of C code that produces different sizeof(struct old)
> > > > vs.
> > > > sizeof(struct new) on some platform?
> > > [...]
> > > > 
> > > > On Mon, Aug 28, 2017 at 9:19 AM, John Baldwin <jhb at freebsd.org>
> > > > wrote:
> > > > 
> > > > > 
> > > > > On Monday, August 28, 2017 03:54:08 PM Maxim Sobolev wrote:
> > > > > > 
> > > > > > Author: sobomax
> > > > > > Date: Mon Aug 28 15:54:07 2017
> > > > > > New Revision: 322969
> > > > > > URL: https://svnweb.freebsd.org/changeset/base/322969
> > > > > > 
> > > > > > Log:
> > > > > >   Add ability to label md(4) devices.
> > > > > > 
> > > > > >   This feature comes from the fact that we rely memory-
> > > > > > backed md(4)
> > > > > >   in our build process heavily. However, if the build goes
> > > > > > haywire
> > > > > >   the allocated resources (i.e. swap and memory-backed
> > > > > > md(4)'s) need
> > > > > >   to be purged. It is extremely useful to have ability to
> > > > > > attach
> > > > > >   arbitrary labels to each of the virtual disks so that
> > > > > > they can
> > > > > >   be identified and GC'ed if neecessary.
> > > > > > 
> > > > > >   MFC after:  4 weeks
> > > > > >   Differential Revision:      https://reviews.freebsd.org/D
> > > > > > 10457
> > > > > > 
> > > > > > Modified:
> > > > > >   head/sbin/mdconfig/mdconfig.8
> > > > > >   head/sbin/mdconfig/mdconfig.c
> > > > > >   head/sys/dev/md/md.c
> > > > > >   head/sys/sys/mdioctl.h
> > > > > > 
> > > > > > Modified: head/sys/sys/mdioctl.h
> > > > > > ===========================================================
> > > > > > =
> > > > > ==================
> > > > > > 
> > > > > > --- head/sys/sys/mdioctl.h    Mon Aug 28 14:49:26 2017
> > (r322968)
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > +++ head/sys/sys/mdioctl.h    Mon Aug 28 15:54:07 2017
> > (r322969)
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > @@ -49,7 +49,7 @@ enum md_types {MD_MALLOC, MD_PRELOAD,
> > > > > > MD_VNODE,
> > MD_SWA
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > >   * Ioctl definitions for memory disk pseudo-device.
> > > > > >   */
> > > > > > 
> > > > > > -#define MDNPAD               97
> > > > > > +#define MDNPAD               96
> > > > > >  struct md_ioctl {
> > > > > >       unsigned        md_version;     /* Structure layout
> > > > > > version */
> > > > > >       unsigned        md_unit;        /* unit number */
> > > > > > @@ -61,6 +61,7 @@ struct md_ioctl {
> > > > > >       u_int64_t       md_base;        /* base address */
> > > > > >       int             md_fwheads;     /* firmware heads */
> > > > > >       int             md_fwsectors;   /* firmware sectors
> > > > > > */
> > > > > > +     char            *md_label;      /* label of the
> > > > > > device */
> > > > > >       int             md_pad[MDNPAD]; /* padding for future
> > > > > > ideas */
> > > > > >  };
> > > > > This isn't correct on 64-bit platforms.  MDNPAD needs to be
> > > > > 95 on
> > those
> > > 
> > > > 
> > > > > 
> > > > > platforms.
> > > [...]
> > > 
> > > Can you report sizeof(md_ioctl) before and after for 32-bit and
> > > 64-bit?
> > > I think it may be:
> > > 32-bit before: 440
> > > 32-bit after:  440
> > > 64-bit before: 448
> > > 64-bit after:  448
> > > 
> > > In other words, it looks like it used to produce different sizes
> > > on the
> > > different architectures, and still does.  It also looks like 32-
> > > bit
> > > before and after and 64-bit before included some undeclared
> > > padding
> > > after md_pad, so that this would fail:
> > > CTASSERT(sizeof(md_ioctl) == offsetof(struct md_ioctl, md_pad) +
> > >     sizeof(((struct md_ioctl *)NULL)->md_pad));
> > Ugh, yes.  To me that means that MDNPAD is actually wrong and
> > should be
> > fixed to account for the implicit padding.  That probably would
> > result in
> > requiring separate values for MDNPAD.  The current change as-is
> > certainly
> > looks wrong (and would be wrong if the padding were accurate) so it
> > needs
> > to be fixed to reflect reality.
> > 
> > --
> > John Baldwin
> > 
> >