From: Neil Horman <nhorman@tuxdriver.com>
To: Eric Kinzie <ehkinzie@gmail.com>
Cc: dev@dpdk.org, Eric Kinzie <ekinzie@brocade.com>
Subject: Re: [dpdk-dev] [PATCH v3 3/4] bond mode 4: allow external state machine
Date: Wed, 22 Apr 2015 15:51:22 -0400 [thread overview]
Message-ID: <20150422195122.GB29259@hmsreliant.think-freely.org> (raw)
In-Reply-To: <1429720063-9943-4-git-send-email-ehkinzie@gmail.com>
On Wed, Apr 22, 2015 at 09:27:42AM -0700, Eric Kinzie wrote:
> From: Eric Kinzie <ekinzie@brocade.com>
>
> Provide functions to allow an external 802.3ad state machine to transmit
> and recieve LACPDUs and to set the collection/distribution flags on
> slave interfaces.
>
> Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
> ---
> lib/librte_pmd_bond/rte_eth_bond_8023ad.c | 173 +++++++++++++++++++++
> lib/librte_pmd_bond/rte_eth_bond_8023ad.h | 44 ++++++
> lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h | 2 +
> lib/librte_pmd_bond/rte_eth_bond_version.map | 3 +
> 4 files changed, 222 insertions(+)
>
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> index 1009d5b..326e899 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> @@ -42,6 +42,8 @@
>
> #include "rte_eth_bond_private.h"
>
> +static void bond_mode_8023ad_ext_periodic_cb(void *arg);
> +
> #ifdef RTE_LIBRTE_BOND_DEBUG_8023AD
> #define MODE4_DEBUG(fmt, ...) RTE_LOG(DEBUG, PMD, "%6u [Port %u: %s] " fmt, \
> bond_dbg_get_time_diff_ms(), slave_id, \
> @@ -1014,6 +1016,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
> conf->tx_period_ms = mode4->tx_period_timeout / ms_ticks;
> conf->update_timeout_ms = mode4->update_timeout_us / 1000;
> conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
> + conf->slowrx_cb = mode4->slowrx_cb;
> }
>
> void
> @@ -1035,8 +1038,11 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
> conf->tx_period_ms = BOND_8023AD_TX_MACHINE_PERIOD_MS;
> conf->rx_marker_period_ms = BOND_8023AD_RX_MARKER_PERIOD_MS;
> conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS;
> + conf->slowrx_cb = NULL;
> }
>
> + bond_mode_8023ad_stop(dev);
> +
> mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks;
> mode4->slow_periodic_timeout = conf->slow_periodic_ms * ms_ticks;
> mode4->short_timeout = conf->short_timeout_ms * ms_ticks;
> @@ -1045,6 +1051,10 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
> mode4->tx_period_timeout = conf->tx_period_ms * ms_ticks;
> mode4->rx_marker_timeout = conf->rx_marker_period_ms * ms_ticks;
> mode4->update_timeout_us = conf->update_timeout_ms * 1000;
> + mode4->slowrx_cb = conf->slowrx_cb;
> +
> + if (dev->data->dev_started)
> + bond_mode_8023ad_start(dev);
> }
>
> int
> @@ -1062,6 +1072,13 @@ bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev)
> int
> bond_mode_8023ad_start(struct rte_eth_dev *bond_dev)
> {
> + struct bond_dev_private *internals = bond_dev->data->dev_private;
> + struct mode8023ad_private *mode4 = &internals->mode4;
> +
> + if (mode4->slowrx_cb)
> + return rte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000,
> + &bond_mode_8023ad_ext_periodic_cb, bond_dev);
> +
> return rte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000,
> &bond_mode_8023ad_periodic_cb, bond_dev);
> }
> @@ -1069,6 +1086,13 @@ bond_mode_8023ad_start(struct rte_eth_dev *bond_dev)
> void
> bond_mode_8023ad_stop(struct rte_eth_dev *bond_dev)
> {
> + struct bond_dev_private *internals = bond_dev->data->dev_private;
> + struct mode8023ad_private *mode4 = &internals->mode4;
> +
> + if (mode4->slowrx_cb) {
> + rte_eal_alarm_cancel(&bond_mode_8023ad_ext_periodic_cb, bond_dev);
> + return;
> + }
> rte_eal_alarm_cancel(&bond_mode_8023ad_periodic_cb, bond_dev);
> }
>
> @@ -1215,3 +1239,152 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
> info->agg_port_id = port->aggregator_port_id;
> return 0;
> }
> +
> +int
> +rte_eth_bond_8023ad_ext_collect(uint8_t port_id, uint8_t slave_id, int enabled)
> +{
> + struct rte_eth_dev *bond_dev;
> + struct bond_dev_private *internals;
> + struct mode8023ad_private *mode4;
> + struct port *port;
> +
> + if (rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
> + return -EINVAL;
> +
> + bond_dev = &rte_eth_devices[port_id];
> +
> + if (!bond_dev->data->dev_started)
> + return -EINVAL;
> +
> + internals = bond_dev->data->dev_private;
> + if (find_slave_by_id(internals->active_slaves,
> + internals->active_slave_count, slave_id) ==
> + internals->active_slave_count)
> + return -EINVAL;
> +
> + mode4 = &internals->mode4;
> + if (mode4->slowrx_cb == NULL)
> + return -EINVAL;
> +
> + port = &mode_8023ad_ports[slave_id];
> +
> + if (enabled)
> + ACTOR_STATE_SET(port, COLLECTING);
> + else
> + ACTOR_STATE_CLR(port, COLLECTING);
> +
> + return 0;
> +}
> +
> +int
> +rte_eth_bond_8023ad_ext_distrib(uint8_t port_id, uint8_t slave_id, int enabled)
> +{
> + struct rte_eth_dev *bond_dev;
> + struct bond_dev_private *internals;
> + struct mode8023ad_private *mode4;
> + struct port *port;
> +
> + if (rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
> + return -EINVAL;
> +
> + bond_dev = &rte_eth_devices[port_id];
> +
> + if (!bond_dev->data->dev_started)
> + return -EINVAL;
> +
> + internals = bond_dev->data->dev_private;
> + if (find_slave_by_id(internals->active_slaves,
> + internals->active_slave_count, slave_id) ==
> + internals->active_slave_count)
> + return -EINVAL;
> +
> + mode4 = &internals->mode4;
> + if (mode4->slowrx_cb == NULL)
> + return -EINVAL;
> +
> + port = &mode_8023ad_ports[slave_id];
> +
> + if (enabled)
> + ACTOR_STATE_SET(port, DISTRIBUTING);
> + else
> + ACTOR_STATE_CLR(port, DISTRIBUTING);
> +
> + return 0;
> +}
> +
> +int
> +rte_eth_bond_8023ad_ext_slowtx(uint8_t port_id, uint8_t slave_id,
> + struct rte_mbuf *lacp_pkt)
> +{
> + struct rte_eth_dev *bond_dev;
> + struct bond_dev_private *internals;
> + struct mode8023ad_private *mode4;
> + struct port *port;
> +
> + if (rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
> + return -EINVAL;
> +
> + bond_dev = &rte_eth_devices[port_id];
> +
> + if (!bond_dev->data->dev_started)
> + return -EINVAL;
> +
> + internals = bond_dev->data->dev_private;
> + if (find_slave_by_id(internals->active_slaves,
> + internals->active_slave_count, slave_id) ==
> + internals->active_slave_count)
> + return -EINVAL;
> +
> + mode4 = &internals->mode4;
> + if (mode4->slowrx_cb == NULL)
> + return -EINVAL;
> +
> + port = &mode_8023ad_ports[slave_id];
> +
> + if (rte_pktmbuf_pkt_len(lacp_pkt) < sizeof(struct lacpdu_header))
> + return -EINVAL;
> +
> + struct lacpdu_header *lacp;
> +
> + /* only enqueue LACPDUs */
> + lacp = rte_pktmbuf_mtod(lacp_pkt, struct lacpdu_header *);
> + if (lacp->lacpdu.subtype != SLOW_SUBTYPE_LACP)
> + return -EINVAL;
> +
> + MODE4_DEBUG("sending LACP frame\n");
> +
> + return rte_ring_enqueue(port->tx_ring, lacp_pkt);
> +}
> +
> +static void
> +bond_mode_8023ad_ext_periodic_cb(void *arg)
> +{
> + struct rte_eth_dev *bond_dev = arg;
> + struct bond_dev_private *internals = bond_dev->data->dev_private;
> + struct mode8023ad_private *mode4 = &internals->mode4;
> + struct port *port;
> + void *pkt = NULL;
> + uint16_t i, slave_id;
> +
> + for (i = 0; i < internals->active_slave_count; i++) {
> + slave_id = internals->active_slaves[i];
> + port = &mode_8023ad_ports[slave_id];
> +
> + if (rte_ring_dequeue(port->rx_ring, &pkt) == 0) {
> + struct rte_mbuf *lacp_pkt = pkt;
> + struct lacpdu_header *lacp;
> +
> + lacp = rte_pktmbuf_mtod(lacp_pkt,
> + struct lacpdu_header *);
> + RTE_VERIFY(lacp->lacpdu.subtype == SLOW_SUBTYPE_LACP);
> +
> + /* This is LACP frame so pass it to rx callback.
> + * Callback is responsible for freeing mbuf.
> + */
> + mode4->slowrx_cb(slave_id, lacp_pkt);
> + }
> + }
> +
> + rte_eal_alarm_set(internals->mode4.update_timeout_us,
> + bond_mode_8023ad_ext_periodic_cb, arg);
> +}
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad.h b/lib/librte_pmd_bond/rte_eth_bond_8023ad.h
> index ebd0e93..8cfa3d3 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_8023ad.h
> +++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad.h
> @@ -64,6 +64,8 @@ extern "C" {
> #define MARKER_TLV_TYPE_INFO 0x01
> #define MARKER_TLV_TYPE_RESP 0x02
>
> +typedef void (*rte_eth_bond_8023ad_ext_slowrx_fn)(uint8_t slave_id, struct rte_mbuf *lacp_pkt);
> +
> enum rte_bond_8023ad_selection {
> UNSELECTED,
> STANDBY,
> @@ -157,6 +159,7 @@ struct rte_eth_bond_8023ad_conf {
> uint32_t tx_period_ms;
> uint32_t rx_marker_period_ms;
> uint32_t update_timeout_ms;
> + rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
> };
>
This is dangerous. While it doesn't break ABI, it does create a situation in
which you will be prone to writing to invalid memory. By the examples that you
provide in the test application, you declare rte_eth_bond_8023ad_conf statically
within an application, so if someone is using the DPDK built as a shared
library, they may have less allocated space in the .data section then you think
with this new extended structure. As a result in that situation you will read
from and write to unallocated memory when you access slowrx_cb. I recommend
that you version the setup routine, so that the older version is modified to not
touch slowrx_cb.
Neil
next prev parent reply other threads:[~2015-04-22 19:51 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-22 16:27 [dpdk-dev] [PATCH v3 0/4] bonding corrections and additions Eric Kinzie
2015-04-22 16:27 ` [dpdk-dev] [PATCH v3 1/4] bond mode 4: copy entire config structure Eric Kinzie
2015-04-22 16:27 ` [dpdk-dev] [PATCH v3 2/4] bond mode 4: do not ignore multicast Eric Kinzie
2015-04-22 16:27 ` [dpdk-dev] [PATCH v3 3/4] bond mode 4: allow external state machine Eric Kinzie
2015-04-22 19:51 ` Neil Horman [this message]
2015-04-22 16:27 ` [dpdk-dev] [PATCH v3 4/4] bond mode 4: tests for " Eric Kinzie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150422195122.GB29259@hmsreliant.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=dev@dpdk.org \
--cc=ehkinzie@gmail.com \
--cc=ekinzie@brocade.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).