From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f172.google.com (mail-wr0-f172.google.com [209.85.128.172]) by dpdk.org (Postfix) with ESMTP id 781F52BAF for ; Tue, 5 Sep 2017 15:22:56 +0200 (CEST) Received: by mail-wr0-f172.google.com with SMTP id 108so8737834wra.5 for ; Tue, 05 Sep 2017 06:22:56 -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=iHabw7/DCrCVeqDQrmd4Lc6hkfldkJduOyz9XOEF/YI=; b=apkNCYemnDbPeb7/8yJCvkR0pFwrlKKm9z+iTm5wOY0H96j1CM8lOkgXaEb/F2AQqY 4F80cCY78Wk9S8iZmy/60+SkkfNbsfR7hbmxroNHelZrxwQxyizvh+lSgE4YqBQfKsYG M6PiWeUKkOtj4Vu7LP5DK2v7c/FPlITi2isvPy5WWwK9k+C2AIsTcgc3fjJNpLPqa3Xp B5RQGLdPwPRvFIe7+nS7ZYH2LpBr6B0SB95CJb8MBY4IDLbBagztzMi3ex7GCAPUnek5 yYyiMU4abgiMNvhAlvHN0QNnN9Rs6pgth+yKeaAqBzHd9IuAgKpoeyV8AJ83fX1DHVrN pPQw== 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=iHabw7/DCrCVeqDQrmd4Lc6hkfldkJduOyz9XOEF/YI=; b=fN/ZKBtrHEIcDA6xbEtfXO5r236zmtaxFOl+S8T7hq5N0uf66EDr/rE61o9rjz5IMf IRcpRe0yfB5m4QfoLdupkXgPgV4fspqjXNVKLLWadtMVb6A0Xzl1bE/TbUW9bae3wigA oZoWZVfplt+hRpEJD2YOtEFD+k5WsAl3lLpbfLUKv6ZP2KyrO8+URlvwi1AMgchwUyQ3 icxLCfLya2OO771NW23YwYuCTBLQqIU2dXYv9GzoVoJ0dLuJ193QX0XxV2DIMJOX1Mjk S6LPfZLwHki68+zNRIBSFc9bPOFVy7bks3VTJAoes+q68c8enTCdZ4ZuJ3gPU8E7ediP Be0w== X-Gm-Message-State: AHPjjUiihjQBL0L8ae1JuRp1jJmq47nNpNdkPqKuh9boN/6mHMeBrStk VrHwVv7N4CmiwkWI X-Google-Smtp-Source: ADKCNb7uZ5q5PLvdzn38OL3GqGX+jEQuUhNh7zBzDj2NjMAYJCaBNhUiR9U+OuxlFiq9eMKyae+Jlw== X-Received: by 10.223.135.66 with SMTP id 2mr2319913wrz.208.1504617775805; Tue, 05 Sep 2017 06:22:55 -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 r65sm583943wmf.38.2017.09.05.06.22.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 05 Sep 2017 06:22:54 -0700 (PDT) Date: Tue, 5 Sep 2017 15:22:44 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: dev@dpdk.org Message-ID: <20170905132244.GF21444@bidouze.vm.6wind.com> References: <1504605394-6450-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: <1504605394-6450-1-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] net/failsafe: stat support enhancement 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: Tue, 05 Sep 2017 13:22:56 -0000 Hi Matan, On Tue, Sep 05, 2017 at 12:56:34PM +0300, Matan Azrad wrote: > The previous stats code returned only the current TX sub > device stats. > > This enhancement extends it to return the sum of all sub > devices stats with history of removed sub-devices. > > Dedicated stats accumulator saves the stat history of all > sub device remove events. > > Each failsafe sub device contains the last stats asked by > the user and updates the accumulator in removal time. > > I would like to implement ultimate snapshot on removal time. > The stats_get API needs to be changed to return error in the > case it is too late to retrieve statistics. You need to have this API evolution first. It is not assured to be accepted by the community. As it is, this version is incorrect, because the only available stats for a removed slave is the last snapshot. Thus it complicates the rules while still being incorrect. Even if you were able to push for this API evolution, PMDs with hard counters would be the ones to return an error code on stat read while removed. You may be lucky at the moment because MLX drivers do not support hard counters, but this won't always be true (and it will be false soon enough). You will thus be back at square one, with a new useless API and still incorrect stats in the fail-safe. On the other hand, the fail-safe should strive to be as easy to use as possible with most drivers, and not cater specifically to soft-counters ones. So, my take on this: I understand that aggregated stats would be interesting. Keep it simple & stupid: simple aggregation on stats_get. You will have a rollback at some point on device removal, but this is still easier to detect / deal with than partially / sometimes incorrect stats. > By this way, failsafe can get stats snapshot in removal interrupt > callback for each PMD which can give stats after removal event. > > Signed-off-by: Matan Azrad > --- > drivers/net/failsafe/failsafe_ether.c | 33 +++++++++++++++++++++++++++++++++ > drivers/net/failsafe/failsafe_ops.c | 16 ++++++++++++---- > drivers/net/failsafe/failsafe_private.h | 5 +++++ > 3 files changed, 50 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c > index a3a8cce..133080d 100644 > --- a/drivers/net/failsafe/failsafe_ether.c > +++ b/drivers/net/failsafe/failsafe_ether.c > @@ -399,6 +399,37 @@ failsafe_eth_dev_state_sync(struct rte_eth_dev *dev) > return ret; > } > > +void > +fs_increment_stats(struct rte_eth_stats *from, struct rte_eth_stats *to) > +{ > + uint8_t i; > + > + RTE_ASSERT(from != NULL && to != NULL); > + to->ipackets += from->ipackets; > + to->opackets += from->opackets; > + to->ibytes += from->ibytes; > + to->obytes += from->obytes; > + to->imissed += from->imissed; > + to->ierrors += from->ierrors; > + to->oerrors += from->oerrors; > + to->rx_nombuf += from->rx_nombuf; > + for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) { > + to->q_ipackets[i] += from->q_ipackets[i]; > + to->q_opackets[i] += from->q_opackets[i]; > + to->q_ibytes[i] += from->q_ibytes[i]; > + to->q_obytes[i] += from->q_obytes[i]; > + to->q_errors[i] += from->q_errors[i]; > + } > +} > + > +void > +fs_increment_stats_accumulator(struct sub_device *sdev) > +{ > + fs_increment_stats(&sdev->stats_snapshot, > + &PRIV(sdev->fs_dev)->stats_accumulator); > + memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats)); > +} > + > int > failsafe_eth_rmv_event_callback(uint8_t port_id __rte_unused, > enum rte_eth_event_type event __rte_unused, > @@ -410,6 +441,8 @@ failsafe_eth_rmv_event_callback(uint8_t port_id __rte_unused, > fs_switch_dev(sdev->fs_dev, sdev); > /* Use safe bursts in any case. */ > set_burst_fn(sdev->fs_dev, 1); > + /* Increment the stats accumulator by the last stats snapshot. */ > + fs_increment_stats_accumulator(sdev); > /* > * Async removal, the sub-PMD will try to unregister > * the callback at the source of the current thread context. > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c > index ff9ad15..e47cc85 100644 > --- a/drivers/net/failsafe/failsafe_ops.c > +++ b/drivers/net/failsafe/failsafe_ops.c > @@ -586,9 +586,14 @@ static void > fs_stats_get(struct rte_eth_dev *dev, > struct rte_eth_stats *stats) > { > - if (TX_SUBDEV(dev) == NULL) > - return; > - rte_eth_stats_get(PORT_ID(TX_SUBDEV(dev)), stats); > + struct sub_device *sdev; > + uint8_t i; > + > + memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats)); > + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > + rte_eth_stats_get(PORT_ID(sdev), &sdev->stats_snapshot); > + fs_increment_stats(&sdev->stats_snapshot, stats); > + } > } > > static void > @@ -597,8 +602,11 @@ fs_stats_reset(struct rte_eth_dev *dev) > struct sub_device *sdev; > uint8_t i; > > - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) > + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > rte_eth_stats_reset(PORT_ID(sdev)); > + memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats)); > + } > + memset(&PRIV(dev)->stats_accumulator, 0, sizeof(struct rte_eth_stats)); > } > > /** > diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h > index 0361cf4..267c749 100644 > --- a/drivers/net/failsafe/failsafe_private.h > +++ b/drivers/net/failsafe/failsafe_private.h > @@ -102,6 +102,8 @@ struct sub_device { > uint8_t sid; > /* Device state machine */ > enum dev_state state; > + /* Last stats snapshot passed to user */ > + struct rte_eth_stats stats_snapshot; > /* Some device are defined as a command line */ > char *cmdline; > /* fail-safe device backreference */ > @@ -140,6 +142,7 @@ struct fs_priv { > * synchronized state. > */ > enum dev_state state; > + struct rte_eth_stats stats_accumulator; > unsigned int pending_alarm:1; /* An alarm is pending */ > /* flow isolation state */ > int flow_isolated:1; > @@ -180,6 +183,8 @@ int failsafe_eal_uninit(struct rte_eth_dev *dev); > > int failsafe_eth_dev_state_sync(struct rte_eth_dev *dev); > void failsafe_dev_remove(struct rte_eth_dev *dev); > +void fs_increment_stats(struct rte_eth_stats *from, struct rte_eth_stats *to); > +void fs_increment_stats_accumulator(struct sub_device *sdev); > int failsafe_eth_rmv_event_callback(uint8_t port_id, > enum rte_eth_event_type type, > void *arg, void *out); > -- > 2.7.4 > -- Gaëtan Rivet 6WIND