lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 30 Apr 2022 21:33:45 +0000 From: Vladimir Oltean <vladimir.oltean@....com> To: Colin Foster <colin.foster@...advantage.com> CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, aolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, "David S. Miller" <davem@...emloft.net>, Florian Fainelli <f.fainelli@...il.com>, Vivien Didelot <vivien.didelot@...il.com>, Andrew Lunn <andrew@...n.ch>, "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>, Alexandre Belloni <alexandre.belloni@...tlin.com>, Claudiu Manoil <claudiu.manoil@....com> Subject: Re: [PATCH v1 net-next 1/1] net: ethernet: ocelot: remove the need for num_stats initializer 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. > 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. > > > char name[ETH_GSTRING_LEN]; > > > }; > > > > > > +#define OCELOT_STAT_END { .flags = OCELOT_STAT_FLAG_END }
Powered by blists - more mailing lists