DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
@ 2016-04-26 17:39 Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 1/4] pmd: Modify PMD_REGISTER_DRIVER to emit a marker symbol Neil Horman
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Neil Horman @ 2016-04-26 17:39 UTC (permalink / raw)
  To: dev
  Cc: David Marchand, Stephen Hemminger, bruce.richardson,
	Panu Matilainen, Thomas Monjalon

Hey-
	So a few days ago we were reviewing Davids patch series to introduce the
abiilty to dump hardware support from pmd DSO's in a human readable format.
That effort encountered some problems, most notably the fact that stripping a
DSO removed the required information that the proposed tool keyed off, as well
as the need to dead reckon offsets between symbols that may not be constant
(dependent on architecture).

	I was going to start looking into the possibility of creating a modinfo
section in a simmilar fashion to what kernel modules do in linux or BSD.  I
decided to propose this solution instead though, because the kernel style
solution requires a significant amount of infrastructure that I think we can
maybe avoid maintaining, if we accept some minor caviats

To do this We emit a set of well known marker symbols for each DSO that an
external application can search for (in this case I called them
this_pmd_driver<n>, where n is a counter macro).  These marker symbols are
n is a counter macro).  These marker symbols are exported by PMDs for
external access.  External tools can then access these symbols via the
dlopen/dlsym api (or via elfutils libraries)

The symbols above alias the rte_driver struct for each PMD, and the external
application can then interrogate the registered driver information.  

I also add a pointer to the pci id table struct for each PMD so that we can
export hardware support.

This approach has a few pros and cons:

pros:
1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
don't need a new tool to extract driver information and emit the C code to build
the binary data for the special section, nor do we need a custom linker script
to link said special section in place

2) Its stable.  Because the marker symbols are explicitly exported, this
approach is resilient against stripping.

cons:
1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
compilation unit per DSO.  As an example em and igb effectively merge two
drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
C files for the same single linked DSO.  Because of the use of the __COUNTER__
macro we get multiple definitions of the same marker symbols.  

I would make the argument that the downside of the above artifact isn't that big
a deal.  Traditionally in other projects a unit like a module (or DSO in our
case) only ever codifies a single driver (e.g. module_init() in the linux kernel
is only ever used once per built module).  If we have code like igb/em that
shares some core code, we should build the shared code to object files and link
them twice, once to an em.so pmd and again to an igb.so pmd.

But regardless, I thought I would propose this to see what you all thought of
it.

FWIW, heres sample output of the pmdinfo tool from this series probing the
librte_pmd_ena.so module:

[nhorman@hmsreliant dpdk]$ ./build/app/pmdinfo
~/git/dpdk/build/lib/librte_pmd_ena.so
PMD 0 Information:
Driver Name: ena_driver
Driver Type: PCI
|====================PCI Table========================|
| VENDOR ID | DEVICE ID | SUBVENDOR ID | SUBDEVICE ID |
|-----------------------------------------------------|
|       1d0f|       ec20|          ffff|          ffff|
|       1d0f|       ec21|          ffff|          ffff|
|-----------------------------------------------------|

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [RFC PATCH 1/4] pmd: Modify PMD_REGISTER_DRIVER to emit a marker symbol
  2016-04-26 17:39 [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
@ 2016-04-26 17:39 ` Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 2/4] pmds: export this_pmd_driver* symbols Neil Horman
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2016-04-26 17:39 UTC (permalink / raw)
  To: dev
  Cc: David Marchand, Stephen Hemminger, bruce.richardson,
	Panu Matilainen, Thomas Monjalon, Neil Horman

modify PMD_REGISTER_DRIVER so that, when building as a DSO, PMD's emit an
additional set of symbols named this_pmd_driver<n>, where <n> is an incrementing
counter.  This gives well known symbol names that external apps can search for
when looking up PMD information.  These new symbols are aliased to the passed in
rte_driver_struct, which future apps can use to interrogate the PMD's for useful
information

Also modify the rte_driver struct to add a union that can hold pmd type specific
information.  Currently, only PMD_PDEV uses this to store a pointer to the PMD's
pci id table.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: David Marchand <david.marchand@6wind.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: "Richardson, Bruce" <bruce.richardson@intel.com>
CC: Panu Matilainen <pmatilai@redhat.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 lib/librte_eal/common/include/rte_dev.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index f1b5507..a81d901 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -50,6 +50,9 @@ extern "C" {
 #include <sys/queue.h>
 
 #include <rte_log.h>
+#include <rte_config.h>
+#include <rte_common.h>
+#include <rte_pci.h>
 
 __attribute__((format(printf, 2, 0)))
 static inline void
@@ -131,6 +134,9 @@ struct rte_driver {
 	const char *name;                   /**< Driver name. */
 	rte_dev_init_t *init;              /**< Device init. function. */
 	rte_dev_uninit_t *uninit;          /**< Device uninit. function. */
+	union {
+		const struct rte_pci_id *pci_table;
+	};
 };
 
 /**
@@ -178,12 +184,27 @@ int rte_eal_vdev_init(const char *name, const char *args);
  */
 int rte_eal_vdev_uninit(const char *name);
 
+#ifdef RTE_BUILD_SHARED_LIB 
+#define DRIVER_EXPORT_NAME(name, idx) name##idx
+#define DECLARE_DRIVER_EXPORT(src, idx)\
+extern struct rte_driver DRIVER_EXPORT_NAME(this_pmd_driver, idx)\
+ __attribute__((alias(RTE_STR(src))))
+
+#define PMD_REGISTER_DRIVER(d)\
+void devinitfn_ ##d(void);\
+void __attribute__((constructor, used)) devinitfn_ ##d(void)\
+{\
+	rte_eal_driver_register(&d);\
+}\
+DECLARE_DRIVER_EXPORT(d, __COUNTER__)
+#else
 #define PMD_REGISTER_DRIVER(d)\
 void devinitfn_ ##d(void);\
 void __attribute__((constructor, used)) devinitfn_ ##d(void)\
 {\
 	rte_eal_driver_register(&d);\
 }
+#endif
 
 #ifdef __cplusplus
 }
-- 
2.5.5

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [RFC PATCH 2/4] pmds: export this_pmd_driver* symbols
  2016-04-26 17:39 [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 1/4] pmd: Modify PMD_REGISTER_DRIVER to emit a marker symbol Neil Horman
@ 2016-04-26 17:39 ` Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 3/4] pmd: Modify drivers to export appropriate information Neil Horman
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2016-04-26 17:39 UTC (permalink / raw)
  To: dev
  Cc: David Marchand, Stephen Hemminger, bruce.richardson,
	Panu Matilainen, Thomas Monjalon, Neil Horman

Because the DPDK DSO's are opt-in for symbol export, we need to add the symbols
that the modified PMD_REGISTER_DRIVER macro creates so that external
applications can see them

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: David Marchand <david.marchand@6wind.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: "Richardson, Bruce" <bruce.richardson@intel.com>
CC: Panu Matilainen <pmatilai@redhat.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 drivers/crypto/aesni_gcm/rte_pmd_aesni_gcm_version.map | 1 +
 drivers/crypto/aesni_mb/rte_pmd_aesni_version.map      | 1 +
 drivers/crypto/null/rte_pmd_null_crypto_version.map    | 1 +
 drivers/crypto/qat/rte_pmd_qat_version.map             | 3 ++-
 drivers/crypto/snow3g/rte_pmd_snow3g_version.map       | 1 +
 drivers/net/af_packet/rte_pmd_af_packet_version.map    | 2 +-
 drivers/net/bnx2x/rte_pmd_bnx2x_version.map            | 1 +
 drivers/net/bonding/rte_eth_bond_version.map           | 1 +
 drivers/net/cxgbe/rte_pmd_cxgbe_version.map            | 2 +-
 drivers/net/e1000/rte_pmd_e1000_version.map            | 2 +-
 drivers/net/ena/rte_pmd_ena_version.map                | 1 +
 drivers/net/enic/rte_pmd_enic_version.map              | 1 +
 drivers/net/fm10k/rte_pmd_fm10k_version.map            | 1 +
 drivers/net/i40e/rte_pmd_i40e_version.map              | 1 +
 drivers/net/ixgbe/rte_pmd_ixgbe_version.map            | 2 +-
 drivers/net/mlx4/rte_pmd_mlx4_version.map              | 1 +
 drivers/net/mlx5/rte_pmd_mlx5_version.map              | 1 +
 drivers/net/mpipe/rte_pmd_mpipe_version.map            | 1 +
 drivers/net/nfp/rte_pmd_nfp_version.map                | 1 +
 drivers/net/null/rte_pmd_null_version.map              | 2 +-
 drivers/net/pcap/rte_pmd_pcap_version.map              | 2 +-
 drivers/net/szedata2/rte_pmd_szedata2_version.map      | 1 +
 drivers/net/vhost/rte_pmd_vhost_version.map            | 1 +
 drivers/net/virtio/rte_pmd_virtio_version.map          | 2 +-
 drivers/net/vmxnet3/rte_pmd_vmxnet3_version.map        | 2 +-
 25 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/aesni_gcm/rte_pmd_aesni_gcm_version.map b/drivers/crypto/aesni_gcm/rte_pmd_aesni_gcm_version.map
