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 B77AAA04FF; Thu, 5 May 2022 18:40:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3EC344281C; Thu, 5 May 2022 18:40:32 +0200 (CEST) Received: from mail-pj1-f42.google.com (mail-pj1-f42.google.com [209.85.216.42]) by mails.dpdk.org (Postfix) with ESMTP id 986714014F for ; Thu, 5 May 2022 18:40:30 +0200 (CEST) Received: by mail-pj1-f42.google.com with SMTP id gj17-20020a17090b109100b001d8b390f77bso8512760pjb.1 for ; Thu, 05 May 2022 09:40:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=yhR8/d9/DLhC3yDsEel+5cEfh5tM+fGXrCJPEwFQHK0=; b=LJPs18AahULYa5xUceg+4Nuz7OhfAdg+WyVUiWEXAP4TFgSc9oWGYTI5PLJrZKpD/A AzNMXOoALej6kWrvwgU+sdpBl1IKgJNwNJBZMobzsiNWD4i/gifk+K2FNctONpLzCLqX Pq86gppUezbFjVOwMRopwRVqZ0hhmXkACR5dpWah6CnVXsbZz1+6ajeLg9EJA2aBMJZq iDd36GKhEy45UZNufDbDNIMIYh2fNsIIUp8kBA6pW8tskwr2L05HGMSjgDwSdvk7lU3N EoQTBDem846ytK3ZPjc8Qul8vYMrUT/NRekKb8zdEZd+61QChEVvDTxhCXaAB9gyImiS JtWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=yhR8/d9/DLhC3yDsEel+5cEfh5tM+fGXrCJPEwFQHK0=; b=DcEDtujWRqULGP+YrzaPiXQHMYf5eAfkFBp1bx5W84kHE+B8ZkGQ5fQHPnjvaUbPjw wvVSh9wesFINIFKkA111R4FpHhSHGmBCM1LGq1SZK8C7a5yRe/5ErzpryCTTAC52glsC NAR/u9rghtQzNEB4cNyESM6Duq4BVNP21SxguN8nvXqzy6sE1sjrCj9EP/g5TGEWl65d SrhQkWQzDoGpNA5wgOZkKqBiL37n/sGk2gfjJ3YzunYTlzM90esTvo45CmJ7oddXWmGe 9NUazGiwiR7XkbcXcID0d0iWjB238Zxn4ql6S4uHs1LIytOWBzIaNbNpPmPicFASqtW6 krQw== X-Gm-Message-State: AOAM531sfA+nX7btCdfmBmHaITugn9ONA2Tu2JLSWVnDI9/sVJtChyCw 4f2peaTMXPUMZ1TXmBbOc+eHrkDl3Candg== X-Google-Smtp-Source: ABdhPJwV8uftfKlcD3WvHmh2NvEnCyc3cZ8S+TBiYNEQgAnOMYlvMYv7ncIF7MLMd+xnXkqAmUNGLQ== X-Received: by 2002:a17:902:e791:b0:151:dbbd:aeae with SMTP id cp17-20020a170902e79100b00151dbbdaeaemr28187382plb.171.1651768829768; Thu, 05 May 2022 09:40:29 -0700 (PDT) Received: from hermes.local (204-195-112-199.wavecable.com. [204.195.112.199]) by smtp.gmail.com with ESMTPSA id 28-20020a17090a1a5c00b001cd4989ff41sm1748927pjl.8.2022.05.05.09.40.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 May 2022 09:40:29 -0700 (PDT) Date: Thu, 5 May 2022 09:40:26 -0700 From: Stephen Hemminger To: Ferruh Yigit Cc: Long Li , "longli@linuxonhyperv.com" , "dev@dpdk.org" , Stephen Hemminger , "stable@dpdk.org" Subject: Re: [Patch v2] net/netvsc: report correct stats values Message-ID: <20220505094026.22e74f43@hermes.local> In-Reply-To: <7809f41b-c21b-5ebe-d830-91015edb0cb8@xilinx.com> References: <1648143948-17821-1-git-send-email-longli@linuxonhyperv.com> <7f51e773-6ded-b736-fb02-5e3b391353b9@xilinx.com> <20220426154524.49502217@hermes.local> <924d7398-6c78-6318-52f3-d671edfc8aad@xilinx.com> <04de7df6-3d4a-21e5-7be5-15f2ef88be16@xilinx.com> <99a629d6-642e-db25-eeaa-a9eceec577cb@xilinx.com> <7809f41b-c21b-5ebe-d830-91015edb0cb8@xilinx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 On Thu, 5 May 2022 17:28:38 +0100 Ferruh Yigit wrote: > On 5/4/2022 7:38 PM, Long Li wrote: > >> Subject: Re: [Patch v2] net/netvsc: report correct stats values > >> > >> On 5/3/2022 9:48 PM, Long Li wrote: > >>>> Subject: Re: [Patch v2] net/netvsc: report correct stats values > >>>> > >>>> On 5/3/2022 8:14 PM, Long Li wrote: > >>>>>> Subject: Re: [Patch v2] net/netvsc: report correct stats values > >>>>>> > >>>>>> On 5/3/2022 7:18 PM, Long Li wrote: > >>>>>>>> Subject: Re: [Patch v2] net/netvsc: report correct stats values > >>>>>>>> > >>>>>>>> On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>>> if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) { > >>>>>>>>>> - stats->q_opackets[i] = txq->stats.packets; > >>>>>>>>>> - stats->q_obytes[i] = txq->stats.bytes; > >>>>>>>>>> + stats->q_opackets[i] += txq->stats.packets; > >>>>>>>>>> + stats->q_obytes[i] += txq->stats.bytes; > >>>>>>>>> > >>>>>>>>> This is per queue stats, 'stats->q_opackets[i]', in next > >>>>>>>>> iteration of the loop, 'i' will be increased and 'txq' will be > >>>>>>>>> updated, so as far as I can see the above change has no affect. > >>>>>>>> > >>>>>>>> Agree, that is why it was just assignment originally. > >>>>>>> > >>>>>>> The condition here is a little different. NETVSC is a master > >>>>>>> device with > >>>>>> another PMD running as a slave. When reporting stats values, it > >>>>>> needs to add the values from the slave PMD. The original code just > >>>>>> overwrites the values from its slave PMD. > >>>>>> > >>>>>> Where the initial values are coming from, 'hn_vf_stats_get()'? > >>>>>> > >>>>>> If 'hn_vf_stats_get()' fills the stats, what are the values kept in > >>>>>> 'txq- > >>>>> stats.*' > >>>>>> in above updated loop? > >>>>> > >>>>> Yes, hn_vf_stats_get() fills in the stats from the slave PMD. > >>>>> txq->stats > >>>> values are from the master PMD. Those values are different and > >>>> accounted separated from the values from the slave PMD. > >>>> > >>>> I see, since this is a little different than what most of the PMDs > >>>> do, can you please put a little more info to the commit log? Or > >>>> perhaps can add some comments to the code. > >>> > >>> Ok, will do. > >>> > >>>> > >>>> And still 'stats->rx_nombuf' change is not required right? If so can > >>>> you remove it in the next version? > >>> > >>> It is still needed. NETVSC unconditionally calls the slave PMD to receive > >> packets, even if it can't allocate a mbuf to receive a synthetic packet itself. The > >> accounting of rx_nombuf is valid because the synthetic packets (to NETVSC) and > >> VF packets (to slave PMD) are routed separately from Hyper-V. > >> > >> I am not referring to the "+=" update, my comment was because 'stats- > >>> rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1]. > >> Is it still required? > > > > Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that, it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats->rx_nombuf. > > > > But its own stat also will be overwritten (not in PMD function, but in > ethdev layer). > 'stats->rx_nombuf' assignment in the PMD seems has no effect and can be > removed. > > I can't see how it is needed, can you please put a call stack to describe? This here: int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) { struct rte_eth_dev *dev; RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); dev = &rte_eth_devices[port_id]; if (stats == NULL) { RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to NULL\n", port_id); return -EINVAL; } memset(stats, 0, sizeof(*stats)); RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP); stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); } Will fill in rx_nombuf from the current rx_mbuf_alloc_failed. But it happens before the PMD specific stats function. For the case of nested device like netvsc PMD that value needs to be summed. What happens with VF would be: Call of rte_eth_stats_get on the netvsc PMD port. stats->rx_nombuf comes from rx_mbuf_alloc_failed of the vmbus part Netvsc PMD stats get calls rte_eth_stats_get on the VF Nested rte_eth_stats_get will set stats->rx_nombuf to the value from VF the previous value is overwritten. At end of Netvsc PMD stats get it should be adding the value of any allocation failures in the VMBUS part. Summary: rte_eth_stats_get: stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; // original rte_eth_stats_get(vf): stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; // VF alloc failed hn_dev_stats_get: stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed; // Vmbus alloc failed