Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sat, 30 Apr 2022 15:31:28 -0700
From:   Colin Foster <colin.foster@in-advantage.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        aolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>
Subject: Re: [PATCH v1 net-next 1/1] net: ethernet: ocelot: remove the need
 for num_stats initializer
Message-ID: <20220430223128.GB3871052@euler>
References: <20220429213036.3482333-1-colin.foster@in-advantage.com>
 <20220429213036.3482333-2-colin.foster@in-advantage.com>
 <20220430151530.zaf7hyagln5jqjyi@skbuf>
 <20220430174735.GD3846867@euler>
 <20220430213344.ifiw2wjtxqd2dqbj@skbuf>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220430213344.ifiw2wjtxqd2dqbj@skbuf>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?iTO1rS2u95aHIHWiRilHW89Plh9Egf55j51KSpj/jJMDPL6rke/Z+7XBauii?=
 =?us-ascii?Q?YLT/JzzH/4AcW/q2zCECSbc3hVU36RkYpKDIsBa3TX7Vbcilz1K3QOqRed11?=
 =?us-ascii?Q?6Op/J9sMKypD11VIehVxLswvV9jXDKnIvslYsv8QiG1jbGo+Uebn9p4OwEFI?=
 =?us-ascii?Q?n6B7rGXVhdbVlr3ULFjNt0QjEySc67GlnixiR5+6cSSrWPwMs02Sxd8hhVNO?=
 =?us-ascii?Q?y2g98RDXcazNIahJWyyAqG1GcD92aeQ/yobEcoVnXkYvTK5splbtgnuMzl0j?=
 =?us-ascii?Q?iZmXS63ogLZ/XQ1eodhEz8vXS54Z27zB2zav7U2+mfe9PH2+Wwow/HGWlZDy?=
 =?us-ascii?Q?jK+NWU14rbRTRt2zMXKk8a0IK6wl/LpGqgKzp/JfvZMWXsHjNvT+jLqZkgFs?=
 =?us-ascii?Q?M75bV1V6mJ2ynahdbjhdAnQqr8UUA/5SKCRwzfpgk9QfV67RMrr4IE6CXdum?=
 =?us-ascii?Q?zPs/ib0PRW/gLP8+U389lU5iQltHBxAQMNHd0KTBXPYvsnXKNB5RTi2ZpUSH?=
 =?us-ascii?Q?6hKZ36QP9Hq1KCX8u3A9eO+8h8Fht7WYTdQ/2VNMQEKcv7qcBRCVei/5qJ88?=
 =?us-ascii?Q?ZoeMRhNy98U+g9QZKzzt4jF3R4PtMNmSlbFCbzfNtbt2wxYHOWwU9GzK1Kgy?=
 =?us-ascii?Q?ZwEo/fMw+MTYZfBVoeeA1IidJMSi6aB8duBdEfUuzhTz5kXYzi1tf8lYdbkL?=
 =?us-ascii?Q?ZwkMlBwc2oth5afOa3FNbfSAbpPDEPamD2zBPfmmW/oPBNO6Xp9p0qmHXAei?=
 =?us-ascii?Q?x4b1CNQN0xAv9fyh8CT4+z4ypgcJEc/26oBxxnLsJscKc89DyEqAxk0aSpUl?=
 =?us-ascii?Q?mGhBlGmDcMJheOcfKH1Jwqs/sfxy2L6i+GxGudfCoQZFWuabbFFSgqucUpAp?=
 =?us-ascii?Q?6gKJZnBTYCYWGmVtGf8H9fWN2kb4HUtfNivf0x99nmwTG41IKaWHn/jazPJ9?=
 =?us-ascii?Q?OJutT4rWDj1ifQNg8/zcCYHkoUa4hoGcLTHoZgX8P4oZxrTu5/uLAv9nvvXg?=
 =?us-ascii?Q?2eap3ZZrrinFicDrRDKU7/OdRcX5+22AxOkJN9ha1gcMCUbY1cjnKMdxIXB2?=
 =?us-ascii?Q?PVUFJu0qZobpcTxhj9D6/G55hxdV6ahWEsQ31rNFlyJtiREg6+lgLlp8W3xM?=
 =?us-ascii?Q?oYHYOwLbSrCVqV++hQZAY1CbDCnG7ahncrpyT2uzcpyJnOxFxttZq9GJvNi/?=
 =?us-ascii?Q?nA7IuqZL6sP9uRDPoWF3h/QUTEsq5Fi/M638NuLljfItg1Bz7w6J/w0s1Abd?=
 =?us-ascii?Q?lY28ZVx6ro9lPyiJfrKmqkOYDjYN+1UNIrtA15WxENXJrN6s5+dOHEQcFiFa?=
 =?us-ascii?Q?YPmPvdYGChlK0qd/dbcRi6F43AN1NvcyG+ux8x5Yq754fEWAZfFfceNkiqZv?=
 =?us-ascii?Q?JoPATOe2EV3bPaqNeAXhZW4ObFqrPp7EOhs8rCPkjFvOIeXdB0eL+U/cd7iW?=
 =?us-ascii?Q?CQvsbwjBUV3DWSvoemmLA1LSAnFC7lArPMXMLHc0ER7qpKfDB0XJXuaYoIul?=
 =?us-ascii?Q?95sMS1eb9iq9NT36leJgAaPrh15kOXJZWkCgYY+MqPw+BhaDGAs1P/Fb1aqX?=
 =?us-ascii?Q?w9lp/cJKoPT0GFELZpGRThZYZOoPZtJWyZxlfS/jjzuKwvps1s0fFj7+bCsb?=
 =?us-ascii?Q?z7sogBGncRwEyyg1Kv19S0NxhrJy2ZNH0hqn6tdQBnAXlSLpqxwzDGosbcxM?=
 =?us-ascii?Q?v7fmnZcb8GrJJd7ms4+ojCN3/GcnOhReQ+hwKttRWcSBZ/IrIGWUW3vhGjuk?=
 =?us-ascii?Q?nXZu4lQBAZRVI0zCblqC6rDjDAWqSdvkIHz3pmOVoTYzk33BUd53?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2582e727-2c08-4227-7513-08da2af92d0f
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2351.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2022 22:31:31.8963
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VDtAx3bpoe82+DKZHHHO+89FHftn6Og4OjDGmC795auzqcznpajVYbeHaHNRTdZK9zOOfR1cZLa5LPgO9/mmeS0zw2pxJDsm255HevYqUeE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR1001MB2407
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RDNS_NONE,
	SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, Apr 30, 2022 at 09:33:45PM +0000, Vladimir Oltean wrote:
> On Sat, Apr 30, 2022 at 10:47:35AM -0700, Colin Foster wrote:
> > > >  struct ocelot_stat_layout {
> > > >  	u32 offset;
> > > > +	u32 flags;
> > > 
> > > Was it really necessary to add an extra u32 to struct ocelot_stat_layout?
> > > Couldn't you check for the end of stats by looking at stat->name[0] and
> > > comparing against the null terminator, for an empty string?
> > 
> > I considered this as well. I could either have explicitly added the
> > flags field, as I did, or implicitly looked for .name == NULL (or
> > name[0] == '\0' as you suggest).
> 
> No, you cannot check for .name == NULL. The "name" member of struct
> ocelot_stat_layout is most definitely not NULL, but has the value of the
> memory address of the first char from that array. Contrast this with
> "char *name", where a NULL comparison can indeed be made.

My apologies - I had the structure wrong in my head and thought it was a
const char *. Checking for NULL is clearly not an option.

> 
> > I figured it might be better to make this an explicit relationship by
> > way of flags - but I'm happy to change OCELOT_STAT_END and for_each_stat
> > to rely on .name if you prefer.
> 
> I would have understood introducing a flag to mark the last element of
> an array as special (as opposed to introducing a dummy extra element).
> But even that calculation would have been wrong.
> 
> Before:
> 
> pahole -C ocelot_stat_layout drivers/net/ethernet/mscc/ocelot.o
> struct ocelot_stat_layout {
>         u32                        offset;               /*     0     4 */
>         char                       name[32];             /*     4    32 */
> 
>         /* size: 36, cachelines: 1, members: 2 */
>         /* last cacheline: 36 bytes */
> };
> 
> After:
> 
> pahole -C ocelot_stat_layout drivers/net/ethernet/mscc/ocelot.o
> struct ocelot_stat_layout {
>         u32                        offset;               /*     0     4 */
>         u32                        flags;                /*     4     4 */
>         char                       name[32];             /*     8    32 */
> 
>         /* size: 40, cachelines: 1, members: 3 */
>         /* last cacheline: 40 bytes */
> };
> 
> For example, vsc9959_stats_layout has 92 elements (93 with the dummy one
> you've added now). The overhead of 4 bytes per element amounts to 368
> extra bytes. Whereas a single dummy element at the end would have
> amounted to just 36 extra bytes.
> 
> With your approach, what we get is 372 extra bytes, so worst of both worlds.

Understood. I'll send an update momentarily. I also didn't know about
'pahole' which looks to be a useful tool!  Thanks again for the
feedback.

> 
> > > >  	char name[ETH_GSTRING_LEN];
> > > >  };
> > > >  
> > > > +#define OCELOT_STAT_END { .flags = OCELOT_STAT_FLAG_END }