From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 148DAECAAA1 for ; Tue, 30 Aug 2022 09:36:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229994AbiH3Jgf (ORCPT ); Tue, 30 Aug 2022 05:36:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50768 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229750AbiH3JgO (ORCPT ); Tue, 30 Aug 2022 05:36:14 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6216BE9905; Tue, 30 Aug 2022 02:33:56 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 7CA07B818C4; Tue, 30 Aug 2022 09:33:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9EC2FC433D7; Tue, 30 Aug 2022 09:33:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1661852034; bh=RtTBoBmIFnTDD6x72hBHnoVvBe1GpBrLQzVhYk4MgBQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rCWxQH8FcNcRmBJklBuDvw/CdqD37yq141CRU2jMGMPcbFVnqwXbg6PHxDMp2T0dq mKGSYSwq/CtUnQeBi0xTaTxIu4lvHNvglmfL0MzJUnJa8yep5+8StTjKTxYdLVp/42 z01SenxMYwyJSwzbD9ylNbvgOVTM5g4d8DrsUnNM= Date: Tue, 30 Aug 2022 11:33:51 +0200 From: Greg Kroah-Hartman To: Ilpo =?iso-8859-1?Q?J=E4rvinen?= Cc: Jiri Slaby , linux-serial , Andy Shevchenko , Jonathan Corbet , Vladimir Zapolskiy , Russell King , Richard Genoud , Nicolas Ferre , Alexandre Belloni , Claudiu Beznea , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Maxime Coquelin , Alexandre Torgue , linux-doc@vger.kernel.org, LKML , linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Lino Sanfilippo Subject: Re: [PATCH v2 4/4] serial: Add kserial_rs485 to avoid wasted space due to .padding Message-ID: References: <20220830072956.3630-1-ilpo.jarvinen@linux.intel.com> <20220830072956.3630-5-ilpo.jarvinen@linux.intel.com> <58d6748-ebd-e637-c1b2-b8e469e6d86d@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <58d6748-ebd-e637-c1b2-b8e469e6d86d@linux.intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org On Tue, Aug 30, 2022 at 12:26:29PM +0300, Ilpo Järvinen wrote: > On Tue, 30 Aug 2022, Greg Kroah-Hartman wrote: > > > On Tue, Aug 30, 2022 at 10:29:56AM +0300, Ilpo Järvinen wrote: > > > -static int serial_rs485_from_user(struct serial_rs485 *rs485, > > > +static int serial_rs485_from_user(struct kserial_rs485 *rs485, > > > const struct serial_rs485 __user *rs485_user) > > > { > > > - if (copy_from_user(rs485, rs485_user, sizeof(*rs485))) > > > + struct serial_rs485 rs485_uapi; > > > + > > > + if (copy_from_user(&rs485_uapi, rs485_user, sizeof(*rs485))) > > > return -EFAULT; > > > > > > + *rs485 = *((struct kserial_rs485 *)&rs485_uapi); > > > > Ah, you are mapping this on top of the existing structure, so there was > > no padding in the original one, why say that? > > While I'm not exactly sure what you tried to say with this, I'll try to > answer regardless. > > It's the opposite, there's padding in rs485_user, and therefore also in > rs485_uapi. Struct serial_rs485 has padding and is part of uapi so it > cannot be changed to remove the extra padding. > > I cannot directly copy_from_user into *rs485 because it lacks the padding. > Thus, the immediate rs485_uapi and then assign to rs485. Padding could be in the middle of the structure, it's not obvious that it is not there. You are just trying to drop the trailing "unused space", while all of the fields are identical otherwise. So be specific about that, as padding is often in the middle of a structure. > > > +/* > > > + * Compile-time asserts for struct kserial_rs485 and struct serial_rs485 equality > > > + * (except padding). > > > > This does not take into account any padding, in fact it's the opposite > > as all of this: > > ?? I said: "(except padding)" which I thought implies that padding is > intentionally excluded (it doesn't exist in kserial_rs485). > > > > + */ > > > +static_assert(offsetof(struct kserial_rs485, flags) == > > > + offsetof(struct serial_rs485, flags)); > > > +static_assert(offsetof(struct kserial_rs485, delay_rts_before_send) == > > > + offsetof(struct serial_rs485, delay_rts_before_send)); > > > +static_assert(offsetof(struct kserial_rs485, delay_rts_after_send) == > > > + offsetof(struct serial_rs485, delay_rts_after_send)); > > > +static_assert(offsetof(struct kserial_rs485, addr_recv) == > > > + offsetof(struct serial_rs485, addr_recv)); > > > +static_assert(offsetof(struct kserial_rs485, addr_dest) == > > > + offsetof(struct serial_rs485, addr_dest)); > > > +static_assert(sizeof(struct kserial_rs485) <= sizeof(struct serial_rs485)); > > > > Is there to ensure that the offsets are exactly the same, no padding > > involved anywhere. > > That's because for kernel padding "doesn't matter", it doesn't want it, > it would be just wasted space. After this series, padding is used only for > uapi, no longer for the in-kernel structs. Again, you are talking about padding at the end, not in the middle, hence my confusion. > > So I don't understand the problem you are trying to solve here, > > struct serial_rs485 has padding that is ~16B long currently. serial_rs485 > is currently used for a few things: > - Keep track of rs485 state (per port) > - Record what rs485 options the port supports (per port) > - Record rs485 options a driver supports (per driver with rs485 support) > - Exchange rs485 config/state information with userspace > > Only the last of those requires the padding (because it has been part of > uapi since day 1). With kserial_rs485, the padding can eliminated for the > first 3 cases. > > If you feel ~32B per uart_port too little to be useful (and a little > more per driver), I can just drop this patch. I think 32 bytes per serial port is totally lost in the noise and would not even be able to be measured at all due to how slabs are aligned (meaning you are not actually saving any memory at all.) Can you notice any measurable savings on your systems? And what is the code increase overall with this patch series? :) I'm all for making things const, to prevent errors, but that could probably be done without this type of change, right? thanks, greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9EC66C0502A for ; Tue, 30 Aug 2022 09:35:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=gSPJljAupn+1h5UXBo+RAnB1Jh9xU8bVNBNfsWxoxbA=; b=fHFPaIOQy29lpq cYRUMKQ12Bz2pl8jq+KAsO1Cfw4wId7W2aXYyt0rJuVvBaXDFNdx9cXm7vbp2zkA3WH08eZyRZqaI crrIYq+rYsGXweUidpIFi9c2qt/U2OxFtQBcolr+dMyOJBLSvhYoKprOnsXhkwhK+kT+78AXHf7Kl AzVEax89T+IrlxK/5eL/GgKfZiLXXbq5mpOVO1wC6hoSgJBTsRvOLBVmbeuW6zQxuiHgciKjNvrRc LLp1HxqYLXis64nA5zRoKpi/hBPCZMNe2SLEIQmzGUGtWwyGeL/GyHRWlmS/9IFR3+sZr+DXO6PtC izMdpjoir3RO2XeoLhgQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oSxdM-00FgzT-8W; Tue, 30 Aug 2022 09:34:12 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oSxd6-00FguI-0K for linux-arm-kernel@lists.infradead.org; Tue, 30 Aug 2022 09:34:06 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id BF0C3604F5; Tue, 30 Aug 2022 09:33:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9EC2FC433D7; Tue, 30 Aug 2022 09:33:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1661852034; bh=RtTBoBmIFnTDD6x72hBHnoVvBe1GpBrLQzVhYk4MgBQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rCWxQH8FcNcRmBJklBuDvw/CdqD37yq141CRU2jMGMPcbFVnqwXbg6PHxDMp2T0dq mKGSYSwq/CtUnQeBi0xTaTxIu4lvHNvglmfL0MzJUnJa8yep5+8StTjKTxYdLVp/42 z01SenxMYwyJSwzbD9ylNbvgOVTM5g4d8DrsUnNM= Date: Tue, 30 Aug 2022 11:33:51 +0200 From: Greg Kroah-Hartman To: Ilpo =?iso-8859-1?Q?J=E4rvinen?= Subject: Re: [PATCH v2 4/4] serial: Add kserial_rs485 to avoid wasted space due to .padding Message-ID: References: <20220830072956.3630-1-ilpo.jarvinen@linux.intel.com> <20220830072956.3630-5-ilpo.jarvinen@linux.intel.com> <58d6748-ebd-e637-c1b2-b8e469e6d86d@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <58d6748-ebd-e637-c1b2-b8e469e6d86d@linux.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220830_023404_458000_1E1815F9 X-CRM114-Status: GOOD ( 35.91 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , linux-doc@vger.kernel.org, Alexandre Torgue , LKML , Fabio Estevam , linux-stm32@st-md-mailman.stormreply.com, Jonathan Corbet , Jiri Slaby , Russell King , Lino Sanfilippo , NXP Linux Team , linux-serial , Pengutronix Kernel Team , Sascha Hauer , Vladimir Zapolskiy , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, Richard Genoud , Maxime Coquelin , Shawn Guo , Claudiu Beznea Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Aug 30, 2022 at 12:26:29PM +0300, Ilpo J=E4rvinen wrote: > On Tue, 30 Aug 2022, Greg Kroah-Hartman wrote: > = > > On Tue, Aug 30, 2022 at 10:29:56AM +0300, Ilpo J=E4rvinen wrote: > > > -static int serial_rs485_from_user(struct serial_rs485 *rs485, > > > +static int serial_rs485_from_user(struct kserial_rs485 *rs485, > > > const struct serial_rs485 __user *rs485_user) > > > { > > > - if (copy_from_user(rs485, rs485_user, sizeof(*rs485))) > > > + struct serial_rs485 rs485_uapi; > > > + > > > + if (copy_from_user(&rs485_uapi, rs485_user, sizeof(*rs485))) > > > return -EFAULT; > > > = > > > + *rs485 =3D *((struct kserial_rs485 *)&rs485_uapi); > > = > > Ah, you are mapping this on top of the existing structure, so there was > > no padding in the original one, why say that? > = > While I'm not exactly sure what you tried to say with this, I'll try to = > answer regardless. > = > It's the opposite, there's padding in rs485_user, and therefore also in = > rs485_uapi. Struct serial_rs485 has padding and is part of uapi so it = > cannot be changed to remove the extra padding. > = > I cannot directly copy_from_user into *rs485 because it lacks the padding= . = > Thus, the immediate rs485_uapi and then assign to rs485. Padding could be in the middle of the structure, it's not obvious that it is not there. You are just trying to drop the trailing "unused space", while all of the fields are identical otherwise. So be specific about that, as padding is often in the middle of a structure. > > > +/* > > > + * Compile-time asserts for struct kserial_rs485 and struct serial_r= s485 equality > > > + * (except padding). > > = > > This does not take into account any padding, in fact it's the opposite > > as all of this: > = > ?? I said: "(except padding)" which I thought implies that padding is = > intentionally excluded (it doesn't exist in kserial_rs485). > = > > > + */ > > > +static_assert(offsetof(struct kserial_rs485, flags) =3D=3D > > > + offsetof(struct serial_rs485, flags)); > > > +static_assert(offsetof(struct kserial_rs485, delay_rts_before_send) = =3D=3D > > > + offsetof(struct serial_rs485, delay_rts_before_send)); > > > +static_assert(offsetof(struct kserial_rs485, delay_rts_after_send) = =3D=3D > > > + offsetof(struct serial_rs485, delay_rts_after_send)); > > > +static_assert(offsetof(struct kserial_rs485, addr_recv) =3D=3D > > > + offsetof(struct serial_rs485, addr_recv)); > > > +static_assert(offsetof(struct kserial_rs485, addr_dest) =3D=3D > > > + offsetof(struct serial_rs485, addr_dest)); > > > +static_assert(sizeof(struct kserial_rs485) <=3D sizeof(struct serial= _rs485)); > > = > > Is there to ensure that the offsets are exactly the same, no padding > > involved anywhere. > = > That's because for kernel padding "doesn't matter", it doesn't want it, > it would be just wasted space. After this series, padding is used only fo= r = > uapi, no longer for the in-kernel structs. Again, you are talking about padding at the end, not in the middle, hence my confusion. > > So I don't understand the problem you are trying to solve here, > = > struct serial_rs485 has padding that is ~16B long currently. serial_rs485 = > is currently used for a few things: > - Keep track of rs485 state (per port) > - Record what rs485 options the port supports (per port) > - Record rs485 options a driver supports (per driver with rs485 support) > - Exchange rs485 config/state information with userspace > = > Only the last of those requires the padding (because it has been part of = > uapi since day 1). With kserial_rs485, the padding can eliminated for the = > first 3 cases. > = > If you feel ~32B per uart_port too little to be useful (and a little = > more per driver), I can just drop this patch. I think 32 bytes per serial port is totally lost in the noise and would not even be able to be measured at all due to how slabs are aligned (meaning you are not actually saving any memory at all.) Can you notice any measurable savings on your systems? And what is the code increase overall with this patch series? :) I'm all for making things const, to prevent errors, but that could probably be done without this type of change, right? thanks, greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel