From owner-svn-src-head@freebsd.org Tue Aug 29 19:11:09 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 40F87DE34F5 for ; Tue, 29 Aug 2017 19:11:09 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: from mail-vk0-x236.google.com (mail-vk0-x236.google.com [IPv6:2607:f8b0:400c:c05::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id EEBDA6DA46 for ; Tue, 29 Aug 2017 19:11:08 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: by mail-vk0-x236.google.com with SMTP id d124so12153199vkf.3 for ; Tue, 29 Aug 2017 12:11:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sippysoft-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=i/t8aivK+boPUbPrWL3andOfTkHlK0e+DfFnYxIqofw=; b=MGQnMNGruAu9l8RXrHS8Jhq//x2QRykoVfcv4t02W4aRcYfsX278dfFdHPFTTSGP8K O+FD3aiH0ifpFbHuLM/bzShwD1l2KsG2UxMO9jVcBvS6sxUlPX8iZ9OJv2EIEtief22M XmAhVW7/OiBkvMRe1FizVuQohHXaZ769YxIA3JxcVgL270WAwLEAOMikW56cDnZzi4OW /k3aM0HzqiBplnqSGhXrfMF3PicpmmlJsXBLsQpPK/4ta+rq9MF3acyLQYw/YKkKhrNv aN9embjTR2UAGqevgxavE46uUXgerAe5IAye6/pC4oJD+T6XQsm6LGmNSclK7JGayvmD 4X7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=i/t8aivK+boPUbPrWL3andOfTkHlK0e+DfFnYxIqofw=; b=qnyCljyObtvYwGg278BxKcRxNaLCoMVwUoSQ6LV9pQtQm4JKaW0ha/hukygBJb63hJ xuVO/GTOLZx3w479AJR8ED/YYxmRLOughedwRRAobbJo6SkD9/HFI0CNOSkReqz02RNt rKvVCRUK7m0QqGxWynLTqQWvI8iESBxGEFQ5nJmDUlQ1DzAvwASmHiWGVQXdzPR6zE2E t2/Ta2Sdb8QvZiBFMWEBoWGu+tD86lskIWLZr1RxbkrAznX/xY9O+RQy5W/5I004Ujrd dxRARy5yZvQo+UqhKtxXI+1B48Cv5HtkAxM5edMddN3LDrl3vmMHkK4B1bDV2rrKaC/h TnQw== X-Gm-Message-State: AHYfb5i7v2GgWRE2K8jusUI5yPD6N6fTrc7pvJjO2f9vvkOu/xCbBz8z fy8yu7GEK4Q0uis/GqrkXuu7tKFOduHx X-Google-Smtp-Source: ADKCNb5dfvO00mlYbfJgJm3KbXUawSL2YWUCm4EdvTqL1wW2X/QBGcuWp1OGZhZnqGOm+7z+PFWBp3Lg3a4+4Fjv8fY= X-Received: by 10.31.191.83 with SMTP id p80mr949378vkf.119.1504033867498; Tue, 29 Aug 2017 12:11:07 -0700 (PDT) MIME-Version: 1.0 Sender: sobomax@sippysoft.com Received: by 10.176.6.137 with HTTP; Tue, 29 Aug 2017 12:11:06 -0700 (PDT) In-Reply-To: <1504016363.56799.71.camel@freebsd.org> References: <201708281554.v7SFs8fr014268@repo.freebsd.org> <2937323.CvTEtZnL2T@ralph.baldwin.cx> <1504016363.56799.71.camel@freebsd.org> From: Maxim Sobolev Date: Tue, 29 Aug 2017 12:11:06 -0700 X-Google-Sender-Auth: O-mNGBiyf8SZQgLO_zkLyHFLTxk Message-ID: Subject: Re: svn commit: r322969 - in head: sbin/mdconfig sys/dev/md sys/sys To: Ian Lepore Cc: John Baldwin , Ryan Libby , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Aug 2017 19:11:09 -0000 Ian, yes, you are right, there appears to be md_ioctl wrapper in the FREEBSD32 layer, which I've completely missed it turns out. I add the label support into that as well. Thanks for bringing it up, I'll follow-up with the patch to make 32-bit code fully functional WRT labels. It's not broken at the moment but the label is not going to be visible to the 32-bit version of the mdconfig(8) running on 64-bit kernels. -Max On Tue, Aug 29, 2017 at 7:19 AM, Ian Lepore wrote: > 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 > > wrote: > > > > > > > > On Monday, August 28, 2017 12:46:48 PM Ryan Libby wrote: > > > > > > > > On Mon, Aug 28, 2017 at 11:24 AM, Maxim Sobolev > > > 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 > > > > > 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 > > > > > > > >