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 On Mon, Aug 28, 2017 at 1:49 PM, John Baldwin <j...@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 <sobo...@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 <j...@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/D10457 > > >> > > > >> > 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 > > _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
