On Sun, 6 Sep 2020 13:47:13 +0300 Shay Agroskin wrote:
> Maciej Fijalkowski <maciej.fijalkow...@intel.com> writes:
> > I don't want to stir up the pot, but do you really need the
> > offsetof() of
> > each member in the stats struct? Couldn't you piggyback on
> > assumption that
> > these stats need to be u64 and just walk the struct with
> > pointer?
> >
> > struct ena_ring *ring;
> > int offset;
> > int i, j;
> > u8 *ptr;
> >
> > for (i = 0; i < adapter->num_io_queues; i++) {
> > /* Tx stats */
> > ring = &adapter->tx_ring[i];
> > ptr = (u8 *)&ring->tx_stats;
> >
> > for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
> > ena_safe_update_stat((u64 *)ptr,
> > (*data)++, &ring->syncp);
> > ptr += sizeof(u64);
> > }
> > }
> >
> > I find this as a simpler and lighter solution. There might be
> > issues with
> > code typed in email client, but you get the idea.
> >
> >>
> >> of course we need to convert the stat_offset field to be in 8
> >> bytes resolution instead.
> >>
> >> This approach has a potential bug hidden in it. If in the
> >> future
> >> someone decides to expand the "ena_stats_tx" struct and add a
> >> field preceding cnt,
> >> cnt will no longer be the beginning of the struct, which will
> >> cause a bug."
> >>
> >> Therefore, if you have another way to do this, please share
> >> it. Otherwise I'd
> >> rather leave this code as it is for the sake of robustness.
> >>
> >> >
> >> > Andrew
>
> Hi all,
>
> We tried to implement your suggestion, and found that removing the
> stat_offset
> field causes problems that are challenging to solve.
> Removing stat_offset introduces a requirement that the statistics
> in a stat
> strings array (check [1] for example) and stat variables struct
> (check [2] for
> example) must be in the same order.
> This requirement is prone to future bugs that might be challenging
> to locate.
> We also tried to unify the array and struct creation by
> using X macros. At the moment this change requires more time and
> effort by us
> and our customers need this code merged asap.
>
> [1] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71
> [2] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232
>
> (This message was sent before but didn't seem to get into the
> mailing list. Apologies if you got it twice)
Divide the offset by 8, cast &ring->tx_stats to u64 * (without
referencing cnt). That should be fine.
