[dpdk-dev] [PATCH v6 0/4] bonding corrections and additions

[dpdk-dev] [PATCH v6 0/4] bonding corrections and additions

[Eric Kinzie]

This patchset makes a couple of small corrections to the bonding driver
and introduces the ability to use an external state machine for mode
4 operation.

Changes in v2:
  . eliminate external_sm field in 802.3ad configuration 
    (rte_eth_bond_8023ad_conf).
  . stop bonding device before changing the periodic callback function.
    start again if needed.
  . remove unnecessary calls to valid_bonded_port_id().
  . do not check for NULL tx_ring.
  . return error in rte_eth_bond_8023ad_ext_slowtx() if packet is not LACP.
  . remove check for external sm configuration in periodic callback
  . check for valid LACPDU in test application's rx callback
  . add "Fixes:" tags

Changes in v3:
  . update rte_eth_bond_version.map

Changes in v4:
  . version functions that modify slowrx_cb

Changes in v5:
  . remove "__vsym" from source files and use updated symbol version macros

Changes in v6:
  . updated symbols have version 2.2
  . specify versioned local symbol names in map so they don't become exported

Eric Kinzie (4):
  bond mode 4: copy entire config structure
  bond mode 4: do not ignore multicast
  bond mode 4: allow external state machine
  bond mode 4: tests for external state machine

 app/test/test_link_bonding_mode4.c                |  217 ++++++++++++++++++-
 drivers/net/bonding/Makefile                      |    2 +-
 drivers/net/bonding/rte_eth_bond_8023ad.c         |  241 ++++++++++++++++++++-
 drivers/net/bonding/rte_eth_bond_8023ad.h         |   44 ++++
 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   28 +++
 drivers/net/bonding/rte_eth_bond_pmd.c            |    1 +
 drivers/net/bonding/rte_eth_bond_version.map      |   10 +
 7 files changed, 529 insertions(+), 14 deletions(-)

-- 
1.7.10.4

[dpdk-dev] [PATCH v6 1/4] bond mode 4: copy entire config structure

[Eric Kinzie]

From: Eric Kinzie <ekinzie@brocade.com>

  Copy all needed fields from the mode8023ad_private structure in
  bond_mode_8023ad_conf_get().  This help ensure that a subsequent call
  to rte_eth_bond_8023ad_setup() is not passed uninitialized data that
  would result in either incorrect behavior or a failed sanity check.

Fixes: 46fb43683679 ("bond: add mode 4")

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index c0f0b99..08f679f 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -1013,6 +1013,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	conf->aggregate_wait_timeout_ms = mode4->aggregate_wait_timeout / ms_ticks;
 	conf->tx_period_ms = mode4->tx_period_timeout / ms_ticks;
 	conf->update_timeout_ms = mode4->update_timeout_us / 1000;
+	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
 }
 
 void
-- 
1.7.10.4

[dpdk-dev] [PATCH v6 2/4] bond mode 4: do not ignore multicast

[Eric Kinzie]

From: Eric Kinzie <ekinzie@brocade.com>

The bonding PMD in mode 4 puts all enslaved interfaces into promiscuous
mode in order to receive LACPDUs and must filter unwanted packets
after the traffic has been "collected".  Allow broadcast and multicast
through so that ARP and IPv6 neighbor discovery continue to work.

Fixes: 46fb43683679 ("bond: add mode 4")

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 app/test/test_link_bonding_mode4.c     |    7 +++++--
 drivers/net/bonding/rte_eth_bond_pmd.c |    1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 460539d..d785393 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -749,8 +749,11 @@ test_mode4_rx(void)
 	rte_eth_macaddr_get(test_params.bonded_port_id, &bonded_mac);
 	ether_addr_copy(&bonded_mac, &dst_mac);
 
-	/* Assert that dst address is not bonding address */
-	dst_mac.addr_bytes[0]++;
+	/* Assert that dst address is not bonding address.  Do not set the
+	 * least significant bit of the zero byte as this would create a
+	 * multicast address.
+	 */
+	dst_mac.addr_bytes[0] += 2;
 
 	/* First try with promiscuous mode enabled.
 	 * Add 2 packets to each slave. First with bonding MAC address, second with
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 5cc6372..733022b 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -170,6 +170,7 @@ bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
 			 * mode and packet address does not match. */
 			if (unlikely(hdr->ether_type == ether_type_slow_be ||
 				!collecting || (!promisc &&
+					!is_multicast_ether_addr(&hdr->d_addr) &&
 					!is_same_ether_addr(&bond_mac, &hdr->d_addr)))) {
 
 				if (hdr->ether_type == ether_type_slow_be) {
-- 
1.7.10.4

[dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Eric Kinzie]

From: Eric Kinzie <ekinzie@brocade.com>

  Provide functions to allow an external 802.3ad state machine to transmit
  and recieve LACPDUs and to set the collection/distribution flags on
  slave interfaces.

  Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
  and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
  functions.

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 drivers/net/bonding/Makefile                      |    2 +-
 drivers/net/bonding/rte_eth_bond_8023ad.c         |  240 ++++++++++++++++++++-
 drivers/net/bonding/rte_eth_bond_8023ad.h         |   44 ++++
 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   28 +++
 drivers/net/bonding/rte_eth_bond_version.map      |   10 +
 5 files changed, 321 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/Makefile b/drivers/net/bonding/Makefile
index dee0875..8b892ca 100644
--- a/drivers/net/bonding/Makefile
+++ b/drivers/net/bonding/Makefile
@@ -41,7 +41,7 @@ CFLAGS += $(WERROR_FLAGS)
 
 EXPORT_MAP := rte_eth_bond_version.map
 
-LIBABIVER := 1
+LIBABIVER := 2
 
 #
 # all source are stored in SRCS-y
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 08f679f..0461fb5 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -39,9 +39,12 @@
 #include <rte_malloc.h>
 #include <rte_errno.h>
 #include <rte_cycles.h>
+#include <rte_compat.h>
 
 #include "rte_eth_bond_private.h"
 
+static void bond_mode_8023ad_ext_periodic_cb(void *arg);
+
 #ifdef RTE_LIBRTE_BOND_DEBUG_8023AD
 #define MODE4_DEBUG(fmt, ...) RTE_LOG(DEBUG, PMD, "%6u [Port %u: %s] " fmt, \
 			bond_dbg_get_time_diff_ms(), slave_id, \
@@ -999,7 +1002,28 @@ bond_mode_8023ad_mac_address_update(struct rte_eth_dev *bond_dev)
 }
 
 void
-bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
+bond_mode_8023ad_conf_get_v20(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf)
+{
+	struct bond_dev_private *internals = dev->data->dev_private;
+	struct mode8023ad_private *mode4 = &internals->mode4;
+	uint64_t ms_ticks = rte_get_tsc_hz() / 1000;
+
+	conf->fast_periodic_ms = mode4->fast_periodic_timeout / ms_ticks;
+	conf->slow_periodic_ms = mode4->slow_periodic_timeout / ms_ticks;
+	conf->short_timeout_ms = mode4->short_timeout / ms_ticks;
+	conf->long_timeout_ms = mode4->long_timeout / ms_ticks;
+	conf->aggregate_wait_timeout_ms = mode4->aggregate_wait_timeout / ms_ticks;
+	conf->tx_period_ms = mode4->tx_period_timeout / ms_ticks;
+	conf->update_timeout_ms = mode4->update_timeout_us / 1000;
+	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
+	conf->slowrx_cb = mode4->slowrx_cb;
+}
+
+VERSION_SYMBOL(bond_mode_8023ad_conf_get, _v20, 2.0);
+
+void
+bond_mode_8023ad_conf_get_v22(struct rte_eth_dev *dev,
 		struct rte_eth_bond_8023ad_conf *conf)
 {
 	struct bond_dev_private *internals = dev->data->dev_private;
@@ -1014,10 +1038,15 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	conf->tx_period_ms = mode4->tx_period_timeout / ms_ticks;
 	conf->update_timeout_ms = mode4->update_timeout_us / 1000;
 	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
+	conf->slowrx_cb = mode4->slowrx_cb;
 }
 
+BIND_DEFAULT_SYMBOL(bond_mode_8023ad_conf_get, _v22, 2.2);
+MAP_STATIC_SYMBOL(void bond_mode_8023ad_conf_get(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
+		  bond_mode_8023ad_conf_get_v22);
+
 void
-bond_mode_8023ad_setup(struct rte_eth_dev *dev,
+bond_mode_8023ad_setup_v20(struct rte_eth_dev *dev,
 		struct rte_eth_bond_8023ad_conf *conf)
 {
 	struct rte_eth_bond_8023ad_conf def_conf;
@@ -1047,6 +1076,50 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
 	mode4->update_timeout_us = conf->update_timeout_ms * 1000;
 }
 
+VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
+
+void
+bond_mode_8023ad_setup_v22(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf)
+{
+	struct rte_eth_bond_8023ad_conf def_conf;
+	struct bond_dev_private *internals = dev->data->dev_private;
+	struct mode8023ad_private *mode4 = &internals->mode4;
+	uint64_t ms_ticks = rte_get_tsc_hz() / 1000;
+
+	if (conf == NULL) {
+		conf = &def_conf;
+		conf->fast_periodic_ms = BOND_8023AD_FAST_PERIODIC_MS;
+		conf->slow_periodic_ms = BOND_8023AD_SLOW_PERIODIC_MS;
+		conf->short_timeout_ms = BOND_8023AD_SHORT_TIMEOUT_MS;
+		conf->long_timeout_ms = BOND_8023AD_LONG_TIMEOUT_MS;
+		conf->aggregate_wait_timeout_ms = BOND_8023AD_AGGREGATE_WAIT_TIMEOUT_MS;
+		conf->tx_period_ms = BOND_8023AD_TX_MACHINE_PERIOD_MS;
+		conf->rx_marker_period_ms = BOND_8023AD_RX_MARKER_PERIOD_MS;
+		conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS;
+		conf->slowrx_cb = NULL;
+	}
+
+	bond_mode_8023ad_stop(dev);
+
+	mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks;
+	mode4->slow_periodic_timeout = conf->slow_periodic_ms * ms_ticks;
+	mode4->short_timeout = conf->short_timeout_ms * ms_ticks;
+	mode4->long_timeout = conf->long_timeout_ms * ms_ticks;
+	mode4->aggregate_wait_timeout = conf->aggregate_wait_timeout_ms * ms_ticks;
+	mode4->tx_period_timeout = conf->tx_period_ms * ms_ticks;
+	mode4->rx_marker_timeout = conf->rx_marker_period_ms * ms_ticks;
+	mode4->update_timeout_us = conf->update_timeout_ms * 1000;
+	mode4->slowrx_cb = conf->slowrx_cb;
+
+	if (dev->data->dev_started)
+		bond_mode_8023ad_start(dev);
+}
+
+BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
+MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
+		  bond_mode_8023ad_setup_v22);
+
 int
 bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev)
 {
@@ -1062,6 +1135,13 @@ bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev)
 int
 bond_mode_8023ad_start(struct rte_eth_dev *bond_dev)
 {
+	struct bond_dev_private *internals = bond_dev->data->dev_private;
+	struct mode8023ad_private *mode4 = &internals->mode4;
+
+	if (mode4->slowrx_cb)
+		return rte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000,
+			&bond_mode_8023ad_ext_periodic_cb, bond_dev);
+
 	return rte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000,
 			&bond_mode_8023ad_periodic_cb, bond_dev);
 }
@@ -1069,6 +1149,13 @@ bond_mode_8023ad_start(struct rte_eth_dev *bond_dev)
 void
 bond_mode_8023ad_stop(struct rte_eth_dev *bond_dev)
 {
+	struct bond_dev_private *internals = bond_dev->data->dev_private;
+	struct mode8023ad_private *mode4 = &internals->mode4;
+
+	if (mode4->slowrx_cb) {
+		rte_eal_alarm_cancel(&bond_mode_8023ad_ext_periodic_cb, bond_dev);
+		return;
+	}
 	rte_eal_alarm_cancel(&bond_mode_8023ad_periodic_cb, bond_dev);
 }
 
@@ -1215,3 +1302,152 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
 	info->agg_port_id = port->aggregator_port_id;
 	return 0;
 }
