List comments

GET /api/patches/418/comments/
Content-Type: application/json
Vary: Accept

        "id": 834,
        "web_url": "",
        "msgid": "<20140917145102.GC4213@localhost.localdomain>",
        "date": "2014-09-17T14:51:02",
        "subject": "Re: [dpdk-dev] [PATCH 1/2] bond: extract common code to separate\n\tfunctions",
        "submitter": {
            "id": 32,
            "url": "",
            "name": "Neil Horman",
            "email": ""
        "content": "On Wed, Sep 17, 2014 at 03:21:52PM +0100, Pawel Wodkowski wrote:\n> Signed-off-by: Pawel Wodkowski <>\n> Reviewed-by: Declan Doherty <>\n> ---\n>  lib/librte_pmd_bond/rte_eth_bond_api.c     |   59 +++++++++++++++++++++-------\n>  lib/librte_pmd_bond/rte_eth_bond_pmd.c     |   47 ++++++++++++++--------\n>  lib/librte_pmd_bond/rte_eth_bond_private.h |   30 ++++++++++++--\n>  3 files changed, 102 insertions(+), 34 deletions(-)\n> \n> diff --git a/lib/librte_pmd_bond/rte_eth_bond_api.c b/lib/librte_pmd_bond/rte_eth_bond_api.c\n> index dd33119..460df65 100644\n> --- a/lib/librte_pmd_bond/rte_eth_bond_api.c\n> +++ b/lib/librte_pmd_bond/rte_eth_bond_api.c\n> @@ -31,6 +31,8 @@\n>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n>   */\n>  \n> +#include <string.h>\n> +\n>  #include <rte_mbuf.h>\n>  #include <rte_malloc.h>\n>  #include <rte_ethdev.h>\n> @@ -104,6 +106,39 @@ valid_slave_port_id(uint8_t port_id)\n>  \treturn 0;\n>  }\n>  \n> +void\n> +rte_eth_bond_activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id )\n> +{\n> +\tstruct bond_dev_private *internals = eth_dev->data->dev_private;\n> +\tuint8_t active_count = internals->active_slave_count;\n> +\n> +\tinternals->active_slaves[active_count] = port_id;\n> +\n> +\n> +\tinternals->active_slave_count = active_count + 1;\n> +}\n> +\n> +void\n> +rte_eth_bond_deactive_slave(struct rte_eth_dev *eth_dev,\nI think you intended for this to be deactivate, not deactive, yes?\n\n\n> +\tuint8_t slave_pos )\n> +{\n> +\tstruct bond_dev_private *internals = eth_dev->data->dev_private;\n> +\tuint8_t active_count = internals->active_slave_count;\n> +\n> +\tactive_count--;\n> +\n> +\t/* If slave was not at the end of the list\n> +\t * shift active slaves up active array list */\n> +\tif (slave_pos < active_count) {\n> +\t\tmemmove(internals->active_slaves + slave_pos,\n> +\t\t\t\tinternals->active_slaves + slave_pos + 1,\n> +\t\t\t\t(active_count - slave_pos) *\n> +\t\t\t\t\tsizeof(internals->active_slaves[0]));\n> +\t}\n> +\n> +\tinternals->active_slave_count = active_count;\n> +}\n> +\n>  uint8_t\n>  number_of_sockets(void)\nStatic?\n\n>  {\n> @@ -356,10 +391,8 @@ rte_eth_bond_slave_add(uint8_t bonded_port_id, uint8_t slave_port_id)\n>  \tif (bonded_eth_dev->data->dev_started) {\n>  \t\trte_eth_link_get_nowait(slave_port_id, &link_props);\n>  \n> -\t\t if (link_props.link_status == 1) {\n> -\t\t\tinternals->active_slaves[internals->active_slave_count++] =\n> -\t\t\t\t\tslave_port_id;\n> -\t\t}\n> +\t\t if (link_props.link_status == 1)\n> +\t\t\trte_eth_bond_activate_slave(bonded_eth_dev, slave_port_id);\n>  \t}\n>  \n>  \treturn 0;\n> @@ -373,6 +406,7 @@ err_add:\n>  int\n>  rte_eth_bond_slave_remove(uint8_t bonded_port_id, uint8_t slave_port_id)\n>  {\n> +\tstruct rte_eth_dev *eth_dev;\n>  \tstruct bond_dev_private *internals;\n>  \tstruct slave_conf *slave_conf;\n>  \n> @@ -386,20 +420,15 @@ rte_eth_bond_slave_remove(uint8_t bonded_port_id, uint8_t slave_port_id)\n>  \tif (valid_slave_port_id(slave_port_id) != 0)\n>  \t\tgoto err_del;\n>  \n> -\tinternals = rte_eth_devices[bonded_port_id].data->dev_private;\n> +\teth_dev = &rte_eth_devices[bonded_port_id];\n> +\tinternals = eth_dev->data->dev_private;\n>  \n>  \t/* first remove from active slave list */\n> -\tfor (i = 0; i < internals->active_slave_count; i++) {\n> -\t\tif (internals->active_slaves[i] == slave_port_id)\n> -\t\t\tpos = i;\n> -\n> -\t\t/* shift active slaves up active array list */\n> -\t\tif (pos >= 0 && i < (internals->active_slave_count - 1))\n> -\t\t\tinternals->active_slaves[i] = internals->active_slaves[i+1];\n> -\t}\n> +\tpos = find_slave_by_id(internals->active_slaves, internals->active_slave_count,\n> +\t\t\tslave_port_id);\n>  \n> -\tif (pos >= 0)\n> -\t\tinternals->active_slave_count--;\n> +\tif (pos < internals->active_slave_count)\n> +\t\trte_eth_bond_deactive_slave(eth_dev, pos);\n>  \n>  \tpos = -1;\n>  \t/* now remove from slave list */\n> diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c b/lib/librte_pmd_bond/rte_eth_bond_pmd.c\n> index 38cc1ae..482ddb8 100644\n> --- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c\n> +++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c\n> @@ -447,6 +447,27 @@ link_properties_valid(struct rte_eth_link *bonded_dev_link,\n>  }\n>  \n>  int\n> +mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr)\n> +{\n> +\tstruct ether_addr *mac_addr;\n> +\n> +\tmac_addr = eth_dev->data->mac_addrs;\n> +\n> +\tif (eth_dev == NULL) {\n> +\t\tRTE_LOG(ERR, PMD, \"%s: NULL pointer eth_dev specified\\n\", __func__);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tif (dst_mac_addr == NULL) {\n> +\t\tRTE_LOG(ERR, PMD, \"%s: NULL pointer MAC specified\\n\", __func__);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tether_addr_copy(mac_addr, dst_mac_addr);\n> +\treturn 0;\n> +}\n> +\n> +int\n>  mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)\n>  {\n>  \tstruct ether_addr *mac_addr;\n> @@ -817,7 +838,7 @@ bond_ethdev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,\n>  \t\t\t\t\t0, dev->pci_dev->numa_node);\n>  \n>  \tif (bd_tx_q == NULL)\n> -\t\t\treturn -1;\n> +\t\treturn -1;\n>  \n>  \tbd_tx_q->queue_id = tx_queue_id;\n>  \tbd_tx_q->dev_private = dev->data->dev_private;\n> @@ -940,7 +961,6 @@ bond_ethdev_promiscuous_enable(struct rte_eth_dev *eth_dev)\n>  \tcase BONDING_MODE_ACTIVE_BACKUP:\n>  \tdefault:\n>  \t\trte_eth_promiscuous_enable(internals->current_primary_port);\n> -\n>  \t}\n>  }\n>  \n> @@ -975,7 +995,8 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,\n>  \tstruct bond_dev_private *internals;\n>  \tstruct rte_eth_link link;\n>  \n> -\tint i, valid_slave = 0, active_pos = -1;\n> +\tint i, valid_slave = 0;\n> +\tuint8_t active_pos;\n>  \tuint8_t lsc_flag = 0;\n>  \n>  \tif (type != RTE_ETH_EVENT_INTR_LSC || param == NULL)\n> @@ -1005,16 +1026,12 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,\n>  \t\treturn;\n>  \n>  \t/* Search for port in active port list */\n> -\tfor (i = 0; i < internals->active_slave_count; i++) {\n> -\t\tif (port_id == internals->active_slaves[i]) {\n> -\t\t\tactive_pos = i;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n> +\tactive_pos = find_slave_by_id(internals->active_slaves,\n> +\t\t\tinternals->active_slave_count, port_id);\n>  \n>  \trte_eth_link_get_nowait(port_id, &link);\n>  \tif (link.link_status) {\n> -\t\tif (active_pos >= 0)\n> +\t\tif (active_pos < internals->active_slave_count)\n>  \t\t\treturn;\n>  \n>  \t\t/* if no active slave ports then set this port to be primary port */\n> @@ -1028,21 +1045,19 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,\n>  \t\t\tlink_properties_set(bonded_eth_dev,\n>  \t\t\t\t\t&(slave_eth_dev->data->dev_link));\n>  \t\t}\n> -\t\tinternals->active_slaves[internals->active_slave_count++] = port_id;\n> +\n> +\t\trte_eth_bond_activate_slave(bonded_eth_dev, port_id);\n>  \n>  \t\t/* If user has defined the primary port then default to using it */\n>  \t\tif (internals->user_defined_primary_port &&\n>  \t\t\t\tinternals->primary_port == port_id)\n>  \t\t\tbond_ethdev_primary_set(internals, port_id);\n>  \t} else {\n> -\t\tif (active_pos < 0)\n> +\t\tif (active_pos == internals->active_slave_count)\n>  \t\t\treturn;\n>  \n>  \t\t/* Remove from active slave list */\n> -\t\tfor (i = active_pos; i < (internals->active_slave_count - 1); i++)\n> -\t\t\tinternals->active_slaves[i] = internals->active_slaves[i+1];\n> -\n> -\t\tinternals->active_slave_count--;\n> +\t\trte_eth_bond_deactive_slave(bonded_eth_dev, active_pos);\n>  \n>  \t\t/* No active slaves, change link status to down and reset other\n>  \t\t * link properties */\n> diff --git a/lib/librte_pmd_bond/rte_eth_bond_private.h b/lib/librte_pmd_bond/rte_eth_bond_private.h\n> index 60f1e8d..6742a4c 100644\n> --- a/lib/librte_pmd_bond/rte_eth_bond_private.h\n> +++ b/lib/librte_pmd_bond/rte_eth_bond_private.h\n> @@ -115,11 +115,11 @@ struct bond_dev_private {\n>  \tuint16_t nb_rx_queues;\t\t\t/**< Total number of rx queues */\n>  \tuint16_t nb_tx_queues;\t\t\t/**< Total number of tx queues*/\n>  \n> -\tuint8_t slave_count;\t\t\t/**< Number of active slaves */\n> -\tuint8_t active_slave_count;\t\t/**< Number of slaves */\n> +\tuint8_t slave_count;\t\t\t/**< Number of slaves */\n> +\tuint8_t active_slave_count;\t\t/**< Number of active slaves */\n>  \n> -\tuint8_t active_slaves[RTE_MAX_ETHPORTS];\t/**< Active slave list */\n>  \tuint8_t slaves[RTE_MAX_ETHPORTS];\t\t\t/**< Slave list */\n> +\tuint8_t active_slaves[RTE_MAX_ETHPORTS];\t/**< Active slave list */\n>  \n>  \t/** Persisted configuration of slaves */\n>  \tstruct slave_conf presisted_slaves_conf[RTE_MAX_ETHPORTS];\n> @@ -130,6 +130,19 @@ extern struct eth_dev_ops default_dev_ops;\n>  int\n>  valid_bonded_ethdev(struct rte_eth_dev *eth_dev);\n>  \n> +static inline uint8_t\n> +find_slave_by_id(uint8_t *slaves, uint8_t slaves_count,\n> +\tuint8_t slave_id ) {\n> +\n> +\tuint8_t pos;\n> +\tfor (pos = 0; pos < slaves_count; pos++) {\n> +\t\tif (slave_id == slaves[pos])\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\treturn pos;\n> +}\n> +\n>  int\n>  valid_port_id(uint8_t port_id);\n>  \n> @@ -140,6 +153,14 @@ int\n>  valid_slave_port_id(uint8_t port_id);\n>  \n>  void\n> +rte_eth_bond_deactive_slave(struct rte_eth_dev *eth_dev,\n> +\tuint8_t slave_pos );\n> +\n> +void\n> +rte_eth_bond_activate_slave(struct rte_eth_dev *eth_dev,\n> +\tuint8_t port_id );\n> +\n> +void\n>  link_properties_set(struct rte_eth_dev *bonded_eth_dev,\n>  \t\tstruct rte_eth_link *slave_dev_link);\n>  void\n> @@ -153,6 +174,9 @@ int\n>  mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr);\n>  \n>  int\n> +mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr);\n> +\n> +int\n>  mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev);\n>  \n>  uint8_t\n> -- \n>\n> \n>",
        "headers": {
            "List-Subscribe": "<>,\n\t<>",
            "Cc": "",
            "Content-Type": "text/plain; charset=us-ascii",
            "X-Original-To": "",
            "Date": "Wed, 17 Sep 2014 10:51:02 -0400",
            "List-Help": "<>",
            "In-Reply-To": "<>",
            "Precedence": "list",
            "X-BeenThere": "",
            "References": "<>\n\t<>",
            "Content-Disposition": "inline",
            "X-Spam-Status": "No",
            "To": "Pawel Wodkowski <>",
            "User-Agent": "Mutt/1.5.23 (2014-03-12)",
            "Errors-To": "",
            "MIME-Version": "1.0",
            "Received": [
                "from [] (localhost [IPv6:::1])\n\tby (Postfix) with ESMTP id 9D96DB3C0;\n\tWed, 17 Sep 2014 16:45:35 +0200 (CEST)",
                "from ( [])\n\tby (Postfix) with ESMTP id DD094B3BF\n\tfor <>; Wed, 17 Sep 2014 16:45:33 +0200 (CEST)",
                "from ([] helo=localhost)\n\tby with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63)\n\t(envelope-from <>)\n\tid 1XUGZY-0000yf-79; Wed, 17 Sep 2014 10:51:14 -0400"
            "From": "Neil Horman <>",
            "Message-ID": "<20140917145102.GC4213@localhost.localdomain>",
            "List-Post": "<>",
            "X-Spam-Score": "-2.9 (--)",
            "Sender": "\"dev\" <>",
            "List-Id": "patches and discussions about DPDK <>",
            "Subject": "Re: [dpdk-dev] [PATCH 1/2] bond: extract common code to separate\n\tfunctions",
            "List-Archive": "<>",
            "List-Unsubscribe": "<>,\n\t<>",
            "Return-Path": "<>",
            "Delivered-To": "",
            "X-Mailman-Version": "2.1.15"