netdev - Re: [PATCH v1 net-next 1/1] net: ethernet: ocelot: remove the need for num

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 30 Apr 2022 15:31:28 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....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 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 }