From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <reshma.pattan@intel.com>
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
by dpdk.org (Postfix) with ESMTP id 4B5C628EE
for <dev@dpdk.org>; Mon, 22 May 2017 18:16:11 +0200 (CEST)
Received: from fmsmga001.fm.intel.com ([10.253.24.23])
by orsmga105.jf.intel.com with ESMTP; 22 May 2017 09:16:10 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.38,377,1491289200"; d="scan'208";a="1151322865"
Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155])
by fmsmga001.fm.intel.com with ESMTP; 22 May 2017 09:16:10 -0700
Received: from irsmsx110.ger.corp.intel.com ([169.254.15.151]) by
IRSMSX102.ger.corp.intel.com ([169.254.2.153]) with mapi id 14.03.0319.002;
Mon, 22 May 2017 17:16:09 +0100
From: "Pattan, Reshma" <reshma.pattan@intel.com>
To: "Jastrzebski, MichalX K" <michalx.k.jastrzebski@intel.com>, "dev@dpdk.org"
<dev@dpdk.org>
CC: "Jain, Deepak K" <deepak.k.jain@intel.com>, "Van Haaren, Harry"
<harry.van.haaren@intel.com>, "Azarewicz, PiotrX T"
<piotrx.t.azarewicz@intel.com>
Thread-Topic: [PATCH 1/3] drivers/net: add support for IF-MIB and
EtherLike-MIB for e1000
Thread-Index: AQHS0whjQv3OoFbORkGfkk/aprB3c6IAcZwQ
Date: Mon, 22 May 2017 16:16:09 +0000
Message-ID: <3AEA2BF9852C6F48A459DA490692831F0113CF11@irsmsx110.ger.corp.intel.com>
References: <20170522143202.22424-1-michalx.k.jastrzebski@intel.com>
<20170522143202.22424-2-michalx.k.jastrzebski@intel.com>
In-Reply-To: <20170522143202.22424-2-michalx.k.jastrzebski@intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 1/3] drivers/net: add support for IF-MIB and
EtherLike-MIB for e1000
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 22 May 2017 16:16:12 -0000
Hi,
-----Original Message-----
From: Jastrzebski, MichalX K=20
Sent: Monday, May 22, 2017 3:32 PM
To: dev@dpdk.org
Cc: Pattan, Reshma <reshma.pattan@intel.com>; Jain, Deepak K <deepak.k.jain=
@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com>; Jastrzebski, M=
ichalX K <michalx.k.jastrzebski@intel.com>; Azarewicz, PiotrX T <piotrx.t.a=
zarewicz@intel.com>
Subject: [PATCH 1/3] drivers/net: add support for IF-MIB and EtherLike-MIB =
for e1000
Can you add the description to the commit message, describing what has been=
done.
General doubt I have , I see all IF-MIB and Ether-Like-MIB attributes are a=
dded as Xstats.
But most of IF-MIB attributes are not stats, instead they are information =
about the interface/port.
So I guess such attributes should be displayed using separate method? Eve=
rything being displayed as xstats might not be correct.
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_eth=
dev.h
index 8352d0a..d242990 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -254,6 +254,62 @@ struct e1000_filter_info {
struct e1000_2tuple_filter_list twotuple_list; };
=20
+#define E1000_MIB_IF_TYPE_ETHERNETCSMACD 6
+
+enum e1000_mib_truth_value {
+ e1000_mib_truth_true =3D 1,
+ e1000_mib_truth_false
+};
1)All enums should be in capital as per coding guidelines. Similarly correc=
t author enums too.
We no need to have enum identifier, just enumerator list should be suffice =
like below. So you can check all enumerations added newly and correct them?
enum
{ =20
enumerator-list =20
} =20
=20
+
+/* IF-MIB statistics */
+struct e1000_if_mib_stats {
+ uint64_t if_number; /* ifNumber */
+ uint64_t if_index; /* ifIndex */
+ uint64_t if_type; /* ifType */
+ uint64_t if_mtu; /* ifMtu */
+ uint64_t if_speed; /* ifSpeed */
+ uint64_t if_phys_address; /* ifPhysAddress */
+ uint64_t if_oper_status; /* ifOperStatus */
+ uint64_t if_last_change; /* ifLastChange */
+ uint64_t if_high_speed; /* ifHighSpeed */
+ uint64_t if_connector_present; /* ifConnectorPresent */
+ uint64_t if_counter_discontinuity_time; /* ifCounterDiscontinuityTime */
+};
+
The comments should be inside /**< Here the comment goes. */
Refer other files to see how it is done.
+#define E1000_DOT3_CF_PAUSE (1 << 0) /* PAUSE command implemented */
+#define E1000_DOT3_CF_MPCP (1 << 1) /* MPCP implemented */
+#define E1000_DOT3_CF_PFC (1 << 2) /* PFC implemented */
The comments should be inside /**< Here the comment goes. */
+ uint64_t dot3_stats_rate_control_ability;
+ /* dot3StatsRateControlAbility */
+ uint64_t dot3_stats_rate_control_status;/* dot3StatsRateControlStatus */
+ uint64_t dot3_control_functions_supported;
+ /* dot3ControlFunctionsSupported */
+};
The comments line should go on top of the variables.
+
/*
* Structure to store private data for each driver instance (for each port=
).
*/
@@ -268,6 +324,9 @@ struct e1000_adapter {
struct rte_timecounter systime_tc;
struct rte_timecounter rx_tstamp_tc;
struct rte_timecounter tx_tstamp_tc;
+ uint64_t sys_up_time_start;
+ uint64_t if_last_change;
+ uint64_t if_counter_discontinuity_time;
};
=20
#define E1000_DEV_PRIVATE(adapter) \
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.=
c index e1702d8..705b591 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
Can you rename igb_if_mib_strings to rte_ igb_if_mib_strings and igb_if_mib=
_strings to rte_ igb_if_mib_strings? Similar to other stats/xstats.
eth_igb_stats_reset(), inside this function we might not need below piece =
of code, as below piece of code you have already added to=20
eth_igb_xstats_reset().
+
+ adapter->if_counter_discontinuity_time =3D
+ rte_rdtsc() - adapter->sys_up_time_start;
}
Thanks,
Reshma