+
+int
+rte_eth_bond_8023ad_ext_collect(uint8_t port_id, uint8_t slave_id, int enabled)
+{
+	struct rte_eth_dev *bond_dev;
+	struct bond_dev_private *internals;
+	struct mode8023ad_private *mode4;
+	struct port *port;
+
+	if (rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
+		return -EINVAL;
+
+	bond_dev = &rte_eth_devices[port_id];
+
+	if (!bond_dev->data->dev_started)
+		return -EINVAL;
+
+	internals = bond_dev->data->dev_private;
+	if (find_slave_by_id(internals->active_slaves,
+			internals->active_slave_count, slave_id) ==
+				internals->active_slave_count)
+		return -EINVAL;
+
+	mode4 = &internals->mode4;
+	if (mode4->slowrx_cb == NULL)
+		return -EINVAL;
+
+	port = &mode_8023ad_ports[slave_id];
+
+	if (enabled)
+		ACTOR_STATE_SET(port, COLLECTING);
+	else
+		ACTOR_STATE_CLR(port, COLLECTING);
+
+	return 0;
+}
+
+int
+rte_eth_bond_8023ad_ext_distrib(uint8_t port_id, uint8_t slave_id, int enabled)
+{
+	struct rte_eth_dev *bond_dev;
+	struct bond_dev_private *internals;
+	struct mode8023ad_private *mode4;
+	struct port *port;
+
+	if (rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
+		return -EINVAL;
+
+	bond_dev = &rte_eth_devices[port_id];
+
+	if (!bond_dev->data->dev_started)
+		return -EINVAL;
+
+	internals = bond_dev->data->dev_private;
+	if (find_slave_by_id(internals->active_slaves,
+			internals->active_slave_count, slave_id) ==
+				internals->active_slave_count)
+		return -EINVAL;
+
+	mode4 = &internals->mode4;
+	if (mode4->slowrx_cb == NULL)
+		return -EINVAL;
+
+	port = &mode_8023ad_ports[slave_id];
+
+	if (enabled)
+		ACTOR_STATE_SET(port, DISTRIBUTING);
+	else
+		ACTOR_STATE_CLR(port, DISTRIBUTING);
+
+	return 0;
+}
+
+int
+rte_eth_bond_8023ad_ext_slowtx(uint8_t port_id, uint8_t slave_id,
+		struct rte_mbuf *lacp_pkt)
+{
+	struct rte_eth_dev *bond_dev;
+	struct bond_dev_private *internals;
+	struct mode8023ad_private *mode4;
+	struct port *port;
+
+	if (rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
+		return -EINVAL;
+
+	bond_dev = &rte_eth_devices[port_id];
+
+	if (!bond_dev->data->dev_started)
+		return -EINVAL;
+
+	internals = bond_dev->data->dev_private;
+	if (find_slave_by_id(internals->active_slaves,
+			internals->active_slave_count, slave_id) ==
+				internals->active_slave_count)
+		return -EINVAL;
+
+	mode4 = &internals->mode4;
+	if (mode4->slowrx_cb == NULL)
+		return -EINVAL;
+
+	port = &mode_8023ad_ports[slave_id];
+
+	if (rte_pktmbuf_pkt_len(lacp_pkt) < sizeof(struct lacpdu_header))
+		return -EINVAL;
+
+	struct lacpdu_header *lacp;
+
+	/* only enqueue LACPDUs */
+	lacp = rte_pktmbuf_mtod(lacp_pkt, struct lacpdu_header *);
+	if (lacp->lacpdu.subtype != SLOW_SUBTYPE_LACP)
+		return -EINVAL;
+
+	MODE4_DEBUG("sending LACP frame\n");
+
+	return rte_ring_enqueue(port->tx_ring, lacp_pkt);
+}
+
+static void
+bond_mode_8023ad_ext_periodic_cb(void *arg)
+{
+	struct rte_eth_dev *bond_dev = arg;
+	struct bond_dev_private *internals = bond_dev->data->dev_private;
+	struct mode8023ad_private *mode4 = &internals->mode4;
+	struct port *port;
+	void *pkt = NULL;
+	uint16_t i, slave_id;
+
+	for (i = 0; i < internals->active_slave_count; i++) {
+		slave_id = internals->active_slaves[i];
+		port = &mode_8023ad_ports[slave_id];
+
+		if (rte_ring_dequeue(port->rx_ring, &pkt) == 0) {
+			struct rte_mbuf *lacp_pkt = pkt;
+			struct lacpdu_header *lacp;
+
+			lacp = rte_pktmbuf_mtod(lacp_pkt,
+						struct lacpdu_header *);
+			RTE_VERIFY(lacp->lacpdu.subtype == SLOW_SUBTYPE_LACP);
+
+			/* This is LACP frame so pass it to rx callback.
+			 * Callback is responsible for freeing mbuf.
+			 */
+			mode4->slowrx_cb(slave_id, lacp_pkt);
+		}
+	}
+
+	rte_eal_alarm_set(internals->mode4.update_timeout_us,
+			bond_mode_8023ad_ext_periodic_cb, arg);
+}
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
index ebd0e93..8cfa3d3 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
@@ -64,6 +64,8 @@ extern "C" {
 #define MARKER_TLV_TYPE_INFO                0x01
 #define MARKER_TLV_TYPE_RESP                0x02
 
+typedef void (*rte_eth_bond_8023ad_ext_slowrx_fn)(uint8_t slave_id, struct rte_mbuf *lacp_pkt);
+
 enum rte_bond_8023ad_selection {
 	UNSELECTED,
 	STANDBY,
@@ -157,6 +159,7 @@ struct rte_eth_bond_8023ad_conf {
 	uint32_t tx_period_ms;
 	uint32_t rx_marker_period_ms;
 	uint32_t update_timeout_ms;
+	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 };
 
 struct rte_eth_bond_8023ad_slave_info {
@@ -219,4 +222,45 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
 }
 #endif
 
+/**
+ * Configure a slave port to start collecting.
+ *
+ * @param port_id	Bonding device id
+ * @param slave_id	Port id of valid slave.
+ * @param enabled	Non-zero when collection enabled.
+ * @return
+ *   0 - if ok
+ *   -EINVAL if slave is not valid.
+ */
+int
+rte_eth_bond_8023ad_ext_collect(uint8_t port_id, uint8_t slave_id, int enabled);
+
+/**
+ * Configure a slave port to start distributing.
+ *
+ * @param port_id	Bonding device id
+ * @param slave_id	Port id of valid slave.
+ * @param enabled	Non-zero when distribution enabled.
+ * @return
+ *   0 - if ok
+ *   -EINVAL if slave is not valid.
+ */
+int
+rte_eth_bond_8023ad_ext_distrib(uint8_t port_id, uint8_t slave_id, int enabled);
+
+/**
+ * LACPDU transmit path for external 802.3ad state machine.  Caller retains
+ * ownership of the packet on failure.
+ *
+ * @param port_id	Bonding device id
+ * @param slave_id	Port ID of valid slave device.
+ * @param lacp_pkt	mbuf containing LACPDU.
+ *
+ * @return
+ *   0 on success, negative value otherwise.
+ */
+int
+rte_eth_bond_8023ad_ext_slowtx(uint8_t port_id, uint8_t slave_id,
+		struct rte_mbuf *lacp_pkt);
+
 #endif /* RTE_ETH_BOND_8023AD_H_ */
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index 8adee70..965ab93 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -39,6 +39,7 @@
 #include <rte_ether.h>
 #include <rte_byteorder.h>
 #include <rte_atomic.h>
+#include <rte_compat.h>
 
 #include "rte_eth_bond_8023ad.h"
 
@@ -173,6 +174,8 @@ struct mode8023ad_private {
 	uint64_t tx_period_timeout;
 	uint64_t rx_marker_timeout;
 	uint64_t update_timeout_us;
+	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
+	uint8_t external_sm;
 };
 
 /**
@@ -211,6 +214,31 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 void
 bond_mode_8023ad_setup(struct rte_eth_dev *dev,
 		struct rte_eth_bond_8023ad_conf *conf);
+void __vsym
+bond_mode_8023ad_setup_v20(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf);
+void __vsym
+bond_mode_8023ad_setup_v22(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf);
+
+/**
+ * @internal
+ *
+ * Retreive mode 4 configuration of bonded interface.
+ *
+ * @param dev Bonded interface
+ * @param conf Bonded interface configuration
+ */
+void
+bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf);
+void __vsym
+bond_mode_8023ad_conf_get_v20(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf);
+void __vsym
+bond_mode_8023ad_conf_get_v22(struct rte_eth_dev *dev,
+		struct rte_eth_bond_8023ad_conf *conf);
+
 
 /**
  * @internal
diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
index 22bd920..7f78717 100644
--- a/drivers/net/bonding/rte_eth_bond_version.map
+++ b/drivers/net/bonding/rte_eth_bond_version.map
@@ -17,6 +17,9 @@ DPDK_2.0 {
 	rte_eth_bond_slaves_get;
 	rte_eth_bond_xmit_policy_get;
 	rte_eth_bond_xmit_policy_set;
+	rte_eth_bond_8023ad_ext_collect;
+	rte_eth_bond_8023ad_ext_distrib;
+	rte_eth_bond_8023ad_ext_slowtx;
 
 	local: *;
 };
@@ -27,3 +30,10 @@ DPDK_2.1 {
 	rte_eth_bond_free;
 
 } DPDK_2.0;
+
+DPDK_2.2 {
+	local:
+
+	bond_mode_8023ad_conf_get;
+	bond_mode_8023ad_setup;
+} DPDK_2.1;
-- 
1.7.10.4

[dpdk-dev] [PATCH v6 4/4] bond mode 4: tests for external state machine

[Eric Kinzie]

From: Eric Kinzie <ekinzie@brocade.com>

  This adds test cases for exercising the external state machine API to
  the mode 4 autotest.

Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
---
 app/test/test_link_bonding_mode4.c |  210 ++++++++++++++++++++++++++++++++++--
 1 file changed, 201 insertions(+), 9 deletions(-)

diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index d785393..6a459cd 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -151,6 +151,8 @@ static struct rte_eth_conf default_pmd_conf = {
 	.lpbk_mode = 0,
 };
 
+static uint8_t lacpdu_rx_count[RTE_MAX_ETHPORTS] = {0, };
+
 #define FOR_EACH(_i, _item, _array, _size) \
 	for (_i = 0, _item = &_array[0]; _i < _size && (_item = &_array[_i]); _i++)
 
@@ -320,8 +322,26 @@ remove_slave(struct slave_conf *slave)
 	return 0;
 }
 
+static void
+lacp_recv_cb(uint8_t slave_id, struct rte_mbuf *lacp_pkt)
+{
+	struct ether_hdr *hdr;
+	struct slow_protocol_frame *slow_hdr;
+
+	RTE_VERIFY(lacp_pkt != NULL);
+
+	hdr = rte_pktmbuf_mtod(lacp_pkt, struct ether_hdr *);
+	RTE_VERIFY(hdr->ether_type == rte_cpu_to_be_16(ETHER_TYPE_SLOW));
+
+	slow_hdr = rte_pktmbuf_mtod(lacp_pkt, struct slow_protocol_frame *);
+	RTE_VERIFY(slow_hdr->slow_protocol.subtype == SLOW_SUBTYPE_LACP);
+
+	lacpdu_rx_count[slave_id]++;
+	rte_pktmbuf_free(lacp_pkt);
+}
+
 static int
-initialize_bonded_device_with_slaves(uint8_t slave_count, uint8_t start)
+initialize_bonded_device_with_slaves(uint8_t slave_count, uint8_t external_sm)
 {
 	uint8_t i;
 
@@ -337,9 +357,17 @@ initialize_bonded_device_with_slaves(uint8_t slave_count, uint8_t start)
 	rte_eth_bond_8023ad_setup(test_params.bonded_port_id, NULL);
 	rte_eth_promiscuous_disable(test_params.bonded_port_id);
 
-	if (start)
-		TEST_ASSERT_SUCCESS(rte_eth_dev_start(test_params.bonded_port_id),
-			"Failed to start bonded device");
+	if (external_sm) {
+		struct rte_eth_bond_8023ad_conf conf;
+
+		rte_eth_bond_8023ad_conf_get(test_params.bonded_port_id, &conf);
+		conf.slowrx_cb = lacp_recv_cb;
+		rte_eth_bond_8023ad_setup(test_params.bonded_port_id, &conf);
+
+	}
+
+	TEST_ASSERT_SUCCESS(rte_eth_dev_start(test_params.bonded_port_id),
+		"Failed to start bonded device");
 
 	return TEST_SUCCESS;
 }
@@ -642,7 +670,7 @@ test_mode4_lacp(void)
 {
 	int retval;
 
-	retval = initialize_bonded_device_with_slaves(TEST_LACP_SLAVE_COUT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_LACP_SLAVE_COUT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	/* Test LACP handshake function */
@@ -740,7 +768,7 @@ test_mode4_rx(void)
 	struct ether_addr dst_mac;
 	struct ether_addr bonded_mac;
 
-	retval = initialize_bonded_device_with_slaves(TEST_PROMISC_SLAVE_COUNT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_PROMISC_SLAVE_COUNT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	retval = bond_handshake();
@@ -917,7 +945,7 @@ test_mode4_tx_burst(void)
 	struct ether_addr dst_mac = { { 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 } };
 	struct ether_addr bonded_mac;
 
-	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	retval = bond_handshake();
@@ -1101,7 +1129,7 @@ test_mode4_marker(void)
 	uint8_t i, j;
 	const uint16_t ethtype_slow_be = rte_be_to_cpu_16(ETHER_TYPE_SLOW);
 
-	retval = initialize_bonded_device_with_slaves(TEST_MARKER_SLAVE_COUT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_MARKER_SLAVE_COUT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	/* Test LACP handshake function */
@@ -1186,7 +1214,7 @@ test_mode4_expired(void)
 
 	struct rte_eth_bond_8023ad_conf conf;
 
-	retval = initialize_bonded_device_with_slaves(TEST_EXPIRED_SLAVE_COUNT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_EXPIRED_SLAVE_COUNT, 0);
 	/* Set custom timeouts to make test last shorter. */
 	rte_eth_bond_8023ad_conf_get(test_params.bonded_port_id, &conf);
 	conf.fast_periodic_ms = 100;
@@ -1268,6 +1296,156 @@ test_mode4_expired(void)
 }
 
 static int
+test_mode4_ext_ctrl(void)
+{
+	/*
+	 * configure bonded interface without the external sm enabled
+	 *   . try to transmit lacpdu (should fail)
+	 *   . try to set collecting and distributing flags (should fail)
+	 * reconfigure w/external sm
+	 *   . transmit one lacpdu on each slave using new api
+	 *   . make sure each slave receives one lacpdu using the callback api
+	 *   . transmit one data pdu on each slave (should fail)
+	 *   . enable distribution and collection, send one data pdu each again
+	 */
+
+	int retval;
+	struct slave_conf *slave = NULL;
+	uint8_t i;
+
+	struct rte_mbuf *lacp_tx_buf[SLAVE_COUNT];
+	struct ether_addr src_mac, dst_mac;
+	struct lacpdu_header lacpdu = {
+		.lacpdu = {
+			.subtype = SLOW_SUBTYPE_LACP,
+		},
+	};
+
+	ether_addr_copy(&parnter_system, &src_mac);
+	ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
+
+	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
+			      ETHER_TYPE_SLOW, 0, 0);
+
+	for (i = 0; i < SLAVE_COUNT; i++) {
+		lacp_tx_buf[i] = rte_pktmbuf_alloc(test_params.mbuf_pool);
+		rte_memcpy(rte_pktmbuf_mtod(lacp_tx_buf[i], char *),
+			   &lacpdu, sizeof(lacpdu));
+		rte_pktmbuf_pkt_len(lacp_tx_buf[i]) = sizeof(lacpdu);
+	}
+
+	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 0);
+	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
+
+	FOR_EACH_SLAVE(i, slave) {
+		TEST_ASSERT_FAIL(rte_eth_bond_8023ad_ext_slowtx(
+						test_params.bonded_port_id,
+						slave->port_id, lacp_tx_buf[i]),
+				 "Slave should not allow manual LACP xmit");
+		TEST_ASSERT_FAIL(rte_eth_bond_8023ad_ext_collect(
+						test_params.bonded_port_id,
+						slave->port_id, 1),
+				 "Slave should not allow external state controls");
+	}
+
+	free_pkts(lacp_tx_buf, RTE_DIM(lacp_tx_buf));
+
+	retval = remove_slaves_and_stop_bonded_device();
+	TEST_ASSERT_SUCCESS(retval, "Bonded device cleanup failed.");
+
+	return TEST_SUCCESS;
+}
+
+
+static int
+test_mode4_ext_lacp(void)
+{
+	int retval;
+	struct slave_conf *slave = NULL;
+	uint8_t all_slaves_done = 0, i;
+	uint16_t nb_pkts;
+	const unsigned delay = bond_get_update_timeout_ms();
+
+	struct rte_mbuf *lacp_tx_buf[SLAVE_COUNT];
+	struct rte_mbuf *buf[SLAVE_COUNT];
+	struct ether_addr src_mac, dst_mac;
+	struct lacpdu_header lacpdu = {
+		.lacpdu = {
+			.subtype = SLOW_SUBTYPE_LACP,
+		},
+	};
+
+	ether_addr_copy(&parnter_system, &src_mac);
+	ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
+
+	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
+			      ETHER_TYPE_SLOW, 0, 0);
+
+	for (i = 0; i < SLAVE_COUNT; i++) {
+		lacp_tx_buf[i] = rte_pktmbuf_alloc(test_params.mbuf_pool);
+		rte_memcpy(rte_pktmbuf_mtod(lacp_tx_buf[i], char *),
+			   &lacpdu, sizeof(lacpdu));
+		rte_pktmbuf_pkt_len(lacp_tx_buf[i]) = sizeof(lacpdu);
+	}
+
+	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 1);
+	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
+
+	memset(lacpdu_rx_count, 0, sizeof(lacpdu_rx_count));
+
+	/* Wait for new settings to be applied. */
+	for (i = 0; i < 30; ++i)
+		rte_delay_ms(delay);
+
+	FOR_EACH_SLAVE(i, slave) {
+		retval = rte_eth_bond_8023ad_ext_slowtx(
+						test_params.bonded_port_id,
+						slave->port_id, lacp_tx_buf[i]);
+		TEST_ASSERT_SUCCESS(retval,
+				    "Slave should allow manual LACP xmit");
+	}
+
+	nb_pkts = bond_tx(NULL, 0);
+	TEST_ASSERT_EQUAL(nb_pkts, 0, "Packets transmitted unexpectedly");
+
+	FOR_EACH_SLAVE(i, slave) {
+		nb_pkts = slave_get_pkts(slave, buf, RTE_DIM(buf));
+		TEST_ASSERT_EQUAL(nb_pkts, 1, "found %u packets on slave %d\n",
+				  nb_pkts, i);
+		slave_put_pkts(slave, buf, nb_pkts);
+	}
+
+	nb_pkts = bond_rx(buf, RTE_DIM(buf));
+	free_pkts(buf, nb_pkts);
+	TEST_ASSERT_EQUAL(nb_pkts, 0, "Packets received unexpectedly");
+
+	/* wait for the periodic callback to run */
+	for (i = 0; i < 30 && all_slaves_done == 0; ++i) {
+		uint8_t s, total = 0;
+
+		rte_delay_ms(delay);
+		FOR_EACH_SLAVE(s, slave) {
+			total += lacpdu_rx_count[slave->port_id];
+		}
+
+		if (total >= SLAVE_COUNT)
+			all_slaves_done = 1;
+	}
+
+	FOR_EACH_SLAVE(i, slave) {
+		TEST_ASSERT_EQUAL(lacpdu_rx_count[slave->port_id], 1,
+				  "Slave port %u should have received 1 lacpdu (count=%u)",
+				  slave->port_id,
+				  lacpdu_rx_count[slave->port_id]);
+	}
+
+	retval = remove_slaves_and_stop_bonded_device();
+	TEST_ASSERT_SUCCESS(retval, "Test cleanup failed.");
+
+	return TEST_SUCCESS;
+}
+
+static int
 check_environment(void)
 {
 	struct slave_conf *port;
@@ -1383,6 +1561,18 @@ test_mode4_expired_wrapper(void)
 	return test_mode4_executor(&test_mode4_expired);
 }
 
+static int
+test_mode4_ext_ctrl_wrapper(void)
+{
+	return test_mode4_executor(&test_mode4_ext_ctrl);
+}
+
+static int
+test_mode4_ext_lacp_wrapper(void)
+{
+	return test_mode4_executor(&test_mode4_ext_lacp);
+}
+
 static struct unit_test_suite link_bonding_mode4_test_suite  = {
 	.suite_name = "Link Bonding mode 4 Unit Test Suite",
 	.setup = test_setup,
@@ -1393,6 +1583,8 @@ static struct unit_test_suite link_bonding_mode4_test_suite  = {
 		TEST_CASE_NAMED("test_mode4_tx_burst", test_mode4_tx_burst_wrapper),
 		TEST_CASE_NAMED("test_mode4_marker", test_mode4_marker_wrapper),
 		TEST_CASE_NAMED("test_mode4_expired", test_mode4_expired_wrapper),
+		TEST_CASE_NAMED("test_mode4_ext_ctrl", test_mode4_ext_ctrl_wrapper),
+		TEST_CASE_NAMED("test_mode4_ext_lacp", test_mode4_ext_lacp_wrapper),
 		{ NULL, NULL, NULL, NULL, NULL } /**< NULL terminate unit test array */
 	}
 };
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH v6 0/4] bonding corrections and additions

