Messages in this thread | | | Date | Sat, 30 Apr 2022 15:31:28 -0700 | From | Colin Foster <> | 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 }
| |