From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 15753A0032; Tue, 28 Sep 2021 10:53:58 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D0973410E5; Tue, 28 Sep 2021 10:53:57 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 9E326410D7 for ; Tue, 28 Sep 2021 10:53:56 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1632819236; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lcLM4WQDYSIvqNhskjJZVvwTxtYhLcw1IxS59pHgMGk=; b=YvuJImc+wdFqS+lhCpi6/LPEp2cFRJPoCugaOITXe31a6mfNkDKMQHqZJb6AyPbANVj7jD LOOSrVmIbmXKEH4XyD1N3M3r9GGtKwS1k14hgFser5drT/r8NnUcd7yiXvlfWY0XC2r6TH Yd1WbujaUMji38h9dpo2RnQfpe2VSBI= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-268-8m3a5crwOUmJZrmHxAP7Jw-1; Tue, 28 Sep 2021 04:53:54 -0400 X-MC-Unique: 8m3a5crwOUmJZrmHxAP7Jw-1 Received: by mail-wr1-f71.google.com with SMTP id r7-20020a5d6947000000b0015e0f68a63bso14851679wrw.22 for ; Tue, 28 Sep 2021 01:53:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent :content-language:to:cc:references:from:subject:in-reply-to :content-transfer-encoding; bh=lcLM4WQDYSIvqNhskjJZVvwTxtYhLcw1IxS59pHgMGk=; b=FHXFCLCAs1o3X8EHwO+1tk/FCvelHtijXzLTi0JE3hf1TshKLDzqbUQbaf78msWWcq ovZ4WIl2qydmHMlHKnV2dfLPxeUJx4ffkpLbBhWEqO02VMYNf/TLLeAwuA02FlwjKTpJ rldBtrMxbrejK41S77AtxBVETn/r204urShN4bkzVAjx+3WYBvtTB+fXmO+jv9YOG38a M1tiNwRC+beOTzcNj+AoUwQvg2ibzxy5JoWkYiJuzmsOq1AAwXgsx/SYNO0rPRE0d/ZR Kycmfz4H3zUTGUHWMjfvQTRbS/MMfLi0sjRSfJjVTpoX4vDfSgG91mipRmSP/3U83oON qyfw== X-Gm-Message-State: AOAM532h6pf1sypH9uE7RX+E4y+6zXrv+qqMr+FRJKU2pqtDQhIwK0fL 2AFyJc+J96KnzAXOnTRo+ZRZg/sLQjABHsk939BxDd8624Mw/aZEIecfikZd/v0H2SHoL9yi/LK 5cPQ= X-Received: by 2002:a7b:c314:: with SMTP id k20mr3515965wmj.50.1632819233500; Tue, 28 Sep 2021 01:53:53 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz13yb6bbmPSiVsf2KYubSBZr1XNIXs0xnUT70v/5TmEQXG/qCNN4kdzVC6PvQOyowMzFtoQg== X-Received: by 2002:a7b:c314:: with SMTP id k20mr3515942wmj.50.1632819233230; Tue, 28 Sep 2021 01:53:53 -0700 (PDT) Received: from [192.168.0.36] ([78.18.26.217]) by smtp.gmail.com with ESMTPSA id l26sm2007059wmi.25.2021.09.28.01.53.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Sep 2021 01:53:52 -0700 (PDT) Message-ID: <05c31f17-e00b-f08f-9b46-136ff8306474@redhat.com> Date: Tue, 28 Sep 2021 09:53:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 To: "Zhang, AlvinX" , "Xing, Beilei" , "Guo, Junfeng" Cc: "dev@dpdk.org" , "stable@dpdk.org" , "Zhang, Qi Z" , "Yigit, Ferruh" References: <20210926075757.15116-1-alvinx.zhang@intel.com> <863055cd-d436-6bdd-e6fe-2aa4d57db19a@redhat.com> From: Kevin Traynor In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ktraynor@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 28/09/2021 03:12, Zhang, AlvinX wrote: >> -----Original Message----- >> From: Kevin Traynor >> Sent: Tuesday, September 28, 2021 12:00 AM >> To: Zhang, AlvinX ; Xing, Beilei >> ; Guo, Junfeng >> Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z ; >> Yigit, Ferruh >> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics >> >> On 26/09/2021 08:57, Alvin Zhang wrote: >>> Some packets are discarded by the NIC because they are larger than the >>> MTU, these packets should be counted as "RX error" instead of "RX >>> packet". >>> >>> The register 'GL_RXERR1' can count above discarded packets. >>> This patch adds reading and calculation of the 'GL_RXERR1' counter >>> when reporting DPDK statistics. >>> >>> Fixes: f4a91c38b4ad ("i40e: add extended stats") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Alvin Zhang >>> --- >>> drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++--- >>> drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++ >>> 2 files changed, 23 insertions(+), 3 deletions(-) >>> >> >> It's a bit hard to understand the code for someone not familiar with the i40e >> stats. I think it needs careful review from i40e maintainers. A few questions >> below, >> >> Did you test this with testpmd? Can you show an example of a test where these >> packets are now correctly accounted for? > > The issue as below: > > sendp(Ether()/IP()/Raw('x' * 1500), iface="enp24s0f1") > ---------------------- Forward statistics for port 0 ---------------------- > RX-packets: 1 RX-dropped: 0 RX-total: 1 > TX-packets: 0 TX-dropped: 0 TX-total: 0 > ---------------------------------------------------------------------------- > > Although we didn't really got the packet, but the statistic indicates a packet has been received successfully. > We can add above example to commit log in V2. > Hi Alvin. Thanks for answering my questions and showing how it was tested. I don't have any more comments. I won't ack just because I am not familiar enough with the i40e stats in general to give a meaningful ack. Kevin. >> >> I see there is also an RXERR2 register that catches other errors, does it need to >> be considered as well? > > We are not sure whether the packets counted by RXERR2 also will be counted by "Rx-packets". > So in this patch we only consider RXERR1 and fix the issue mentioned above. > >> >>> diff --git a/drivers/net/i40e/i40e_ethdev.c >>> b/drivers/net/i40e/i40e_ethdev.c index 7a2a828..30a2cdf 100644 >>> --- a/drivers/net/i40e/i40e_ethdev.c >>> +++ b/drivers/net/i40e/i40e_ethdev.c >>> @@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf >> *pf, >>> /* store statistics names and its offset in stats structure */ >>> struct rte_i40e_xstats_name_off { >>> char name[RTE_ETH_XSTATS_NAME_SIZE]; >>> - unsigned offset; >>> + int offset; >> >> It is unusual to see you changing an offset to an int. You are expecting negative >> offsets? >> >>> }; >>> >>> static const struct rte_i40e_xstats_name_off >>> rte_i40e_stats_strings[] = { @@ -542,6 +542,8 @@ struct >> rte_i40e_xstats_name_off { >>> {"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)}, >>> {"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats, >>> rx_unknown_protocol)}, >>> + {"rx_err1", offsetof(struct i40e_pf, rx_err1) - >>> + offsetof(struct i40e_pf, stats)}, > > Here offsetof(struct i40e_pf, rx_err1) - offsetof(struct i40e_pf, stats) may be a negative value. > >> >> rx_err1 is correct by datasheet but meaningless to a user. Suggest to find a more >> descriptive name, or document what it is, or tell the user to reference the >> datasheet. > > I will update it in v2. > >> >>> {"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)}, >>> {"tx_multicast_packets", offsetof(struct i40e_eth_stats, >> tx_multicast)}, >>> {"tx_broadcast_packets", offsetof(struct i40e_eth_stats, >>> tx_broadcast)}, @@ -3238,6 +3240,10 @@ void >> i40e_flex_payload_reg_set_default(struct i40e_hw *hw) >>> pf->offset_loaded, >>> &os->eth.rx_unknown_protocol, >>> &ns->eth.rx_unknown_protocol); >>> + i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id + >> I40E_MAX_VF), >>> + I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF), >>> + pf->offset_loaded, &pf->rx_err1_offset, >>> + &pf->rx_err1); >>> i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port), >>> I40E_GLPRT_GOTCL(hw->port), >>> pf->offset_loaded, &os->eth.tx_bytes, @@ -3437,7 >> +3443,8 @@ >>> void i40e_flex_payload_reg_set_default(struct i40e_hw *hw) >>> stats->ipackets = pf->main_vsi->eth_stats.rx_unicast + >>> pf->main_vsi->eth_stats.rx_multicast + >>> pf->main_vsi->eth_stats.rx_broadcast - >>> - pf->main_vsi->eth_stats.rx_discards; >>> + pf->main_vsi->eth_stats.rx_discards - >>> + pf->rx_err1; >>> stats->opackets = ns->eth.tx_unicast + >>> ns->eth.tx_multicast + >>> ns->eth.tx_broadcast; >>> @@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct >> i40e_hw *hw) >>> pf->main_vsi->eth_stats.rx_discards; >>> stats->ierrors = ns->crc_errors + >>> ns->rx_length_errors + ns->rx_undersize + >>> - ns->rx_oversize + ns->rx_fragments + ns->rx_jabber; >>> + ns->rx_oversize + ns->rx_fragments + ns->rx_jabber + >>> + pf->rx_err1; >>> >>> if (pf->vfs) { >>> for (i = 0; i < pf->vf_num; i++) { @@ -6232,6 +6240,8 @@ struct >>> i40e_vsi * >>> memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats)); >>> memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats)); >>> memset(&pf->internal_stats_offset, 0, sizeof(struct >>> i40e_eth_stats)); >>> + pf->rx_err1 = 0; >>> + pf->rx_err1_offset = 0; >>> >>> ret = i40e_pf_get_switch_config(pf); >>> if (ret != I40E_SUCCESS) { >>> diff --git a/drivers/net/i40e/i40e_ethdev.h >>> b/drivers/net/i40e/i40e_ethdev.h index cd6deab..846c8d4 100644 >>> --- a/drivers/net/i40e/i40e_ethdev.h >>> +++ b/drivers/net/i40e/i40e_ethdev.h >>> @@ -19,6 +19,13 @@ >>> #include "base/i40e_type.h" >>> #include "base/virtchnl.h" >>> >>> +#define I40E_GL_RXERR1_H(_i) (0x00318004 + ((_i) * 8)) >>> +/** >>> + * _i=0...143, >>> + * counters 0-127 are for the 128 VFs, >>> + * counters 128-143 are for the 16 PFs */ >>> + >>> #define I40E_VLAN_TAG_SIZE 4 >>> >>> #define I40E_AQ_LEN 32 >>> @@ -1134,6 +1141,9 @@ struct i40e_pf { >>> >>> struct i40e_hw_port_stats stats_offset; >>> struct i40e_hw_port_stats stats; >>> + u64 rx_err1; /* rxerr1 */ >>> + u64 rx_err1_offset; >>> + >>> /* internal packet statistics, it should be excluded from the total */ >>> struct i40e_eth_stats internal_stats_offset; >>> struct i40e_eth_stats internal_stats; >>> >