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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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 }