From: Matan Azrad <matan@mellanox.com>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/failsafe: improve stats accuracy
Date: Mon, 16 Oct 2017 08:51:49 +0000 [thread overview]
Message-ID: <DB6PR0502MB30482BF99A6C0802EED0F9D0D24F0@DB6PR0502MB3048.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20171016082750.GC3408@bidouze.vm.6wind.com>
Hi Geatan
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Monday, October 16, 2017 11:28 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/failsafe: improve stats accuracy
>
> 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.
>
Looks ok for me, thanks.
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> > 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.
>
OK, I think it is good idea, I will try to add this here.
> > memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats)); }
> >
> > --
> > 1.8.3.1
> >
>
> --
> Gaëtan Rivet
> 6WIND
next prev parent reply other threads:[~2017-10-16 8:51 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-16 7:41 Matan Azrad
2017-10-16 8:27 ` Gaëtan Rivet
2017-10-16 8:51 ` Matan Azrad [this message]
2017-10-19 14:31 ` [dpdk-dev] [PATCH v2] " Matan Azrad
2017-10-19 15:08 ` Gaëtan Rivet
2017-10-21 20:54 ` [dpdk-dev] [PATCH v3 1/2] " Matan Azrad
2017-10-21 20:54 ` [dpdk-dev] [PATCH v3 2/2] net/failsafe: add timestamp to stats snapshot Matan Azrad
2017-10-23 8:46 ` Gaëtan Rivet
2017-10-23 8:46 ` [dpdk-dev] [PATCH v3 1/2] net/failsafe: improve stats accuracy Gaëtan Rivet
2017-10-23 21:01 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DB6PR0502MB30482BF99A6C0802EED0F9D0D24F0@DB6PR0502MB3048.eurprd05.prod.outlook.com \
--to=matan@mellanox.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).