On Thu Feb 25 15:22:35 +0000 2016, Iremonger, Bernard wrote:

Hi Eric,
<snip>

quoted

quoted

@@ -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;
  };

This still is a likely an ABI break, previously discussed around here:
http://dpdk.org/ml/archives/dev/2015-November/027321.html

It might not be embedded anywhere in DPDK codebase, but there's no
telling what others might have done with it (have an array of them, embed in
other structs etc).

Also ultimately ABI compatibility goes both ways: when the library soname
does not change then an application is free to assume both downgrading and
upgrading are safe. In this case, upgrading *might* be okay, downgrading
certainly is not. So by that measure it definitely is an ABI break.

[...]

quoted

diff --git a/drivers/net/bonding/rte_eth_bond_version.map

b/drivers/net/bonding/rte_eth_bond_version.map
index 22bd920..33d73ff 100644

--- a/drivers/net/bonding/rte_eth_bond_version.map
+++ b/drivers/net/bonding/rte_eth_bond_version.map

@@ -27,3 +27,9 @@ DPDK_2.1 {
  	rte_eth_bond_free;

  } DPDK_2.0;
+
+DPDK_2.2 {
+	rte_eth_bond_8023ad_ext_collect;
+	rte_eth_bond_8023ad_ext_distrib;
+	rte_eth_bond_8023ad_ext_slowtx;
+} DPDK_2.1;

These symbols are not part of DPDK 2.2, the version here is wrong.
Technically it would not actually matter much but better not to confuse
things unnecessarily.

	- Panu -

It looks like Panu's points are valid, a V3 of this patch set which takes care of these issues will be needed.

Patches 1/6, 5/6 and 6/6 of the patch set are bug fixes, so each patch should contain a fixes line.
Patches 2/6, 3/6 and 4/6 are a new feature, the release notes should be updated for this feature.

Could I suggest splitting the patch set into two patch sets, a bug fix patch set and a new feature patch set.

Regards,

Bernard.

Bernard, a v3 is on the way.  I included only things that are fixes,
but the patch set doesn't quite match the set of patch numbers you listed
above.  1/6 (bond: use existing enslaved device queues) is an improvement,
but doesn't really fix anything that was broken, so I left that out.
2/6 (bond mode 4: copy entire config structure) and 3/6 (bond mode 4:
do not ignore multicast) fix bugs and are included.


Eric