ethdev: add comment to warn of ABI breakage

PATCH Superseded Ciara Power 4 participants 6 messages Feb 18, 2020

Share & tools

Ciara Power

author

2020-02-18 10:57 UTC #0 lore raw

PATCH 1/1 ethdev: add comment to warn of ABI breakage

If a function is added to the eth_dev_ops struct before tx_descriptor_status function, this will cause ABI breakage. This is due to static inline functions using this function, and some other functions above it in the struct, so they cannot change position. A comment is added to inform developers of this possible breakage.

Signed-off-by Ciara Power <[email protected]>

---
 lib/librte_ethdev/rte_ethdev_core.h | 3 +++
 1 file changed, 3 insertions(+)

lib/librte_ethdev/rte_ethdev_core.h +3 -0

index 7bf97e24e..1ad04a129 100644
@@ -667,6 +667,9 @@ struct eth_dev_ops {
 	/**< Check the status of a Rx descriptor. */
 	eth_tx_descriptor_status_t tx_descriptor_status;
 	/**< Check the status of a Tx descriptor. */
+	/* Static inline functions use functions above this comment.
+	 * New dev_ops functions should be added below to avoid breaking ABI.
+	 */
 	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
 	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
 	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */

Signature · 1 line

2.17.1

Ferruh Yigit

Reviewed-by on v1 1/1

2020-02-18 11:59 UTC #1 lore raw

Quoted 17 lines · Ciara Power

If a function is added to the eth_dev_ops struct before tx_descriptor_status function, this will cause ABI breakage. This is due to static inline functions using this function, and some other functions above it in the struct, so they cannot change position. A comment is added to inform developers of this possible breakage.
Signed-off-by Ciara Power <[email protected]>
---
 lib/librte_ethdev/rte_ethdev_core.h | 3 +++
 1 file changed, 3 insertions(+)
lib/librte_ethdev/rte_ethdev_core.h +0 -0
index 7bf97e24e..1ad04a129 100644

lib/librte_ethdev/rte_ethdev_core.h struct eth_dev_ops { · line 675

Quoted 9 lines

lib/librte_ethdev/rte_ethdev_core.h +3 -0

@@ -667,6 +667,9 @@ struct eth_dev_ops {
 	/**< Check the status of a Rx descriptor. */
 	eth_tx_descriptor_status_t tx_descriptor_status;
 	/**< Check the status of a Tx descriptor. */
+	/* Static inline functions use functions above this comment.
+	 * New dev_ops functions should be added below to avoid breaking ABI.
+	 */
 	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
 	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
 	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */

Reviewed-by Ferruh Yigit <[email protected]>

Andrew Rybchenko

Reviewed-by

2020-02-18 13:13 UTC #2 lore raw

Quoted 21 lines · Ferruh Yigit

Quoted 20 lines · Ciara Power
If a function is added to the eth_dev_ops struct before tx_descriptor_status function, this will cause ABI breakage. This is due to static inline functions using this function, and some other functions above it in the struct, so they cannot change position. A comment is added to inform developers of this possible breakage.
Signed-off-by Ciara Power <[email protected]>
---
 lib/librte_ethdev/rte_ethdev_core.h | 3 +++
 1 file changed, 3 insertions(+)
lib/librte_ethdev/rte_ethdev_core.h +1 -0
index 7bf97e24e..1ad04a129 100644
@@ -667,6 +667,9 @@ struct eth_dev_ops {
 	/**< Check the status of a Rx descriptor. */
 	eth_tx_descriptor_status_t tx_descriptor_status;
 	/**< Check the status of a Tx descriptor. */
+	/* Static inline functions use functions above this comment.

Quoted 8 lines

lib/librte_ethdev/rte_ethdev_core.h struct eth_dev_ops { · line 675

Quoted 6 lines

lib/librte_ethdev/rte_ethdev_core.h +2 -0

+	 * New dev_ops functions should be added below to avoid breaking ABI.
+	 */
 	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
 	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
 	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */

Reviewed-by Ferruh Yigit <[email protected]>

LGTM, but may I suggest to make the first line in the comment empty to make it easier to notice. I.e.
/*
* Static inline functions use functions above this comment.
* ...
*/

Or even add empty lines before and after the comment.

Reviewed-by Andrew Rybchenko <[email protected]>

Thomas Monjalon

Acked-by

2020-02-18 14:20 UTC #3 lore raw

18/02/2020 14:13, Andrew Rybchenko:

Quoted 38 lines · Ferruh Yigit

Quoted 21 lines · Ciara Power
Quoted 20 lines
If a function is added to the eth_dev_ops struct before tx_descriptor_status function, this will cause ABI breakage. This is due to static inline functions using this function, and some other functions above it in the struct, so they cannot change position. A comment is added to inform developers of this possible breakage.
Signed-off-by Ciara Power <[email protected]>
---
 lib/librte_ethdev/rte_ethdev_core.h | 3 +++
 1 file changed, 3 insertions(+)
lib/librte_ethdev/rte_ethdev_core.h +1 -0
index 7bf97e24e..1ad04a129 100644
@@ -667,6 +667,9 @@ struct eth_dev_ops {
 	/**< Check the status of a Rx descriptor. */
 	eth_tx_descriptor_status_t tx_descriptor_status;
 	/**< Check the status of a Tx descriptor. */
+	/* Static inline functions use functions above this comment.
Quoted 8 lines
lib/librte_ethdev/rte_ethdev_core.h struct eth_dev_ops { · line 675
Quoted 6 lines
lib/librte_ethdev/rte_ethdev_core.h +2 -0
+	 * New dev_ops functions should be added below to avoid breaking ABI.
+	 */
 	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
 	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
 	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */
Reviewed-by Ferruh Yigit <[email protected]>
LGTM, but may I suggest to make the first line in the comment empty to make it easier to notice. I.e.
/*
* Static inline functions use functions above this comment.
* ...
*/

Quoted 3 lines

Or even add empty lines before and after the comment.
Reviewed-by Andrew Rybchenko <[email protected]>

In the same idea as Andrew, I would suggest some uppercases, maybe "ABOVE" and "BELOW".

Acked-by Thomas Monjalon <[email protected]>

Ciara Power

author

2020-02-18 13:51 UTC #4 lore raw

PATCH v2 1/1 ethdev: add comment to warn of ABI breakage

In reply to Ciara Power

Signed-off-by Ciara Power <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

Reviewed-by Andrew Rybchenko <[email protected]>

---
V2: Added empty first line in comment

---
 lib/librte_ethdev/rte_ethdev_core.h | 4 ++++
 1 file changed, 4 insertions(+)

lib/librte_ethdev/rte_ethdev_core.h +4 -0

index 7bf97e24e..9933ffdc1 100644
@@ -667,6 +667,10 @@ struct eth_dev_ops {
 	/**< Check the status of a Rx descriptor. */
 	eth_tx_descriptor_status_t tx_descriptor_status;
 	/**< Check the status of a Tx descriptor. */
+	/*
+	 * Static inline functions use functions above this comment.
+	 * New dev_ops functions should be added below to avoid breaking ABI.
+	 */
 	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
 	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
 	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */

Signature · 1 line

2.17.1

Ferruh Yigit

Acked-by Thomas Monjalon on v2 1/1 Applied

2020-02-18 18:40 UTC #5 lore raw

Quoted 9 lines · Ciara Power

If a function is added to the eth_dev_ops struct before tx_descriptor_status function, this will cause ABI breakage. This is due to static inline functions using this function, and some other functions above it in the struct, so they cannot change position. A comment is added to inform developers of this possible breakage.
Signed-off-by Ciara Power <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>
Reviewed-by Andrew Rybchenko <[email protected]>

Acked-by Thomas Monjalon <[email protected]>

Applied to dpdk-next-net/master, thanks.

(uppercase ABOVE/BELOW as suggested applied while merging)