Re: [Intel-wired-lan] [net-next, v3 1/2] ice: Accumulate HW and Netdev statistics over reset

[Benjamin Mikailenko <benjamin.mikailenko@intel.com>]

On 10/31/2022 2:46 AM, Paul Menzel wrote:
> Dear Benjamin,
> 
> 
> Thank you for the patch.
> 

Hello Paul,
Thanks for the review!
I apologize for the prolonged response. I wanted to reach out to other 
developers before responding.

> Am 27.10.22 um 17:24 schrieb Benjamin Mikailenko:
>> Resets happen with or without user interaction. For example, incidents
>> such as TX hang or a reconfiguration of parameters will result in a reset.
> 
> Are these resets logged somewhere?
> 

Dmesg will show a reset occurred.
The ICE driver also maintains a counter of how many resets were performed. 
But this information isn't currently reported to the user.

>> During reset, hardware and software statistics were set to zero. This
>> created an issue for the user where a reset happens in the background,
>> statistics set to zero, and the user checks statistics expecting them to
>> be populated.
>>
>> To ensure this doesn't happen, keep accumulating stats over reset.
> 
> Are there users, which expect the statistics to be set to 0 after a reset?
> 

There's currently no functionality for a user to purposefully initiate a standalone
reset. The only time a reset will occur is as result of another process. Since the
ability isn't possible, I don't see why a current user would expect statistics to
set to zero over reset. If they did want to reset statistics unloading/reloading
the driver will reset statistics.

> Also, `rtnl_link_stats64` (`include/uapi/linux/if_link.h`) contains more fields than rx/tx packets/bytes. Do they need to be adapted too?
> 

The other fields in rtnl_link_stats64 aren't currently used.

>> 1. Remove function calls which reset hardware and netdev statistics.
>> 2. Do not rollover statistics in ice_stat_update40 during reset.
>>
>> Signed-off-by: Benjamin Mikailenko <benjamin.mikailenko@intel.com>
>> ---
> 
> Do you have an overview, what changed between v1 and v3?
> 

Adding this to the next version (v4):
v2: Fixed styling
v3: Allocate statistic structures when XDP is enabled
v4: Added revision notes

