From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f169.google.com (mail-wr0-f169.google.com [209.85.128.169]) by dpdk.org (Postfix) with ESMTP id 1F91C374E for ; Thu, 7 Sep 2017 10:49:16 +0200 (CEST) Received: by mail-wr0-f169.google.com with SMTP id 108so17837998wra.5 for ; Thu, 07 Sep 2017 01:49:16 -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=oRUzdKrB/7nEVbZ3pbLmj06rTRL030uvx7g1hFKvC+U=; b=Nu5uFYOHsGmaav8LQONOY75ClesOXKFJ/js4mNrfpsL2mmrCAAKfpnZZUw5ASf9wUd zwxE++4aRBDtD2r1WeXh3CPZuA+frfuMypfe06aKYIgDiewJyRbQIXwVXwR6v1lz0OZO VLCqh27SzbSnFTpzBiuOGzMhbAUsW+MX/r/utMUcJRTwbDKaA4cpml9gIJ7AMrba6KM/ iewvWlYNOdItbDyorwxlw6/3l0PjtMxzWBQ8nrFLLn1/ljKrA7+rpQI7ZD1AxTKsc1Ca t9NHE15z+EU4GMGWDjL2tyBALpA8wqz0N48I57TfsJYjBgdu00/H31WFiPlimPOFM1Hu aeYw== 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=oRUzdKrB/7nEVbZ3pbLmj06rTRL030uvx7g1hFKvC+U=; b=RNoZYetfkkNH0j5kiYL/jR1meIAg/QNPs1uv2XT2u82s/Y1k2m6EEPvdtZHHVQ4hHP se0US9ZXVQOm38QcZ3HrwOtIBr06nMN5ZgGeQSTR2oOjUi9UST0BRwo8ysvo56FBZ6bg Y9YEwxymywaC0OETV7qohzbypO7L4oamlRmZBGl3rfL4JfduGhCQr6crKJZNGDX8ATy1 JJyE+xno60Z3TUqDVrXvsP3BRZO+ZyZXXa/+86qBj/bW50mOkAbcw1M9XiEf7MZgPcJ3 VNmozZ/3fRxs+ttahbab2q2F71zvNLLcaxooaynqcVimHTPLH+7xx7RJ7y+VbdQqTt6i LYdg== X-Gm-Message-State: AHPjjUh+d9fRZMtdfL474lC53ov/fpnRIJLMIVlWjgDgN1+YMaCsw0qa 1N6ArWUQaaMydWuE X-Google-Smtp-Source: ADKCNb6dSOHVEnu7Ii7KauXMbHT4KHgr5yqUlnsYce7G1HUUZjl8CPZFDwuUxKVaIVlZpZpwN2NlJg== X-Received: by 10.223.174.17 with SMTP id x17mr1280340wrc.132.1504774155482; Thu, 07 Sep 2017 01:49:15 -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 w62sm1534587wrc.52.2017.09.07.01.49.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Sep 2017 01:49:14 -0700 (PDT) Date: Thu, 7 Sep 2017 10:49:04 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: dev@dpdk.org Message-ID: <20170907084904.GK21444@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: Thu, 07 Sep 2017 08:49:16 -0000 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. > 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) Please use usual memcpy parameter order: dst first, src second. It would be easier to read afterward, when call to this function are mixed with calls to memcpy / memset. > +{ > + uint8_t i; Use a regular unsigned int instead. > + > + 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); This call should be done prior to the actual removal of the device, within failsafe_dev_remove instead of the interrupt handler itself. (I guess right before the call to fs_dev_remove, once all bursts have been completed). > /* > * 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)); Why not rte_memcpy? > + 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); The naming convention is not respected. functions prefixed by fs_ are static, private to subsystems. Those exposed to other files are prefixed with failsafe_ Also, function names should designate the object operated upon, from the least specific to the most specific, and end with the action: failsafe_stats_increment failsafe_stats_accumulator_increment Moreover, fs_increment_stats_accumulator seems misnamed, I'd prefer failsafe_dev_stats_store The function name should designate the higher order functionality accomplished, not the underlying mean of doing so. The stats are backed-up, kept for later reference upon device removal. It is done by using the accumulator. The function to store those stats should not be tied to the specifics of the implementation for doing so. > 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