[Declan Doherty]

On 19/10/15 16:36, Eric Kinzie wrote:
> This patchset makes a couple of small corrections to the bonding driver
> and introduces the ability to use an external state machine for mode
> 4 operation.
>
> Changes in v2:
>    . eliminate external_sm field in 802.3ad configuration
>      (rte_eth_bond_8023ad_conf).
>    . stop bonding device before changing the periodic callback function.
>      start again if needed.
>    . remove unnecessary calls to valid_bonded_port_id().
>    . do not check for NULL tx_ring.
>    . return error in rte_eth_bond_8023ad_ext_slowtx() if packet is not LACP.
>    . remove check for external sm configuration in periodic callback
>    . check for valid LACPDU in test application's rx callback
>    . add "Fixes:" tags
>
> Changes in v3:
>    . update rte_eth_bond_version.map
>
> Changes in v4:
>    . version functions that modify slowrx_cb
>
> Changes in v5:
>    . remove "__vsym" from source files and use updated symbol version macros
>
> Changes in v6:
>    . updated symbols have version 2.2
>    . specify versioned local symbol names in map so they don't become exported
>
> Eric Kinzie (4):
>    bond mode 4: copy entire config structure
>    bond mode 4: do not ignore multicast
>    bond mode 4: allow external state machine
>    bond mode 4: tests for external state machine
>
>   app/test/test_link_bonding_mode4.c                |  217 ++++++++++++++++++-
>   drivers/net/bonding/Makefile                      |    2 +-
>   drivers/net/bonding/rte_eth_bond_8023ad.c         |  241 ++++++++++++++++++++-
>   drivers/net/bonding/rte_eth_bond_8023ad.h         |   44 ++++
>   drivers/net/bonding/rte_eth_bond_8023ad_private.h |   28 +++
>   drivers/net/bonding/rte_eth_bond_pmd.c            |    1 +
>   drivers/net/bonding/rte_eth_bond_version.map      |   10 +
>   7 files changed, 529 insertions(+), 14 deletions(-)
>

Series Acked-by: Declan Doherty <declan.doherty@intel.com>

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Thomas Monjalon]

2015-10-19 08:36, Eric Kinzie:
>   Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>   and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>   functions.
[...]
> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
[...]
> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
> +		  bond_mode_8023ad_setup_v22);

I'm sorry it doesn't work well when trying to build a combined lib:

ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2

The symbols are OK in the .o file:
0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup

I don't understand the problem and I am considering disabling versioning in
combined library.

Any idea?

[dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

Fixes following error:
  LD libdpdk.so
  /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
  for symbol <function>@DPDK_x.y

Defines version symbols in a fixed path libdpdk.map file and this
value hardcoded into makefile

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/libdpdk.map    | 12 ++++++++++++
 mk/rte.sharelib.mk |  5 +++--
 2 files changed, 15 insertions(+), 2 deletions(-)
 create mode 100644 lib/libdpdk.map

diff --git a/lib/libdpdk.map b/lib/libdpdk.map
new file mode 100644
index 0000000..3988a3f
--- /dev/null
+++ b/lib/libdpdk.map
@@ -0,0 +1,12 @@
+DPDK_2.0 {
+
+};
+
+DPDK_2.1 {
+
+} DPDK_2.0;
+
+DPDK_2.2 {
+
+} DPDK_2.1;
+
diff --git a/mk/rte.sharelib.mk b/mk/rte.sharelib.mk
index 7bb7219..e206ce1 100644
--- a/mk/rte.sharelib.mk
+++ b/mk/rte.sharelib.mk
@@ -40,6 +40,7 @@ LIB_ONE := lib$(RTE_LIBNAME).so
 else
 LIB_ONE := lib$(RTE_LIBNAME).a
 endif
+SHARED_LDFLAGS += --version-script=./lib/libdpdk.map
 endif
 
 .PHONY:sharelib
@@ -50,10 +51,10 @@ OBJS = $(wildcard $(RTE_OUTPUT)/build/lib/*.o)
 ifeq ($(LINK_USING_CC),1)
 # Override the definition of LD here, since we're linking with CC
 LD := $(CC) $(CPU_CFLAGS)
-O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
+O_TO_S = $(LD) $(call linkerprefix,$(SHARED_LDFLAGS)) \
 	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
 else
-O_TO_S = $(LD) $(CPU_LDFLAGS) \
+O_TO_S = $(LD) $(SHARED_LDFLAGS) \
 	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
 endif
 
-- 
1.9.3

Re: [dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Thomas Monjalon]

2015-11-01 23:17, Ferruh Yigit:
> Fixes following error:
>   LD libdpdk.so
>   /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
>   for symbol <function>@DPDK_x.y

> --- a/mk/rte.sharelib.mk
> +++ b/mk/rte.sharelib.mk
> @@ -40,6 +40,7 @@ LIB_ONE := lib$(RTE_LIBNAME).so
>  else
>  LIB_ONE := lib$(RTE_LIBNAME).a
>  endif
> +SHARED_LDFLAGS += --version-script=./lib/libdpdk.map
>  endif

It looks to be the right thing to do.

>  ifeq ($(LINK_USING_CC),1)
>  # Override the definition of LD here, since we're linking with CC
>  LD := $(CC) $(CPU_CFLAGS)
> -O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
> +O_TO_S = $(LD) $(call linkerprefix,$(SHARED_LDFLAGS)) \
>  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
>  else
> -O_TO_S = $(LD) $(CPU_LDFLAGS) \
> +O_TO_S = $(LD) $(SHARED_LDFLAGS) \
>  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
>  endif

Why do you remove CPU_LDFLAGS?

Re: [dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

On Mon, Nov 02, 2015 at 12:24:51AM +0100, Thomas Monjalon wrote:
> >  ifeq ($(LINK_USING_CC),1)
> >  # Override the definition of LD here, since we're linking with CC
> >  LD := $(CC) $(CPU_CFLAGS)
> > -O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
> > +O_TO_S = $(LD) $(call linkerprefix,$(SHARED_LDFLAGS)) \
> >  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
> >  else
> > -O_TO_S = $(LD) $(CPU_LDFLAGS) \
> > +O_TO_S = $(LD) $(SHARED_LDFLAGS) \
> >  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
> >  endif
> 
> Why do you remove CPU_LDFLAGS?

CPU_LDFLAGS shared by other libraries and confilicts with this usage.

Re: [dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Thomas Monjalon]

2015-11-02 00:05, Ferruh Yigit:
> On Mon, Nov 02, 2015 at 12:24:51AM +0100, Thomas Monjalon wrote:
> > >  ifeq ($(LINK_USING_CC),1)
> > >  # Override the definition of LD here, since we're linking with CC
> > >  LD := $(CC) $(CPU_CFLAGS)
> > > -O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
> > > +O_TO_S = $(LD) $(call linkerprefix,$(SHARED_LDFLAGS)) \
> > >  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
> > >  else
> > > -O_TO_S = $(LD) $(CPU_LDFLAGS) \
> > > +O_TO_S = $(LD) $(SHARED_LDFLAGS) \
> > >  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
> > >  endif
> > 
> > Why do you remove CPU_LDFLAGS?
> 
> CPU_LDFLAGS shared by other libraries and confilicts with this usage.

Sorry I don't understand.
CPU_LDFLAGS is -melf_i386 for Intel 32-bit.
Why removing it for combined library? What is the conflict?

Re: [dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

On Mon, Nov 02, 2015 at 08:10:25AM +0100, Thomas Monjalon wrote:
> 2015-11-02 00:05, Ferruh Yigit:
> > On Mon, Nov 02, 2015 at 12:24:51AM +0100, Thomas Monjalon wrote:
> > > >  ifeq ($(LINK_USING_CC),1)
> > > >  # Override the definition of LD here, since we're linking with CC
> > > >  LD := $(CC) $(CPU_CFLAGS)
> > > > -O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
> > > > +O_TO_S = $(LD) $(call linkerprefix,$(SHARED_LDFLAGS)) \
> > > >  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
> > > >  else
> > > > -O_TO_S = $(LD) $(CPU_LDFLAGS) \
> > > > +O_TO_S = $(LD) $(SHARED_LDFLAGS) \
> > > >  	-shared $(OBJS) -o $(RTE_OUTPUT)/lib/$(LIB_ONE)
> > > >  endif
> > > 
> > > Why do you remove CPU_LDFLAGS?
> > 
> > CPU_LDFLAGS shared by other libraries and confilicts with this usage.
> 
> Sorry I don't understand.
> CPU_LDFLAGS is -melf_i386 for Intel 32-bit.
> Why removing it for combined library? What is the conflict?

When combined shared option enabled, CPP_LDFLAGS will be processed and other libraries
CPU_LDFLAGS value will contain this value, and generate an compilation eroor like following:

"
gcc -m64 -Wl,--version-script=./lib/libdpdk.map -Wl,--version-script=/root/dpdk/lib/librte_eal/linuxapp/eal/rte_eal_version.map -shared <objs> -Wl,-soname,librte_eal.so.2.1 -o librte_eal.so.2.1
/usr/bin/ld: cannot open linker script file ./lib/libdpdk.map: No such file or directory
collect2: error: ld returned 1 exit status
"

That is why I have created a new variable, and CPU_LDFLAGS was empty,
but with double check what CPU_LDFLAGS is, it should stay there,

I will update the patch, at worst I can add a new flag instead of replacing CPU_LDFLAGS

Regards,
ferruh

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Panu Matilainen]

On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
> 2015-10-19 08:36, Eric Kinzie:
>>    Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>    and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>    functions.
> [...]
>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
> [...]
>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>> +		  bond_mode_8023ad_setup_v22);
>
> I'm sorry it doesn't work well when trying to build a combined lib:
>
> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>
> The symbols are OK in the .o file:
> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>
> I don't understand the problem and I am considering disabling versioning in
> combined library.
>
> Any idea?
>

The .map additions look incorrect to me:

> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
> index 22bd920..7f78717 100644
> --- a/drivers/net/bonding/rte_eth_bond_version.map
> +++ b/drivers/net/bonding/rte_eth_bond_version.map
> @@ -17,6 +17,9 @@  DPDK_2.0 {
>  	rte_eth_bond_slaves_get;
>  	rte_eth_bond_xmit_policy_get;
>  	rte_eth_bond_xmit_policy_set;
> +	rte_eth_bond_8023ad_ext_collect;
> +	rte_eth_bond_8023ad_ext_distrib;
> +	rte_eth_bond_8023ad_ext_slowtx;

These symbols didn't exist in DPDK 2.0 but are only being added here. So 
why are they being added to the 2.0 section?

>
>  	local: *;
>  };
> @@ -27,3 +30,10 @@  DPDK_2.1 {
>  	rte_eth_bond_free;
>
>  } DPDK_2.0;
> +
> +DPDK_2.2 {
> +	local
> +
> +	bond_mode_8023ad_conf_get;
> +	bond_mode_8023ad_setup;
> +} DPDK_2.1;

These are marked local, as in, "not exported" which doesn't seem right. 
Also they're lacking the rte_eth_ prefix. AFAICS this is what the symbol 
export map should look like here:

DPDK_2.2 {
     global:

     rte_eth_bond_8023ad_ext_collect;
     rte_eth_bond_8023ad_ext_distrib;
     rte_eth_bond_8023ad_ext_slowtx;
     rte_eth_bond_mode_8023ad_conf_get;
     rte_eth_bond_mode_8023ad_setup;
} DPDK_2.1;

That said, problems with symbol versioning and the combined library were 
predicted from the start, once the individual library versions start 
advancing. For one, the combined library itself is not versioned at all 
it is offending the ABI policy all the time.

Replacing the library with a linker script, as suggested before by Neil 
Horman and Sergio Gonzales Monroy would eliminate these problems.

	- Panu -

[dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

Fixes following error:
  LD libdpdk.so
  /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
  for symbol <function>@DPDK_x.y

Defines version symbols in a fixed path libdpdk.map file and this
value hardcoded into makefile

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/Makefile |  3 +++
 lib/Makefile         |  3 +++
 lib/libdpdk.map      | 12 ++++++++++++
 mk/rte.sdkbuild.mk   |  2 +-
 mk/rte.sharelib.mk   |  1 +
 5 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 lib/libdpdk.map

diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 6da1ce2..d30018c 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -50,5 +50,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
 DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
 DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
 
+ifeq ($(COMBINED_BUILD),1)
 include $(RTE_SDK)/mk/rte.sharelib.mk
+endif
+
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/lib/Makefile b/lib/Makefile
index 9727b83..33d76a6 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -62,5 +62,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_KNI) += librte_kni
 DIRS-$(CONFIG_RTE_LIBRTE_IVSHMEM) += librte_ivshmem
 endif
 
+ifeq ($(COMBINED_BUILD),1)
 include $(RTE_SDK)/mk/rte.sharelib.mk
+endif
+
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/lib/libdpdk.map b/lib/libdpdk.map
new file mode 100644
index 0000000..3988a3f
--- /dev/null
+++ b/lib/libdpdk.map
@@ -0,0 +1,12 @@
+DPDK_2.0 {
+
+};
+
+DPDK_2.1 {
+
+} DPDK_2.0;
+
+DPDK_2.2 {
+
+} DPDK_2.1;
+
diff --git a/mk/rte.sdkbuild.mk b/mk/rte.sdkbuild.mk
index 38ec7bd..d4e3abf 100644
--- a/mk/rte.sdkbuild.mk
+++ b/mk/rte.sdkbuild.mk
@@ -94,7 +94,7 @@ $(ROOTDIRS-y):
 	@echo "== Build $@"
 	$(Q)$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile -C $(BUILDDIR)/$@ all
 	@if [ $@ = drivers -a $(CONFIG_RTE_BUILD_COMBINE_LIBS) = y ]; then \
-		$(MAKE) -f $(RTE_SDK)/lib/Makefile sharelib; \
+		COMBINED_BUILD=1 $(MAKE) -f $(RTE_SDK)/lib/Makefile sharelib; \
 	fi
 
 %_clean:
diff --git a/mk/rte.sharelib.mk b/mk/rte.sharelib.mk
index 7bb7219..1f71fcb 100644
--- a/mk/rte.sharelib.mk
+++ b/mk/rte.sharelib.mk
@@ -40,6 +40,7 @@ LIB_ONE := lib$(RTE_LIBNAME).so
 else
 LIB_ONE := lib$(RTE_LIBNAME).a
 endif
+CPU_LDFLAGS += --version-script=$(SRCDIR)/lib/libdpdk.map
 endif
 
 .PHONY:sharelib
-- 
1.9.3

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Thomas Monjalon]

2015-11-02 15:23, Ferruh Yigit:
> +ifeq ($(COMBINED_BUILD),1)
>  include $(RTE_SDK)/mk/rte.sharelib.mk
> +endif

Why this ifeq?
rte.sharelib.mk is always used for combined lib.

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Eric Kinzie]

On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
> >2015-10-19 08:36, Eric Kinzie:
> >>   Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
> >>   and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
> >>   functions.
> >[...]
> >>+VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
> >[...]
> >>+BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>+MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
> >>+		  bond_mode_8023ad_setup_v22);
> >
> >I'm sorry it doesn't work well when trying to build a combined lib:
> >
> >ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
> >
> >The symbols are OK in the .o file:
> >0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
> >0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
> >0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
> >0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
> >0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
> >
> >I don't understand the problem and I am considering disabling versioning in
> >combined library.
> >
> >Any idea?
> >
> 
> The .map additions look incorrect to me:
> 
> >diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
> >index 22bd920..7f78717 100644
> >--- a/drivers/net/bonding/rte_eth_bond_version.map
> >+++ b/drivers/net/bonding/rte_eth_bond_version.map
> >@@ -17,6 +17,9 @@  DPDK_2.0 {
> > 	rte_eth_bond_slaves_get;
> > 	rte_eth_bond_xmit_policy_get;
> > 	rte_eth_bond_xmit_policy_set;
> >+	rte_eth_bond_8023ad_ext_collect;
> >+	rte_eth_bond_8023ad_ext_distrib;
> >+	rte_eth_bond_8023ad_ext_slowtx;
> 
> These symbols didn't exist in DPDK 2.0 but are only being added
> here. So why are they being added to the 2.0 section?

Yes, I think these should probably be moved.


> >
> > 	local: *;
> > };
> >@@ -27,3 +30,10 @@  DPDK_2.1 {
> > 	rte_eth_bond_free;
> >
> > } DPDK_2.0;
> >+
> >+DPDK_2.2 {
> >+	local
> >+
> >+	bond_mode_8023ad_conf_get;
> >+	bond_mode_8023ad_setup;
> >+} DPDK_2.1;
> 
> These are marked local, as in, "not exported" which doesn't seem
> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
> the symbol export map should look like here:


These were not exported to begin with.  But after versioning these
functions, they are exported unless explicitly declared to be local here.


> DPDK_2.2 {
>     global:
> 
>     rte_eth_bond_8023ad_ext_collect;
>     rte_eth_bond_8023ad_ext_distrib;
>     rte_eth_bond_8023ad_ext_slowtx;
>     rte_eth_bond_mode_8023ad_conf_get;
>     rte_eth_bond_mode_8023ad_setup;
> } DPDK_2.1;
> 
> That said, problems with symbol versioning and the combined library
> were predicted from the start, once the individual library versions
> start advancing. For one, the combined library itself is not
> versioned at all it is offending the ABI policy all the time.
> 
> Replacing the library with a linker script, as suggested before by
> Neil Horman and Sergio Gonzales Monroy would eliminate these
> problems.
> 
> 	- Panu -
> 
> 

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

On Mon, Nov 02, 2015 at 05:26:21PM +0100, Thomas Monjalon wrote:
> 2015-11-02 15:23, Ferruh Yigit:
> > +ifeq ($(COMBINED_BUILD),1)
> >  include $(RTE_SDK)/mk/rte.sharelib.mk
> > +endif
> 
> Why this ifeq?
> rte.sharelib.mk is always used for combined lib.
> 

When combine lib config selected, other libraries still build
And all libraris include sharelib.mk
This cause CPU_LDFLAGS conflict, the CPU_LDFLAGS set specially for combined lib used by all libraries.

When not linking combined library, there is no need/benefit of including sharelib.mk
COMBINED_BUILD only set when linking combined library and that is when sharelib.mk included

I can try to prevent linking other libraries when combined library enabled, then sharelib.mk included only for combined lib, and we can remove that check.
But I am not sure how hard it is, and does it worth spending extra effort to it?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Panu Matilainen]

On 11/02/2015 06:42 PM, Eric Kinzie wrote:
> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
>>> 2015-10-19 08:36, Eric Kinzie:
>>>>    Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>>>    and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>>>    functions.
>>> [...]
>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
>>> [...]
>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>>>> +		  bond_mode_8023ad_setup_v22);
>>>
>>> I'm sorry it doesn't work well when trying to build a combined lib:
>>>
>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>>>
>>> The symbols are OK in the .o file:
>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>>>
>>> I don't understand the problem and I am considering disabling versioning in
>>> combined library.
>>>
>>> Any idea?
>>>
>>
>> The .map additions look incorrect to me:
>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
>>> index 22bd920..7f78717 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
>>> 	rte_eth_bond_slaves_get;
>>> 	rte_eth_bond_xmit_policy_get;
>>> 	rte_eth_bond_xmit_policy_set;
>>> +	rte_eth_bond_8023ad_ext_collect;
>>> +	rte_eth_bond_8023ad_ext_distrib;
>>> +	rte_eth_bond_8023ad_ext_slowtx;
>>
>> These symbols didn't exist in DPDK 2.0 but are only being added
>> here. So why are they being added to the 2.0 section?
>
> Yes, I think these should probably be moved.
>
>
>>>
>>> 	local: *;
>>> };
>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
>>> 	rte_eth_bond_free;
>>>
>>> } DPDK_2.0;
>>> +
>>> +DPDK_2.2 {
>>> +	local
>>> +
>>> +	bond_mode_8023ad_conf_get;
>>> +	bond_mode_8023ad_setup;
>>> +} DPDK_2.1;
>>
>> These are marked local, as in, "not exported" which doesn't seem
>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
>> the symbol export map should look like here:
>
>
> These were not exported to begin with.  But after versioning these
> functions, they are exported unless explicitly declared to be local here.
>

And this does not ring any warning bells? :)

Sorry, I was not looking at the patch as a whole. You're declaring these 
symbols as exported with the versioning macros, eg

BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);

...and then explicitly telling it to not export them by declaring local, 
and then we wonder why it has trouble finding the symbols.
The versioning macros wont invent the librte_ prefix for you, you need 
to rename the functions accordingly.

But all this versioning gymnastics is moot anyway because you declare 
the ABI incompatible:

-LIBABIVER := 1
+LIBABIVER := 2

This changes the library soname, so no binary compiled against the 
previous version can possibly use it anymore. As in, by definition there 
can be no callers of the _v20 variants after this ABI version bump.

	- Panu -

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Panu Matilainen]

On 11/02/2015 05:23 PM, Ferruh Yigit wrote:
> Fixes following error:
>    LD libdpdk.so
>    /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
>    for symbol <function>@DPDK_x.y
>
> Defines version symbols in a fixed path libdpdk.map file and this
> value hardcoded into makefile

Hold on. What problem is this supposed to be fixing? If its for the 
bonding thing (as it'd appear from the threading), NAK. The symbol 
versioning in that patch is confused needs to be fixed there instead of 
inventing strange workarounds elsewhere.

	- Panu -

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Thomas Monjalon]

2015-11-03 08:48, Panu Matilainen:
> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
> > On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
> >> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
> >>> 2015-10-19 08:36, Eric Kinzie:
> >>>>    Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
> >>>>    and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
> >>>>    functions.
> >>> [...]
> >>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
> >>> [...]
> >>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
> >>>> +		  bond_mode_8023ad_setup_v22);
> >>>
> >>> I'm sorry it doesn't work well when trying to build a combined lib:
> >>>
> >>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
> >>>
> >>> The symbols are OK in the .o file:
> >>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
> >>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
> >>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
> >>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
> >>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
> >>>
> >>> I don't understand the problem and I am considering disabling versioning in
> >>> combined library.
> >>>
> >>> Any idea?
> >>>
> >>
> >> The .map additions look incorrect to me:
> >>
> >>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
> >>> index 22bd920..7f78717 100644
> >>> --- a/drivers/net/bonding/rte_eth_bond_version.map
> >>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
> >>> @@ -17,6 +17,9 @@  DPDK_2.0 {
> >>> 	rte_eth_bond_slaves_get;
> >>> 	rte_eth_bond_xmit_policy_get;
> >>> 	rte_eth_bond_xmit_policy_set;
> >>> +	rte_eth_bond_8023ad_ext_collect;
> >>> +	rte_eth_bond_8023ad_ext_distrib;
> >>> +	rte_eth_bond_8023ad_ext_slowtx;
> >>
> >> These symbols didn't exist in DPDK 2.0 but are only being added
> >> here. So why are they being added to the 2.0 section?
> >
> > Yes, I think these should probably be moved.
> >
> >
> >>>
> >>> 	local: *;
> >>> };
> >>> @@ -27,3 +30,10 @@  DPDK_2.1 {
> >>> 	rte_eth_bond_free;
> >>>
> >>> } DPDK_2.0;
> >>> +
> >>> +DPDK_2.2 {
> >>> +	local
> >>> +
> >>> +	bond_mode_8023ad_conf_get;
> >>> +	bond_mode_8023ad_setup;
> >>> +} DPDK_2.1;
> >>
> >> These are marked local, as in, "not exported" which doesn't seem
> >> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
> >> the symbol export map should look like here:
> >
> >
> > These were not exported to begin with.  But after versioning these
> > functions, they are exported unless explicitly declared to be local here.
> >
> 
> And this does not ring any warning bells? :)
> 
> Sorry, I was not looking at the patch as a whole. You're declaring these 
> symbols as exported with the versioning macros, eg
> 
> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> 
> ...and then explicitly telling it to not export them by declaring local, 
> and then we wonder why it has trouble finding the symbols.
> The versioning macros wont invent the librte_ prefix for you, you need 
> to rename the functions accordingly.
> 
> But all this versioning gymnastics is moot anyway because you declare 
> the ABI incompatible:
> 
> -LIBABIVER := 1
> +LIBABIVER := 2
> 
> This changes the library soname, so no binary compiled against the 
> previous version can possibly use it anymore. As in, by definition there 
> can be no callers of the _v20 variants after this ABI version bump.

Panu, thanks for your careful review.
We must remove the private functions from the export map.and not bump LIBABIVER.

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Panu Matilainen]

On 11/03/2015 10:16 AM, Thomas Monjalon wrote:
> 2015-11-03 08:48, Panu Matilainen:
>> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
>>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
>>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
>>>>> 2015-10-19 08:36, Eric Kinzie:
>>>>>>     Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>>>>>     and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>>>>>     functions.
>>>>> [...]
>>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
>>>>> [...]
>>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>>>>>> +		  bond_mode_8023ad_setup_v22);
>>>>>
>>>>> I'm sorry it doesn't work well when trying to build a combined lib:
>>>>>
>>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>>>>>
>>>>> The symbols are OK in the .o file:
>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
>>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>>>>>
>>>>> I don't understand the problem and I am considering disabling versioning in
>>>>> combined library.
>>>>>
>>>>> Any idea?
>>>>>
>>>>
>>>> The .map additions look incorrect to me:
>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
>>>>> index 22bd920..7f78717 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
>>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
>>>>> 	rte_eth_bond_slaves_get;
>>>>> 	rte_eth_bond_xmit_policy_get;
>>>>> 	rte_eth_bond_xmit_policy_set;
>>>>> +	rte_eth_bond_8023ad_ext_collect;
>>>>> +	rte_eth_bond_8023ad_ext_distrib;
>>>>> +	rte_eth_bond_8023ad_ext_slowtx;
>>>>
>>>> These symbols didn't exist in DPDK 2.0 but are only being added
>>>> here. So why are they being added to the 2.0 section?
>>>
>>> Yes, I think these should probably be moved.
>>>
>>>
>>>>>
>>>>> 	local: *;
>>>>> };
>>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
>>>>> 	rte_eth_bond_free;
>>>>>
>>>>> } DPDK_2.0;
>>>>> +
>>>>> +DPDK_2.2 {
>>>>> +	local
>>>>> +
>>>>> +	bond_mode_8023ad_conf_get;
>>>>> +	bond_mode_8023ad_setup;
>>>>> +} DPDK_2.1;
>>>>
>>>> These are marked local, as in, "not exported" which doesn't seem
>>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
>>>> the symbol export map should look like here:
>>>
>>>
>>> These were not exported to begin with.  But after versioning these
>>> functions, they are exported unless explicitly declared to be local here.
>>>
>>
>> And this does not ring any warning bells? :)
>>
>> Sorry, I was not looking at the patch as a whole. You're declaring these
>> symbols as exported with the versioning macros, eg
>>
>> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>
>> ...and then explicitly telling it to not export them by declaring local,
>> and then we wonder why it has trouble finding the symbols.
>> The versioning macros wont invent the librte_ prefix for you, you need
>> to rename the functions accordingly.
>>
>> But all this versioning gymnastics is moot anyway because you declare
>> the ABI incompatible:
>>
>> -LIBABIVER := 1
>> +LIBABIVER := 2
>>
>> This changes the library soname, so no binary compiled against the
>> previous version can possibly use it anymore. As in, by definition there
>> can be no callers of the _v20 variants after this ABI version bump.
>
> Panu, thanks for your careful review.
> We must remove the private functions from the export map.and not bump LIBABIVER.
>

Actually the LIBABIVER bump seems appropriate since a public struct 
(rte_eth_bond_8023ad_conf) changes size.

As for the rest, I'm not quite sure what the patch is trying to achieve. 
I initially assumed its trying to add a new incompatible version of 
rte_bond_mode_8023ad_setup() and rte_eth_bond_8023ad_conf_get() which 
could be a perfectly reasonable thing to do in some circumstances. As it 
is, the patch actually adds versioning to an internal symbol which just 
doesn't make sense at all. Symbol versioning is only meaningful for 
public symbols.

	- Panu -

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

On Tue, Nov 03, 2015 at 09:06:05AM +0200, Panu Matilainen wrote:
> On 11/02/2015 05:23 PM, Ferruh Yigit wrote:
>> Fixes following error:
>>    LD libdpdk.so
>>    /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
>>    for symbol <function>@DPDK_x.y
>>
>> Defines version symbols in a fixed path libdpdk.map file and this
>> value hardcoded into makefile
>
> Hold on. What problem is this supposed to be fixing?
This patch is to fix combined library compilation error when ABI versining used.
Currently if ABI version macros used (VERSION_SYMBOL), we are not able to compile combined shared library.

> If its for the bonding thing (as it'd appear from the threading), NAK. 
Nothing directly related to the bonding, just issue observed in this thread.

> The symbol versioning in 
> that patch is confused needs to be fixed there instead of inventing strange 
> workarounds elsewhere.
>
This is not intented to be a workaround but a fix, unless you have a better idea.

ferruh

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Thomas Monjalon]

2015-11-03 10:30, Panu Matilainen:
> On 11/03/2015 10:16 AM, Thomas Monjalon wrote:
> > 2015-11-03 08:48, Panu Matilainen:
> >> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
> >>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
> >>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
> >>>>> 2015-10-19 08:36, Eric Kinzie:
> >>>>>>     Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
> >>>>>>     and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
> >>>>>>     functions.
> >>>>> [...]
> >>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
> >>>>> [...]
> >>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
> >>>>>> +		  bond_mode_8023ad_setup_v22);
> >>>>>
> >>>>> I'm sorry it doesn't work well when trying to build a combined lib:
> >>>>>
> >>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
> >>>>>
> >>>>> The symbols are OK in the .o file:
> >>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
> >>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
> >>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
> >>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
> >>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
> >>>>>
> >>>>> I don't understand the problem and I am considering disabling versioning in
> >>>>> combined library.
> >>>>>
> >>>>> Any idea?
> >>>>>
> >>>>
> >>>> The .map additions look incorrect to me:
> >>>>
> >>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
> >>>>> index 22bd920..7f78717 100644
> >>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
> >>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
> >>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
> >>>>> 	rte_eth_bond_slaves_get;
> >>>>> 	rte_eth_bond_xmit_policy_get;
> >>>>> 	rte_eth_bond_xmit_policy_set;
> >>>>> +	rte_eth_bond_8023ad_ext_collect;
> >>>>> +	rte_eth_bond_8023ad_ext_distrib;
> >>>>> +	rte_eth_bond_8023ad_ext_slowtx;
> >>>>
> >>>> These symbols didn't exist in DPDK 2.0 but are only being added
> >>>> here. So why are they being added to the 2.0 section?
> >>>
> >>> Yes, I think these should probably be moved.
> >>>
> >>>
> >>>>>
> >>>>> 	local: *;
> >>>>> };
> >>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
> >>>>> 	rte_eth_bond_free;
> >>>>>
> >>>>> } DPDK_2.0;
> >>>>> +
> >>>>> +DPDK_2.2 {
> >>>>> +	local
> >>>>> +
> >>>>> +	bond_mode_8023ad_conf_get;
> >>>>> +	bond_mode_8023ad_setup;
> >>>>> +} DPDK_2.1;
> >>>>
> >>>> These are marked local, as in, "not exported" which doesn't seem
> >>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
> >>>> the symbol export map should look like here:
> >>>
> >>>
> >>> These were not exported to begin with.  But after versioning these
> >>> functions, they are exported unless explicitly declared to be local here.
> >>>
> >>
> >> And this does not ring any warning bells? :)
> >>
> >> Sorry, I was not looking at the patch as a whole. You're declaring these
> >> symbols as exported with the versioning macros, eg
> >>
> >> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>
> >> ...and then explicitly telling it to not export them by declaring local,
> >> and then we wonder why it has trouble finding the symbols.
> >> The versioning macros wont invent the librte_ prefix for you, you need
> >> to rename the functions accordingly.
> >>
> >> But all this versioning gymnastics is moot anyway because you declare
> >> the ABI incompatible:
> >>
> >> -LIBABIVER := 1
> >> +LIBABIVER := 2
> >>
> >> This changes the library soname, so no binary compiled against the
> >> previous version can possibly use it anymore. As in, by definition there
> >> can be no callers of the _v20 variants after this ABI version bump.
> >
> > Panu, thanks for your careful review.
> > We must remove the private functions from the export map.and not bump LIBABIVER.
> 
> Actually the LIBABIVER bump seems appropriate since a public struct 
> (rte_eth_bond_8023ad_conf) changes size.

Not sure. It is added at the end of a struct which is not embedded elsewhere.

> As for the rest, I'm not quite sure what the patch is trying to achieve. 
> I initially assumed its trying to add a new incompatible version of 
> rte_bond_mode_8023ad_setup() and rte_eth_bond_8023ad_conf_get() which 
> could be a perfectly reasonable thing to do in some circumstances. As it 
> is, the patch actually adds versioning to an internal symbol which just 
> doesn't make sense at all. Symbol versioning is only meaningful for 
> public symbols.

Yes

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Panu Matilainen]

On 11/03/2015 11:20 AM, Ferruh Yigit wrote:
> On Tue, Nov 03, 2015 at 09:06:05AM +0200, Panu Matilainen wrote:
>> On 11/02/2015 05:23 PM, Ferruh Yigit wrote:
>>> Fixes following error:
>>>     LD libdpdk.so
>>>     /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
>>>     for symbol <function>@DPDK_x.y
>>>
>>> Defines version symbols in a fixed path libdpdk.map file and this
>>> value hardcoded into makefile
>>
>> Hold on. What problem is this supposed to be fixing?
> This patch is to fix combined library compilation error when ABI versining used.
> Currently if ABI version macros used (VERSION_SYMBOL), we are not able to compile combined shared library.
>
>> If its for the bonding thing (as it'd appear from the threading), NAK.
> Nothing directly related to the bonding, just issue observed in this thread.

Okay, understood, it was just not clear from the description. Since 
there are no users of VERSION_SYMBOL etc macros this error has not come 
up until now.

>
>> The symbol versioning in
>> that patch is confused needs to be fixed there instead of inventing strange
>> workarounds elsewhere.
>>
> This is not intented to be a workaround but a fix, unless you have a better idea.

The problem is, it undoes the other benefit of symbol versioning: hiding 
the internal symbols. So in my eyes its more of a hack or a workaround 
than a fix.

I'd much much rather see this as a solution:
http://dpdk.org/dev/patchwork/patch/4262/

	- Panu -


>
>

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Ferruh Yigit]

On Tue, Nov 03, 2015 at 08:48:16AM +0200, Panu Matilainen wrote:
> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
>>>> 2015-10-19 08:36, Eric Kinzie:
>>>>>    Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>>>>    and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>>>>    functions.
>>>> [...]
>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
>>>> [...]
>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>>>>> +		  bond_mode_8023ad_setup_v22);
>>>>
>>>> I'm sorry it doesn't work well when trying to build a combined lib:
>>>>
>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>>>>
>>>> The symbols are OK in the .o file:
>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>>>>
>>>> I don't understand the problem and I am considering disabling versioning in
>>>> combined library.
>>>>
>>>> Any idea?
>>>>
>>>
>>> The .map additions look incorrect to me:
>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
>>>> index 22bd920..7f78717 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
>>>> 	rte_eth_bond_slaves_get;
>>>> 	rte_eth_bond_xmit_policy_get;
>>>> 	rte_eth_bond_xmit_policy_set;
>>>> +	rte_eth_bond_8023ad_ext_collect;
>>>> +	rte_eth_bond_8023ad_ext_distrib;
>>>> +	rte_eth_bond_8023ad_ext_slowtx;
>>>
>>> These symbols didn't exist in DPDK 2.0 but are only being added
>>> here. So why are they being added to the 2.0 section?
>>
>> Yes, I think these should probably be moved.
>>
>>
>>>>
>>>> 	local: *;
>>>> };
>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
>>>> 	rte_eth_bond_free;
>>>>
>>>> } DPDK_2.0;
>>>> +
>>>> +DPDK_2.2 {
>>>> +	local
>>>> +
>>>> +	bond_mode_8023ad_conf_get;
>>>> +	bond_mode_8023ad_setup;
>>>> +} DPDK_2.1;
>>>
>>> These are marked local, as in, "not exported" which doesn't seem
>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
>>> the symbol export map should look like here:
>>
>>
>> These were not exported to begin with.  But after versioning these
>> functions, they are exported unless explicitly declared to be local here.
>>
>
> And this does not ring any warning bells? :)
>
> Sorry, I was not looking at the patch as a whole. You're declaring these 
> symbols as exported with the versioning macros, eg
>
> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>
> ...and then explicitly telling it to not export them by declaring local, and 
> then we wonder why it has trouble finding the symbols.
> The versioning macros wont invent the librte_ prefix for you, you need to 
> rename the functions accordingly.
>
> But all this versioning gymnastics is moot anyway because you declare the 
> ABI incompatible:
>
> -LIBABIVER := 1
> +LIBABIVER := 2
>
> This changes the library soname, so no binary compiled against the previous 
> version can possibly use it anymore. As in, by definition there can be no 
> callers of the _v20 variants after this ABI version bump.
>
An observation: even soname is different, just renaming .so file itself works.

And this can be useful for the case:
libx.so.1 provides functions A, B, C
app1 compiled against libx.so.1, using only function B

libx.so.1 updated only thefunction A and become libx.so.2
app1 still can run successfully by re-naming lib to libx.so.1 (even soname is libx.so.2)

But for this usage, user needs to know which function updated and is it safe or not to use this library,
I wonder if there is an automatic way of resolving this dependency.


> 	- Panu -
>

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Panu Matilainen]

On 11/03/2015 01:02 PM, Ferruh Yigit wrote:
> On Tue, Nov 03, 2015 at 08:48:16AM +0200, Panu Matilainen wrote:
>> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
>>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
>>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
>>>>> 2015-10-19 08:36, Eric Kinzie:
>>>>>>     Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>>>>>     and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>>>>>     functions.
>>>>> [...]
>>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
>>>>> [...]
>>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>>>>>> +		  bond_mode_8023ad_setup_v22);
>>>>>
>>>>> I'm sorry it doesn't work well when trying to build a combined lib:
>>>>>
>>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>>>>>
>>>>> The symbols are OK in the .o file:
>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
>>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>>>>>
>>>>> I don't understand the problem and I am considering disabling versioning in
>>>>> combined library.
>>>>>
>>>>> Any idea?
>>>>>
>>>>
>>>> The .map additions look incorrect to me:
>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
>>>>> index 22bd920..7f78717 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
>>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
>>>>> 	rte_eth_bond_slaves_get;
>>>>> 	rte_eth_bond_xmit_policy_get;
>>>>> 	rte_eth_bond_xmit_policy_set;
>>>>> +	rte_eth_bond_8023ad_ext_collect;
>>>>> +	rte_eth_bond_8023ad_ext_distrib;
>>>>> +	rte_eth_bond_8023ad_ext_slowtx;
>>>>
>>>> These symbols didn't exist in DPDK 2.0 but are only being added
>>>> here. So why are they being added to the 2.0 section?
>>>
>>> Yes, I think these should probably be moved.
>>>
>>>
>>>>>
>>>>> 	local: *;
>>>>> };
>>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
>>>>> 	rte_eth_bond_free;
>>>>>
>>>>> } DPDK_2.0;
>>>>> +
>>>>> +DPDK_2.2 {
>>>>> +	local
>>>>> +
>>>>> +	bond_mode_8023ad_conf_get;
>>>>> +	bond_mode_8023ad_setup;
>>>>> +} DPDK_2.1;
>>>>
>>>> These are marked local, as in, "not exported" which doesn't seem
>>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
>>>> the symbol export map should look like here:
>>>
>>>
>>> These were not exported to begin with.  But after versioning these
>>> functions, they are exported unless explicitly declared to be local here.
>>>
>>
>> And this does not ring any warning bells? :)
>>
>> Sorry, I was not looking at the patch as a whole. You're declaring these
>> symbols as exported with the versioning macros, eg
>>
>> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>
>> ...and then explicitly telling it to not export them by declaring local, and
>> then we wonder why it has trouble finding the symbols.
>> The versioning macros wont invent the librte_ prefix for you, you need to
>> rename the functions accordingly.
>>
>> But all this versioning gymnastics is moot anyway because you declare the
>> ABI incompatible:
>>
>> -LIBABIVER := 1
>> +LIBABIVER := 2
>>
>> This changes the library soname, so no binary compiled against the previous
>> version can possibly use it anymore. As in, by definition there can be no
>> callers of the _v20 variants after this ABI version bump.
>>
> An observation: even soname is different, just renaming .so file itself works.
>
> And this can be useful for the case:
> libx.so.1 provides functions A, B, C
> app1 compiled against libx.so.1, using only function B
>
> libx.so.1 updated only thefunction A and become libx.so.2
> app1 still can run successfully by re-naming lib to libx.so.1 (even soname is libx.so.2)
>
> But for this usage, user needs to know which function updated and is it safe or not to use this library,
> I wonder if there is an automatic way of resolving this dependency.

Erm, no. The whole point of changing soname and the physical filename is 
to tell others it is incompatible with earlier versions. Yes you can 
rename the file and get lucky (or not), just like you can play Russian 
roulette. Neither are particularly healthy ideas.

Symbol version exists in part to allow libraries to evolve while 
maintaining compatibility, but it requires careful planning and 
programming. When public structs change, the structs would have to be 
versioned too, and from there on it starts getting more and more 
complicated.

	- Panu -

>
>
>> 	- Panu -
>>

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

On Tue, Nov 03, 2015 at 12:41:10PM +0200, Panu Matilainen wrote:
> On 11/03/2015 11:20 AM, Ferruh Yigit wrote:
>> On Tue, Nov 03, 2015 at 09:06:05AM +0200, Panu Matilainen wrote:
>>> On 11/02/2015 05:23 PM, Ferruh Yigit wrote:
>>>> Fixes following error:
>>>>     LD libdpdk.so
>>>>     /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
>>>>     for symbol <function>@DPDK_x.y
>>>>
>>>> Defines version symbols in a fixed path libdpdk.map file and this
>>>> value hardcoded into makefile
>>>
>>> Hold on. What problem is this supposed to be fixing?
>> This patch is to fix combined library compilation error when ABI versining used.
>> Currently if ABI version macros used (VERSION_SYMBOL), we are not able to compile combined shared library.
>>
>>> If its for the bonding thing (as it'd appear from the threading), NAK.
>> Nothing directly related to the bonding, just issue observed in this thread.
>
> Okay, understood, it was just not clear from the description. Since there 
> are no users of VERSION_SYMBOL etc macros this error has not come up until 
> now.
>
>>
>>> The symbol versioning in
>>> that patch is confused needs to be fixed there instead of inventing strange
>>> workarounds elsewhere.
>>>
>> This is not intented to be a workaround but a fix, unless you have a better idea.
>
> The problem is, it undoes the other benefit of symbol versioning: hiding the 
> internal symbols. So in my eyes its more of a hack or a workaround than a 
> fix.
>
> I'd much much rather see this as a solution:
> http://dpdk.org/dev/patchwork/patch/4262/
>
What I observed from this, how/why combined library should be generated is under question, from that perspective I agree this patch is a hack.
If current method is used, this patch just fixes compile error, also patch is not related to versioning in combined library, which is still broken.

Thanks,
ferruh

> 	- Panu -

Re: [dpdk-dev] [PATCH v2] mk: fix ABI versioning compile error for combined shared library

[Panu Matilainen]

On 11/03/2015 01:33 PM, Ferruh Yigit wrote:
> On Tue, Nov 03, 2015 at 12:41:10PM +0200, Panu Matilainen wrote:
>> On 11/03/2015 11:20 AM, Ferruh Yigit wrote:
>>> On Tue, Nov 03, 2015 at 09:06:05AM +0200, Panu Matilainen wrote:
>>>> On 11/02/2015 05:23 PM, Ferruh Yigit wrote:
>>>>> Fixes following error:
>>>>>      LD libdpdk.so
>>>>>      /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
>>>>>      for symbol <function>@DPDK_x.y
>>>>>
>>>>> Defines version symbols in a fixed path libdpdk.map file and this
>>>>> value hardcoded into makefile
>>>>
>>>> Hold on. What problem is this supposed to be fixing?
>>> This patch is to fix combined library compilation error when ABI versining used.
>>> Currently if ABI version macros used (VERSION_SYMBOL), we are not able to compile combined shared library.
>>>
>>>> If its for the bonding thing (as it'd appear from the threading), NAK.
>>> Nothing directly related to the bonding, just issue observed in this thread.
>>
>> Okay, understood, it was just not clear from the description. Since there
>> are no users of VERSION_SYMBOL etc macros this error has not come up until
>> now.
>>
>>>
>>>> The symbol versioning in
>>>> that patch is confused needs to be fixed there instead of inventing strange
>>>> workarounds elsewhere.
>>>>
>>> This is not intented to be a workaround but a fix, unless you have a better idea.
>>
>> The problem is, it undoes the other benefit of symbol versioning: hiding the
>> internal symbols. So in my eyes its more of a hack or a workaround than a
>> fix.
>>
>> I'd much much rather see this as a solution:
>> http://dpdk.org/dev/patchwork/patch/4262/
>>
> What I observed from this, how/why combined library should be generated is under question, from that perspective I agree this patch is a hack.
> If current method is used, this patch just fixes compile error, also patch is not related to versioning in combined library, which is still broken.

The linker script eliminates the whole versioning problem because it 
just makes the actual individual libraries *appear* as a single entity 
during build-time, whereas the current approach physically lumps it all 
together.

	- Panu -

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Ferruh Yigit]

On Tue, Nov 03, 2015 at 01:31:45PM +0200, Panu Matilainen wrote:
> On 11/03/2015 01:02 PM, Ferruh Yigit wrote:
>> On Tue, Nov 03, 2015 at 08:48:16AM +0200, Panu Matilainen wrote:
>>> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
>>>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
>>>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
>>>>>> 2015-10-19 08:36, Eric Kinzie:
>>>>>>>     Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>>>>>>     and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>>>>>>     functions.
>>>>>> [...]
>>>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
>>>>>> [...]
>>>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>>>>>>> +		  bond_mode_8023ad_setup_v22);
>>>>>>
>>>>>> I'm sorry it doesn't work well when trying to build a combined lib:
>>>>>>
>>>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>>>>>>
>>>>>> The symbols are OK in the .o file:
>>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
>>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
>>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
>>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
>>>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>>>>>>
>>>>>> I don't understand the problem and I am considering disabling versioning in
>>>>>> combined library.
>>>>>>
>>>>>> Any idea?
>>>>>>
>>>>>
>>>>> The .map additions look incorrect to me:
>>>>>
>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
>>>>>> index 22bd920..7f78717 100644
>>>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
>>>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
>>>>>> 	rte_eth_bond_slaves_get;
>>>>>> 	rte_eth_bond_xmit_policy_get;
>>>>>> 	rte_eth_bond_xmit_policy_set;
>>>>>> +	rte_eth_bond_8023ad_ext_collect;
>>>>>> +	rte_eth_bond_8023ad_ext_distrib;
>>>>>> +	rte_eth_bond_8023ad_ext_slowtx;
>>>>>
>>>>> These symbols didn't exist in DPDK 2.0 but are only being added
>>>>> here. So why are they being added to the 2.0 section?
>>>>
>>>> Yes, I think these should probably be moved.
>>>>
>>>>
>>>>>>
>>>>>> 	local: *;
>>>>>> };
>>>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
>>>>>> 	rte_eth_bond_free;
>>>>>>
>>>>>> } DPDK_2.0;
>>>>>> +
>>>>>> +DPDK_2.2 {
>>>>>> +	local
>>>>>> +
>>>>>> +	bond_mode_8023ad_conf_get;
>>>>>> +	bond_mode_8023ad_setup;
>>>>>> +} DPDK_2.1;
>>>>>
>>>>> These are marked local, as in, "not exported" which doesn't seem
>>>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
>>>>> the symbol export map should look like here:
>>>>
>>>>
>>>> These were not exported to begin with.  But after versioning these
>>>> functions, they are exported unless explicitly declared to be local here.
>>>>
>>>
>>> And this does not ring any warning bells? :)
>>>
>>> Sorry, I was not looking at the patch as a whole. You're declaring these
>>> symbols as exported with the versioning macros, eg
>>>
>>> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>
>>> ...and then explicitly telling it to not export them by declaring local, and
>>> then we wonder why it has trouble finding the symbols.
>>> The versioning macros wont invent the librte_ prefix for you, you need to
>>> rename the functions accordingly.
>>>
>>> But all this versioning gymnastics is moot anyway because you declare the
>>> ABI incompatible:
>>>
>>> -LIBABIVER := 1
>>> +LIBABIVER := 2
>>>
>>> This changes the library soname, so no binary compiled against the previous
>>> version can possibly use it anymore. As in, by definition there can be no
>>> callers of the _v20 variants after this ABI version bump.
>>>
>> An observation: even soname is different, just renaming .so file itself works.
>>
>> And this can be useful for the case:
>> libx.so.1 provides functions A, B, C
>> app1 compiled against libx.so.1, using only function B
>>
>> libx.so.1 updated only thefunction A and become libx.so.2
>> app1 still can run successfully by re-naming lib to libx.so.1 (even soname is libx.so.2)
>>
>> But for this usage, user needs to know which function updated and is it safe or not to use this library,
>> I wonder if there is an automatic way of resolving this dependency.
>
> Erm, no. The whole point of changing soname and the physical filename is to 
> tell others it is incompatible with earlier versions. Yes you can rename the 
> file and get lucky (or not), just like you can play Russian roulette. 
> Neither are particularly healthy ideas.
>
> Symbol version exists in part to allow libraries to evolve while maintaining 
> compatibility, but it requires careful planning and programming. When public 
> structs change, the structs would have to be versioned too, and from there 
> on it starts getting more and more complicated.
>

If we strictly want to prevent using old library, whenever LIBABIVER increased, we should update .map as following, right?

 FROM: (dpdk2.1)
================
DPDK_2.0 {
	A;
	B;
	C;
};

DPDK_2.1 {
	D;
	E;
};

LIBABIVER=1
================

 TO: (dpdk2.2)
================
DPDK_2.2 {
	A;
	B;
	C;
	D;
	E;
};

LIBABIVER=2
================


So this won't work for anybody without luck factor, I think currently we are not doing this.


thanks,
ferruh

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Eric Kinzie]

On Tue Nov 03 10:30:56 +0200 2015, Panu Matilainen wrote:
> On 11/03/2015 10:16 AM, Thomas Monjalon wrote:
> >2015-11-03 08:48, Panu Matilainen:
> >>On 11/02/2015 06:42 PM, Eric Kinzie wrote:
> >>>On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
> >>>>On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
> >>>>>2015-10-19 08:36, Eric Kinzie:
> >>>>>>    Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
> >>>>>>    and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
> >>>>>>    functions.
> >>>>>[...]
> >>>>>>+VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
> >>>>>[...]
> >>>>>>+BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>>>>>+MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
> >>>>>>+		  bond_mode_8023ad_setup_v22);
> >>>>>
> >>>>>I'm sorry it doesn't work well when trying to build a combined lib:
> >>>>>
> >>>>>ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
> >>>>>
> >>>>>The symbols are OK in the .o file:
> >>>>>0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
> >>>>>0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
> >>>>>0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
> >>>>>0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
> >>>>>0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
> >>>>>
> >>>>>I don't understand the problem and I am considering disabling versioning in
> >>>>>combined library.
> >>>>>
> >>>>>Any idea?
> >>>>>
> >>>>
> >>>>The .map additions look incorrect to me:
> >>>>
> >>>>>diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
> >>>>>index 22bd920..7f78717 100644
> >>>>>--- a/drivers/net/bonding/rte_eth_bond_version.map
> >>>>>+++ b/drivers/net/bonding/rte_eth_bond_version.map
> >>>>>@@ -17,6 +17,9 @@  DPDK_2.0 {
> >>>>>	rte_eth_bond_slaves_get;
> >>>>>	rte_eth_bond_xmit_policy_get;
> >>>>>	rte_eth_bond_xmit_policy_set;
> >>>>>+	rte_eth_bond_8023ad_ext_collect;
> >>>>>+	rte_eth_bond_8023ad_ext_distrib;
> >>>>>+	rte_eth_bond_8023ad_ext_slowtx;
> >>>>
> >>>>These symbols didn't exist in DPDK 2.0 but are only being added
> >>>>here. So why are they being added to the 2.0 section?
> >>>
> >>>Yes, I think these should probably be moved.
> >>>
> >>>
> >>>>>
> >>>>>	local: *;
> >>>>>};
> >>>>>@@ -27,3 +30,10 @@  DPDK_2.1 {
> >>>>>	rte_eth_bond_free;
> >>>>>
> >>>>>} DPDK_2.0;
> >>>>>+
> >>>>>+DPDK_2.2 {
> >>>>>+	local
> >>>>>+
> >>>>>+	bond_mode_8023ad_conf_get;
> >>>>>+	bond_mode_8023ad_setup;
> >>>>>+} DPDK_2.1;
> >>>>
> >>>>These are marked local, as in, "not exported" which doesn't seem
> >>>>right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
> >>>>the symbol export map should look like here:
> >>>
> >>>
> >>>These were not exported to begin with.  But after versioning these
> >>>functions, they are exported unless explicitly declared to be local here.
> >>>
> >>
> >>And this does not ring any warning bells? :)
> >>
> >>Sorry, I was not looking at the patch as a whole. You're declaring these
> >>symbols as exported with the versioning macros, eg
> >>
> >>BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>
> >>...and then explicitly telling it to not export them by declaring local,
> >>and then we wonder why it has trouble finding the symbols.
> >>The versioning macros wont invent the librte_ prefix for you, you need
> >>to rename the functions accordingly.
> >>
> >>But all this versioning gymnastics is moot anyway because you declare
> >>the ABI incompatible:
> >>
> >>-LIBABIVER := 1
> >>+LIBABIVER := 2
> >>
> >>This changes the library soname, so no binary compiled against the
> >>previous version can possibly use it anymore. As in, by definition there
> >>can be no callers of the _v20 variants after this ABI version bump.
> >
> >Panu, thanks for your careful review.
> >We must remove the private functions from the export map.and not bump LIBABIVER.
> >
> 
> Actually the LIBABIVER bump seems appropriate since a public struct
> (rte_eth_bond_8023ad_conf) changes size.
> 
> As for the rest, I'm not quite sure what the patch is trying to
> achieve. I initially assumed its trying to add a new incompatible
> version of rte_bond_mode_8023ad_setup() and
> rte_eth_bond_8023ad_conf_get() which could be a perfectly reasonable
> thing to do in some circumstances. As it is, the patch actually adds
> versioning to an internal symbol which just doesn't make sense at
> all. Symbol versioning is only meaningful for public symbols.
> 
> 	- Panu -

It's likely I misintepreted some advice from an ealier review of
this code.  The intention was to prevent an application accidentally
reading/writing past the end of a struct rte_eth_bond_8023ad_conf after
upgrading the library.

I can send another version of these patches that, instead,
adds new versions of rte_bond_mode_8023ad_setup() and
rte_eth_bond_8023ad_conf_get().

Eric

[dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

Sorry for duplication, previous patch is not in the patchwork, this is the
exact same patch and re-sent for patchwork.

Ferruh Yigit (1):
  mk: fix ABI versioning compile error for combined shared library

 drivers/net/Makefile |  3 +++
 lib/Makefile         |  3 +++
 lib/libdpdk.map      | 12 ++++++++++++
 mk/rte.sdkbuild.mk   |  2 +-
 mk/rte.sharelib.mk   |  1 +
 5 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 lib/libdpdk.map

-- 
2.5.0

[dpdk-dev] [PATCH] mk: fix ABI versioning compile error for combined shared library

[Ferruh Yigit]

Fixes following error:
  LD libdpdk.so
  /usr/bin/ld: /root/dpdk/build/lib/libdpdk.so: version node not found
  for symbol <function>@DPDK_x.y

Defines version symbols in a fixed path libdpdk.map file and this
value hardcoded into makefile

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/Makefile |  3 +++
 lib/Makefile         |  3 +++
 lib/libdpdk.map      | 12 ++++++++++++
 mk/rte.sdkbuild.mk   |  2 +-
 mk/rte.sharelib.mk   |  1 +
 5 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 lib/libdpdk.map

diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 6da1ce2..d30018c 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -50,5 +50,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
 DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
 DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
 
+ifeq ($(COMBINED_BUILD),1)
 include $(RTE_SDK)/mk/rte.sharelib.mk
+endif
+
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/lib/Makefile b/lib/Makefile
index 9727b83..33d76a6 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -62,5 +62,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_KNI) += librte_kni
 DIRS-$(CONFIG_RTE_LIBRTE_IVSHMEM) += librte_ivshmem
 endif
 
+ifeq ($(COMBINED_BUILD),1)
 include $(RTE_SDK)/mk/rte.sharelib.mk
+endif
+
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/lib/libdpdk.map b/lib/libdpdk.map
new file mode 100644
index 0000000..3988a3f
--- /dev/null
+++ b/lib/libdpdk.map
@@ -0,0 +1,12 @@
+DPDK_2.0 {
+
+};
+
+DPDK_2.1 {
+
+} DPDK_2.0;
+
+DPDK_2.2 {
+
+} DPDK_2.1;
+
diff --git a/mk/rte.sdkbuild.mk b/mk/rte.sdkbuild.mk
index 38ec7bd..d4e3abf 100644
--- a/mk/rte.sdkbuild.mk
+++ b/mk/rte.sdkbuild.mk
@@ -94,7 +94,7 @@ $(ROOTDIRS-y):
 	@echo "== Build $@"
 	$(Q)$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile -C $(BUILDDIR)/$@ all
 	@if [ $@ = drivers -a $(CONFIG_RTE_BUILD_COMBINE_LIBS) = y ]; then \
-		$(MAKE) -f $(RTE_SDK)/lib/Makefile sharelib; \
+		COMBINED_BUILD=1 $(MAKE) -f $(RTE_SDK)/lib/Makefile sharelib; \
 	fi
 
 %_clean:
diff --git a/mk/rte.sharelib.mk b/mk/rte.sharelib.mk
index 7bb7219..1f71fcb 100644
--- a/mk/rte.sharelib.mk
+++ b/mk/rte.sharelib.mk
@@ -40,6 +40,7 @@ LIB_ONE := lib$(RTE_LIBNAME).so
 else
 LIB_ONE := lib$(RTE_LIBNAME).a
 endif
+CPU_LDFLAGS += --version-script=$(SRCDIR)/lib/libdpdk.map
 endif
 
 .PHONY:sharelib
-- 
2.5.0

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Eric Kinzie]

On Tue Nov 03 11:48:57 +0000 2015, Ferruh Yigit wrote:
> On Tue, Nov 03, 2015 at 01:31:45PM +0200, Panu Matilainen wrote:
> > On 11/03/2015 01:02 PM, Ferruh Yigit wrote:
> >> On Tue, Nov 03, 2015 at 08:48:16AM +0200, Panu Matilainen wrote:
> >>> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
> >>>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
> >>>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
> >>>>>> 2015-10-19 08:36, Eric Kinzie:
> >>>>>>>     Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
> >>>>>>>     and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
> >>>>>>>     functions.
> >>>>>> [...]
> >>>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
> >>>>>> [...]
> >>>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
> >>>>>>> +		  bond_mode_8023ad_setup_v22);
> >>>>>>
> >>>>>> I'm sorry it doesn't work well when trying to build a combined lib:
> >>>>>>
> >>>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
> >>>>>>
> >>>>>> The symbols are OK in the .o file:
> >>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
> >>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
> >>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
> >>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
> >>>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
> >>>>>>
> >>>>>> I don't understand the problem and I am considering disabling versioning in
> >>>>>> combined library.
> >>>>>>
> >>>>>> Any idea?
> >>>>>>
> >>>>>
> >>>>> The .map additions look incorrect to me:
> >>>>>
> >>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
> >>>>>> index 22bd920..7f78717 100644
> >>>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
> >>>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
> >>>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
> >>>>>> 	rte_eth_bond_slaves_get;
> >>>>>> 	rte_eth_bond_xmit_policy_get;
> >>>>>> 	rte_eth_bond_xmit_policy_set;
> >>>>>> +	rte_eth_bond_8023ad_ext_collect;
> >>>>>> +	rte_eth_bond_8023ad_ext_distrib;
> >>>>>> +	rte_eth_bond_8023ad_ext_slowtx;
> >>>>>
> >>>>> These symbols didn't exist in DPDK 2.0 but are only being added
> >>>>> here. So why are they being added to the 2.0 section?
> >>>>
> >>>> Yes, I think these should probably be moved.
> >>>>
> >>>>
> >>>>>>
> >>>>>> 	local: *;
> >>>>>> };
> >>>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
> >>>>>> 	rte_eth_bond_free;
> >>>>>>
> >>>>>> } DPDK_2.0;
> >>>>>> +
> >>>>>> +DPDK_2.2 {
> >>>>>> +	local
> >>>>>> +
> >>>>>> +	bond_mode_8023ad_conf_get;
> >>>>>> +	bond_mode_8023ad_setup;
> >>>>>> +} DPDK_2.1;
> >>>>>
> >>>>> These are marked local, as in, "not exported" which doesn't seem
> >>>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
> >>>>> the symbol export map should look like here:
> >>>>
> >>>>
> >>>> These were not exported to begin with.  But after versioning these
> >>>> functions, they are exported unless explicitly declared to be local here.
> >>>>
> >>>
> >>> And this does not ring any warning bells? :)
> >>>
> >>> Sorry, I was not looking at the patch as a whole. You're declaring these
> >>> symbols as exported with the versioning macros, eg
> >>>
> >>> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
> >>>
> >>> ...and then explicitly telling it to not export them by declaring local, and
> >>> then we wonder why it has trouble finding the symbols.
> >>> The versioning macros wont invent the librte_ prefix for you, you need to
> >>> rename the functions accordingly.
> >>>
> >>> But all this versioning gymnastics is moot anyway because you declare the
> >>> ABI incompatible:
> >>>
> >>> -LIBABIVER := 1
> >>> +LIBABIVER := 2
> >>>
> >>> This changes the library soname, so no binary compiled against the previous
> >>> version can possibly use it anymore. As in, by definition there can be no
> >>> callers of the _v20 variants after this ABI version bump.
> >>>
> >> An observation: even soname is different, just renaming .so file itself works.
> >>
> >> And this can be useful for the case:
> >> libx.so.1 provides functions A, B, C
> >> app1 compiled against libx.so.1, using only function B
> >>
> >> libx.so.1 updated only thefunction A and become libx.so.2
> >> app1 still can run successfully by re-naming lib to libx.so.1 (even soname is libx.so.2)
> >>
> >> But for this usage, user needs to know which function updated and is it safe or not to use this library,
> >> I wonder if there is an automatic way of resolving this dependency.
> >
> > Erm, no. The whole point of changing soname and the physical filename is to 
> > tell others it is incompatible with earlier versions. Yes you can rename the 
> > file and get lucky (or not), just like you can play Russian roulette. 
> > Neither are particularly healthy ideas.
> >
> > Symbol version exists in part to allow libraries to evolve while maintaining 
> > compatibility, but it requires careful planning and programming. When public 
> > structs change, the structs would have to be versioned too, and from there 
> > on it starts getting more and more complicated.
> >
> 
> If we strictly want to prevent using old library, whenever LIBABIVER increased, we should update .map as following, right?
> 
>  FROM: (dpdk2.1)
> ================
> DPDK_2.0 {
> 	A;
> 	B;
> 	C;
> };
> 
> DPDK_2.1 {
> 	D;
> 	E;
> };
> 
> LIBABIVER=1
> ================
> 
>  TO: (dpdk2.2)
> ================
> DPDK_2.2 {
> 	A;
> 	B;
> 	C;
> 	D;
> 	E;
> };
> 
> LIBABIVER=2
> ================
> 
> 
> So this won't work for anybody without luck factor, I think currently we are not doing this.
> 
> 
> thanks,
> ferruh
> 
> 

Panu, Ferruh, is there agreement on an acceptable approach to this?

Eric

Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine

[Panu Matilainen]

On 11/20/2015 09:46 PM, Eric Kinzie wrote:
> On Tue Nov 03 11:48:57 +0000 2015, Ferruh Yigit wrote:
>> On Tue, Nov 03, 2015 at 01:31:45PM +0200, Panu Matilainen wrote:
>>> On 11/03/2015 01:02 PM, Ferruh Yigit wrote:
>>>> On Tue, Nov 03, 2015 at 08:48:16AM +0200, Panu Matilainen wrote:
>>>>> On 11/02/2015 06:42 PM, Eric Kinzie wrote:
>>>>>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote:
>>>>>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote:
>>>>>>>> 2015-10-19 08:36, Eric Kinzie:
>>>>>>>>>      Size of struct rte_eth_bond_8023ad_conf changed.  Increment LIBABIVER
>>>>>>>>>      and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get
>>>>>>>>>      functions.
>>>>>>>> [...]
>>>>>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0);
>>>>>>>> [...]
>>>>>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>>>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \
>>>>>>>>> +		  bond_mode_8023ad_setup_v22);
>>>>>>>>
>>>>>>>> I'm sorry it doesn't work well when trying to build a combined lib:
>>>>>>>>
>>>>>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2
>>>>>>>>
>>>>>>>> The symbols are OK in the .o file:
>>>>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2
>>>>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup@DPDK_2.0
>>>>>>>> 0000000000002260 g     F .text  00000000000000da bond_mode_8023ad_setup_v20
>>>>>>>> 0000000000002340 g     F .text  0000000000000171 bond_mode_8023ad_setup_v22
>>>>>>>> 0000000000000000         *UND*  0000000000000000 bond_mode_8023ad_setup
>>>>>>>>
>>>>>>>> I don't understand the problem and I am considering disabling versioning in
>>>>>>>> combined library.
>>>>>>>>
>>>>>>>> Any idea?
>>>>>>>>
>>>>>>>
>>>>>>> The .map additions look incorrect to me:
>>>>>>>
>>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map
>>>>>>>> index 22bd920..7f78717 100644
>>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map
>>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map
>>>>>>>> @@ -17,6 +17,9 @@  DPDK_2.0 {
>>>>>>>> 	rte_eth_bond_slaves_get;
>>>>>>>> 	rte_eth_bond_xmit_policy_get;
>>>>>>>> 	rte_eth_bond_xmit_policy_set;
>>>>>>>> +	rte_eth_bond_8023ad_ext_collect;
>>>>>>>> +	rte_eth_bond_8023ad_ext_distrib;
>>>>>>>> +	rte_eth_bond_8023ad_ext_slowtx;
>>>>>>>
>>>>>>> These symbols didn't exist in DPDK 2.0 but are only being added
>>>>>>> here. So why are they being added to the 2.0 section?
>>>>>>
>>>>>> Yes, I think these should probably be moved.
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> 	local: *;
>>>>>>>> };
>>>>>>>> @@ -27,3 +30,10 @@  DPDK_2.1 {
>>>>>>>> 	rte_eth_bond_free;
>>>>>>>>
>>>>>>>> } DPDK_2.0;
>>>>>>>> +
>>>>>>>> +DPDK_2.2 {
>>>>>>>> +	local
>>>>>>>> +
>>>>>>>> +	bond_mode_8023ad_conf_get;
>>>>>>>> +	bond_mode_8023ad_setup;
>>>>>>>> +} DPDK_2.1;
>>>>>>>
>>>>>>> These are marked local, as in, "not exported" which doesn't seem
>>>>>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what
>>>>>>> the symbol export map should look like here:
>>>>>>
>>>>>>
>>>>>> These were not exported to begin with.  But after versioning these
>>>>>> functions, they are exported unless explicitly declared to be local here.
>>>>>>
>>>>>
>>>>> And this does not ring any warning bells? :)
>>>>>
>>>>> Sorry, I was not looking at the patch as a whole. You're declaring these
>>>>> symbols as exported with the versioning macros, eg
>>>>>
>>>>> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2);
>>>>>
>>>>> ...and then explicitly telling it to not export them by declaring local, and
>>>>> then we wonder why it has trouble finding the symbols.
>>>>> The versioning macros wont invent the librte_ prefix for you, you need to
>>>>> rename the functions accordingly.
>>>>>
>>>>> But all this versioning gymnastics is moot anyway because you declare the
>>>>> ABI incompatible:
>>>>>
>>>>> -LIBABIVER := 1
>>>>> +LIBABIVER := 2
>>>>>
>>>>> This changes the library soname, so no binary compiled against the previous
>>>>> version can possibly use it anymore. As in, by definition there can be no
>>>>> callers of the _v20 variants after this ABI version bump.
>>>>>
>>>> An observation: even soname is different, just renaming .so file itself works.
>>>>
>>>> And this can be useful for the case:
>>>> libx.so.1 provides functions A, B, C
>>>> app1 compiled against libx.so.1, using only function B
>>>>
>>>> libx.so.1 updated only thefunction A and become libx.so.2
>>>> app1 still can run successfully by re-naming lib to libx.so.1 (even soname is libx.so.2)
>>>>
>>>> But for this usage, user needs to know which function updated and is it safe or not to use this library,
>>>> I wonder if there is an automatic way of resolving this dependency.
>>>
>>> Erm, no. The whole point of changing soname and the physical filename is to
>>> tell others it is incompatible with earlier versions. Yes you can rename the
>>> file and get lucky (or not), just like you can play Russian roulette.
>>> Neither are particularly healthy ideas.
>>>
>>> Symbol version exists in part to allow libraries to evolve while maintaining
>>> compatibility, but it requires careful planning and programming. When public
>>> structs change, the structs would have to be versioned too, and from there
>>> on it starts getting more and more complicated.
>>>
>>
>> If we strictly want to prevent using old library, whenever LIBABIVER increased, we should update .map as following, right?
>>
>>   FROM: (dpdk2.1)
>> ================
>> DPDK_2.0 {
>> 	A;
>> 	B;
>> 	C;
>> };
>>
>> DPDK_2.1 {
>> 	D;
>> 	E;
>> };
>>
>> LIBABIVER=1
>> ================
>>
>>   TO: (dpdk2.2)
>> ================
>> DPDK_2.2 {
>> 	A;
>> 	B;
>> 	C;
>> 	D;
>> 	E;
>> };
>>
>> LIBABIVER=2
>> ================
>>
>>
>> So this won't work for anybody without luck factor, I think currently we are not doing this.
>>
>>
>> thanks,
>> ferruh
>>
>>
>
> Panu, Ferruh, is there agreement on an acceptable approach to this?

Define "this" :)

As discussed before in (and around) these:
http://dpdk.org/ml/archives/dev/2015-November/027317.html
http://dpdk.org/ml/archives/dev/2015-November/027321.html

Thomas thinks the struct modification might not even need a LIBABIVER 
bump, but that all depends on the details of how the struct is used, 
I've no clue about that so I wont comment further on it.

Since the added symbols seem to be intended to be private, the 
versioning macro uses and .map changes should be just dropped. Only 
exported symbols are to be versioned.

So ultimately the answer is, "yes, but it depends on the details" :)

	- Panu -

	- Panu -