[dpdk-dev,1/3] drivers/net: add support for IF-MIB and EtherLike-MIB for e1000
Checks
Commit Message
Hi,
-----Original Message-----
From: Jastrzebski, MichalX K
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, MichalX K <michalx.k.jastrzebski@intel.com>; Azarewicz, PiotrX T <piotrx.t.azarewicz@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 added 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? Everything being displayed as xstats might not be correct.
};
#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
eth_igb_xstats_reset().
+
+ adapter->if_counter_discontinuity_time =
+ rte_rdtsc() - adapter->sys_up_time_start;
}
Thanks,
Reshma
@@ -254,6 +254,62 @@ struct e1000_filter_info {
struct e1000_2tuple_filter_list twotuple_list; };
+#define E1000_MIB_IF_TYPE_ETHERNETCSMACD 6
+
+enum e1000_mib_truth_value {
+ e1000_mib_truth_true = 1,
+ e1000_mib_truth_false
+};
1)All enums should be in capital as per coding guidelines. Similarly correct 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
{
enumerator-list
}
+
+/* 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;