index dc4d417..62341f9 100644
--- a/drivers/crypto/aesni_gcm/rte_pmd_aesni_gcm_version.map
+++ b/drivers/crypto/aesni_gcm/rte_pmd_aesni_gcm_version.map
@@ -1,3 +1,4 @@
 DPDK_16.04 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/crypto/aesni_mb/rte_pmd_aesni_version.map b/drivers/crypto/aesni_mb/rte_pmd_aesni_version.map
index ad607bb..6f727b0 100644
--- a/drivers/crypto/aesni_mb/rte_pmd_aesni_version.map
+++ b/drivers/crypto/aesni_mb/rte_pmd_aesni_version.map
@@ -1,3 +1,4 @@
 DPDK_2.2 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/crypto/null/rte_pmd_null_crypto_version.map b/drivers/crypto/null/rte_pmd_null_crypto_version.map
index dc4d417..62341f9 100644
--- a/drivers/crypto/null/rte_pmd_null_crypto_version.map
+++ b/drivers/crypto/null/rte_pmd_null_crypto_version.map
@@ -1,3 +1,4 @@
 DPDK_16.04 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/crypto/qat/rte_pmd_qat_version.map b/drivers/crypto/qat/rte_pmd_qat_version.map
index bbaf1c8..6f727b0 100644
--- a/drivers/crypto/qat/rte_pmd_qat_version.map
+++ b/drivers/crypto/qat/rte_pmd_qat_version.map
@@ -1,3 +1,4 @@
 DPDK_2.2 {
+	global: this_pmd_driver*;
 	local: *;
-};
\ No newline at end of file
+};
diff --git a/drivers/crypto/snow3g/rte_pmd_snow3g_version.map b/drivers/crypto/snow3g/rte_pmd_snow3g_version.map
index dc4d417..62341f9 100644
--- a/drivers/crypto/snow3g/rte_pmd_snow3g_version.map
+++ b/drivers/crypto/snow3g/rte_pmd_snow3g_version.map
@@ -1,3 +1,4 @@
 DPDK_16.04 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/af_packet/rte_pmd_af_packet_version.map b/drivers/net/af_packet/rte_pmd_af_packet_version.map
index ef35398..55e2bb1 100644
--- a/drivers/net/af_packet/rte_pmd_af_packet_version.map
+++ b/drivers/net/af_packet/rte_pmd_af_packet_version.map
@@ -1,4 +1,4 @@
 DPDK_2.0 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/bnx2x/rte_pmd_bnx2x_version.map b/drivers/net/bnx2x/rte_pmd_bnx2x_version.map