>>   drivers/net/ethernet/intel/ice/ice.h         |  1 +
>>   drivers/net/ethernet/intel/ice/ice_dcb_lib.c |  3 ++
>>   drivers/net/ethernet/intel/ice/ice_lib.c     |  7 +++++
>>   drivers/net/ethernet/intel/ice/ice_main.c    | 30 +++++++++++++++++---
>>   4 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>> index 001500afc4a6..e0ed2f1cc434 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -358,6 +358,7 @@ struct ice_vsi {
>>         /* VSI stats */
>>       struct rtnl_link_stats64 net_stats;
>> +    struct rtnl_link_stats64 net_stats_prev;
>>       struct ice_eth_stats eth_stats;
>>       struct ice_eth_stats eth_stats_prev;
>>   diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
>> index 5e6d168bac48..4f3a848f4e99 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
>> @@ -874,6 +874,9 @@ void ice_update_dcb_stats(struct ice_pf *pf)
>>       prev_ps = &pf->stats_prev;
>>       cur_ps = &pf->stats;
>>   +    if (ice_is_reset_in_progress(pf->state))
>> +        pf->stat_prev_loaded = false;
>> +
>>       for (i = 0; i < 8; i++) {
>>           ice_stat_update32(hw, GLPRT_PXOFFRXC(port, i),
>>                     pf->stat_prev_loaded,
>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>> index 938ba8c215cb..a5945319b62e 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>> @@ -1795,11 +1795,15 @@ void ice_update_eth_stats(struct ice_vsi *vsi)
>>   {
>>       struct ice_eth_stats *prev_es, *cur_es;
>>       struct ice_hw *hw = &vsi->back->hw;
>> +    struct ice_pf *pf = vsi->back;
>>       u16 vsi_num = vsi->vsi_num;    /* HW absolute index of a VSI */
>>         prev_es = &vsi->eth_stats_prev;
>>       cur_es = &vsi->eth_stats;
>>   +    if (ice_is_reset_in_progress(pf->state))
>> +        vsi->stat_offsets_loaded = false;
>> +
>>       ice_stat_update40(hw, GLV_GORCL(vsi_num), vsi->stat_offsets_loaded,
>>                 &prev_es->rx_bytes, &cur_es->rx_bytes);
>>   @@ -3279,6 +3283,8 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi)
>>               goto err_vectors;
>>             ice_vsi_map_rings_to_vectors(vsi);
>> +
>> +        vsi->stat_offsets_loaded = false;
>>           if (ice_is_xdp_ena_vsi(vsi)) {
>>               ret = ice_vsi_determine_xdp_res(vsi);
>>               if (ret)
>> @@ -3315,6 +3321,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi)
>>           if (ret)
>>               goto err_vectors;
>>   +        vsi->stat_offsets_loaded = false;
>>           break;
>>       case ICE_VSI_CHNL:
>>           if (test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 189160dacad9..ac893ce39e5e 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -6487,6 +6487,7 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi *vsi,
>>    */
>>   static void ice_update_vsi_ring_stats(struct ice_vsi *vsi)
>>   {
>> +    struct rtnl_link_stats64 *net_stats, *stats_prev;
>>       struct rtnl_link_stats64 *vsi_stats;
>>       u64 pkts, bytes;
>>       int i;
>> @@ -6526,10 +6527,28 @@ static void ice_update_vsi_ring_stats(struct ice_vsi *vsi)
>>         rcu_read_unlock();
>>   -    vsi->net_stats.tx_packets = vsi_stats->tx_packets;
>> -    vsi->net_stats.tx_bytes = vsi_stats->tx_bytes;
>> -    vsi->net_stats.rx_packets = vsi_stats->rx_packets;
>> -    vsi->net_stats.rx_bytes = vsi_stats->rx_bytes;
>> +    net_stats = &vsi->net_stats;
>> +    stats_prev = &vsi->net_stats_prev;
>> +
>> +    /* clear prev counters after reset */
>> +    if (vsi_stats->tx_packets < stats_prev->tx_packets ||
>> +        vsi_stats->rx_packets < stats_prev->rx_packets) {
> 
> Why not unconditionally clear it?
> 
>> +        stats_prev->tx_packets = 0;
>> +        stats_prev->tx_bytes = 0;
>> +        stats_prev->rx_packets = 0;
>> +        stats_prev->rx_bytes = 0;
>> +    }
>> +
>> +    /* update netdev counters */
>> +    net_stats->tx_packets += vsi_stats->tx_packets - stats_prev->tx_packets;
>> +    net_stats->tx_bytes += vsi_stats->tx_bytes - stats_prev->tx_bytes;
>> +    net_stats->rx_packets += vsi_stats->rx_packets - stats_prev->rx_packets;
>> +    net_stats->rx_bytes += vsi_stats->rx_bytes - stats_prev->rx_bytes;
>> +
>> +    stats_prev->tx_packets = vsi_stats->tx_packets;
>> +    stats_prev->tx_bytes = vsi_stats->tx_bytes;
>> +    stats_prev->rx_packets = vsi_stats->rx_packets;
>> +    stats_prev->rx_bytes = vsi_stats->rx_bytes;
>>         kfree(vsi_stats);
>>   }
>> @@ -6591,6 +6610,9 @@ void ice_update_pf_stats(struct ice_pf *pf)
>>       prev_ps = &pf->stats_prev;
>>       cur_ps = &pf->stats;
>>   +    if (ice_is_reset_in_progress(pf->state))
>> +        pf->stat_prev_loaded = false;
>> +
>>       ice_stat_update40(hw, GLPRT_GORCL(port), pf->stat_prev_loaded,
>>                 &prev_ps->eth.rx_bytes,
>>                 &cur_ps->eth.rx_bytes);
> 
> 
> 
> Kind regards,
> 
> Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan