Date: Thu, 31 Aug 2017 08:26:00 -0600 From: Ian Lepore <ian@freebsd.org> To: Maxim Sobolev <sobomax@freebsd.org> Cc: John Baldwin <jhb@freebsd.org>, Ryan Libby <rlibby@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers <src-committers@freebsd.org> Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys Message-ID: <1504189560.56799.94.camel@freebsd.org> In-Reply-To: <CAH7qZfvxGk%2B16Fidnx744%2BzDKfM=QWWKOmC6h6HUzb0=-HTGGw@mail.gmail.com> References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <CAH7qZfv=RT%2BZ7uZ8EY_Dm07-gAS3fhcchBv8Wj_YMo6nmLF0qQ@mail.gmail.com> <CAHgpiFzTuNvqrDpU-dmtAauHAFHXroHyxNRdignozUy_D=3fKQ@mail.gmail.com> <2937323.CvTEtZnL2T@ralph.baldwin.cx> <CAH7qZftFAzfsgfgHV2=59Ah2LPfMC_CFYUEfiFZSWbf7jMZUTg@mail.gmail.com> <1504016363.56799.71.camel@freebsd.org> <CAH7qZfvxGk%2B16Fidnx744%2BzDKfM=QWWKOmC6h6HUzb0=-HTGGw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Unfortunately, I'm not in a position to test those changes. =A0My setup
that uses mdconfig in a 32-bit jail runs on freebsd 10-stable and 11-
stable systems. =A0I don't have any 64-bit hardware available to run
-current.
-- Ian
On Wed, 2017-08-30 at 10:10 -0700, Maxim Sobolev wrote:
> Hi Ian, I've committed support for the md_label into 32-bit-to-64-bit=20
> ioctl
> translation later and tested it for a bit here. It would be nice if
> you
> give it a go as well so if there are any bugs I'd like to iron them
> out
> before I MFC this piece in few weeks. Interestingly enough, I found
> mdconfig -l[v] is not working when i386 binary is run on amd64
> kernels, but
> it seems to have nothing to do with this change. It fails to
> mmap("/dev/devstat") in the geom userland libs. Turns out the
> mdconfig(8)
> is not using ioctl() but rather geom layer for the "list / query"
> functionality. In fact there is no CLI code in the system that would
> exercise either MDIOCLIST or MDIOCQUERY.=A0=A0No userland code for the
> MDIOCLIST at all in the base and the only code that uses MDIOCQUERY
> is in
> some bsnmpd modules. For that reason I could not verify that it works
> 100%,
> but I've tested allocating device with and without label using i386
> binary
> on amd64 and it seems to DTRT. The label is retrieved properly when I
> query
> device with native 64-bit mdconfig.
>=20
> It would be really nice to add extra mode flag into mdconfig so that
> it
> goes via those ioctl() rather than geom to fetch the device data
> data. If
> nothing else it would provide a basis to create an unit test for
> those
> ioctls.
>=20
> -Max
>=20
> On Tue, Aug 29, 2017 at 7:19 AM, Ian Lepore <ian@freebsd.org> wrote:
>=20
> >=20
> > On Mon, 2017-08-28 at 16:40 -0700, Maxim Sobolev wrote:
> > >=20
> > > John, well, this depends on how you look at it. The padding
> > > element size
> > is
> > >=20
> > > "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
> > >=20
> > > 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
> > >=20
> > > 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.
> > >=20
> > >=20
> > > -Max
> > >=20
> > mdconfig works in 32-bit jails on a 64-bit host, and I rely on that
> > being the case.=A0=A0With poudriere evolving to become a general imag=
e
> > building tool, and with it being so jail-centric, it's likely to be
> > another use case.
> >=20
> > -- Ian
> >=20
> > >=20
> > > On Mon, Aug 28, 2017 at 1:49 PM, John Baldwin <jhb@freebsd.org>
> > > wrote:
> > >=20
> > > >=20
> > > >=20
> > > > On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote:
> > > > >=20
> > > > >=20
> > > > > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev <sobomax@free
> > > > > bsd.
> > > > > org>
> > > > wrote:
> > > > >=20
> > > > >=20
> > > > > >=20
> > > > > >=20
> > > > > > Hi John,
> > > > > >=20
> > > > > > Thanks for your feedback! To address the points that you've
> > > > > > raised:
> > > > > >=20
> > > > > > 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
> > > > >=20
> > > > >=20
> > > > > >=20
> > > > > >=20
> > > > > > post piece of C code that produces different sizeof(struct
> > > > > > old)
> > > > > > vs.
> > > > > > sizeof(struct new) on some platform?
> > > > > [...]
> > > > > >=20
> > > > > >=20
> > > > > > On Mon, Aug 28, 2017 at 9:19 AM, John Baldwin <jhb@freebsd.
> > > > > > org>
> > > > > > wrote:
> > > > > >=20
> > > > > > >=20
> > > > > > >=20
> > > > > > > On Monday, August 28, 2017 03:54:08 PM Maxim Sobolev
> > > > > > > wrote:
> > > > > > > >=20
> > > > > > > >=20
> > > > > > > > Author: sobomax
> > > > > > > > Date: Mon Aug 28 15:54:07 2017
> > > > > > > > New Revision: 322969
> > > > > > > > URL: https://svnweb.freebsd.org/changeset/base/322969
> > > > > > > >=20
> > > > > > > > Log:
> > > > > > > > =A0 Add ability to label md(4) devices.
> > > > > > > >=20
> > > > > > > > =A0 This feature comes from the fact that we rely memory-
> > > > > > > > backed md(4)
> > > > > > > > =A0 in our build process heavily. However, if the build
> > > > > > > > goes
> > > > > > > > haywire
> > > > > > > > =A0 the allocated resources (i.e. swap and memory-backed
> > > > > > > > md(4)'s) need
> > > > > > > > =A0 to be purged. It is extremely useful to have ability
> > > > > > > > to
> > > > > > > > attach
> > > > > > > > =A0 arbitrary labels to each of the virtual disks so that
> > > > > > > > they can
> > > > > > > > =A0 be identified and GC'ed if neecessary.
> > > > > > > >=20
> > > > > > > > =A0 MFC after:=A0=A04 weeks
> > > > > > > > =A0 Differential Revision:=A0=A0=A0=A0=A0=A0https://revie=
ws.freebsd.o
> > > > > > > > rg/D
> > > > > > > > 10457
> > > > > > > >=20
> > > > > > > > Modified:
> > > > > > > > =A0 head/sbin/mdconfig/mdconfig.8
> > > > > > > > =A0 head/sbin/mdconfig/mdconfig.c
> > > > > > > > =A0 head/sys/dev/md/md.c
> > > > > > > > =A0 head/sys/sys/mdioctl.h
> > > > > > > >=20
> > > > > > > > Modified: head/sys/sys/mdioctl.h
> > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > > > > > > =3D=3D=3D=3D
> > > > > > > > =3D
> > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > > > > > >=20
> > > > > > > >=20
> > > > > > > > --- head/sys/sys/mdioctl.h=A0=A0=A0=A0Mon Aug 28 14:49:26=
2017
> > > > (r322968)
> > > > >=20
> > > > >=20
> > > > > >=20
> > > > > >=20
> > > > > > >=20
> > > > > > >=20
> > > > > > > >=20
> > > > > > > >=20
> > > > > > > > +++ head/sys/sys/mdioctl.h=A0=A0=A0=A0Mon Aug 28 15:54:07=
2017
> > > > (r322969)
> > > > >=20
> > > > >=20
> > > > > >=20
> > > > > >=20
> > > > > > >=20
> > > > > > >=20
> > > > > > > >=20
> > > > > > > >=20
> > > > > > > > @@ -49,7 +49,7 @@ enum md_types {MD_MALLOC, MD_PRELOAD,
> > > > > > > > MD_VNODE,
> > > > MD_SWA
> > > > >=20
> > > > >=20
> > > > > >=20
> > > > > >=20
> > > > > > >=20
> > > > > > >=20
> > > > > > > >=20
> > > > > > > >=20
> > > > > > > > =A0 * Ioctl definitions for memory disk pseudo-device.
> > > > > > > > =A0 */
> > > > > > > >=20
> > > > > > > > -#define MDNPAD=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A097
> > > > > > > > +#define MDNPAD=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A096
> > > > > > > > =A0struct md_ioctl {
> > > > > > > > =A0=A0=A0=A0=A0=A0unsigned=A0=A0=A0=A0=A0=A0=A0=A0md_vers=
ion;=A0=A0=A0=A0=A0/* Structure
> > > > > > > > layout
> > > > > > > > version */
> > > > > > > > =A0=A0=A0=A0=A0=A0unsigned=A0=A0=A0=A0=A0=A0=A0=A0md_unit=
;=A0=A0=A0=A0=A0=A0=A0=A0/* unit number */
> > > > > > > > @@ -61,6 +61,7 @@ struct md_ioctl {
> > > > > > > > =A0=A0=A0=A0=A0=A0u_int64_t=A0=A0=A0=A0=A0=A0=A0md_base;=A0=
=A0=A0=A0=A0=A0=A0=A0/* base address
> > > > > > > > */
> > > > > > > > =A0=A0=A0=A0=A0=A0int=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0md_fwheads;=A0=A0=A0=A0=A0/* firmware heads
> > > > > > > > */
> > > > > > > > =A0=A0=A0=A0=A0=A0int=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0md_fwsectors;=A0=A0=A0/* firmware
> > > > > > > > sectors
> > > > > > > > */
> > > > > > > > +=A0=A0=A0=A0=A0char=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0*=
md_label;=A0=A0=A0=A0=A0=A0/* label of the
> > > > > > > > device */
> > > > > > > > =A0=A0=A0=A0=A0=A0int=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0md_pad[MDNPAD]; /* padding for
> > > > > > > > future
> > > > > > > > ideas */
> > > > > > > > =A0};
> > > > > > > This isn't correct on 64-bit platforms.=A0=A0MDNPAD needs t=
o
> > > > > > > be
> > > > > > > 95 on
> > > > those
> > > > >=20
> > > > >=20
> > > > > >=20
> > > > > >=20
> > > > > > >=20
> > > > > > >=20
> > > > > > > platforms.
> > > > > [...]
> > > > >=20
> > > > > 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:=A0=A0440
> > > > > 64-bit before: 448
> > > > > 64-bit after:=A0=A0448
> > > > >=20
> > > > > In other words, it looks like it used to produce different
> > > > > sizes
> > > > > on the
> > > > > different architectures, and still does.=A0=A0It also looks lik=
e
> > > > > 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) =3D=3D offsetof(struct md_ioctl,
> > > > > md_pad) +
> > > > > =A0=A0=A0=A0sizeof(((struct md_ioctl *)NULL)->md_pad));
> > > > Ugh, yes.=A0=A0To me that means that MDNPAD is actually wrong and
> > > > should be
> > > > fixed to account for the implicit padding.=A0=A0That probably wou=
ld
> > > > result in
> > > > requiring separate values for MDNPAD.=A0=A0The 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.
> > > >=20
> > > > --
> > > > John Baldwin
> > > >=20
> > > >=20
> >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1504189560.56799.94.camel>