index bd8138a..0fccfa3 100644
--- a/drivers/net/bnx2x/rte_pmd_bnx2x_version.map
+++ b/drivers/net/bnx2x/rte_pmd_bnx2x_version.map
@@ -1,4 +1,5 @@
 DPDK_2.1 {
+	global: this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
index 22bd920..1071960 100644
--- a/drivers/net/bonding/rte_eth_bond_version.map
+++ b/drivers/net/bonding/rte_eth_bond_version.map
@@ -17,6 +17,7 @@ DPDK_2.0 {
 	rte_eth_bond_slaves_get;
 	rte_eth_bond_xmit_policy_get;
 	rte_eth_bond_xmit_policy_set;
+	this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/cxgbe/rte_pmd_cxgbe_version.map b/drivers/net/cxgbe/rte_pmd_cxgbe_version.map
index bd8138a..6d92937 100644
--- a/drivers/net/cxgbe/rte_pmd_cxgbe_version.map
+++ b/drivers/net/cxgbe/rte_pmd_cxgbe_version.map
@@ -1,4 +1,4 @@
 DPDK_2.1 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/e1000/rte_pmd_e1000_version.map b/drivers/net/e1000/rte_pmd_e1000_version.map
index ef35398..55e2bb1 100644
--- a/drivers/net/e1000/rte_pmd_e1000_version.map
+++ b/drivers/net/e1000/rte_pmd_e1000_version.map
@@ -1,4 +1,4 @@
 DPDK_2.0 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/ena/rte_pmd_ena_version.map b/drivers/net/ena/rte_pmd_ena_version.map
index 349c6e1..1151a0a 100644
--- a/drivers/net/ena/rte_pmd_ena_version.map
+++ b/drivers/net/ena/rte_pmd_ena_version.map
@@ -1,4 +1,5 @@
 DPDK_16.04 {
+	global: this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/enic/rte_pmd_enic_version.map b/drivers/net/enic/rte_pmd_enic_version.map
index ef35398..c86becc 100644
--- a/drivers/net/enic/rte_pmd_enic_version.map
+++ b/drivers/net/enic/rte_pmd_enic_version.map
@@ -1,4 +1,5 @@
 DPDK_2.0 {
+	global: this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/fm10k/rte_pmd_fm10k_version.map b/drivers/net/fm10k/rte_pmd_fm10k_version.map
index ef35398..c86becc 100644
--- a/drivers/net/fm10k/rte_pmd_fm10k_version.map
+++ b/drivers/net/fm10k/rte_pmd_fm10k_version.map
@@ -1,4 +1,5 @@
 DPDK_2.0 {
+	global: this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/i40e/rte_pmd_i40e_version.map b/drivers/net/i40e/rte_pmd_i40e_version.map
index ef35398..c86becc 100644
--- a/drivers/net/i40e/rte_pmd_i40e_version.map
+++ b/drivers/net/i40e/rte_pmd_i40e_version.map
@@ -1,4 +1,5 @@
 DPDK_2.0 {
+	global: this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe_version.map b/drivers/net/ixgbe/rte_pmd_ixgbe_version.map
index ef35398..55e2bb1 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe_version.map
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe_version.map
@@ -1,4 +1,4 @@
 DPDK_2.0 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/mlx4/rte_pmd_mlx4_version.map b/drivers/net/mlx4/rte_pmd_mlx4_version.map
index ef35398..c86becc 100644
--- a/drivers/net/mlx4/rte_pmd_mlx4_version.map
+++ b/drivers/net/mlx4/rte_pmd_mlx4_version.map
@@ -1,4 +1,5 @@
 DPDK_2.0 {
+	global: this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/mlx5/rte_pmd_mlx5_version.map b/drivers/net/mlx5/rte_pmd_mlx5_version.map
index ad607bb..6f727b0 100644
--- a/drivers/net/mlx5/rte_pmd_mlx5_version.map
+++ b/drivers/net/mlx5/rte_pmd_mlx5_version.map
@@ -1,3 +1,4 @@
 DPDK_2.2 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/mpipe/rte_pmd_mpipe_version.map b/drivers/net/mpipe/rte_pmd_mpipe_version.map
index ad607bb..6f727b0 100644
--- a/drivers/net/mpipe/rte_pmd_mpipe_version.map
+++ b/drivers/net/mpipe/rte_pmd_mpipe_version.map
@@ -1,3 +1,4 @@
 DPDK_2.2 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/nfp/rte_pmd_nfp_version.map b/drivers/net/nfp/rte_pmd_nfp_version.map
index ad607bb..6f727b0 100644
--- a/drivers/net/nfp/rte_pmd_nfp_version.map
+++ b/drivers/net/nfp/rte_pmd_nfp_version.map
@@ -1,3 +1,4 @@
 DPDK_2.2 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/null/rte_pmd_null_version.map b/drivers/net/null/rte_pmd_null_version.map
index 84b1d0f..15488cf 100644
--- a/drivers/net/null/rte_pmd_null_version.map
+++ b/drivers/net/null/rte_pmd_null_version.map
@@ -4,7 +4,7 @@ DPDK_2.0 {
 };
 
 DPDK_2.2 {
-	global:
+	global: this_pmd_driver*;
 
 	eth_dev_null_create;
 
diff --git a/drivers/net/pcap/rte_pmd_pcap_version.map b/drivers/net/pcap/rte_pmd_pcap_version.map
index ef35398..55e2bb1 100644
--- a/drivers/net/pcap/rte_pmd_pcap_version.map
+++ b/drivers/net/pcap/rte_pmd_pcap_version.map
@@ -1,4 +1,4 @@
 DPDK_2.0 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/szedata2/rte_pmd_szedata2_version.map b/drivers/net/szedata2/rte_pmd_szedata2_version.map
index ad607bb..6f727b0 100644
--- a/drivers/net/szedata2/rte_pmd_szedata2_version.map
+++ b/drivers/net/szedata2/rte_pmd_szedata2_version.map
@@ -1,3 +1,4 @@
 DPDK_2.2 {
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/vhost/rte_pmd_vhost_version.map b/drivers/net/vhost/rte_pmd_vhost_version.map
index 65bf3a8..16c142a 100644
--- a/drivers/net/vhost/rte_pmd_vhost_version.map
+++ b/drivers/net/vhost/rte_pmd_vhost_version.map
@@ -5,6 +5,7 @@ DPDK_16.04 {
 	rte_eth_vhost_feature_enable;
 	rte_eth_vhost_feature_get;
 	rte_eth_vhost_get_queue_event;
+	this_pmd_driver*;
 
 	local: *;
 };
diff --git a/drivers/net/virtio/rte_pmd_virtio_version.map b/drivers/net/virtio/rte_pmd_virtio_version.map
index ef35398..55e2bb1 100644
--- a/drivers/net/virtio/rte_pmd_virtio_version.map
+++ b/drivers/net/virtio/rte_pmd_virtio_version.map
@@ -1,4 +1,4 @@
 DPDK_2.0 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
diff --git a/drivers/net/vmxnet3/rte_pmd_vmxnet3_version.map b/drivers/net/vmxnet3/rte_pmd_vmxnet3_version.map
index ef35398..55e2bb1 100644
--- a/drivers/net/vmxnet3/rte_pmd_vmxnet3_version.map
+++ b/drivers/net/vmxnet3/rte_pmd_vmxnet3_version.map
@@ -1,4 +1,4 @@
 DPDK_2.0 {
-
+	global: this_pmd_driver*;
 	local: *;
 };
-- 
2.5.5

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [RFC PATCH 3/4] pmd: Modify drivers to export appropriate information
  2016-04-26 17:39 [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 1/4] pmd: Modify PMD_REGISTER_DRIVER to emit a marker symbol Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 2/4] pmds: export this_pmd_driver* symbols Neil Horman
@ 2016-04-26 17:39 ` Neil Horman
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 4/4] pmdinfo: Add application to extract pmd driver info Neil Horman
  2016-05-03 11:57 ` [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
  4 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2016-04-26 17:39 UTC (permalink / raw)
  To: dev
  Cc: David Marchand, Stephen Hemminger, bruce.richardson,
	Panu Matilainen, Thomas Monjalon, Neil Horman

For the PMD's which support pci devices, add the appropriate pci table pointer
to the rte_driver structure so external applications can find it.

Also note, some modifications to the em/igb and i40e drivers.  These are done to
support an artifact of the use of the __COUNTER__ macro in PMD_REGISTER_DRIVER.
The __COUNTER__ macro evaluates to a string integer that gets incremented each
time it expands.  While that offers a great predictable indexing feature, it
resets for each compilation unit (as would be expected).  However, for the two
aforementioned DSO's they register multiple drivers in multiple compilation
units, which leads to multiple definitions of variables.  I would make the
argument that a single DSO should support only a single driver (which is
analgous to how kernel modules work in linux and bsd), and if there is common
code between multiple modules, that common code should be built into a archive
and linked to each separate DSO.  However, that is a significant amount of work,
and so here, I instead modified the affected DSO's to simply register all
drivers in the same compilation unit.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: David Marchand <david.marchand@6wind.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: "Richardson, Bruce" <bruce.richardson@intel.com>
CC: Panu Matilainen <pmatilai@redhat.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 drivers/crypto/qat/rte_qat_cryptodev.c  |  1 +
 drivers/net/bnx2x/bnx2x_ethdev.c        |  2 ++
 drivers/net/cxgbe/cxgbe_ethdev.c        |  1 +
 drivers/net/e1000/Makefile              |  1 +
 drivers/net/e1000/em_ethdev.c           | 13 +++------
 drivers/net/e1000/igb_ethdev.c          | 24 ++++++-----------
 drivers/net/e1000/pmds.c                | 48 +++++++++++++++++++++++++++++++++
 drivers/net/ena/ena_ethdev.c            |  1 +
 drivers/net/enic/enic_ethdev.c          |  1 +
 drivers/net/fm10k/fm10k_ethdev.c        |  1 +
 drivers/net/i40e/i40e_ethdev.c          | 14 ++++++++++
 drivers/net/i40e/i40e_ethdev_vf.c       | 14 ++++------
 drivers/net/ixgbe/ixgbe_ethdev.c        |  2 ++
 drivers/net/mlx4/mlx4.c                 |  1 +
 drivers/net/mlx5/mlx5.c                 |  1 +
 drivers/net/nfp/nfp_net.c               |  1 +
 drivers/net/szedata2/rte_eth_szedata2.c |  1 +
 drivers/net/virtio/virtio_ethdev.c      |  1 +
 drivers/net/vmxnet3/vmxnet3_ethdev.c    |  1 +
 19 files changed, 95 insertions(+), 34 deletions(-)
 create mode 100644 drivers/net/e1000/pmds.c

diff --git a/drivers/crypto/qat/rte_qat_cryptodev.c b/drivers/crypto/qat/rte_qat_cryptodev.c
index a7912f5..20426e8 100644
--- a/drivers/crypto/qat/rte_qat_cryptodev.c
+++ b/drivers/crypto/qat/rte_qat_cryptodev.c
@@ -135,6 +135,7 @@ rte_qat_pmd_init(const char *name __rte_unused, const char *params __rte_unused)
 static struct rte_driver pmd_qat_drv = {
 	.type = PMD_PDEV,
 	.init = rte_qat_pmd_init,
+	.pci_table = pci_id_qat_map,
 };
 
 PMD_REGISTER_DRIVER(pmd_qat_drv);
diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c b/drivers/net/bnx2x/bnx2x_ethdev.c
index 071b44f..ba7d009 100644
--- a/drivers/net/bnx2x/bnx2x_ethdev.c
+++ b/drivers/net/bnx2x/bnx2x_ethdev.c
@@ -543,11 +543,13 @@ static int rte_bnx2xvf_pmd_init(const char *name __rte_unused, const char *param
 static struct rte_driver rte_bnx2x_driver = {
 	.type = PMD_PDEV,
 	.init = rte_bnx2x_pmd_init,
+	.pci_table = pci_id_bnx2x_map,
 };
 
 static struct rte_driver rte_bnx2xvf_driver = {
 	.type = PMD_PDEV,
 	.init = rte_bnx2xvf_pmd_init,
+	.pci_table = pci_id_bnx2xvf_map,
 };
 
 PMD_REGISTER_DRIVER(rte_bnx2x_driver);
diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 04eddaf..c047096 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -892,6 +892,7 @@ static struct rte_driver rte_cxgbe_driver = {
 	.name = "cxgbe_driver",
 	.type = PMD_PDEV,
 	.init = rte_cxgbe_pmd_init,
+	.pci_table = cxgb4_pci_tbl,
 };
 
 PMD_REGISTER_DRIVER(rte_cxgbe_driver);
diff --git a/drivers/net/e1000/Makefile b/drivers/net/e1000/Makefile
index f4879e6..058faff 100644
--- a/drivers/net/e1000/Makefile
+++ b/drivers/net/e1000/Makefile
@@ -93,6 +93,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_IGB_PMD) += igb_rxtx.c
 SRCS-$(CONFIG_RTE_LIBRTE_IGB_PMD) += igb_pf.c
 SRCS-$(CONFIG_RTE_LIBRTE_EM_PMD) += em_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_EM_PMD) += em_rxtx.c
+SRCS-y += pmds.c
 
 # this lib depends upon:
 DEPDIRS-$(CONFIG_RTE_LIBRTE_E1000_PMD) += lib/librte_eal lib/librte_ether
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 653be09..62bd811 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -60,7 +60,8 @@
 
 #define PMD_ROUNDUP(x,y)	(((x) + (y) - 1)/(y) * (y))
 
-
+int
+rte_em_pmd_init(const char *name __rte_unused, const char *params __rte_unused);
 static int eth_em_configure(struct rte_eth_dev *dev);
 static int eth_em_start(struct rte_eth_dev *dev);
 static void eth_em_stop(struct rte_eth_dev *dev);
@@ -136,7 +137,7 @@ static enum e1000_fc_mode em_fc_setting = e1000_fc_full;
 /*
  * The set of PCI devices this driver supports
  */
-static const struct rte_pci_id pci_id_em_map[] = {
+const struct rte_pci_id pci_id_em_map[] = {
 
 #define RTE_PCI_DEV_ID_DECL_EM(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -374,7 +375,7 @@ static struct eth_driver rte_em_pmd = {
 	.dev_private_size = sizeof(struct e1000_adapter),
 };
 
-static int
+int
 rte_em_pmd_init(const char *name __rte_unused, const char *params __rte_unused)
 {
 	rte_eth_driver_register(&rte_em_pmd);
@@ -1772,9 +1773,3 @@ eth_em_set_mc_addr_list(struct rte_eth_dev *dev,
 	return 0;
 }
 
-struct rte_driver em_pmd_drv = {
-	.type = PMD_PDEV,
-	.init = rte_em_pmd_init,
-};
-
-PMD_REGISTER_DRIVER(em_pmd_drv);
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index f0921ee..047e592 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -86,6 +86,10 @@
 #define E1000_INCVALUE_82576         (16 << IGB_82576_TSYNC_SHIFT)
 #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000
 
+int
+rte_igb_pmd_init(const char *name __rte_unused, const char *params __rte_unused);
+int
+rte_igbvf_pmd_init(const char *name __rte_unused, const char *params __rte_unused);
 static int  eth_igb_configure(struct rte_eth_dev *dev);
 static int  eth_igb_start(struct rte_eth_dev *dev);
 static void eth_igb_stop(struct rte_eth_dev *dev);
@@ -281,7 +285,7 @@ static enum e1000_fc_mode igb_fc_setting = e1000_fc_full;
 /*
  * The set of PCI devices this driver supports
  */
-static const struct rte_pci_id pci_id_igb_map[] = {
+const struct rte_pci_id pci_id_igb_map[] = {
 
 #define RTE_PCI_DEV_ID_DECL_IGB(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -292,7 +296,7 @@ static const struct rte_pci_id pci_id_igb_map[] = {
 /*
  * The set of PCI devices this driver supports (for 82576&I350 VF)
  */
-static const struct rte_pci_id pci_id_igbvf_map[] = {
+const struct rte_pci_id pci_id_igbvf_map[] = {
 
 #define RTE_PCI_DEV_ID_DECL_IGBVF(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
@@ -995,7 +999,7 @@ static struct eth_driver rte_igbvf_pmd = {
 	.dev_private_size = sizeof(struct e1000_adapter),
 };
 
-static int
+int
 rte_igb_pmd_init(const char *name __rte_unused, const char *params __rte_unused)
 {
 	rte_eth_driver_register(&rte_igb_pmd);
@@ -1018,7 +1022,7 @@ igb_vmdq_vlan_hw_filter_enable(struct rte_eth_dev *dev)
  * Invoked one at EAL init time.
  * Register itself as the [Virtual Poll Mode] Driver of PCI IGB devices.
  */
-static int
+int
 rte_igbvf_pmd_init(const char *name __rte_unused, const char *params __rte_unused)
 {
 	PMD_INIT_FUNC_TRACE();
@@ -4802,16 +4806,6 @@ eth_igb_set_eeprom(struct rte_eth_dev *dev,
 	return nvm->ops.write(hw,  first, length, data);
 }
 
-static struct rte_driver pmd_igb_drv = {
-	.type = PMD_PDEV,
-	.init = rte_igb_pmd_init,
-};
-
-static struct rte_driver pmd_igbvf_drv = {
-	.type = PMD_PDEV,
-	.init = rte_igbvf_pmd_init,
-};
-
 static int
 eth_igb_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id)
 {
@@ -4973,5 +4967,3 @@ eth_igb_configure_msix_intr(struct rte_eth_dev *dev)
 	E1000_WRITE_FLUSH(hw);
 }
 
-PMD_REGISTER_DRIVER(pmd_igb_drv);
-PMD_REGISTER_DRIVER(pmd_igbvf_drv);
diff --git a/drivers/net/e1000/pmds.c b/drivers/net/e1000/pmds.c
new file mode 100644
index 0000000..e4645d0
--- /dev/null
+++ b/drivers/net/e1000/pmds.c
@@ -0,0 +1,48 @@
+#include <rte_dev.h>
+#include <rte_config.h>
+
+#if RTE_LIBRTE_IGB_PMD
+extern int
+rte_igb_pmd_init(const char *name __rte_unused, const char *params __rte_unused);
+extern int
+rte_igbvf_pmd_init(const char *name __rte_unused, const char *params __rte_unused);
+
+extern const struct rte_pci_id pci_id_igb_map[];
+extern const struct rte_pci_id pci_id_igbvf_map[];
+
+struct rte_driver __attribute__((used)) pmd_igb_drv = {
+	.type = PMD_PDEV,
+	.name = "igb_pmd",
+	.init = rte_igb_pmd_init,
+	.pci_table = pci_id_igb_map,
+};
+
+struct rte_driver __attribute__((used)) pmd_igbvf_drv = {
+	.type = PMD_PDEV,
+	.name = "igbvf_pmd",
+	.init = rte_igbvf_pmd_init,
+	.pci_table = pci_id_igbvf_map,
+};
+
+PMD_REGISTER_DRIVER(pmd_igb_drv);
+PMD_REGISTER_DRIVER(pmd_igbvf_drv);
+#endif
+
+#if RTE_LIBRTE_EM_PMD
+extern int
+rte_em_pmd_init(const char *name __rte_unused, const char *params __rte_unused);
+
+/*
+ *  * The set of PCI devices this driver supports
+ *   */
+extern const struct rte_pci_id pci_id_em_map[];
+
+static struct rte_driver __attribute__((used)) em_pmd_drv = {
+	.type = PMD_PDEV,
+	.name = "em_pmd",
+	.init = rte_em_pmd_init,
+	.pci_table = pci_id_em_map,
+};
+
+PMD_REGISTER_DRIVER(em_pmd_drv);
+#endif
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index e157587..edefd2a 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -1448,6 +1448,7 @@ struct rte_driver ena_pmd_drv = {
 	.name = "ena_driver",
 	.type = PMD_PDEV,
 	.init = rte_ena_pmd_init,
+	.pci_table = pci_id_ena_map,
 };
 
 PMD_REGISTER_DRIVER(ena_pmd_drv);
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 6bea940..d88e648 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -682,6 +682,7 @@ rte_enic_pmd_init(__rte_unused const char *name,
 static struct rte_driver rte_enic_driver = {
 	.type = PMD_PDEV,
 	.init = rte_enic_pmd_init,
+	.pci_table = pci_id_enic_map,
 };
 
 PMD_REGISTER_DRIVER(rte_enic_driver);
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index c2d377f..3e00479 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -3053,6 +3053,7 @@ rte_pmd_fm10k_init(__rte_unused const char *name,
 static struct rte_driver rte_fm10k_driver = {
 	.type = PMD_PDEV,
 	.init = rte_pmd_fm10k_init,
+	.pci_table = pci_id_fm10k_map,
 };
 
 PMD_REGISTER_DRIVER(rte_fm10k_driver);
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d8b6bd7..3c7ab13 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -693,9 +693,23 @@ rte_i40e_pmd_init(const char *name __rte_unused,
 static struct rte_driver rte_i40e_driver = {
 	.type = PMD_PDEV,
 	.init = rte_i40e_pmd_init,
+	.pci_table = pci_id_i40e_map,
+};
+
+extern int
+rte_i40evf_pmd_init(const char *name __rte_unused,
+                    const char *params __rte_unused);
+
+extern const struct rte_pci_id pci_id_i40evf_map[];
+
+struct rte_driver rte_i40evf_driver = {
+	.type = PMD_PDEV,
+	.init = rte_i40evf_pmd_init,
+	.pci_table = pci_id_i40evf_map,
 };
 
 PMD_REGISTER_DRIVER(rte_i40e_driver);
+PMD_REGISTER_DRIVER(rte_i40evf_driver);
 
 /*
  * Initialize registers for flexible payload, which should be set by NVM.
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 2bce69b..3c6b57c 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -101,6 +101,9 @@ enum i40evf_aq_result {
 	I40EVF_MSG_CMD,      /* Read async command result */
 };
 
+int
+rte_i40evf_pmd_init(const char *name __rte_unused,
+		    const char *params __rte_unused);
 static int i40evf_dev_configure(struct rte_eth_dev *dev);
 static int i40evf_dev_start(struct rte_eth_dev *dev);
 static void i40evf_dev_stop(struct rte_eth_dev *dev);
@@ -1088,7 +1091,7 @@ i40evf_get_link_status(struct rte_eth_dev *dev, struct rte_eth_link *link)
 	return 0;
 }
 
-static const struct rte_pci_id pci_id_i40evf_map[] = {
+const struct rte_pci_id pci_id_i40evf_map[] = {
 #define RTE_PCI_DEV_ID_DECL_I40EVF(vend, dev) {RTE_PCI_DEVICE(vend, dev)},
 #include "rte_pci_dev_ids.h"
 { .vendor_id = 0, /* sentinel */ },
@@ -1544,7 +1547,7 @@ static struct eth_driver rte_i40evf_pmd = {
  * Invoked one at EAL init time.
  * Register itself as the [Virtual Poll Mode] Driver of PCI Fortville devices.
  */
-static int
+int
 rte_i40evf_pmd_init(const char *name __rte_unused,
 		    const char *params __rte_unused)
 {
@@ -1555,13 +1558,6 @@ rte_i40evf_pmd_init(const char *name __rte_unused,
 	return 0;
 }
 
-static struct rte_driver rte_i40evf_driver = {
-	.type = PMD_PDEV,
-	.init = rte_i40evf_pmd_init,
-};
-
-PMD_REGISTER_DRIVER(rte_i40evf_driver);
-
 static int
 i40evf_dev_configure(struct rte_eth_dev *dev)
 {
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index eec607c..b41108c 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -7134,11 +7134,13 @@ ixgbevf_dev_allmulticast_disable(struct rte_eth_dev *dev)
 static struct rte_driver rte_ixgbe_driver = {
 	.type = PMD_PDEV,
 	.init = rte_ixgbe_pmd_init,
+	.pci_table = pci_id_ixgbe_map,
 };
 
 static struct rte_driver rte_ixgbevf_driver = {
 	.type = PMD_PDEV,
 	.init = rte_ixgbevf_pmd_init,
+	.pci_table = pci_id_ixgbevf_map,
 };
 
 PMD_REGISTER_DRIVER(rte_ixgbe_driver);
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 4f21dbe..c7991a0 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -5815,6 +5815,7 @@ static struct rte_driver rte_mlx4_driver = {
 	.type = PMD_PDEV,
 	.name = MLX4_DRIVER_NAME,
 	.init = rte_mlx4_pmd_init,
+	.pci_table = mlx4_pci_id_map,
 };
 
 PMD_REGISTER_DRIVER(rte_mlx4_driver)
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 041cfc3..8825feb 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -670,6 +670,7 @@ static struct rte_driver rte_mlx5_driver = {
 	.type = PMD_PDEV,
 	.name = MLX5_DRIVER_NAME,
 	.init = rte_mlx5_pmd_init,
+	.pci_table = mlx5_pci_id_map,
 };
 
 PMD_REGISTER_DRIVER(rte_mlx5_driver)
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index bc0a3d8..0280b81 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -2480,6 +2480,7 @@ nfp_net_pmd_init(const char *name __rte_unused,
 static struct rte_driver rte_nfp_net_driver = {
 	.type = PMD_PDEV,
 	.init = nfp_net_pmd_init,
+	.pci_table = pci_id_nfp_net_map,
 };
 
 PMD_REGISTER_DRIVER(rte_nfp_net_driver);
diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index 78c43b0..7af758a 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -1599,6 +1599,7 @@ static struct rte_driver rte_szedata2_driver = {
 	.name = RTE_SZEDATA2_DRIVER_NAME,
 	.init = rte_szedata2_init,
 	.uninit = rte_szedata2_uninit,
+	.pci_table = rte_szedata2_pci_id_table,
 };
 
 PMD_REGISTER_DRIVER(rte_szedata2_driver);
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 63a368a..93b2562 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1456,6 +1456,7 @@ __rte_unused uint8_t is_rx)
 static struct rte_driver rte_virtio_driver = {
 	.type = PMD_PDEV,
 	.init = rte_virtio_pmd_init,
+	.pci_table = pci_id_virtio_map,
 };
 
 PMD_REGISTER_DRIVER(rte_virtio_driver);
diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 29b469c..83420c8 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -952,6 +952,7 @@ vmxnet3_process_events(struct vmxnet3_hw *hw)
 static struct rte_driver rte_vmxnet3_driver = {
 	.type = PMD_PDEV,
 	.init = rte_vmxnet3_pmd_init,
+	.pci_table = pci_id_vmxnet3_map,
 };
 
 PMD_REGISTER_DRIVER(rte_vmxnet3_driver);
-- 
2.5.5

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [RFC PATCH 4/4] pmdinfo: Add application to extract pmd driver info
  2016-04-26 17:39 [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
                   ` (2 preceding siblings ...)
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 3/4] pmd: Modify drivers to export appropriate information Neil Horman
@ 2016-04-26 17:39 ` Neil Horman
  2016-05-03 11:57 ` [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
  4 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2016-04-26 17:39 UTC (permalink / raw)
  To: dev
  Cc: David Marchand, Stephen Hemminger, bruce.richardson,
	Panu Matilainen, Thomas Monjalon, Neil Horman

This tool uses the prior infrastructure to provide human readable information to
a user about the devices which a given pmd DSO supports.

Usage:
pmdinfo /path/to/driver/pmd

pmdinfo dlopens the specified file, then iteratively looks up the
this_pmd_driver<n> symbol.  For each found symbol in the DSO, it prints
information found in the corresponding rte_driver struct

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: David Marchand <david.marchand@6wind.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: "Richardson, Bruce" <bruce.richardson@intel.com>
CC: Panu Matilainen <pmatilai@redhat.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 app/Makefile          |  1 +
 app/pmdinfo/Makefile  | 55 +++++++++++++++++++++++++++++
 app/pmdinfo/pmdinfo.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 app/pmdinfo/Makefile
 create mode 100644 app/pmdinfo/pmdinfo.c

diff --git a/app/Makefile b/app/Makefile
index 1151e09..42ea130 100644
--- a/app/Makefile
+++ b/app/Makefile
@@ -37,5 +37,6 @@ DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += test-pipeline
 DIRS-$(CONFIG_RTE_TEST_PMD) += test-pmd
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline_test
 DIRS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += proc_info
+DIRS-$(CONFIG_RTE_BUILD_SHARED_LIB) += pmdinfo
 
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/app/pmdinfo/Makefile b/app/pmdinfo/Makefile
new file mode 100644
index 0000000..eb38aab
--- /dev/null
+++ b/app/pmdinfo/Makefile
@@ -0,0 +1,55 @@
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above copyright
+#       notice, this list of conditions and the following disclaimer in
+#       the documentation and/or other materials provided with the
+#       distribution.
+#     * Neither the name of Intel Corporation nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
+
+#
+# library name
+#
+APP = pmdinfo
+
+CFLAGS += -Os -g
+CFLAGS += $(WERROR_FLAGS)
+
+LDLIBS := -ldl
+
+DEPDIRS-y += lib
+
+#
+# all source are stored in SRCS-y
+#
+SRCS-y := pmdinfo.c
+
+include $(RTE_SDK)/mk/rte.app.mk
+
+endif
diff --git a/app/pmdinfo/pmdinfo.c b/app/pmdinfo/pmdinfo.c
new file mode 100644
index 0000000..7db61b2
--- /dev/null
+++ b/app/pmdinfo/pmdinfo.c
@@ -0,0 +1,96 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <dlfcn.h>
+#include <string.h>
+#include <rte_dev.h>
+
+
+static void dump_pci_table(struct rte_driver *driver)
+{
+	int i;
+
+	if (!driver->pci_table) {
+		printf(" No PCI Table defined for this driver\n");
+		return;
+	}
+
+	printf("|====================PCI Table========================|\n");
+	printf("| VENDOR ID | DEVICE ID | SUBVENDOR ID | SUBDEVICE ID |\n");
+	printf("|-----------------------------------------------------|\n");
+	for (i=0; driver->pci_table[i].vendor_id != 0; i++) {
+		printf("|%11x|%11x|%14x|%14x|\n",
+		driver->pci_table[i].vendor_id, driver->pci_table[i].device_id,
+		driver->pci_table[i].subsystem_vendor_id,
+		driver->pci_table[i].subsystem_device_id);
+	}
+	printf("|-----------------------------------------------------|\n");
+}
+
+static void dump_driver_info(int idx, struct rte_driver *driver)
+{
+	printf("PMD %d Information:\n", idx);
+	printf("Driver Name: %s\n", driver->name);
+
+	switch (driver->type) {
+	case PMD_VDEV:
+		printf("Driver Type: Virtual\n");
+		break;
+	case PMD_PDEV:
+		printf("Driver Type: PCI\n");
+		dump_pci_table(driver);
+		break;
+	default:
+		printf("Driver Type: UNKNOWN (%d)\n", driver->type);
+		break;
+	}
+}
+
+int main(int argc, char **argv)
+{
+	void *pmd;
+	struct rte_driver *driver;
+	int i = 0;
+	int rc = 0;
+	const char *basename = "this_pmd_driver";
+	char symname[512];
+
+	if (argc <= 1) {
+		printf("You must specify a pmd to load\n");
+		rc = 127;
+		goto out;
+	}
+
+	pmd = dlopen(argv[1], RTLD_LAZY);
+
+
+	if (!pmd) {
+		printf("Unalbe to open dlopen library: %s\n", dlerror());
+		rc = 128;
+		goto out;
+	}
+
+	do {
+		memset(symname, 0, 512);
+		snprintf(symname, 512, "%s%d", basename, i);
+		driver = dlsym(pmd, symname);
+		
+		if (!driver) {
+			char *err = dlerror();
+			if (err && !i) {
+				printf("%s\n", err);
+				rc = 129;
+			}
+			goto out_close;
+		}
+		dump_driver_info(i, driver);
+		i++;
+	} while(driver);
+
+out_close:
+	dlclose(pmd);
+out:
+	exit(rc);
+}
+
+
+
-- 
2.5.5

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
  2016-04-26 17:39 [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
                   ` (3 preceding siblings ...)
  2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 4/4] pmdinfo: Add application to extract pmd driver info Neil Horman
@ 2016-05-03 11:57 ` Neil Horman
  2016-05-04  8:24   ` David Marchand
  4 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2016-05-03 11:57 UTC (permalink / raw)
  To: dev
  Cc: David Marchand, Stephen Hemminger, bruce.richardson,
	Panu Matilainen, Thomas Monjalon

On Tue, Apr 26, 2016 at 01:39:47PM -0400, Neil Horman wrote:
> Hey-
> 	So a few days ago we were reviewing Davids patch series to introduce the
> abiilty to dump hardware support from pmd DSO's in a human readable format.
> That effort encountered some problems, most notably the fact that stripping a
> DSO removed the required information that the proposed tool keyed off, as well
> as the need to dead reckon offsets between symbols that may not be constant
> (dependent on architecture).
> 
> 	I was going to start looking into the possibility of creating a modinfo
> section in a simmilar fashion to what kernel modules do in linux or BSD.  I
> decided to propose this solution instead though, because the kernel style
> solution requires a significant amount of infrastructure that I think we can
> maybe avoid maintaining, if we accept some minor caviats
> 
> To do this We emit a set of well known marker symbols for each DSO that an
> external application can search for (in this case I called them
> this_pmd_driver<n>, where n is a counter macro).  These marker symbols are
> n is a counter macro).  These marker symbols are exported by PMDs for
> external access.  External tools can then access these symbols via the
> dlopen/dlsym api (or via elfutils libraries)
> 
> The symbols above alias the rte_driver struct for each PMD, and the external
> application can then interrogate the registered driver information.  
> 
> I also add a pointer to the pci id table struct for each PMD so that we can
> export hardware support.
> 
> This approach has a few pros and cons:
> 
> pros:
> 1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
> don't need a new tool to extract driver information and emit the C code to build
> the binary data for the special section, nor do we need a custom linker script
> to link said special section in place
> 
> 2) Its stable.  Because the marker symbols are explicitly exported, this
> approach is resilient against stripping.
> 
> cons:
> 1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
> compilation unit per DSO.  As an example em and igb effectively merge two
> drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
> C files for the same single linked DSO.  Because of the use of the __COUNTER__
> macro we get multiple definitions of the same marker symbols.  
> 
> I would make the argument that the downside of the above artifact isn't that big
> a deal.  Traditionally in other projects a unit like a module (or DSO in our
> case) only ever codifies a single driver (e.g. module_init() in the linux kernel
> is only ever used once per built module).  If we have code like igb/em that
> shares some core code, we should build the shared code to object files and link
> them twice, once to an em.so pmd and again to an igb.so pmd.
> 
> But regardless, I thought I would propose this to see what you all thought of
> it.
> 
> FWIW, heres sample output of the pmdinfo tool from this series probing the
> librte_pmd_ena.so module:
> 
> [nhorman@hmsreliant dpdk]$ ./build/app/pmdinfo
> ~/git/dpdk/build/lib/librte_pmd_ena.so
> PMD 0 Information:
> Driver Name: ena_driver
> Driver Type: PCI
> |====================PCI Table========================|
> | VENDOR ID | DEVICE ID | SUBVENDOR ID | SUBDEVICE ID |
> |-----------------------------------------------------|
> |       1d0f|       ec20|          ffff|          ffff|
> |       1d0f|       ec21|          ffff|          ffff|
> |-----------------------------------------------------|
> 
> 
> 
> 
Ping, thoughts here?

Neil

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
  2016-05-03 11:57 ` [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
@ 2016-05-04  8:24   ` David Marchand
  2016-05-04 11:43     ` Neil Horman
  0 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2016-05-04  8:24 UTC (permalink / raw)
  To: Neil Horman
  Cc: dev, Stephen Hemminger, Richardson, Bruce, Panu Matilainen,
	Thomas Monjalon

On Tue, May 3, 2016 at 1:57 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Tue, Apr 26, 2016 at 01:39:47PM -0400, Neil Horman wrote:
>> Hey-
>>       So a few days ago we were reviewing Davids patch series to introduce the
>> abiilty to dump hardware support from pmd DSO's in a human readable format.
>> That effort encountered some problems, most notably the fact that stripping a
>> DSO removed the required information that the proposed tool keyed off, as well
>> as the need to dead reckon offsets between symbols that may not be constant
>> (dependent on architecture).
>>
>>       I was going to start looking into the possibility of creating a modinfo
>> section in a simmilar fashion to what kernel modules do in linux or BSD.  I
>> decided to propose this solution instead though, because the kernel style
>> solution requires a significant amount of infrastructure that I think we can
>> maybe avoid maintaining, if we accept some minor caviats
>>
>> To do this We emit a set of well known marker symbols for each DSO that an
>> external application can search for (in this case I called them
>> this_pmd_driver<n>, where n is a counter macro).  These marker symbols are
>> n is a counter macro).  These marker symbols are exported by PMDs for
>> external access.  External tools can then access these symbols via the
>> dlopen/dlsym api (or via elfutils libraries)
>>
>> The symbols above alias the rte_driver struct for each PMD, and the external
>> application can then interrogate the registered driver information.
>>
>> I also add a pointer to the pci id table struct for each PMD so that we can
>> export hardware support.
>>
>> This approach has a few pros and cons:
>>
>> pros:
>> 1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
>> don't need a new tool to extract driver information and emit the C code to build
>> the binary data for the special section, nor do we need a custom linker script
>> to link said special section in place
>>
>> 2) Its stable.  Because the marker symbols are explicitly exported, this
>> approach is resilient against stripping.
>>
>> cons:
>> 1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
>> compilation unit per DSO.  As an example em and igb effectively merge two
>> drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
>> C files for the same single linked DSO.  Because of the use of the __COUNTER__
>> macro we get multiple definitions of the same marker symbols.
>>
>> I would make the argument that the downside of the above artifact isn't that big
>> a deal.  Traditionally in other projects a unit like a module (or DSO in our
>> case) only ever codifies a single driver (e.g. module_init() in the linux kernel
>> is only ever used once per built module).  If we have code like igb/em that
>> shares some core code, we should build the shared code to object files and link
>> them twice, once to an em.so pmd and again to an igb.so pmd.
>>
>> But regardless, I thought I would propose this to see what you all thought of
>> it.
>>
>> FWIW, heres sample output of the pmdinfo tool from this series probing the
>> librte_pmd_ena.so module:
>>
>> [nhorman@hmsreliant dpdk]$ ./build/app/pmdinfo
>> ~/git/dpdk/build/lib/librte_pmd_ena.so
>> PMD 0 Information:
>> Driver Name: ena_driver
>> Driver Type: PCI
>> |====================PCI Table========================|
>> | VENDOR ID | DEVICE ID | SUBVENDOR ID | SUBDEVICE ID |
>> |-----------------------------------------------------|
>> |       1d0f|       ec20|          ffff|          ffff|
>> |       1d0f|       ec21|          ffff|          ffff|
>> |-----------------------------------------------------|
>>
>>
>>
>>
> Ping, thoughts here?

- This implementation does not support binaries, so it is not suitable
for people who don't want dso, this is partially why I used bfd rather
than just dlopen.
- The name of the tool "pmdinfo" is likely to cause problems, I would
say we need to prefix t with dpdk.
- How does it behave if we strip the dsos ?
- Using __COUNTER__ seeems a bit tricky to me, can't this cause misalignments ?
- The tool output format is not script friendly from my pov.


-- 
David Marchand

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
  2016-05-04  8:24   ` David Marchand
@ 2016-05-04 11:43     ` Neil Horman
  2016-05-04 21:16       ` Thomas Monjalon
  0 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2016-05-04 11:43 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Stephen Hemminger, Richardson, Bruce, Panu Matilainen,
	Thomas Monjalon

On Wed, May 04, 2016 at 10:24:18AM +0200, David Marchand wrote:
> On Tue, May 3, 2016 at 1:57 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Tue, Apr 26, 2016 at 01:39:47PM -0400, Neil Horman wrote:
> >> Hey-
> >>       So a few days ago we were reviewing Davids patch series to introduce the
> >> abiilty to dump hardware support from pmd DSO's in a human readable format.
> >> That effort encountered some problems, most notably the fact that stripping a
> >> DSO removed the required information that the proposed tool keyed off, as well
> >> as the need to dead reckon offsets between symbols that may not be constant
> >> (dependent on architecture).
> >>
> >>       I was going to start looking into the possibility of creating a modinfo
> >> section in a simmilar fashion to what kernel modules do in linux or BSD.  I
> >> decided to propose this solution instead though, because the kernel style
> >> solution requires a significant amount of infrastructure that I think we can
> >> maybe avoid maintaining, if we accept some minor caviats
> >>
> >> To do this We emit a set of well known marker symbols for each DSO that an
> >> external application can search for (in this case I called them
> >> this_pmd_driver<n>, where n is a counter macro).  These marker symbols are
> >> n is a counter macro).  These marker symbols are exported by PMDs for
> >> external access.  External tools can then access these symbols via the
> >> dlopen/dlsym api (or via elfutils libraries)
> >>
> >> The symbols above alias the rte_driver struct for each PMD, and the external
> >> application can then interrogate the registered driver information.
> >>
> >> I also add a pointer to the pci id table struct for each PMD so that we can
> >> export hardware support.
> >>
> >> This approach has a few pros and cons:
> >>
> >> pros:
> >> 1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
> >> don't need a new tool to extract driver information and emit the C code to build
> >> the binary data for the special section, nor do we need a custom linker script
> >> to link said special section in place
> >>
> >> 2) Its stable.  Because the marker symbols are explicitly exported, this
> >> approach is resilient against stripping.
> >>
> >> cons:
> >> 1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
> >> compilation unit per DSO.  As an example em and igb effectively merge two
> >> drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
> >> C files for the same single linked DSO.  Because of the use of the __COUNTER__
> >> macro we get multiple definitions of the same marker symbols.
> >>
> >> I would make the argument that the downside of the above artifact isn't that big
> >> a deal.  Traditionally in other projects a unit like a module (or DSO in our
> >> case) only ever codifies a single driver (e.g. module_init() in the linux kernel
> >> is only ever used once per built module).  If we have code like igb/em that
> >> shares some core code, we should build the shared code to object files and link
> >> them twice, once to an em.so pmd and again to an igb.so pmd.
> >>
> >> But regardless, I thought I would propose this to see what you all thought of
> >> it.
> >>
> >> FWIW, heres sample output of the pmdinfo tool from this series probing the
> >> librte_pmd_ena.so module:
> >>
> >> [nhorman@hmsreliant dpdk]$ ./build/app/pmdinfo
> >> ~/git/dpdk/build/lib/librte_pmd_ena.so
> >> PMD 0 Information:
> >> Driver Name: ena_driver
> >> Driver Type: PCI
> >> |====================PCI Table========================|
> >> | VENDOR ID | DEVICE ID | SUBVENDOR ID | SUBDEVICE ID |
> >> |-----------------------------------------------------|
> >> |       1d0f|       ec20|          ffff|          ffff|
> >> |       1d0f|       ec21|          ffff|          ffff|
> >> |-----------------------------------------------------|
> >>
> >>
> >>
> >>
> > Ping, thoughts here?
> 
> - This implementation does not support binaries, so it is not suitable
> for people who don't want dso, this is partially why I used bfd rather
> than just dlopen.
If you're statically linking an application, you should know what hardware you
support already.  Its going to be very hard, if not impossible to develop a
robust solution that works with static binaries (the prior solutions don't work
consistently with static binaries either).  I really think the static solution
needs to just be built into the application (i.e. the application needs to add a
command line option to dump out the pci id's that are registered).

> - The name of the tool "pmdinfo" is likely to cause problems, I would
> say we need to prefix t with dpdk.
Why?

> - How does it behave if we strip the dsos ?
I described this above, its invariant to stripping, because the symbols for each
pmd are explicitly exported, so strip doesn't touch the symbols that pmdinfo
keys off of.

> - Using __COUNTER__ seeems a bit tricky to me, can't this cause misalignments ?
I'm not sure what you mean here. __COUNTER__ expands to a numerical
string that I concatinate with a fixed string, to provide pmdinfo with a set of
well known symbols to look for.  What does misalignment have to do with anything
here?

> - The tool output format is not script friendly from my pov.
Don't think it really needs to be script friendly, it was meant to provide human
readable output, but script friendly output can be added easily enough if you
want.

Neil

> 
> 
> -- 
> David Marchand
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
  2016-05-04 11:43     ` Neil Horman
@ 2016-05-04 21:16       ` Thomas Monjalon
  2016-05-05  9:42         ` Bruce Richardson
  2016-05-05 11:38         ` Neil Horman
  0 siblings, 2 replies; 11+ messages in thread
From: Thomas Monjalon @ 2016-05-04 21:16 UTC (permalink / raw)
  To: Neil Horman, dev
  Cc: David Marchand, Stephen Hemminger, Richardson, Bruce, Panu Matilainen

This discussion requires more opinions.
Please everybody, READ and COMMENT. Thanks

If it is not enough visible, a new thread could be started later.

2016-05-04 07:43, Neil Horman:
> On Wed, May 04, 2016 at 10:24:18AM +0200, David Marchand wrote:
> > On Tue, May 3, 2016 at 1:57 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > >> This approach has a few pros and cons:
> > >>
> > >> pros:
> > >> 1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
> > >> don't need a new tool to extract driver information and emit the C code to build
> > >> the binary data for the special section, nor do we need a custom linker script
> > >> to link said special section in place
> > >>
> > >> 2) Its stable.  Because the marker symbols are explicitly exported, this
> > >> approach is resilient against stripping.

It is a good point. We need something resilient against stripping.

> > >> cons:
> > >> 1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
> > >> compilation unit per DSO.  As an example em and igb effectively merge two
> > >> drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
> > >> C files for the same single linked DSO.  Because of the use of the __COUNTER__
> > >> macro we get multiple definitions of the same marker symbols.
> > >>
> > >> I would make the argument that the downside of the above artifact isn't that big
> > >> a deal.  Traditionally in other projects a unit like a module (or DSO in our
> > >> case) only ever codifies a single driver (e.g. module_init() in the linux kernel
> > >> is only ever used once per built module).  If we have code like igb/em that
> > >> shares some core code, we should build the shared code to object files and link
> > >> them twice, once to an em.so pmd and again to an igb.so pmd.

It is also a problem for compilation units having PF and VF drivers.

> > >> But regardless, I thought I would propose this to see what you all thought of
> > >> it.

Thanks for proposing.

> > - This implementation does not support binaries, so it is not suitable
> > for people who don't want dso, this is partially why I used bfd rather
> > than just dlopen.
> 
> If you're statically linking an application, you should know what hardware you
> support already.  Its going to be very hard, if not impossible to develop a
> robust solution that works with static binaries (the prior solutions don't work
> consistently with static binaries either).  I really think the static solution
> needs to just be built into the application (i.e. the application needs to add a
> command line option to dump out the pci id's that are registered).

No, we need a tool to know what are the supported devices before running
the application (e.g. for binding).
This tool should not behave differently depending of how DPDK was compiled
(static or shared).

[...]
> > - How does it behave if we strip the dsos ?
> 
> I described this above, its invariant to stripping, because the symbols for each
> pmd are explicitly exported, so strip doesn't touch the symbols that pmdinfo
> keys off of.
> 
[...]
> > - The tool output format is not script friendly from my pov.
> 
> Don't think it really needs to be script friendly, it was meant to provide human
> readable output, but script friendly output can be added easily enough if you
> want.

Yes it needs to be script friendly.

It appears that we must agree on a set of requirements first.
Please let's forget the implementation until we have collected enough
feedbacks on the needs.
I suggest these items to start the list:

- query all drivers in static binary or shared library
- stripping resiliency
- human friendly
- script friendly
- show driver name
- list supported device id / name
- list driver options
- show driver version if available
- show dpdk version
- show kernel dependencies (vfio/uio_pci_generic/etc)
- room for extra information?

Please ack or comment items of this list, thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
  2016-05-04 21:16       ` Thomas Monjalon
@ 2016-05-05  9:42         ` Bruce Richardson
  2016-05-05 11:38         ` Neil Horman
  1 sibling, 0 replies; 11+ messages in thread
From: Bruce Richardson @ 2016-05-05  9:42 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Neil Horman, dev, David Marchand, Stephen Hemminger, Panu Matilainen

On Wed, May 04, 2016 at 11:16:42PM +0200, Thomas Monjalon wrote:
> This discussion requires more opinions.
> Please everybody, READ and COMMENT. Thanks
> 
> If it is not enough visible, a new thread could be started later.
> 
> 2016-05-04 07:43, Neil Horman:
> > On Wed, May 04, 2016 at 10:24:18AM +0200, David Marchand wrote:
> > > On Tue, May 3, 2016 at 1:57 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >> This approach has a few pros and cons:
> > > >>
> > > >> pros:
> > > >> 1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
> > > >> don't need a new tool to extract driver information and emit the C code to build
> > > >> the binary data for the special section, nor do we need a custom linker script
> > > >> to link said special section in place
> > > >>
> > > >> 2) Its stable.  Because the marker symbols are explicitly exported, this
> > > >> approach is resilient against stripping.
> 
> It is a good point. We need something resilient against stripping.
> 
> > > >> cons:
> > > >> 1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
> > > >> compilation unit per DSO.  As an example em and igb effectively merge two
> > > >> drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
> > > >> C files for the same single linked DSO.  Because of the use of the __COUNTER__
> > > >> macro we get multiple definitions of the same marker symbols.
> > > >>
> > > >> I would make the argument that the downside of the above artifact isn't that big
> > > >> a deal.  Traditionally in other projects a unit like a module (or DSO in our
> > > >> case) only ever codifies a single driver (e.g. module_init() in the linux kernel
> > > >> is only ever used once per built module).  If we have code like igb/em that
> > > >> shares some core code, we should build the shared code to object files and link
> > > >> them twice, once to an em.so pmd and again to an igb.so pmd.
> 
> It is also a problem for compilation units having PF and VF drivers.
> 
> > > >> But regardless, I thought I would propose this to see what you all thought of
> > > >> it.
> 
> Thanks for proposing.
> 
> > > - This implementation does not support binaries, so it is not suitable
> > > for people who don't want dso, this is partially why I used bfd rather
> > > than just dlopen.
> > 
> > If you're statically linking an application, you should know what hardware you
> > support already.  Its going to be very hard, if not impossible to develop a
> > robust solution that works with static binaries (the prior solutions don't work
> > consistently with static binaries either).  I really think the static solution
> > needs to just be built into the application (i.e. the application needs to add a
> > command line option to dump out the pci id's that are registered).
> 
> No, we need a tool to know what are the supported devices before running
> the application (e.g. for binding).
> This tool should not behave differently depending of how DPDK was compiled
> (static or shared).
> 
> [...]
> > > - How does it behave if we strip the dsos ?
> > 
> > I described this above, its invariant to stripping, because the symbols for each
> > pmd are explicitly exported, so strip doesn't touch the symbols that pmdinfo
> > keys off of.
> > 
> [...]
> > > - The tool output format is not script friendly from my pov.
> > 
> > Don't think it really needs to be script friendly, it was meant to provide human
> > readable output, but script friendly output can be added easily enough if you
> > want.
> 
> Yes it needs to be script friendly.
> 
> It appears that we must agree on a set of requirements first.
> Please let's forget the implementation until we have collected enough
> feedbacks on the needs.
> I suggest these items to start the list:
> 
> - query all drivers in static binary or shared library
> - stripping resiliency
> - human friendly
> - script friendly
> - show driver name
> - list supported device id / name
> - list driver options
> - show driver version if available
> - show dpdk version
> - show kernel dependencies (vfio/uio_pci_generic/etc)
> - room for extra information?
> 
> Please ack or comment items of this list, thanks.

That's quite a laundry list of requirements! I would view the following as core
requirements:
 - query all drivers in static binary or shared library
 - stripping resiliency
 - human friendly
 - script friendly
 - show driver name
 - list supported device id / name

and the rest as nice-to-have's that are not needed for a first version.
That being said, I would expect those nice-to-have's to be fairly easy to add on
once the base solution is in place.

On a semi-related note, I assume this discussion and a solution here is not going
to block the merging of the other clean-up patches in the driver/pci area?

http://dpdk.org/ml/archives/dev/2016-April/037686.html
http://dpdk.org/ml/archives/dev/2016-April/037708.html (Patches 1-10)


Regards,
/Bruce

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH 0/4]: Implement module information export
  2016-05-04 21:16       ` Thomas Monjalon
  2016-05-05  9:42         ` Bruce Richardson
@ 2016-05-05 11:38         ` Neil Horman
  1 sibling, 0 replies; 11+ messages in thread
From: Neil Horman @ 2016-05-05 11:38 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev, David Marchand, Stephen Hemminger, Richardson, Bruce,
	Panu Matilainen

On Wed, May 04, 2016 at 11:16:42PM +0200, Thomas Monjalon wrote:
> This discussion requires more opinions.
> Please everybody, READ and COMMENT. Thanks
> 
> If it is not enough visible, a new thread could be started later.
> 
> 2016-05-04 07:43, Neil Horman:
> > On Wed, May 04, 2016 at 10:24:18AM +0200, David Marchand wrote:
> > > On Tue, May 3, 2016 at 1:57 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >> This approach has a few pros and cons:
> > > >>
> > > >> pros:
> > > >> 1) Its simple, and doesn't require extra infrastructure to implement.  E.g. we
> > > >> don't need a new tool to extract driver information and emit the C code to build
> > > >> the binary data for the special section, nor do we need a custom linker script
> > > >> to link said special section in place
> > > >>
> > > >> 2) Its stable.  Because the marker symbols are explicitly exported, this
> > > >> approach is resilient against stripping.
> 
> It is a good point. We need something resilient against stripping.
> 
> > > >> cons:
> > > >> 1) It creates an artifact in that PMD_REGISTER_DRIVER has to be used in one
> > > >> compilation unit per DSO.  As an example em and igb effectively merge two
> > > >> drivers into one DSO, and the uses of PMD_REGISTER_DRIVER occur in two separate
> > > >> C files for the same single linked DSO.  Because of the use of the __COUNTER__
> > > >> macro we get multiple definitions of the same marker symbols.
> > > >>
> > > >> I would make the argument that the downside of the above artifact isn't that big
> > > >> a deal.  Traditionally in other projects a unit like a module (or DSO in our
> > > >> case) only ever codifies a single driver (e.g. module_init() in the linux kernel
> > > >> is only ever used once per built module).  If we have code like igb/em that
> > > >> shares some core code, we should build the shared code to object files and link
> > > >> them twice, once to an em.so pmd and again to an igb.so pmd.
> 
> It is also a problem for compilation units having PF and VF drivers.
> 
Thats exactly the problem in the two cases I encountered.  The common code
between the PF and VF drivers was handled by simply building the whole driver
together, which is problematic because it make both drivers bigger than they
need to be.  But its somewhat moot, as the problem is orthogonal to this one if
we have to go with a 'special section' approach to implementing this.

> > > >> But regardless, I thought I would propose this to see what you all thought of
> > > >> it.
> 
> Thanks for proposing.
> 
> > > - This implementation does not support binaries, so it is not suitable
> > > for people who don't want dso, this is partially why I used bfd rather
> > > than just dlopen.
> > 
> > If you're statically linking an application, you should know what hardware you
> > support already.  Its going to be very hard, if not impossible to develop a
> > robust solution that works with static binaries (the prior solutions don't work
> > consistently with static binaries either).  I really think the static solution
> > needs to just be built into the application (i.e. the application needs to add a
> > command line option to dump out the pci id's that are registered).
> 
> No, we need a tool to know what are the supported devices before running
> the application (e.g. for binding).
> This tool should not behave differently depending of how DPDK was compiled
> (static or shared).
> 
Very well.

> [...]
> > > - How does it behave if we strip the dsos ?
> > 
> > I described this above, its invariant to stripping, because the symbols for each
> > pmd are explicitly exported, so strip doesn't touch the symbols that pmdinfo
> > keys off of.
> > 
> [...]
> > > - The tool output format is not script friendly from my pov.
> > 
> > Don't think it really needs to be script friendly, it was meant to provide human
> > readable output, but script friendly output can be added easily enough if you
> > want.
> 
> Yes it needs to be script friendly.
> 
Thats easy enough.

> It appears that we must agree on a set of requirements first.
> Please let's forget the implementation until we have collected enough
> feedbacks on the needs.
> I suggest these items to start the list:
> 
> - query all drivers in static binary or shared library
This does require a some sort of special section then, but thats fine

> - stripping resiliency
easy enough
> - human friendly
done
> - script friendly
Can be done easily
> - show driver name
Thats somewhat orthogonal to this problem, the driver name is codified in the
rte_driver struct, we just have to be sure that driver writers fill that out

> - list supported device id / name
> - list driver options
Hadn't thought of that, but its a good idea.
> - show driver version if available
Can be done.
> - show dpdk version
This is tricky as its not really codified in any DPDK code, especially for DSO's
built independently from the dpdk library

> - show kernel dependencies (vfio/uio_pci_generic/etc)
Same as above, no real way to know that unless driver writers want to manually
call out dependencies.

> - room for extra information?
> 
> Please ack or comment items of this list, thanks.
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-05-05 11:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-26 17:39 [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 1/4] pmd: Modify PMD_REGISTER_DRIVER to emit a marker symbol Neil Horman
2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 2/4] pmds: export this_pmd_driver* symbols Neil Horman
2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 3/4] pmd: Modify drivers to export appropriate information Neil Horman
2016-04-26 17:39 ` [dpdk-dev] [RFC PATCH 4/4] pmdinfo: Add application to extract pmd driver info Neil Horman
2016-05-03 11:57 ` [dpdk-dev] [RFC PATCH 0/4]: Implement module information export Neil Horman
2016-05-04  8:24   ` David Marchand
2016-05-04 11:43     ` Neil Horman
2016-05-04 21:16       ` Thomas Monjalon
2016-05-05  9:42         ` Bruce Richardson
2016-05-05 11:38         ` Neil Horman

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).