From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 20DCD1B3CC for ; Mon, 16 Oct 2017 10:28:04 +0200 (CEST) Received: by mail-wm0-f43.google.com with SMTP id u138so533542wmu.5 for ; Mon, 16 Oct 2017 01:28:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=TTvDGjK0JrBlhq60jkAsP7ztFEC7s+CDXFVRaCcoL7c=; b=DtysDlmmMaX19ZCF8vg3njsx3VPIA/pIajTlLI4fupuELkpcrUISvJI1qep9/hpUIw DrsQ+wsoXYU8EVG/T2sffGaVwlStxXPnXsjZRrYqUxiMJY8xVu/2tWgWfr0sdbRfsXsp rNmqhBJaAGMTUdvf3cporvGL1z3LWkk6AGE0MAH0tQXzYE5Mk9VdICo4pzfuosZZ9RTw AJW9lHnmQsgPIrFm792m3exXAHRFwfuayvO8S+1bPsQ9LBM1UM9fhw887kEiID6/yBP3 OAySBPQdRkHbfZQz8Cs/0blU9y5RVLVUuc5DYxSkAaCxypK+BnCP8KjZfvJiHosnDeLX DpoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=TTvDGjK0JrBlhq60jkAsP7ztFEC7s+CDXFVRaCcoL7c=; b=DD5DoP6mHSq4DXvVn5G36CShj5+dn2hIUJou5jcgMW8mb7qWYIR5LIQ+9DxYowBHI+ GWcIylgeXU+pEXmwf/MfQPOIhkWvNEMieoI0y7AaI12DPFH2HpHuxxiDbdII71NzLS01 fYSiHdZpoHU/2xIQZY7SEQG6ky2KWncjyJpJgmnhJuta5LvUOkPNKXuQe9y7DvN8OvYA vC3HZW59k5/PiiewzBrMZaXeo9G5Qh77ymShEUdyMxDvdXrasF6LLmdmbGYa3z+Ctkjf AO1a6byo5vh3vCgcDtD+VY5EQI3woc8wWixe2M/TE1RPNQdaIcr9566jWNNU2pIsr3ZK dz0w== X-Gm-Message-State: AMCzsaW+vLCWjd83O6xmSDCNXfc833KU7iM4eS1mOz+W6tnGGhd0kl4T qyOg6CtgmtpJ6Gizat8augbkUA== X-Google-Smtp-Source: ABhQp+QCAKM6hvAZNxVO7XJAH9jB8nRU5zVXQq+83TGwDYUbt1YFOC1m/ileAbWU+KpfnTG+CsHkzA== X-Received: by 10.28.208.129 with SMTP id h123mr175078wmg.25.1508142484485; Mon, 16 Oct 2017 01:28:04 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id o24sm9374305wmi.39.2017.10.16.01.28.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Oct 2017 01:28:03 -0700 (PDT) Date: Mon, 16 Oct 2017 10:27:50 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: dev@dpdk.org Message-ID: <20171016082750.GC3408@bidouze.vm.6wind.com> References: <1508139710-12798-1-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1508139710-12798-1-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] net/failsafe: improve stats accuracy X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Oct 2017 08:28:05 -0000 Hey Matan, Thanks for the patch. Here are some remarks: On Mon, Oct 16, 2017 at 07:41:50AM +0000, Matan Azrad wrote: > The stats_get API was changed to return error in case of failure at > stats getting process time. > By this way, failsafe can get stats snapshot in removal process for > each PMD which can give stats after removal event. > > This patch implements ultimate stats snapshot on removal time by > trying to get the removed device stats before calling to dev_close. > I would reformulate the commit log as such: The stats_get API has changed to signal a potential failure to read stats. Furthermore, some PMDs are able to provide statistics even after a removal event occured. Considering this, the fail-safe can try to access the latest statistics of a PMD to improve statistics readings. Attempt an ultimate statistics read on removal time; if that fails, use the latest recorded snapshot. > Signed-off-by: Matan Azrad > --- > drivers/net/failsafe/failsafe_ether.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c > index f4db423..2758d4c 100644 > --- a/drivers/net/failsafe/failsafe_ether.c > +++ b/drivers/net/failsafe/failsafe_ether.c > @@ -312,8 +312,12 @@ > static void > fs_dev_stats_save(struct sub_device *sdev) > { > + struct rte_eth_stats stats; > + > + /* Get stats now or take it from last snapshot. */ > failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator, > - &sdev->stats_snapshot); > + rte_eth_stats_get(PORT_ID(sdev), &stats) ? > + &sdev->stats_snapshot : &stats); The conditional is awkward within the parameter list here. Something like: struct rte_eth_stats stats; int err; /* Attempt to read current stats. */ err = rte_eth_stats_get(PORT_ID(sdev), &stats); if (err) WARN("Could not access latest statistics from sub-device %d, using latest snapshot", SUB_ID(sdev)); /* Use either current stats or latest snapshot. */ failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator, err ? &sdev->stats_snapshot : &stats); Seems easier to read and allows warning the user of potential inaccuracies. On that matter, it may be interesting to measure the time since the last snapshot and display it as well. > memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats)); > } > > -- > 1.8.3.1 > -- Gaëtan Rivet 6WIND