Patch Comment List

HTTP 200 OK
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Link: 
 <http://patches.dpdk.org/api/patches/419/comments/?format=api&page=1>; rel="first", 
 <http://patches.dpdk.org/api/patches/419/comments/?format=api&page=1>; rel="last"
Vary: Accept

[
    {
        "id": 835,
        "web_url": "http://patches.dpdk.org/comment/835/",
        "msgid": "<20140917151304.GD4213@localhost.localdomain>",
        "list_archive_url": "https://inbox.dpdk.org/dev/20140917151304.GD4213@localhost.localdomain",
        "date": "2014-09-17T15:13:04",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 32,
            "url": "http://patches.dpdk.org/api/people/32/?format=api",
            "name": "Neil Horman",
            "email": "nhorman@tuxdriver.com"
        },
        "content": "On Wed, Sep 17, 2014 at 03:21:53PM +0100, Pawel Wodkowski wrote:\n> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>\n> Signed-off-by: Maciej T Gajdzica <maciejx.t.gajdzica@intel.com>\n> Reviewed-by: Declan Doherty <declan.doherty@intel.com>\n> ---\n>  lib/librte_ether/rte_ether.h               |    1 +\n>  lib/librte_pmd_bond/Makefile               |    1 +\n>  lib/librte_pmd_bond/rte_eth_bond.h         |    4 +\n>  lib/librte_pmd_bond/rte_eth_bond_8023ad.c  | 1064 ++++++++++++++++++++++++++++\n>  lib/librte_pmd_bond/rte_eth_bond_8023ad.h  |  411 +++++++++++\n>  lib/librte_pmd_bond/rte_eth_bond_api.c     |   28 +-\n>  lib/librte_pmd_bond/rte_eth_bond_args.c    |    1 +\n>  lib/librte_pmd_bond/rte_eth_bond_pmd.c     |  179 ++++-\n>  lib/librte_pmd_bond/rte_eth_bond_private.h |    9 +-\n>  9 files changed, 1685 insertions(+), 13 deletions(-)\n>  create mode 100644 lib/librte_pmd_bond/rte_eth_bond_8023ad.c\n>  create mode 100644 lib/librte_pmd_bond/rte_eth_bond_8023ad.h\n> \n> diff --git a/lib/librte_ether/rte_ether.h b/lib/librte_ether/rte_ether.h\n> index 2e08f23..1a3711b 100644\n> --- a/lib/librte_ether/rte_ether.h\n> +++ b/lib/librte_ether/rte_ether.h\n> @@ -293,6 +293,7 @@ struct vlan_hdr {\n>  #define ETHER_TYPE_RARP 0x8035 /**< Reverse Arp Protocol. */\n>  #define ETHER_TYPE_VLAN 0x8100 /**< IEEE 802.1Q VLAN tagging. */\n>  #define ETHER_TYPE_1588 0x88F7 /**< IEEE 802.1AS 1588 Precise Time Protocol. */\n> +#define ETHER_TYPE_SLOW 0x8809 /**< Slow protocols (LACP and Marker). */\n>  \n>  #ifdef __cplusplus\n>  }\n> diff --git a/lib/librte_pmd_bond/Makefile b/lib/librte_pmd_bond/Makefile\n> index 953d75e..c2312c2 100644\n> --- a/lib/librte_pmd_bond/Makefile\n> +++ b/lib/librte_pmd_bond/Makefile\n> @@ -44,6 +44,7 @@ CFLAGS += $(WERROR_FLAGS)\n>  #\n>  SRCS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += rte_eth_bond_api.c\n>  SRCS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += rte_eth_bond_pmd.c\n> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += rte_eth_bond_8023ad.c\n>  SRCS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += rte_eth_bond_args.c\n>  \n>  #\n> diff --git a/lib/librte_pmd_bond/rte_eth_bond.h b/lib/librte_pmd_bond/rte_eth_bond.h\n> index bd59780..6aac4ec 100644\n> --- a/lib/librte_pmd_bond/rte_eth_bond.h\n> +++ b/lib/librte_pmd_bond/rte_eth_bond.h\n> @@ -75,6 +75,10 @@ extern \"C\" {\n>  /**< Broadcast (Mode 3).\n>   * In this mode all transmitted packets will be transmitted on all available\n>   * active slaves of the bonded. */\n> +#define BONDING_MODE_8023AD\t\t\t\t(4)\n> +/**< 802.3AD (Mode 4).\n> + * In this mode transmission and reception of packets is managed by LACP\n> + * protocol specified in 802.3AD documentation. */\n>  \n>  /* Balance Mode Transmit Policies */\n>  #define BALANCE_XMIT_POLICY_LAYER2\t\t(0)\n> diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c\n> new file mode 100644\n> index 0000000..6ce6efb\n> --- /dev/null\n> +++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c\n> @@ -0,0 +1,1064 @@\n> +/*-\n> + *   BSD LICENSE\n> + *\n> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.\n> + *   All rights reserved.\n> + *\n> + *   Redistribution and use in source and binary forms, with or without\n> + *   modification, are permitted provided that the following conditions\n> + *   are met:\n> + *\n> + *     * Redistributions of source code must retain the above copyright\n> + *       notice, this list of conditions and the following disclaimer.\n> + *     * Redistributions in binary form must reproduce the above copyright\n> + *       notice, this list of conditions and the following disclaimer in\n> + *       the documentation and/or other materials provided with the\n> + *       distribution.\n> + *     * Neither the name of Intel Corporation nor the names of its\n> + *       contributors may be used to endorse or promote products derived\n> + *       from this software without specific prior written permission.\n> + *\n> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS\n> + *   \"AS IS\" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT\n> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR\n> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT\n> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,\n> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT\n> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,\n> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY\n> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT\n> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE\n> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n> + */\n> +\n> +#include <stddef.h>\n> +#include <string.h>\n> +\n> +#include <rte_alarm.h>\n> +#include <rte_malloc.h>\n> +#include <rte_errno.h>\n> +\n> +#include \"rte_eth_bond_private.h\"\n> +#include \"rte_eth_bond_8023ad.h\"\n> +\n> +#include <rte_cycles.h>\n> +\n> +#define RTE_LIBRTE_BOND_DEBUG_8023AX\n> +\n> +#ifdef RTE_LIBRTE_BOND_DEBUG_8023AX\n> +#define BOND_ASSERT(expr) \\\n> +\t((expr) ? (void) (0) \\\n> +\t: rte_panic(\"%s(%d): assertion failed\" __STRING(expr), __FILE__, __LINE__))\n> +#else\n> +#define BOND_ASSERT(expr) do { } while (0)\n> +#endif\nIt seems like every library re-invents this wheel.  Maybe merge them into a\nsingle assert macro available from a common header?\n\n\n> +\n> +#ifdef RTE_LIBRTE_BOND_DEBUG_8023AX\n> +#define _PORT_ID internals->active_slaves[port_num]\n> +#define BOND_DEBUG(fmt, ...) RTE_LOG(DEBUG, PMD, \"%6u [Port %u: %s] \" fmt, \\\n> +\t\t\tbond_dbg_get_time_diff(), _PORT_ID, __FUNCTION__, ##__VA_ARGS__)\n> +\n> +static unsigned\n> +bond_dbg_get_time_diff(void)\n> +{\n> +\tstatic unsigned ms_start = 0;\n> +\tstruct timespec t;\n> +\tuint64_t ms;\n> +\n> +\tclock_gettime(CLOCK_MONOTONIC, &t);\n> +\tms = (((long)t.tv_sec * 1000000000L) + t.tv_nsec) / 1000000L;\n> +\n> +\tif (ms_start == 0)\n> +\t\tms_start = ms;\n> +\n> +\treturn ms - ms_start;\n> +}\n> +\n> +static void\n> +bond_print_lacp(struct lacpdu *l)\n> +{\n> +\tchar a_state[256] = { 0 };\n> +\tchar p_state[256] = { 0 };\n> +\tstatic const char *state_labels[] = {\n> +\t\t\"ACT\", \"TIMEOUT\", \"AGG\", \"SYNC\", \"COL\", \"DIST\", \"DEF\", \"EXP\"\n> +\t};\n> +\n> +\tint a_len = 0;\n> +\tint p_len = 0;\n> +\tuint8_t i;\n> +\n> +\tfor (i = 0; i < 8; i++) {\n> +\t\tif ((l->actor.state >> i) & 1) {\n> +\t\t\ta_len += snprintf(a_state + a_len, sizeof(a_state) - a_len, \"%s \",\n> +\t\t\t\tstate_labels[i]);\n> +\t\t}\n> +\n> +\t\tif ((l->partner.state >> i) & 1) {\n> +\t\t\tp_len += snprintf(p_state + p_len, sizeof(p_state) - p_len, \"%s \",\n> +\t\t\t\tstate_labels[i]);\n> +\t\t}\n> +\t}\n> +\n> +\tif (a_len && a_state[a_len-1] == ' ')\n> +\t\ta_state[a_len-1] = '\\0';\n> +\n> +\tif (p_len && p_state[p_len-1] == ' ')\n> +\t\t\tp_state[p_len-1] = '\\0';\n> +\n> +\tRTE_LOG(DEBUG, PMD, \"LACP: {\\n\"\\\n> +\t\t\t\"  subtype= %02X\\n\"\\\n> +\t\t\t\"  ver_num=%02X\\n\"\\\n> +\t\t\t\"  actor={ tlv=%02X, len=%02X\\n\"\\\n> +\t\t\t\"    pri=%04X, system=(ADDRESS), key=%04X, p_pri=%04X p_num=%04X\\n\"\\\n> +\t\t\t\"       state={ %s }\\n\"\\\n> +\t\t\t\"  }\\n\"\\\n> +\t\t\t\"  partner={ tlv=%02X, len=%02X\\n\"\\\n> +\t\t\t\"    pri=%04X, system=(ADDRESS), key=%04X, p_pri=%04X p_num=%04X\\n\"\\\n> +\t\t\t\"       state={ %s }\\n\"\\\n> +\t\t\t\"  }\\n\"\\\n> +\t\t\t\"  collector={info=%02X, length=%02X, max_delay=%04X\\n, \" \\\n> +\t\t\t\t\t\t\t\"type_term=%02X, terminator_length = %02X}\\n\",\\\n> +\t\t\tl->subtype,\\\n> +\t\t\tl->version_number,\\\n> +\t\t\tl->actor.tlv_type_info,\\\n> +\t\t\tl->actor.info_length,\\\n> +\t\t\tl->actor.port_params.system_priority,\\\n> +\t\t\tl->actor.port_params.key,\\\n> +\t\t\tl->actor.port_params.port_priority,\\\n> +\t\t\tl->actor.port_params.port_number,\\\n> +\t\t\ta_state,\\\n> +\t\t\tl->partner.tlv_type_info,\\\n> +\t\t\tl->partner.info_length,\\\n> +\t\t\tl->partner.port_params.system_priority,\\\n> +\t\t\tl->partner.port_params.key,\\\n> +\t\t\tl->partner.port_params.port_priority,\\\n> +\t\t\tl->partner.port_params.port_number,\\\n> +\t\t\tp_state,\\\n> +\t\t\tl->tlv_type_collector_info,\\\n> +\t\t\tl->collector_info_length,\\\n> +\t\t\tl->collector_max_delay,\\\n> +\t\t\tl->tlv_type_terminator,\\\n> +\t\t\tl->terminator_length);\n> +\n> +}\n> +#define BOND_PRINT_LACP(lacpdu) bond_print_lacp(lacpdu)\n> +\n> +#else\n> +#define BOND_PRINT_LACP(lacpdu) do { } while (0)\n> +#define BOND_DEBUG(fmt, ...) do { } while (0)\n> +#endif\n> +\n> +static const struct ether_addr lacp_mac_addr = {\n> +\t.addr_bytes = {0x01, 0x80, 0xC2, 0x00, 0x00, 0x02}\n> +};\n> +\n> +static void\n> +timer_expired_cb(void *arg)\n> +{\n> +\tenum timer_state *timer = arg;\n> +\n> +\tBOND_ASSERT(*timer == TIMER_RUNNING);\n> +\t*timer = TIMER_EXPIRED;\n> +}\n> +\n> +static void\n> +timer_cancel(enum timer_state *timer)\n> +{\n> +\trte_eal_alarm_cancel(&timer_expired_cb, timer);\n> +\t*timer = TIMER_NOT_STARTED;\n> +}\n> +\n> +static void\n> +timer_set(enum timer_state *timer, uint64_t timeout)\n> +{\n> +\trte_eal_alarm_cancel(&timer_expired_cb, timer);\n> +\trte_eal_alarm_set(timeout * 1000, &timer_expired_cb, timer);\n> +\t*timer = TIMER_RUNNING;\n> +}\n> +\n> +static bool\n> +timer_is_expired(enum timer_state *timer)\n> +{\n> +\treturn *timer == TIMER_EXPIRED;\n> +}\n> +\n> +static void\n> +record_default(struct port *port)\n> +{\n> +\t/* Record default parametes for partner. Partner admin parameters\n> +\t * are not implemented so nothing to copy. Only mark actor that parner is\n> +\t * in defaulted state. */\n> +\tport->partner_state = STATE_LACP_ACTIVE;\n> +\tACTOR_STATE_SET(port, DEFAULTED);\n> +}\n> +\n> +/** Function handles rx state machine.\n> + *\n> + * This function implements Receive State Machine from point 5.4.12 in\n> + * 802.1AX documentation. It should be called periodically.\n> + *\n> + * @param lacpdu\t\tLACPDU received.\n> + * @param port\t\t\tPort on which LACPDU was received.\n> + */\n> +static void\n> +rx_machine(struct bond_dev_private *internals, uint8_t port_num,\n> +\tstruct lacpdu *lacp)\n> +{\n> +\tstruct port *port = &internals->mode4.port_list[port_num];\n> +\n> +\tif (SM_FLAG(port, BEGIN)) {\n> +\t\t/* Initialize stuff */\n> +\t\tBOND_DEBUG(\"-> INITIALIZE\\n\");\n> +\t\tSM_FLAG_CLR(port, MOVED);\n> +\t\tport->selected = UNSELECTED;\n> +\n> +\t\trecord_default(port);\n> +\n> +\t\tACTOR_STATE_CLR(port, EXPIRED);\n> +\t\ttimer_cancel(&port->current_while_timer);\n> +\n> +\t\t/* DISABLED: On initialization partner is out of sync */\n> +\t\tPARTNER_STATE_CLR(port, SYNCHRONIZATION);\n> +\n> +\t\t/* LACP DISABLED stuff if LACP not enabled on this port */\n> +\t\tif (!SM_FLAG(port, LACP_ENABLED))\n> +\t\t\tPARTNER_STATE_CLR(port, AGGREGATION);\n> +\t}\n> +\n> +\tif (!SM_FLAG(port, LACP_ENABLED)) {\n> +\t\t/* Update parameters only if state changed */\n> +\t\tif (port->current_while_timer) {\n> +\t\t\tport->selected = UNSELECTED;\n> +\t\t\trecord_default(port);\n> +\t\t\tPARTNER_STATE_CLR(port, AGGREGATION);\n> +\t\t\tACTOR_STATE_CLR(port, EXPIRED);\n> +\t\t\ttimer_cancel(&port->current_while_timer);\n> +\t\t}\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (lacp) {\n> +\t\tBOND_DEBUG(\"LACP -> CURRENT\\n\");\n> +\t\tBOND_PRINT_LACP(lacp);\n> +\t\t/* Update selected flag. If partner parameters are defaulted assume they\n> +\t\t * are match. If not defaulted  compare LACP actor with ports parner\n> +\t\t * params. */\n> +\t\tif (!(port->actor_state & STATE_DEFAULTED) &&\n> +\t\t\t(((port->partner_state ^ lacp->actor.state) & STATE_AGGREGATION) ||\n> +\t\t\t\tmemcmp(&port->partner, &lacp->actor.port_params,\n> +\t\t\t\t\tsizeof(port->partner)) != 0)) {\n> +\t\t\tBOND_DEBUG(\"selected <- UNSELECTED\\n\");\n> +\t\t\tport->selected = UNSELECTED;\n> +\t\t}\n> +\n> +\t\t/* Record this PDU actor params as partner params */\n> +\t\tmemcpy(&port->partner, &lacp->actor.port_params,\n> +\t\t\tsizeof(struct port_params));\n> +\t\tport->partner_state = lacp->actor.state;\n> +\t\tACTOR_STATE_CLR(port, DEFAULTED);\n> +\n> +\t\t/* Update NTT if partners information are outdated */\n> +\t\tuint8_t state_mask = STATE_LACP_ACTIVE | STATE_LACP_SHORT_TIMEOUT |\n> +\t\t\tSTATE_SYNCHRONIZATION | STATE_AGGREGATION;\n> +\n> +\t\tif (((port->actor_state ^ lacp->partner.state) & state_mask) ||\n> +\t\t\t\tmemcmp(&port->actor, &lacp->partner.port_params,\n> +\t\t\t\t\tsizeof(struct port_params)) != 0) {\n> +\t\t\tport->sm_flags |= SM_FLAGS_NTT;\n> +\t\t}\n> +\n> +\t\t/* If LACP partner params match this port actor params */\n> +\t\tif (memcmp(&port->actor, &lacp->partner.port_params,\n> +\t\t\t    sizeof(port->actor)) == 0 &&\n> +\t\t\t(port->partner_state & STATE_AGGREGATION) == (port->actor_state\n> +\t\t\t    & STATE_AGGREGATION))\n> +\t\t\tPARTNER_STATE_SET(port, SYNCHRONIZATION);\n> +\t\telse if (!(port->partner_state & STATE_AGGREGATION) &&\n> +\t\t\t    (port->actor_state & STATE_AGGREGATION))\n> +\t\t\tPARTNER_STATE_SET(port, SYNCHRONIZATION);\n> +\t\telse\n> +\t\t\tPARTNER_STATE_CLR(port, SYNCHRONIZATION);\n> +\n> +\t\tif (port->actor_state & STATE_LACP_SHORT_TIMEOUT)\n> +\t\t\ttimer_set(&port->current_while_timer, BOND_8023AD_SHORT_TIMEOUT_MS);\n> +\t\telse\n> +\t\t\ttimer_set(&port->current_while_timer, BOND_8023AD_LONG_TIMEOUT_MS);\n> +\n> +\t\tACTOR_STATE_CLR(port, EXPIRED);\n> +\t\treturn; /* No state change */\n> +\t}\n> +\n> +\tif (port->current_while_timer != TIMER_RUNNING) {\n> +\t\tACTOR_STATE_SET(port, EXPIRED);\n> +\t\tPARTNER_STATE_CLR(port, SYNCHRONIZATION);\n> +\t\tPARTNER_STATE_SET(port, LACP_SHORT_TIMEOUT);\n> +\t\ttimer_set(&port->current_while_timer, BOND_8023AD_SHORT_TIMEOUT_MS);\n> +\t}\n> +}\n> +\n> +/**\n> + * Function handles periodic tx state machine.\n> + *\n> + * Function implements Periodic Transmission state machine from point 5.4.13\n> + * in 802.1AX documentation. It should be called periodically.\n> + *\n> + * @param port\t\t\tPort to handle state machine.\n> + */\n> +static void\n> +periodic_machine(struct bond_dev_private *internals, uint8_t port_num)\n> +{\n> +\tstruct port *port = &internals->mode4.port_list[port_num];\n> +\t/* Calculate if either site is LACP enabled */\n> +\tuint32_t timeout;\n> +\tuint16_t sm_flags = port->sm_flags;\n> +\tuint8_t active = ACTOR_STATE(port, LACP_ACTIVE) ||\n> +\t\tPARTNER_STATE(port, LACP_ACTIVE);\n> +\n> +\tuint8_t is_partner_fast, was_partner_fast;\n> +\t/* No periodic is on BEGIN, LACP DISABLE or when both sides are pasive */\n> +\tif ((sm_flags & SM_FLAGS_BEGIN) || !(sm_flags & SM_FLAGS_LACP_ENABLED) ||\n> +\t\t    active == 0) {\n> +\t\ttimer_cancel(&port->periodic_timer);\n> +\t\tport->tx_machine_timer = TIMER_EXPIRED;\n> +\t\tsm_flags &= ~SM_FLAGS_PARTNER_SHORT_TIMEOUT;\n> +\t\tport->sm_flags = sm_flags;\n> +\n> +\t\tBOND_DEBUG(\"-> NO_PERIODIC ( %s%s%s)\\n\",\n> +\t\t\tSM_FLAG(port, BEGIN) ? \"begind \" : \"\",\n> +\t\t\tSM_FLAG(port, LACP_ENABLED) ? \"\" : \"LACP disabled \",\n> +\t\t\tactive ? \"LACP active \" : \"LACP pasive \");\n> +\t\treturn;\n> +\t}\n> +\n> +\tis_partner_fast = !!(port->partner_state & STATE_LACP_SHORT_TIMEOUT);\n> +\twas_partner_fast = !!(port->sm_flags & SM_FLAGS_PARTNER_SHORT_TIMEOUT);\n> +\n> +\t/* If periodic timer is not started, transit from NO PERIODIC to FAST/SLOW.\n> +\t * Other case: check if timer expire or partners settings changed. */\n> +\tif (port->periodic_timer != TIMER_NOT_STARTED) {\n> +\t\tif (timer_is_expired(&port->periodic_timer))\n> +\t\t\tsm_flags |= SM_FLAGS_NTT;\n> +\t\telse if (is_partner_fast != was_partner_fast) {\n> +\t\t\t/* Partners timeout  was slow and now it is fast -> send LACP.\n> +\t\t\t * In other case (was fast and now it is slow) just switch\n> +\t\t\t * timeout to slow without forcing send of LACP (because standard\n> +\t\t\t * say so)*/\n> +\t\t\tif (!is_partner_fast)\n> +\t\t\t\tsm_flags |= SM_FLAGS_NTT;\n> +\t\t} else\n> +\t\t\treturn; /* Nothing changed */\n> +\t}\n> +\n> +\t/* Handle state transition to FAST/SLOW LACP timeout */\n> +\tif (is_partner_fast) {\n> +\t\ttimeout = BOND_8023AD_FAST_PERIODIC_MS;\n> +\t\tsm_flags |= SM_FLAGS_PARTNER_SHORT_TIMEOUT;\n> +\t} else {\n> +\t\ttimeout = BOND_8023AD_SLOW_PERIODIC_MS;\n> +\t\tsm_flags &= ~SM_FLAGS_PARTNER_SHORT_TIMEOUT;\n> +\t}\n> +\n> +\ttimer_set(&port->periodic_timer, timeout);\n> +\tport->sm_flags = sm_flags;\n> +}\n> +\n> +/**\n> + * Function handles mux state machine.\n> + *\n> + * Function implements Mux Machine from point 5.4.15 in 802.1AX documentation.\n> + * It should be called periodically.\n> + *\n> + * @param port\t\t\tPort to handle state machine.\n> + */\n> +static int\n> +mux_machine(struct bond_dev_private *internals, uint8_t port_num)\n> +{\n> +\tbool ntt = false;\n> +\tstruct port *port = &internals->mode4.port_list[port_num];\n> +\n> +\t/* Save current state for later use */\n> +\tconst uint8_t state_mask = STATE_SYNCHRONIZATION | STATE_DISTRIBUTING |\n> +\t\tSTATE_COLLECTING;\n> +\n> +\t/* Enter DETACHED state on BEGIN condition or from any other state if\n> +\t * port was unselected */\n> +\tif (SM_FLAG(port, BEGIN) ||\n> +\t\t\tport->selected == UNSELECTED || (port->selected == STANDBY &&\n> +\t\t\t\t(port->actor_state & state_mask) != 0)) {\n> +\t\t/* detach mux from aggregator not used */\n> +\t\tport->actor_state &= ~state_mask;\n> +\t\t/* Set ntt to true if BEGIN condition or transition from any other state\n> +\t\t * which is indicated that wait_while_timer was started */\n> +\t\tif (SM_FLAG(port, BEGIN) ||\n> +\t\t\t\tport->wait_while_timer != TIMER_NOT_STARTED) {\n> +\t\t\tSM_FLAG_SET(port, NTT);\n> +\t\t\tBOND_DEBUG(\"-> DETACHED\\n\");\n> +\t\t}\n> +\t\ttimer_cancel(&port->wait_while_timer);\n> +\t}\n> +\n> +\tif (port->wait_while_timer == TIMER_NOT_STARTED) {\n> +\t\tif (port->selected == SELECTED || port->selected == STANDBY) {\n> +\t\t\ttimer_set(&port->wait_while_timer,\n> +\t\t\t\tBOND_8023AD_AGGREGATE_WAIT_TIMEOUT_MS);\n> +\n> +\t\t\tBOND_DEBUG(\"DETACHED -> WAITING\\n\");\n> +\t\t}\n> +\t\t/* Waiting state entered */\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* Transit next state if port is ready */\n> +\tif (!timer_is_expired(&port->wait_while_timer))\n> +\t\treturn 0;\n> +\n> +\tif ((ACTOR_STATE(port, DISTRIBUTING) || ACTOR_STATE(port, COLLECTING)) &&\n> +\t\t!PARTNER_STATE(port, SYNCHRONIZATION)) {\n> +\t\t/* If in COLLECTING or DISTRIBUTING state and partner becomes out of\n> +\t\t * sync transit to ATACHED state.  */\n> +\t\tACTOR_STATE_CLR(port, DISTRIBUTING);\n> +\t\tACTOR_STATE_CLR(port, COLLECTING);\n> +\t\t/* Clear actor sync to activate transit ATACHED in condition bellow */\n> +\t\tACTOR_STATE_CLR(port, SYNCHRONIZATION);\n> +\t\tBOND_DEBUG(\"Out of sync -> ATTACHED\\n\");\n> +\t} else if (!ACTOR_STATE(port, SYNCHRONIZATION)) {\n> +\t\t/* attach mux to aggregator */\n> +\t\tBOND_ASSERT((port->actor_state & (STATE_COLLECTING |\n> +\t\t\tSTATE_DISTRIBUTING)) == 0);\n> +\t\tACTOR_STATE_SET(port, SYNCHRONIZATION);\n> +\t\tntt = true;\n> +\t\tBOND_DEBUG(\"ATTACHED Entered\\n\");\n> +\t} else if (!ACTOR_STATE(port, COLLECTING)) {\n> +\t\t/* Start collecting if in sync */\n> +\t\tif (PARTNER_STATE(port, SYNCHRONIZATION)) {\n> +\t\t\tBOND_DEBUG(\"ATTACHED -> COLLECTING\\n\");\n> +\t\t\tACTOR_STATE_SET(port, COLLECTING);\n> +\t\t}\n> +\t} else if (ACTOR_STATE(port, COLLECTING)) {\n> +\t\t/* Check if partner is in COLLECTING state. If so this port can\n> +\t\t * distribute frames to it */\n> +\t\tif (!ACTOR_STATE(port, DISTRIBUTING)) {\n> +\t\t\tif (PARTNER_STATE(port, COLLECTING)) {\n> +\t\t\t\t/* Enable  DISTRIBUTING if partner is collecting */\n> +\t\t\t\tACTOR_STATE_SET(port, DISTRIBUTING);\n> +\t\t\t\tntt = true;\n> +\t\t\t\tBOND_DEBUG(\"COLLECTING -> DISTRIBUTING\\n\");\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tif (!PARTNER_STATE(port, COLLECTING)) {\n> +\t\t\t\t/* Disable DISTRIBUTING (enter COLLECTING state) if partner\n> +\t\t\t\t * is not collecting */\n> +\t\t\t\tACTOR_STATE_CLR(port, DISTRIBUTING);\n> +\t\t\t\tntt = true;\n> +\t\t\t\tBOND_DEBUG(\"DISTRIBUTING -> COLLECTING\\n\");\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tif (ntt != false)\n> +\t\tSM_FLAG_SET(port, NTT);\n> +\n> +\treturn ntt;\n> +}\n> +\n> +\n> +/**\n> + * Function handles transmit state machine.\n> + *\n> + * Function implements Transmit Machine from point 5.4.16 in 802.1AX\n> + * documentation.\n> + *\n> + * @param port\n> + */\n> +static void\n> +tx_machine(struct rte_eth_dev *bond_dev, uint8_t port_num)\n> +{\n> +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> +\tstruct port *port = &internals->mode4.port_list[port_num];\n> +\tstruct mode8023ad_data *data = &internals->mode4;\n> +\n> +\tstruct slow_protocol_msg *msg;\n> +\tstruct lacpdu_header *hdr;\n> +\tstruct lacpdu *lacpdu;\n> +\n> +\t/* If periodic timer is not running periodic machine is in NO PERIODIC and\n> +\t * acording to 802.3ax standard tx machine should not transmit any frames\n> +\t * and set ntt to false. */\n> +\tif (port->periodic_timer == TIMER_NOT_STARTED)\n> +\t\tSM_FLAG_CLR(port, NTT);\n> +\n> +\tif (!SM_FLAG(port, NTT) || !timer_is_expired(&port->tx_machine_timer))\n> +\t\treturn;\n> +\n> +\t/* If all conditions are met construct packet to send */\n> +\tif (rte_ring_dequeue(data->free_ring, (void **)&msg) == -ENOBUFS) {\n> +\t\tBOND_DEBUG(\"tx_machine: no free_lacpdu_ring\\n\");\n> +\t\treturn;\n> +\t}\n> +\n> +\tmsg->pkt = rte_pktmbuf_alloc(data->mbuf_pool);\n> +\tif (msg->pkt == NULL) {\n> +\t\t/* FIXME: temporal workaround, when packets are no freed by driver */\n> +\t\tstruct bond_rx_queue *bd_rx_q;\n> +\t\tuint8_t i;\n> +\n> +\t\tfor (i = 0; i < bond_dev->data->nb_rx_queues; i++) {\n> +\t\t\tbd_rx_q = (struct bond_rx_queue *)bond_dev->data->rx_queues[i];\n> +\t\t\tBOND_ASSERT(bd_rx_q != NULL && bd_rx_q->mb_pool != NULL);\n> +\t\t\t/* Do not use SP or SC pools as this is unsafe. */\n> +\t\t\tif (bd_rx_q->mb_pool->flags & (MEMPOOL_F_SC_GET | MEMPOOL_F_SP_PUT))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tmsg->pkt = rte_pktmbuf_alloc(bd_rx_q->mb_pool);\n> +\t\t\tif (msg->pkt) {\n> +\t\t\t\tRTE_LOG(WARNING, PMD, \"Failed to allocate LACP from mode4 pool.\"\n> +\t\t\t\t\"Packet was allocated from pool of rx queue %u\\n\", i);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (msg->pkt == NULL) {\n> +\t\t\trte_ring_enqueue(data->free_ring, msg);\n> +\t\t\tRTE_LOG(ERR, PMD, \"Failed to allocate LACP packet from pool\\n\");\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\tmsg->port_id = internals->active_slaves[port_num];\n> +\thdr = rte_pktmbuf_mtod(msg->pkt, struct lacpdu_header *);\n> +\n> +\tmsg->pkt->pkt.data_len = sizeof(*hdr);\n> +\tmsg->pkt->pkt.pkt_len = sizeof(*hdr);\n> +\t/* Source and destination MAC */\n> +\tether_addr_copy(&lacp_mac_addr, &hdr->eth_hdr.d_addr);\n> +\tmac_address_get(bond_dev, &hdr->eth_hdr.s_addr);\n> +\thdr->eth_hdr.ether_type = rte_cpu_to_be_16(ETHER_TYPE_SLOW);\n> +\n> +\tport = &data->port_list[port_num];\n> +\tlacpdu = &hdr->lacpdu;\n> +\tmemset(lacpdu, 0, sizeof(*lacpdu));\n> +\n> +\t/* Initialize LACP part */\n> +\tlacpdu->subtype = SUBTYPE_LACP;\n> +\tlacpdu->version_number = 1;\n> +\n> +\t/* ACTOR */\n> +\tlacpdu->actor.tlv_type_info = TLV_TYPE_ACTOR_INFORMATION;\n> +\tlacpdu->actor.info_length = sizeof(struct lacpdu_actor_partner_params);\n> +\tmemcpy(&hdr->lacpdu.actor.port_params, &port->actor,\n> +\t\t\tsizeof(port->actor));\n> +\tlacpdu->actor.state = port->actor_state;\n> +\n> +\t/* PARTNER */\n> +\tlacpdu->partner.tlv_type_info = TLV_TYPE_PARTNER_INFORMATION;\n> +\tlacpdu->partner.info_length = sizeof(struct lacpdu_actor_partner_params);\n> +\tmemcpy(&lacpdu->partner.port_params, &port->partner,\n> +\t\t\tsizeof(struct port_params));\n> +\tlacpdu->partner.state = port->partner_state;\n> +\n> +\t/* Other fields */\n> +\tlacpdu->tlv_type_collector_info = TLV_TYPE_COLLECTOR_INFORMATION;\n> +\tlacpdu->collector_info_length = 0x10;\n> +\tlacpdu->collector_max_delay = 0;\n> +\n> +\tlacpdu->tlv_type_terminator = TLV_TYPE_TERMINATOR_INFORMATION;\n> +\tlacpdu->terminator_length = 0;\n> +\n> +\tif (rte_ring_enqueue(data->tx_ring, msg) == -ENOBUFS) {\n> +\t\t/* If TX ring full, drop packet and free message. Retransmission\n> +\t\t * will happen in next function call. */\n> +\t\trte_pktmbuf_free(msg->pkt);\n> +\t\trte_ring_enqueue(data->free_ring, msg);\n> +\n> +\t\tRTE_LOG(ERR, PMD, \"Failed to enqueue LACP packet into tx ring\");\n> +\t\treturn;\n> +\t}\n> +\n> +\tBOND_DEBUG(\"sending LACP frame\\n\");\n> +\tBOND_PRINT_LACP(lacpdu);\n> +\n> +\tSM_FLAG_CLR(port, NTT);\n> +\t/* Add 10% random backoff time to better distrube slow packets\n> +\t * between tx bursts. */\n> +\ttimer_set(&port->tx_machine_timer, BOND_8023AD_TX_PERIOD_MS +\n> +\t\trand() % ((BOND_8023AD_TX_PERIOD_MS * 10) / 100));\n> +}\n> +\n> +/**\n> + * Function assigns port to aggregator.\n> + *\n> + * @param bond_dev_private\tPointer to bond_dev_private structure.\n> + * @param port_pos\t\t\tPort to assign.\n> + */\n> +static void\n> +selection_logic(struct bond_dev_private *internals, uint8_t port_num)\n> +{\n> +\tstruct mode8023ad_data *data = &internals->mode4;\n> +\tstruct port *agg, *port, *port_list;\n> +\tuint8_t ports_count;\n> +\tuint8_t i;\n> +\n> +\tports_count = internals->slave_count;\n> +\tport_list = data->port_list;\n> +\tport = &port_list[port_num];\n> +\n> +\t/* Skip port if it is selected */\n> +\tif (port->selected == SELECTED)\n> +\t\treturn;\n> +\n> +\t/* Search for aggregator suitable for this port */\n> +\tfor (i = 0; i < ports_count; ++i) {\n> +\t\tagg = &port_list[i];\n> +\t\t/* Skip ports that are not aggreagators */\n> +\t\tif (agg->used_agregator_idx != i && i == port_num)\n> +\t\t\tcontinue;\n> +\n> +\t\t/* Actors system ID is not checked since all slave device have the same\n> +\t\t * ID (MAC address). */\n> +\t\tif ((agg->actor.key == port->actor.key &&\n> +\t\t\tagg->partner.system_priority == port->partner.system_priority &&\n> +\t\t\tis_same_ether_addr(&agg->partner.system, &port->partner.system) == 1\n> +\t\t\t&& (agg->partner.key == port->partner.key)) &&\n> +\t\t\tis_zero_ether_addr(&port->partner.system) != 1 &&\n> +\t\t\t(agg->actor.key &\n> +\t\t\t\trte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY)) != 0) {\n> +\n> +\t\t\tport->used_agregator_idx = i;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\t/* By default, port uses it self as agregator */\n> +\tif (i == ports_count)\n> +\t\tport->used_agregator_idx = port_num;\n> +\n> +\tport->selected = SELECTED;\n> +\n> +\tBOND_DEBUG(\"-> SELECTED: ID=%3u pos=%3u\\n\"\n> +\t\t\"\\t%s ID=%3u pos=%3u\\n\",\n> +\t\tinternals->active_slaves[port_num], port_num,\n> +\t\tport->used_agregator_idx == port_num ?\n> +\t\t\t\"agregator not found, using default\" : \"agregator found\",\n> +\t\tport->used_agregator_idx,\n> +\t\tinternals->active_slaves[port->used_agregator_idx]);\n> +}\n> +\n> +/**\n> + * Helper function which updates current port\n> + */\n> +static void\n> +update_mux_slaves(struct bond_dev_private *internals)\n> +{\n> +\tstruct mode8023ad_data *data = &internals->mode4;\n> +\tstruct port *port;\n> +\tuint8_t current[RTE_MAX_ETHPORTS];\n> +\tuint8_t count = 0;\n> +\tuint8_t i;\n> +\n> +\tfor (i = 0; i < internals->slave_count; i++) {\n> +\t\tport = &data->port_list[i];\n> +\t\tif (ACTOR_STATE(port, DISTRIBUTING))\n> +\t\t\tcurrent[count++] = i;\n> +\t}\n> +\n> +\tmemcpy(data->distibuting_slaves, current, sizeof(current[0]) * count);\n> +\tdata->distibuting_slaves_count = count;\n> +}\n> +\n> +/* Function maps DPDK speed to bonding speed stored in key field */\n> +static uint16_t\n> +link_speed_key(uint16_t speed) {\n> +\tuint16_t key_speed;\n> +\n> +\tswitch (speed) {\n> +\tcase ETH_LINK_SPEED_AUTONEG:\n> +\t\tkey_speed = 0x00;\n> +\t\tbreak;\n> +\tcase ETH_LINK_SPEED_10:\n> +\t\tkey_speed = BOND_LINK_SPEED_KEY_10M;\n> +\t\tbreak;\n> +\tcase ETH_LINK_SPEED_100:\n> +\t\tkey_speed = BOND_LINK_SPEED_KEY_100M;\n> +\t\tbreak;\n> +\tcase ETH_LINK_SPEED_1000:\n> +\t\tkey_speed = BOND_LINK_SPEED_KEY_1000M;\n> +\t\tbreak;\n> +\tcase ETH_LINK_SPEED_10G:\n> +\t\tkey_speed = BOND_LINK_SPEED_KEY_10G;\n> +\t\tbreak;\n> +\tcase ETH_LINK_SPEED_20G:\n> +\t\tkey_speed = BOND_LINK_SPEED_KEY_20G;\n> +\t\tbreak;\n> +\tcase ETH_LINK_SPEED_40G:\n> +\t\tkey_speed = BOND_LINK_SPEED_KEY_40G;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\t/* Unknown speed*/\n> +\t\tkey_speed = 0xFFFF;\n> +\t}\n> +\n> +\treturn key_speed;\n> +}\n> +\n> +static void\n> +bond_mode_8023ad_periodic_cb(void *arg)\n> +{\n> +\tstruct rte_eth_dev *bond_dev = arg;\n> +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> +\tstruct mode8023ad_data *data = &internals->mode4;\n> +\n> +\tstruct slow_protocol_frame *slow_hdr;\n> +\tstruct rte_eth_link link_info;\n> +\n> +\tstruct slow_protocol_msg *msgs[BOND_MODE_8023AX_RX_RING_SIZE];\n> +\tuint16_t port_num, j, nb_msgs;\n> +\t/* if not 0 collecting/distibuting array need update */\n> +\tuint16_t slaves_changed = 0;\n> +\tbool machines_invoked;\n> +\n> +\t/* Update link status on each port */\n> +\tfor (port_num = 0; port_num < internals->active_slave_count; port_num++) {\n> +\t\tuint8_t key;\n> +\n> +\t\trte_eth_link_get(internals->active_slaves[port_num], &link_info);\n> +\t\tif (link_info.link_status != 0) {\n> +\t\t\tkey = link_speed_key(link_info.link_speed) << 1;\n> +\t\t\tif (link_info.link_duplex == ETH_LINK_FULL_DUPLEX)\n> +\t\t\t\tkey |= BOND_LINK_FULL_DUPLEX_KEY;\n> +\t\t} else\n> +\t\t\tkey = 0;\n> +\n> +\t\tdata->port_list[port_num].actor.key = rte_cpu_to_be_16(key);\n> +\t}\n> +\n> +\tnb_msgs = (uint16_t)rte_ring_dequeue_burst(data->rx_ring, (void **) msgs,\n> +\t\tBOND_MODE_8023AX_RX_RING_SIZE);\n> +\n> +\tfor (port_num = 0; port_num < internals->active_slave_count; port_num++) {\n> +\t\tstruct port *port = &data->port_list[port_num];\n> +\t\tif ((port->actor.key &\n> +\t\t\t\trte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY)) == 0) {\n> +\n> +\t\t\tSM_FLAG_SET(port, BEGIN);\n> +\n> +\t\t\t/* LACP is disabled on half duples or link is down */\n> +\t\t\tif (SM_FLAG(port, LACP_ENABLED)) {\n> +\t\t\t\t/* If port was enabled set it to BEGIN state */\n> +\t\t\t\tSM_FLAG_CLR(port, LACP_ENABLED);\n> +\t\t\t\tACTOR_STATE_CLR(port, DISTRIBUTING);\n> +\t\t\t\tACTOR_STATE_CLR(port, COLLECTING);\n> +\t\t\t\tslaves_changed++;\n> +\t\t\t}\n> +\n> +\t\t\tBOND_DEBUG(\"Port %u is not LACP capable!\\n\",\n> +\t\t\t\tinternals->active_slaves[port_num]);\n> +\t\t\t/* Skip this port processing */\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tSM_FLAG_SET(port, LACP_ENABLED);\n> +\t\tmachines_invoked = false;\n> +\t\t/* Find LACP packet */\n> +\t\tfor (j = 0; j < nb_msgs; j++) {\n> +\t\t\tif (msgs[j] == NULL || msgs[j]->port_id !=\n> +\t\t\t\t\tinternals->active_slaves[port_num])\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tslow_hdr = rte_pktmbuf_mtod(msgs[j]->pkt,\n> +\t\t\t\t\tstruct slow_protocol_frame *);\n> +\n> +\t\t\tif (slow_hdr->slow_protocol.subtype == SLOW_SUBTYPE_LACP) {\n> +\t\t\t\t/* This is LACP frame so pass it to rx_machine */\n> +\t\t\t\tstruct lacpdu *lacp = (struct lacpdu *)&slow_hdr->slow_protocol;\n> +\t\t\t\t/* Invoke state machines on every active slave port */\n> +\t\t\t\trx_machine(internals, port_num, lacp);\n> +\t\t\t\tperiodic_machine(internals, port_num);\n> +\t\t\t\tslaves_changed += mux_machine(internals, port_num);\n> +\t\t\t\ttx_machine(bond_dev, port_num);\n> +\t\t\t\tselection_logic(internals, port_num);\n> +\n> +\t\t\t\tmachines_invoked = true;\n> +\t\t\t} else if (slow_hdr->slow_protocol.subtype == SLOW_SUBTYPE_MARKER) {\n> +\t\t\t\tstruct marker *marker;\n> +\n> +\t\t\t\tmarker = (struct marker *) &slow_hdr->slow_protocol;\n> +\t\t\t\tif (marker->tlv_type_marker == MARKER_TLV_TYPE_MARKER_INFO) {\n> +\t\t\t\t\t/* Reuse received packet to send frame to Marker Responder\n> +\t\t\t\t\t */\n> +\t\t\t\t\tmarker->tlv_type_marker = MARKER_TLV_TYPE_MARKER_RESP;\n> +\n> +\t\t\t\t\t/* Update source MAC, destination MAC is multicast so we\n> +\t\t\t\t\t * don't update it */\n> +\t\t\t\t\tmac_address_get(bond_dev, &slow_hdr->eth_hdr.s_addr);\n> +\n> +\t\t\t\t\tif (rte_ring_enqueue(data->tx_ring, msgs[j]) == -ENOBUFS) {\n> +\t\t\t\t\t\tRTE_LOG(ERR, PMD,\n> +\t\t\t\t\t\t\"Failed to enqueue packet into tx ring\");\n> +\t\t\t\t\t\trte_pktmbuf_free(msgs[j]->pkt);\n> +\t\t\t\t\t\trte_ring_enqueue(data->free_ring, msgs[j]);\n> +\t\t\t\t\t}\n> +\n> +\t\t\t\t\tmsgs[j] = NULL;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (machines_invoked == false) {\n> +\t\t\trx_machine(internals, port_num, NULL);\n> +\t\t\tperiodic_machine(internals, port_num);\n> +\t\t\tslaves_changed += mux_machine(internals, port_num);\n> +\t\t\ttx_machine(bond_dev, port_num);\n> +\t\t\tselection_logic(internals, port_num);\n> +\t\t\tmachines_invoked = true;\n> +\t\t}\n> +\n> +\t\tSM_FLAG_CLR(port, BEGIN);\n> +\t}\n> +\n> +\t/* Update mux if something changed */\n> +\tif (slaves_changed > 0) {\n> +\t\tupdate_mux_slaves(internals);\n> +\t\tBOND_DEBUG(\"mux count %u [%2u%s%2u%s%2u%s%2u%s%s]\\n\",\n> +\t\t\tdata->distibuting_slaves_count,\n> +\t\t\tdata->distibuting_slaves[0],\n> +\t\t\tdata->distibuting_slaves_count > 0 ? \" \" : \"\\b\\b\",\n> +\t\t\tdata->distibuting_slaves[1],\n> +\t\t\tdata->distibuting_slaves_count > 1 ? \" \" : \"\\b\\b\",\n> +\t\t\tdata->distibuting_slaves[2],\n> +\t\t\tdata->distibuting_slaves_count > 2 ? \" \" : \"\\b\\b\",\n> +\t\t\tdata->distibuting_slaves[3],\n> +\t\t\tdata->distibuting_slaves_count > 3 ? \" \" : \"\\b\\b\",\n> +\t\t\tdata->distibuting_slaves_count > 4 ? \"...\" : \"\");\n> +\t}\n> +\n> +\t/* Free packets that was not reused */\n> +\tfor (port_num = 0; port_num < nb_msgs; port_num++) {\n> +\t\tif (msgs[port_num] != NULL) {\n> +\t\t\trte_pktmbuf_free(msgs[port_num]->pkt);\n> +\t\t\trte_ring_enqueue(data->free_ring, msgs[port_num]);\n> +\t\t}\n> +\t}\n> +\n> +\trte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000,\n> +\t\t\tbond_mode_8023ad_periodic_cb, arg);\n> +}\n> +\n> +static void\n> +bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev, uint8_t num)\n> +{\n> +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> +\tstruct mode8023ad_data *data = &internals->mode4;\n> +\n> +\tstruct port *port = &data->port_list[internals->active_slave_count];\n> +\tstruct port_params initial = {\n> +\t\t\t.system = { { 0 } },\n> +\t\t\t.system_priority = rte_cpu_to_be_16(0xFFFF),\n> +\t\t\t.key = rte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY),\n> +\t\t\t.port_priority = rte_cpu_to_be_16(0x00FF),\n> +\t\t\t.port_number = 0,\n> +\t};\n> +\n> +\tmemcpy(&port->actor, &initial, sizeof(struct port_params));\n> +\tmac_address_get(bond_dev, &port->actor.system);\n> +\tport->actor.port_number =\n> +\t\tslave_id_to_port_number(internals->active_slaves[num]);\n> +\n> +\tmemcpy(&port->partner, &initial, sizeof(struct port_params));\n> +\n> +\t/* default states */\n> +\tport->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE | STATE_DEFAULTED;\n> +\tport->partner_state = STATE_LACP_ACTIVE;\n> +\tport->sm_flags = SM_FLAGS_BEGIN;\n> +\n> +\t/* use this port as agregator */\n> +\tport->used_agregator_idx = num;\n> +}\n> +\n> +void\n> +bond_mode_8023ad_slave_append(struct rte_eth_dev *bond_dev)\n> +{\n> +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> +\n> +\tbond_mode_8023ad_activate_slave(bond_dev, internals->active_slave_count);\n> +}\n> +\n> +int\n> +bond_mode_8023ad_deactivate_slave(struct rte_eth_dev *bond_dev,\n> +\t\tuint8_t slave_pos)\n> +{\n> +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> +\tstruct mode8023ad_data *data = &internals->mode4;\n> +\tstruct port *port;\n> +\tuint8_t i;\n> +\n> +\tbond_mode_8023ad_stop(bond_dev);\n> +\n> +\t/* Exclude slave from transmit policy. If this slave is an aggregator\n> +\t * make all aggregated slaves unselected to force sellection logic\n> +\t * to select suitable aggregator for this port\t */\n> +\tfor (i = 0; i < internals->active_slave_count; i++) {\n> +\t\tport = &data->port_list[slave_pos];\n> +\t\tif (port->used_agregator_idx == slave_pos) {\n> +\t\t\tport->selected = UNSELECTED;\n> +\t\t\tport->actor_state &= ~(STATE_SYNCHRONIZATION | STATE_DISTRIBUTING |\n> +\t\t\t\tSTATE_COLLECTING);\n> +\n> +\t\t\t/* Use default aggregator */\n> +\t\t\tport->used_agregator_idx = i;\n> +\t\t}\n> +\t}\n> +\n> +\tport = &data->port_list[slave_pos];\n> +\ttimer_cancel(&port->current_while_timer);\n> +\ttimer_cancel(&port->periodic_timer);\n> +\ttimer_cancel(&port->wait_while_timer);\n> +\ttimer_cancel(&port->tx_machine_timer);\n> +\nThese all seem rather racy.  Alarm callbacks are executed with the alarm list\nlocks not held.  So there is every possibility that you could execute these (or\nany timer_cancel calls in this PMD in parallel with the internal state machine\ntimer callback, and leave either with a corrupted timer list (resulting from a\ndouble free between here, and the actual callback site), or a timer that is\nactually still pending when a slave is removed.",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id 00EFAB39E;\n\tWed, 17 Sep 2014 17:07:43 +0200 (CEST)",
                "from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])\n\tby dpdk.org (Postfix) with ESMTP id DDCC018F\n\tfor <dev@dpdk.org>; Wed, 17 Sep 2014 17:07:40 +0200 (CEST)",
                "from nat-pool-rdu-u.redhat.com ([66.187.233.203] helo=localhost)\n\tby smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63)\n\t(envelope-from <nhorman@tuxdriver.com>)\n\tid 1XUGut-0001E0-22; Wed, 17 Sep 2014 11:13:21 -0400"
            ],
            "Date": "Wed, 17 Sep 2014 11:13:04 -0400",
            "From": "Neil Horman <nhorman@tuxdriver.com>",
            "To": "Pawel Wodkowski <pawelx.wodkowski@intel.com>",
            "Message-ID": "<20140917151304.GD4213@localhost.localdomain>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>",
            "MIME-Version": "1.0",
            "Content-Type": "text/plain; charset=us-ascii",
            "Content-Disposition": "inline",
            "In-Reply-To": "<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>",
            "User-Agent": "Mutt/1.5.23 (2014-03-12)",
            "X-Spam-Score": "-2.9 (--)",
            "X-Spam-Status": "No",
            "Cc": "dev@dpdk.org",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 846,
        "web_url": "http://patches.dpdk.org/comment/846/",
        "msgid": "<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>",
        "list_archive_url": "https://inbox.dpdk.org/dev/F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com",
        "date": "2014-09-18T08:07:31",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 58,
            "url": "http://patches.dpdk.org/api/people/58/?format=api",
            "name": "Wodkowski, PawelX",
            "email": "pawelx.wodkowski@intel.com"
        },
        "content": "> > +int\n> > +bond_mode_8023ad_deactivate_slave(struct rte_eth_dev *bond_dev,\n> > +\t\tuint8_t slave_pos)\n> > +{\n> > +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> > +\tstruct mode8023ad_data *data = &internals->mode4;\n> > +\tstruct port *port;\n> > +\tuint8_t i;\n> > +\n> > +\tbond_mode_8023ad_stop(bond_dev);\n> > +\n> > +\t/* Exclude slave from transmit policy. If this slave is an aggregator\n> > +\t * make all aggregated slaves unselected to force sellection logic\n> > +\t * to select suitable aggregator for this port\t */\n> > +\tfor (i = 0; i < internals->active_slave_count; i++) {\n> > +\t\tport = &data->port_list[slave_pos];\n> > +\t\tif (port->used_agregator_idx == slave_pos) {\n> > +\t\t\tport->selected = UNSELECTED;\n> > +\t\t\tport->actor_state &= ~(STATE_SYNCHRONIZATION |\n> STATE_DISTRIBUTING |\n> > +\t\t\t\tSTATE_COLLECTING);\n> > +\n> > +\t\t\t/* Use default aggregator */\n> > +\t\t\tport->used_agregator_idx = i;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tport = &data->port_list[slave_pos];\n> > +\ttimer_cancel(&port->current_while_timer);\n> > +\ttimer_cancel(&port->periodic_timer);\n> > +\ttimer_cancel(&port->wait_while_timer);\n> > +\ttimer_cancel(&port->tx_machine_timer);\n> > +\n> These all seem rather racy.  Alarm callbacks are executed with the alarm list\n> locks not held.  So there is every possibility that you could execute these (or\n> any timer_cancel calls in this PMD in parallel with the internal state machine\n> timer callback, and leave either with a corrupted timer list (resulting from a\n> double free between here, and the actual callback site),\n\nI don't think so. Yes, callbacks are executed with  alarm list locks not held, but \nthis is not the issue because access to list itself is guarded by lock and \nap->executing variable. So list will not be trashed. Check source of \neal_alarm_callback(), rte_eal_alarm_set() and rte_eal_alarm_cancel().\n\n> or a timer that is\n> actually still pending when a slave is removed.\n> \nThis is not the issue also, but problem might be similar. I assumed that alarms\nare atomic but when I looked at rte alarms closer I saw a race condition\nbetween and rte_eal_alarm_cancel() from  bond_mode_8023ad_stop()\nand rte_eal_alarm_set() from state machines callback. This need to be \nreworked in some way.",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id 544EEB39C;\n\tThu, 18 Sep 2014 10:01:52 +0200 (CEST)",
                "from mga01.intel.com (mga01.intel.com [192.55.52.88])\n\tby dpdk.org (Postfix) with ESMTP id 19596B39B\n\tfor <dev@dpdk.org>; Thu, 18 Sep 2014 10:01:49 +0200 (CEST)",
                "from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby fmsmga101.fm.intel.com with ESMTP; 18 Sep 2014 01:07:33 -0700",
                "from irsmsx103.ger.corp.intel.com ([163.33.3.157])\n\tby FMSMGA003.fm.intel.com with ESMTP; 18 Sep 2014 01:02:00 -0700",
                "from irsmsx155.ger.corp.intel.com (163.33.192.3) by\n\tIRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP\n\tServer (TLS) id 14.3.195.1; Thu, 18 Sep 2014 09:07:32 +0100",
                "from irsmsx102.ger.corp.intel.com ([169.254.2.24]) by\n\tIRSMSX155.ger.corp.intel.com ([169.254.14.85]) with mapi id\n\t14.03.0195.001; Thu, 18 Sep 2014 09:07:32 +0100"
            ],
            "X-ExtLoop1": "1",
            "X-IronPort-AV": "E=Sophos;i=\"4.97,862,1389772800\"; d=\"scan'208\";a=\"387914117\"",
            "From": "\"Wodkowski, PawelX\" <pawelx.wodkowski@intel.com>",
            "To": "Neil Horman <nhorman@tuxdriver.com>",
            "Thread-Topic": "[dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "Thread-Index": "AQHP0oLWm9lr6LYAYECquqxpYdLEtpwFXfgAgAEYBtA=",
            "Date": "Thu, 18 Sep 2014 08:07:31 +0000",
            "Message-ID": "<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>",
            "In-Reply-To": "<20140917151304.GD4213@localhost.localdomain>",
            "Accept-Language": "pl-PL, en-US",
            "Content-Language": "en-US",
            "X-MS-Has-Attach": "",
            "X-MS-TNEF-Correlator": "",
            "x-originating-ip": "[163.33.239.180]",
            "Content-Type": "text/plain; charset=\"us-ascii\"",
            "Content-Transfer-Encoding": "quoted-printable",
            "MIME-Version": "1.0",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Jastrzebski,\n\tMichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 868,
        "web_url": "http://patches.dpdk.org/comment/868/",
        "msgid": "<20140918160234.GJ20389@hmsreliant.think-freely.org>",
        "list_archive_url": "https://inbox.dpdk.org/dev/20140918160234.GJ20389@hmsreliant.think-freely.org",
        "date": "2014-09-18T16:02:34",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 32,
            "url": "http://patches.dpdk.org/api/people/32/?format=api",
            "name": "Neil Horman",
            "email": "nhorman@tuxdriver.com"
        },
        "content": "On Thu, Sep 18, 2014 at 08:07:31AM +0000, Wodkowski, PawelX wrote:\n> > > +int\n> > > +bond_mode_8023ad_deactivate_slave(struct rte_eth_dev *bond_dev,\n> > > +\t\tuint8_t slave_pos)\n> > > +{\n> > > +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> > > +\tstruct mode8023ad_data *data = &internals->mode4;\n> > > +\tstruct port *port;\n> > > +\tuint8_t i;\n> > > +\n> > > +\tbond_mode_8023ad_stop(bond_dev);\n> > > +\n> > > +\t/* Exclude slave from transmit policy. If this slave is an aggregator\n> > > +\t * make all aggregated slaves unselected to force sellection logic\n> > > +\t * to select suitable aggregator for this port\t */\n> > > +\tfor (i = 0; i < internals->active_slave_count; i++) {\n> > > +\t\tport = &data->port_list[slave_pos];\n> > > +\t\tif (port->used_agregator_idx == slave_pos) {\n> > > +\t\t\tport->selected = UNSELECTED;\n> > > +\t\t\tport->actor_state &= ~(STATE_SYNCHRONIZATION |\n> > STATE_DISTRIBUTING |\n> > > +\t\t\t\tSTATE_COLLECTING);\n> > > +\n> > > +\t\t\t/* Use default aggregator */\n> > > +\t\t\tport->used_agregator_idx = i;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tport = &data->port_list[slave_pos];\n> > > +\ttimer_cancel(&port->current_while_timer);\n> > > +\ttimer_cancel(&port->periodic_timer);\n> > > +\ttimer_cancel(&port->wait_while_timer);\n> > > +\ttimer_cancel(&port->tx_machine_timer);\n> > > +\n> > These all seem rather racy.  Alarm callbacks are executed with the alarm list\n> > locks not held.  So there is every possibility that you could execute these (or\n> > any timer_cancel calls in this PMD in parallel with the internal state machine\n> > timer callback, and leave either with a corrupted timer list (resulting from a\n> > double free between here, and the actual callback site),\n> \n> I don't think so. Yes, callbacks are executed with  alarm list locks not held, but \n> this is not the issue because access to list itself is guarded by lock and \n> ap->executing variable. So list will not be trashed. Check source of \n> eal_alarm_callback(), rte_eal_alarm_set() and rte_eal_alarm_cancel().\n> \nYes, you're right, the list is probably safe wht the executing bit.\n\n> > or a timer that is\n> > actually still pending when a slave is removed.\n> > \n> This is not the issue also, but problem might be similar. I assumed that alarms\n> are atomic but when I looked at rte alarms closer I saw a race condition\n> between and rte_eal_alarm_cancel() from  bond_mode_8023ad_stop()\n> and rte_eal_alarm_set() from state machines callback. This need to be \n> reworked in some way.\n\nYes, this is what I was referring to:\n\nCPU0\t\t\t\tCPU1\nrte_eal_alarm_callback\t\tbond_8023ad_deactivate_slave\n-bond_8023_ad_periodic_cb\ttimer_cancel\ntimer_set\n\nIf those timer functions operate on the same timer, the result is that you can\nleave the stop/deactivate slave paths with a timer function for that slave still\npending. The bonding mode needs some internal state to serialize those\noperations and determine if the timer should be reactivated.\n\nNeil",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id 281C8B3C9;\n\tThu, 18 Sep 2014 17:56:54 +0200 (CEST)",
                "from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])\n\tby dpdk.org (Postfix) with ESMTP id 00EE8B3C6\n\tfor <dev@dpdk.org>; Thu, 18 Sep 2014 17:56:53 +0200 (CEST)",
                "from hmsreliant.think-freely.org\n\t([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost)\n\tby smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63)\n\t(envelope-from <nhorman@tuxdriver.com>)\n\tid 1XUeAF-0002l6-6v; Thu, 18 Sep 2014 12:02:37 -0400"
            ],
            "Date": "Thu, 18 Sep 2014 12:02:34 -0400",
            "From": "Neil Horman <nhorman@tuxdriver.com>",
            "To": "\"Wodkowski, PawelX\" <pawelx.wodkowski@intel.com>",
            "Message-ID": "<20140918160234.GJ20389@hmsreliant.think-freely.org>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>",
            "MIME-Version": "1.0",
            "Content-Type": "text/plain; charset=us-ascii",
            "Content-Disposition": "inline",
            "In-Reply-To": "<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>",
            "User-Agent": "Mutt/1.5.23 (2014-03-12)",
            "X-Spam-Score": "-2.9 (--)",
            "X-Spam-Status": "No",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Jastrzebski,\n\tMichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 884,
        "web_url": "http://patches.dpdk.org/comment/884/",
        "msgid": "<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>",
        "list_archive_url": "https://inbox.dpdk.org/dev/F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com",
        "date": "2014-09-19T12:47:35",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 58,
            "url": "http://patches.dpdk.org/api/people/58/?format=api",
            "name": "Wodkowski, PawelX",
            "email": "pawelx.wodkowski@intel.com"
        },
        "content": "> -----Original Message-----\n> From: Neil Horman [mailto:nhorman@tuxdriver.com]\n> Sent: Thursday, September 18, 2014 18:03\n> To: Wodkowski, PawelX\n> Cc: dev@dpdk.org; Jastrzebski, MichalX K; Doherty, Declan\n> Subject: Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support\n> \n> On Thu, Sep 18, 2014 at 08:07:31AM +0000, Wodkowski, PawelX wrote:\n> > > > +int\n> > > > +bond_mode_8023ad_deactivate_slave(struct rte_eth_dev *bond_dev,\n> > > > +\t\tuint8_t slave_pos)\n> > > > +{\n> > > > +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> > > > +\tstruct mode8023ad_data *data = &internals->mode4;\n> > > > +\tstruct port *port;\n> > > > +\tuint8_t i;\n> > > > +\n> > > > +\tbond_mode_8023ad_stop(bond_dev);\n> > > > +\n> > > > +\t/* Exclude slave from transmit policy. If this slave is an aggregator\n> > > > +\t * make all aggregated slaves unselected to force sellection logic\n> > > > +\t * to select suitable aggregator for this port\t */\n> > > > +\tfor (i = 0; i < internals->active_slave_count; i++) {\n> > > > +\t\tport = &data->port_list[slave_pos];\n> > > > +\t\tif (port->used_agregator_idx == slave_pos) {\n> > > > +\t\t\tport->selected = UNSELECTED;\n> > > > +\t\t\tport->actor_state &= ~(STATE_SYNCHRONIZATION |\n> > > STATE_DISTRIBUTING |\n> > > > +\t\t\t\tSTATE_COLLECTING);\n> > > > +\n> > > > +\t\t\t/* Use default aggregator */\n> > > > +\t\t\tport->used_agregator_idx = i;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\tport = &data->port_list[slave_pos];\n> > > > +\ttimer_cancel(&port->current_while_timer);\n> > > > +\ttimer_cancel(&port->periodic_timer);\n> > > > +\ttimer_cancel(&port->wait_while_timer);\n> > > > +\ttimer_cancel(&port->tx_machine_timer);\n> > > > +\n> > > These all seem rather racy.  Alarm callbacks are executed with the alarm list\n> > > locks not held.  So there is every possibility that you could execute these (or\n> > > any timer_cancel calls in this PMD in parallel with the internal state machine\n> > > timer callback, and leave either with a corrupted timer list (resulting from a\n> > > double free between here, and the actual callback site),\n> >\n> > I don't think so. Yes, callbacks are executed with  alarm list locks not held, but\n> > this is not the issue because access to list itself is guarded by lock and\n> > ap->executing variable. So list will not be trashed. Check source of\n> > eal_alarm_callback(), rte_eal_alarm_set() and rte_eal_alarm_cancel().\n> >\n> Yes, you're right, the list is probably safe wht the executing bit.\n> \n> > > or a timer that is\n> > > actually still pending when a slave is removed.\n> > >\n> > This is not the issue also, but problem might be similar. I assumed that alarms\n> > are atomic but when I looked at rte alarms closer I saw a race condition\n> > between and rte_eal_alarm_cancel() from  bond_mode_8023ad_stop()\n> > and rte_eal_alarm_set() from state machines callback. This need to be\n> > reworked in some way.\n> \n> Yes, this is what I was referring to:\n> \n> CPU0\t\t\t\tCPU1\n> rte_eal_alarm_callback\t\tbond_8023ad_deactivate_slave\n> -bond_8023_ad_periodic_cb\ttimer_cancel\n> timer_set\n> \n> If those timer functions operate on the same timer, the result is that you can\n> leave the stop/deactivate slave paths with a timer function for that slave still\n> pending. The bonding mode needs some internal state to serialize those\n> operations and determine if the timer should be reactivated.\n> \n> Neil\n\nI did rethink the issue and problem is much simpler than it looks like. I did the \nfollowing:\n1. Change internal state machine alarms to use rte_rdtsc(). This makes all \n mode 4 internal timer_*() function not affected by any race condition.\n2. Do a busy loop when canceling main callback timer until cancel is successfull.\nThis should do the trick about race condition. Do you agree?\n\nHere is part involving timers I have changed:\n\nstatic void\n-timer_expired_cb(void *arg)\n+timer_stop(uint64_t *timer)\n {\n-\tenum timer_state *timer = arg;\n-\n-\tBOND_ASSERT(*timer == TIMER_RUNNING);\n-\t*timer = TIMER_EXPIRED;\n+\t*timer = 0;\n }\n \n static void\n-timer_cancel(enum timer_state *timer)\n+timer_set(uint64_t *timer, uint64_t timeout_ms)\n {\n-\trte_eal_alarm_cancel(&timer_expired_cb, timer);\n-\t*timer = TIMER_NOT_STARTED;\n+\t*timer = rte_rdtsc() + timeout_ms * rte_get_tsc_hz() / 1000;\n }\n \n+/* Forces given timer to be in expired state. */\n static void\n-timer_set(enum timer_state *timer, uint64_t timeout)\n+timer_force_expired(uint64_t *timer)\n {\n-\trte_eal_alarm_cancel(&timer_expired_cb, timer);\n-\trte_eal_alarm_set(timeout * 1000, &timer_expired_cb, timer);\n-\t*timer = TIMER_RUNNING;\n+\t*timer = rte_rdtsc();\n }\n \n static bool\n-timer_is_expired(enum timer_state *timer)\n+timer_is_stopped(uint64_t *timer)\n {\n-\treturn *timer == TIMER_EXPIRED;\n+\treturn *timer == 0;\n+}\n+\n+/* Timer is in running state if it is not stopped nor expired */\n+static bool\n+timer_is_running(uint64_t *timer)\n+{\n+\treturn *timer > 0 && *timer < rte_rdtsc();\n+}\n+\n+\n+static bool\n+timer_is_expired(uint64_t *timer)\n+{\n+\treturn *timer <= rte_rdtsc();\n }\n---\n\nAnd part stopping mode 4 callback.\n\n-int\n+void\n bond_mode_8023ad_stop(struct rte_eth_dev *bond_dev)\n {\n-\trte_eal_alarm_cancel(bond_mode_8023ad_periodic_cb, bond_dev);\n-\treturn 0;\n+\t/* Loop untill we cancel pending alarm. Alarm that is executing will\n+\t * not be canceled but when reshedules it self it will be canceled. */\n+\twhile (rte_eal_alarm_cancel(&bond_mode_8023ad_periodic_cb, bond_dev) == 0)\n+\t\trte_pause();\n }",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id BF32DB3B0;\n\tFri, 19 Sep 2014 14:42:10 +0200 (CEST)",
                "from mga09.intel.com (mga09.intel.com [134.134.136.24])\n\tby dpdk.org (Postfix) with ESMTP id 8CD48B3AF\n\tfor <dev@dpdk.org>; Fri, 19 Sep 2014 14:42:09 +0200 (CEST)",
                "from orsmga002.jf.intel.com ([10.7.209.21])\n\tby orsmga102.jf.intel.com with ESMTP; 19 Sep 2014 05:41:46 -0700",
                "from irsmsx103.ger.corp.intel.com ([163.33.3.157])\n\tby orsmga002.jf.intel.com with ESMTP; 19 Sep 2014 05:47:54 -0700",
                "from irsmsx106.ger.corp.intel.com (163.33.3.31) by\n\tIRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP\n\tServer (TLS) id 14.3.195.1; Fri, 19 Sep 2014 13:47:36 +0100",
                "from irsmsx102.ger.corp.intel.com ([169.254.2.24]) by\n\tIRSMSX106.ger.corp.intel.com ([169.254.8.3]) with mapi id\n\t14.03.0195.001; Fri, 19 Sep 2014 13:47:36 +0100"
            ],
            "X-ExtLoop1": "1",
            "X-IronPort-AV": "E=Sophos;i=\"5.04,554,1406617200\"; d=\"scan'208\";a=\"605367164\"",
            "From": "\"Wodkowski, PawelX\" <pawelx.wodkowski@intel.com>",
            "To": "Neil Horman <nhorman@tuxdriver.com>",
            "Thread-Topic": "[dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "Thread-Index": "AQHP0oLWm9lr6LYAYECquqxpYdLEtpwFXfgAgAEYBtCAAIgjAIABXSYA",
            "Date": "Fri, 19 Sep 2014 12:47:35 +0000",
            "Message-ID": "<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>\n\t<20140918160234.GJ20389@hmsreliant.think-freely.org>",
            "In-Reply-To": "<20140918160234.GJ20389@hmsreliant.think-freely.org>",
            "Accept-Language": "pl-PL, en-US",
            "Content-Language": "en-US",
            "X-MS-Has-Attach": "",
            "X-MS-TNEF-Correlator": "",
            "x-originating-ip": "[163.33.239.180]",
            "Content-Type": "text/plain; charset=\"us-ascii\"",
            "Content-Transfer-Encoding": "quoted-printable",
            "MIME-Version": "1.0",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Jastrzebski,\n\tMichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 886,
        "web_url": "http://patches.dpdk.org/comment/886/",
        "msgid": "<20140919172907.GE12897@hmsreliant.think-freely.org>",
        "list_archive_url": "https://inbox.dpdk.org/dev/20140919172907.GE12897@hmsreliant.think-freely.org",
        "date": "2014-09-19T17:29:07",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 32,
            "url": "http://patches.dpdk.org/api/people/32/?format=api",
            "name": "Neil Horman",
            "email": "nhorman@tuxdriver.com"
        },
        "content": "On Fri, Sep 19, 2014 at 12:47:35PM +0000, Wodkowski, PawelX wrote:\n> > -----Original Message-----\n> > From: Neil Horman [mailto:nhorman@tuxdriver.com]\n> > Sent: Thursday, September 18, 2014 18:03\n> > To: Wodkowski, PawelX\n> > Cc: dev@dpdk.org; Jastrzebski, MichalX K; Doherty, Declan\n> > Subject: Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support\n> > \n> > On Thu, Sep 18, 2014 at 08:07:31AM +0000, Wodkowski, PawelX wrote:\n> > > > > +int\n> > > > > +bond_mode_8023ad_deactivate_slave(struct rte_eth_dev *bond_dev,\n> > > > > +\t\tuint8_t slave_pos)\n> > > > > +{\n> > > > > +\tstruct bond_dev_private *internals = bond_dev->data->dev_private;\n> > > > > +\tstruct mode8023ad_data *data = &internals->mode4;\n> > > > > +\tstruct port *port;\n> > > > > +\tuint8_t i;\n> > > > > +\n> > > > > +\tbond_mode_8023ad_stop(bond_dev);\n> > > > > +\n> > > > > +\t/* Exclude slave from transmit policy. If this slave is an aggregator\n> > > > > +\t * make all aggregated slaves unselected to force sellection logic\n> > > > > +\t * to select suitable aggregator for this port\t */\n> > > > > +\tfor (i = 0; i < internals->active_slave_count; i++) {\n> > > > > +\t\tport = &data->port_list[slave_pos];\n> > > > > +\t\tif (port->used_agregator_idx == slave_pos) {\n> > > > > +\t\t\tport->selected = UNSELECTED;\n> > > > > +\t\t\tport->actor_state &= ~(STATE_SYNCHRONIZATION |\n> > > > STATE_DISTRIBUTING |\n> > > > > +\t\t\t\tSTATE_COLLECTING);\n> > > > > +\n> > > > > +\t\t\t/* Use default aggregator */\n> > > > > +\t\t\tport->used_agregator_idx = i;\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > +\tport = &data->port_list[slave_pos];\n> > > > > +\ttimer_cancel(&port->current_while_timer);\n> > > > > +\ttimer_cancel(&port->periodic_timer);\n> > > > > +\ttimer_cancel(&port->wait_while_timer);\n> > > > > +\ttimer_cancel(&port->tx_machine_timer);\n> > > > > +\n> > > > These all seem rather racy.  Alarm callbacks are executed with the alarm list\n> > > > locks not held.  So there is every possibility that you could execute these (or\n> > > > any timer_cancel calls in this PMD in parallel with the internal state machine\n> > > > timer callback, and leave either with a corrupted timer list (resulting from a\n> > > > double free between here, and the actual callback site),\n> > >\n> > > I don't think so. Yes, callbacks are executed with  alarm list locks not held, but\n> > > this is not the issue because access to list itself is guarded by lock and\n> > > ap->executing variable. So list will not be trashed. Check source of\n> > > eal_alarm_callback(), rte_eal_alarm_set() and rte_eal_alarm_cancel().\n> > >\n> > Yes, you're right, the list is probably safe wht the executing bit.\n> > \n> > > > or a timer that is\n> > > > actually still pending when a slave is removed.\n> > > >\n> > > This is not the issue also, but problem might be similar. I assumed that alarms\n> > > are atomic but when I looked at rte alarms closer I saw a race condition\n> > > between and rte_eal_alarm_cancel() from  bond_mode_8023ad_stop()\n> > > and rte_eal_alarm_set() from state machines callback. This need to be\n> > > reworked in some way.\n> > \n> > Yes, this is what I was referring to:\n> > \n> > CPU0\t\t\t\tCPU1\n> > rte_eal_alarm_callback\t\tbond_8023ad_deactivate_slave\n> > -bond_8023_ad_periodic_cb\ttimer_cancel\n> > timer_set\n> > \n> > If those timer functions operate on the same timer, the result is that you can\n> > leave the stop/deactivate slave paths with a timer function for that slave still\n> > pending. The bonding mode needs some internal state to serialize those\n> > operations and determine if the timer should be reactivated.\n> > \n> > Neil\n> \n> I did rethink the issue and problem is much simpler than it looks like. I did the \n> following:\n> 1. Change internal state machine alarms to use rte_rdtsc(). This makes all \n>  mode 4 internal timer_*() function not affected by any race condition.\n> 2. Do a busy loop when canceling main callback timer until cancel is successfull.\n> This should do the trick about race condition. Do you agree?\n> \nI think that will work, but I believe you're making it more complicated (and\nless reusable) than it needs to be.  What I think you really need to do is\ncreate a new rte api call, rte_eal_alarm_cancel_sync (something like the\nequivalent of del_timer_sync in linux, that wraps up the\nwhile(rte_eal_alarm_cancel(...) == 0) {rte_pause} in its own function (so other\ncall sites can use it, as I don't think this is an uncommon problem), Then just\ncreate a bonding-internal state flag to signal the periodic callback that it\nshouldn't re-arm the timer.  That way all you have to do is set the flag, and\ncall rte_eal_alarm_cancel_sync, and you're done.  And other applications will be\nable to handle this common type of operation as well\n\nNeil",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id E805AB3A6;\n\tFri, 19 Sep 2014 19:23:26 +0200 (CEST)",
                "from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])\n\tby dpdk.org (Postfix) with ESMTP id 50845B3A5\n\tfor <dev@dpdk.org>; Fri, 19 Sep 2014 19:23:25 +0200 (CEST)",
                "from hmsreliant.think-freely.org\n\t([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost)\n\tby smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63)\n\t(envelope-from <nhorman@tuxdriver.com>)\n\tid 1XV1zY-0002mM-PE; Fri, 19 Sep 2014 13:29:14 -0400"
            ],
            "Date": "Fri, 19 Sep 2014 13:29:07 -0400",
            "From": "Neil Horman <nhorman@tuxdriver.com>",
            "To": "\"Wodkowski, PawelX\" <pawelx.wodkowski@intel.com>",
            "Message-ID": "<20140919172907.GE12897@hmsreliant.think-freely.org>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>\n\t<20140918160234.GJ20389@hmsreliant.think-freely.org>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>",
            "MIME-Version": "1.0",
            "Content-Type": "text/plain; charset=us-ascii",
            "Content-Disposition": "inline",
            "In-Reply-To": "<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>",
            "User-Agent": "Mutt/1.5.23 (2014-03-12)",
            "X-Spam-Score": "-2.9 (--)",
            "X-Spam-Status": "No",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Jastrzebski,\n\tMichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 887,
        "web_url": "http://patches.dpdk.org/comment/887/",
        "msgid": "<F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com>",
        "list_archive_url": "https://inbox.dpdk.org/dev/F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com",
        "date": "2014-09-22T06:26:21",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 58,
            "url": "http://patches.dpdk.org/api/people/58/?format=api",
            "name": "Wodkowski, PawelX",
            "email": "pawelx.wodkowski@intel.com"
        },
        "content": "> I think that will work, but I believe you're making it more complicated (and\n> less reusable) than it needs to be.  What I think you really need to do is\n> create a new rte api call, rte_eal_alarm_cancel_sync (something like the\n> equivalent of del_timer_sync in linux, that wraps up the\n> while(rte_eal_alarm_cancel(...) == 0) {rte_pause} in its own function (so other\n> call sites can use it, as I don't think this is an uncommon problem), Then just\n> create a bonding-internal state flag to signal the periodic callback that it\n> shouldn't re-arm the timer.  That way all you have to do is set the flag, and\n> call rte_eal_alarm_cancel_sync, and you're done.  And other applications will be\n> able to handle this common type of operation as well\n> \n> Neil\n\nI agree with you that alarms should be upgraded but this need to discussed and \ntested. Now there is not time for that.\n\nThank you\nPawel",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id E7B132E8B;\n\tMon, 22 Sep 2014 08:20:23 +0200 (CEST)",
                "from mga02.intel.com (mga02.intel.com [134.134.136.20])\n\tby dpdk.org (Postfix) with ESMTP id AC03E2E81\n\tfor <dev@dpdk.org>; Mon, 22 Sep 2014 08:20:22 +0200 (CEST)",
                "from azsmga001.ch.intel.com ([10.2.17.19])\n\tby orsmga101.jf.intel.com with ESMTP; 21 Sep 2014 23:26:24 -0700",
                "from irsmsx103.ger.corp.intel.com ([163.33.3.157])\n\tby azsmga001.ch.intel.com with ESMTP; 21 Sep 2014 23:26:23 -0700",
                "from irsmsx102.ger.corp.intel.com ([169.254.2.24]) by\n\tIRSMSX103.ger.corp.intel.com ([169.254.3.112]) with mapi id\n\t14.03.0195.001; Mon, 22 Sep 2014 07:26:22 +0100"
            ],
            "X-ExtLoop1": "1",
            "X-IronPort-AV": "E=Sophos;i=\"5.04,570,1406617200\"; d=\"scan'208\";a=\"479101441\"",
            "From": "\"Wodkowski, PawelX\" <pawelx.wodkowski@intel.com>",
            "To": "Neil Horman <nhorman@tuxdriver.com>",
            "Thread-Topic": "[dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "Thread-Index": "AQHP0oLWm9lr6LYAYECquqxpYdLEtpwFXfgAgAEYBtCAAIgjAIABXSYAgABNXYCABAzrYA==",
            "Date": "Mon, 22 Sep 2014 06:26:21 +0000",
            "Message-ID": "<F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>\n\t<20140918160234.GJ20389@hmsreliant.think-freely.org>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>\n\t<20140919172907.GE12897@hmsreliant.think-freely.org>",
            "In-Reply-To": "<20140919172907.GE12897@hmsreliant.think-freely.org>",
            "Accept-Language": "pl-PL, en-US",
            "Content-Language": "en-US",
            "X-MS-Has-Attach": "",
            "X-MS-TNEF-Correlator": "",
            "x-originating-ip": "[163.33.239.180]",
            "Content-Type": "text/plain; charset=\"us-ascii\"",
            "Content-Transfer-Encoding": "quoted-printable",
            "MIME-Version": "1.0",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Jastrzebski,\n\tMichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 897,
        "web_url": "http://patches.dpdk.org/comment/897/",
        "msgid": "<20140922102455.GA25406@hmsreliant.think-freely.org>",
        "list_archive_url": "https://inbox.dpdk.org/dev/20140922102455.GA25406@hmsreliant.think-freely.org",
        "date": "2014-09-22T10:24:55",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 32,
            "url": "http://patches.dpdk.org/api/people/32/?format=api",
            "name": "Neil Horman",
            "email": "nhorman@tuxdriver.com"
        },
        "content": "On Mon, Sep 22, 2014 at 06:26:21AM +0000, Wodkowski, PawelX wrote:\n> > I think that will work, but I believe you're making it more complicated (and\n> > less reusable) than it needs to be.  What I think you really need to do is\n> > create a new rte api call, rte_eal_alarm_cancel_sync (something like the\n> > equivalent of del_timer_sync in linux, that wraps up the\n> > while(rte_eal_alarm_cancel(...) == 0) {rte_pause} in its own function (so other\n> > call sites can use it, as I don't think this is an uncommon problem), Then just\n> > create a bonding-internal state flag to signal the periodic callback that it\n> > shouldn't re-arm the timer.  That way all you have to do is set the flag, and\n> > call rte_eal_alarm_cancel_sync, and you're done.  And other applications will be\n> > able to handle this common type of operation as well\n> > \n> > Neil\n> \n> I agree with you that alarms should be upgraded but this need to discussed and \n> tested. Now there is not time for that.\n> \nNak, thats a completely broken argument.  I've just demonstrated to you a race\ncondition in the driver you are submitting.  Granted it stems from a design\nlmitation in the alarm subsystem, but its what we all have to work with, and we\ncan work around it and make the driver safe.  To say there is \"no time\" to fix\nit, implies to me that you care more about just slamming your code in than\nmaking anything work properly.  What exactly is your rush that makes you think\nits more important for the code to be merged than fixing it to work correctly?\nNeil",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id 7424CB3BC;\n\tMon, 22 Sep 2014 12:19:02 +0200 (CEST)",
                "from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])\n\tby dpdk.org (Postfix) with ESMTP id 87381B3BB\n\tfor <dev@dpdk.org>; Mon, 22 Sep 2014 12:19:00 +0200 (CEST)",
                "from [2001:470:8:a08:18c5:c64e:4bf:67a] (helo=localhost)\n\tby smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63)\n\t(envelope-from <nhorman@tuxdriver.com>)\n\tid 1XW0nh-0001ZD-Cr; Mon, 22 Sep 2014 06:25:02 -0400"
            ],
            "Date": "Mon, 22 Sep 2014 06:24:55 -0400",
            "From": "Neil Horman <nhorman@tuxdriver.com>",
            "To": "\"Wodkowski, PawelX\" <pawelx.wodkowski@intel.com>",
            "Message-ID": "<20140922102455.GA25406@hmsreliant.think-freely.org>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>\n\t<20140918160234.GJ20389@hmsreliant.think-freely.org>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>\n\t<20140919172907.GE12897@hmsreliant.think-freely.org>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com>",
            "MIME-Version": "1.0",
            "Content-Type": "text/plain; charset=us-ascii",
            "Content-Disposition": "inline",
            "In-Reply-To": "<F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com>",
            "User-Agent": "Mutt/1.5.23 (2014-03-12)",
            "X-Spam-Score": "-2.9 (--)",
            "X-Spam-Status": "No",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Jastrzebski,\n\tMichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    },
    {
        "id": 900,
        "web_url": "http://patches.dpdk.org/comment/900/",
        "msgid": "<20140922131524.GF25406@hmsreliant.think-freely.org>",
        "list_archive_url": "https://inbox.dpdk.org/dev/20140922131524.GF25406@hmsreliant.think-freely.org",
        "date": "2014-09-22T13:15:24",
        "subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
        "submitter": {
            "id": 32,
            "url": "http://patches.dpdk.org/api/people/32/?format=api",
            "name": "Neil Horman",
            "email": "nhorman@tuxdriver.com"
        },
        "content": "On Mon, Sep 22, 2014 at 11:59:33AM +0000, Jastrzebski, MichalX K wrote:\n> Hi Neil, \n> I agree with you that the most important is to release a code that works best without errors and this is what we all are working on. Pawel's answer \"no time\" doesn't sounds good here (he meant something else) - I ensure you that Pawel cares a lot to release a very good code. He proposes a solution that fixes this race for 1.8 release. Implementation of a new rte_api_call will take more time, because this is a new functionality for eal_timer library and with this it seems to be not much time left. Having said that, should we abandon hotfix and focus on the long-term solution?\n> \n\nYes, I think the proper solution should be the one we shoot for here, though in\nre-reading my response, perhaps I wasn't as clear as I could have been.  All I'm\nreally advocating here is that the while(...) { rte_pause() } loop that Pawels\nfix puts in place is better wrapped in a function implemented in the rte_alarm\nlibrary, rather than privately to the bonding library, along with the\nreplacement of all the pointer assignments with an internal state variable.  I'm\nnot asserting that we need to audit the code to fix up all other call sites\nusing the rte_alarm api right now (though a quick cscope search reveals the only\nlocations are in the test apps).  I'm just saying lets fix it in such a way that\nother users can take advantage of it now, and write the unit tests for it after\nit ships.\n\nIn fact, looking at the alarm test infrastructure, alarm re-arming stress isn't\ncurrently tested at all, so that could be a large undertaking after shipment.\nNeil\n\n> Best regards\n> Michal\n> \n> \n> -----Original Message-----\n> From: Neil Horman [mailto:nhorman@tuxdriver.com] \n> Sent: Monday, September 22, 2014 12:25 PM\n> To: Wodkowski, PawelX\n> Cc: dev@dpdk.org; Jastrzebski, MichalX K; Doherty, Declan\n> Subject: Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support\n> \n> On Mon, Sep 22, 2014 at 06:26:21AM +0000, Wodkowski, PawelX wrote:\n> > > I think that will work, but I believe you're making it more \n> > > complicated (and less reusable) than it needs to be.  What I think \n> > > you really need to do is create a new rte api call, \n> > > rte_eal_alarm_cancel_sync (something like the equivalent of \n> > > del_timer_sync in linux, that wraps up the\n> > > while(rte_eal_alarm_cancel(...) == 0) {rte_pause} in its own \n> > > function (so other call sites can use it, as I don't think this is \n> > > an uncommon problem), Then just create a bonding-internal state flag \n> > > to signal the periodic callback that it shouldn't re-arm the timer.  \n> > > That way all you have to do is set the flag, and call \n> > > rte_eal_alarm_cancel_sync, and you're done.  And other applications \n> > > will be able to handle this common type of operation as well\n> > > \n> > > Neil\n> > \n> > I agree with you that alarms should be upgraded but this need to \n> > discussed and tested. Now there is not time for that.\n> > \n> Nak, thats a completely broken argument.  I've just demonstrated to you a race condition in the driver you are submitting.  Granted it stems from a design lmitation in the alarm subsystem, but its what we all have to work with, and we can work around it and make the driver safe.  To say there is \"no time\" to fix it, implies to me that you care more about just slamming your code in than making anything work properly.  What exactly is your rush that makes you think its more important for the code to be merged than fixing it to work correctly?\n> Neil\n> \n> --------------------------------------------------------------\n> Intel Shannon Limited\n> Registered in Ireland\n> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare\n> Registered Number: 308263\n> Business address: Dromore House, East Park, Shannon, Co. Clare\n> \n> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.\n> \n> \n>",
        "headers": {
            "Return-Path": "<dev-bounces@dpdk.org>",
            "X-Original-To": "patchwork@dpdk.org",
            "Delivered-To": "patchwork@dpdk.org",
            "Received": [
                "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id 06D66B3AD;\n\tMon, 22 Sep 2014 15:09:30 +0200 (CEST)",
                "from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])\n\tby dpdk.org (Postfix) with ESMTP id 3A5791F5\n\tfor <dev@dpdk.org>; Mon, 22 Sep 2014 15:09:29 +0200 (CEST)",
                "from [2001:470:8:a08:18c5:c64e:4bf:67a] (helo=localhost)\n\tby smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63)\n\t(envelope-from <nhorman@tuxdriver.com>)\n\tid 1XW3Sf-0003Kf-3e; Mon, 22 Sep 2014 09:15:31 -0400"
            ],
            "Date": "Mon, 22 Sep 2014 09:15:24 -0400",
            "From": "Neil Horman <nhorman@tuxdriver.com>",
            "To": "\"Jastrzebski, MichalX K\" <michalx.k.jastrzebski@intel.com>",
            "Message-ID": "<20140922131524.GF25406@hmsreliant.think-freely.org>",
            "References": "<1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com>\n\t<1410963713-13837-3-git-send-email-pawelx.wodkowski@intel.com>\n\t<20140917151304.GD4213@localhost.localdomain>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B24CC2@IRSMSX102.ger.corp.intel.com>\n\t<20140918160234.GJ20389@hmsreliant.think-freely.org>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B2513B@IRSMSX102.ger.corp.intel.com>\n\t<20140919172907.GE12897@hmsreliant.think-freely.org>\n\t<F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com>\n\t<20140922102455.GA25406@hmsreliant.think-freely.org>\n\t<60ABE07DBB3A454EB7FAD707B4BB158213896977@IRSMSX109.ger.corp.intel.com>",
            "MIME-Version": "1.0",
            "Content-Type": "text/plain; charset=us-ascii",
            "Content-Disposition": "inline",
            "In-Reply-To": "<60ABE07DBB3A454EB7FAD707B4BB158213896977@IRSMSX109.ger.corp.intel.com>",
            "User-Agent": "Mutt/1.5.23 (2014-03-12)",
            "X-Spam-Score": "-2.9 (--)",
            "X-Spam-Status": "No",
            "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>, \"Landowski,\n\tMarekX M\" <marekx.m.landowski@intel.com>",
            "Subject": "Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support",
            "X-BeenThere": "dev@dpdk.org",
            "X-Mailman-Version": "2.1.15",
            "Precedence": "list",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "List-Post": "<mailto:dev@dpdk.org>",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Errors-To": "dev-bounces@dpdk.org",
            "Sender": "\"dev\" <dev-bounces@dpdk.org>"
        },
        "addressed": null
    }
]