[PATCH 0/5] bonding corrections and additions

[PATCH 0/5] bonding corrections and additions

[Eric Kinzie]

This patchset makes a couple of small corrections to the bonding driver
and introduces the ability to use an external state machine for mode
4 operation.

Eric Kinzie (5):
  bond: use existing enslaved device queues
  bond mode 4: copy entire config structure
  bond mode 4: do not ignore multicast
  bond mode 4: allow external state machine
  bond mode 4: tests for external state machine

 app/test/test_link_bonding_mode4.c                |  208 +++++++++++++++++++--
 lib/librte_pmd_bond/rte_eth_bond_8023ad.c         |  176 +++++++++++++++++
 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_pmd.c            |   11 +-
 5 files changed, 427 insertions(+), 14 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] bond: use existing enslaved device queues

[Eric Kinzie]

If a device to be enslaved already has transmit and/or receive queues
allocated, use those and then create any additional queues that are
necessary.

Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_bond/rte_eth_bond_pmd.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
index c937e6b..4fd7d97 100644
--- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
+++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
@@ -1318,7 +1318,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	}
 
 	/* Setup Rx Queues */
-	for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
+	/* Use existing queues, if any */
+	for (q_id = slave_eth_dev->data->nb_rx_queues;
+	     q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
 		bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
 
 		errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
@@ -1334,7 +1336,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	}
 
 	/* Setup Tx Queues */
-	for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
+	/* Use existing queues, if any */
+	for (q_id = slave_eth_dev->data->nb_tx_queues;
+	     q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
 		bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
 
 		errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
-- 
1.7.10.4

Re: [PATCH 1/5] bond: use existing enslaved device queues

[Pawel Wodkowski]

On 2015-04-06 19:01, Eric Kinzie wrote:
> If a device to be enslaved already has transmit and/or receive queues
> allocated, use those and then create any additional queues that are
> necessary.
>
> Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   lib/librte_pmd_bond/rte_eth_bond_pmd.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> index c937e6b..4fd7d97 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> @@ -1318,7 +1318,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>   	}
>
>   	/* Setup Rx Queues */
> -	for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> +	/* Use existing queues, if any */
> +	for (q_id = slave_eth_dev->data->nb_rx_queues;
> +	     q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>   		bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>
>   		errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> @@ -1334,7 +1336,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>   	}
>
>   	/* Setup Tx Queues */
> -	for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> +	/* Use existing queues, if any */
> +	for (q_id = slave_eth_dev->data->nb_tx_queues;
> +	     q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>   		bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>
>   		errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>

Why you want to do that?

As far as I am aware (but Declan Doherty should speak here to) purpose
of this part of code is to have configuration of queues in slaves
consistent with bd_rx_q/bd_tx_q. If you skip reconfiguration of queues
that are already configured in port you can have them configured
in different way after enslaving.

So again: what is the purpose of doing so?

-- 
Pawel

Re: [PATCH 1/5] bond: use existing enslaved device queues

[Eric Kinzie]

On Fri Apr 10 09:40:09 +0200 2015, Pawel Wodkowski wrote:
> On 2015-04-06 19:01, Eric Kinzie wrote:
> >If a device to be enslaved already has transmit and/or receive queues
> >allocated, use those and then create any additional queues that are
> >necessary.
> >
> >Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >---
> >  lib/librte_pmd_bond/rte_eth_bond_pmd.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> >index c937e6b..4fd7d97 100644
> >--- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> >+++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> >@@ -1318,7 +1318,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >  	}
> >
> >  	/* Setup Rx Queues */
> >-	for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> >+	/* Use existing queues, if any */
> >+	for (q_id = slave_eth_dev->data->nb_rx_queues;
> >+	     q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> >  		bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
> >
> >  		errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> >@@ -1334,7 +1336,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >  	}
> >
> >  	/* Setup Tx Queues */
> >-	for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> >+	/* Use existing queues, if any */
> >+	for (q_id = slave_eth_dev->data->nb_tx_queues;
> >+	     q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> >  		bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
> >
> >  		errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> >
> 
> Why you want to do that?
> 
> As far as I am aware (but Declan Doherty should speak here to) purpose
> of this part of code is to have configuration of queues in slaves
> consistent with bd_rx_q/bd_tx_q. If you skip reconfiguration of queues
> that are already configured in port you can have them configured
> in different way after enslaving.
> 
> So again: what is the purpose of doing so?
> 
> -- 
> Pawel

Pawel,

I generally test things I've just built using virtio devices and calling
rte_eth_tx_queue_setup() more than once for a given queue id fails.
However, it seems that most PMDs allow re-allocating device queues while
virtio does not (xenvirt also seems to lack this functionality), so I
don't think my approach here is right.  I'll remove this patch when I
send the next version of this series.

Thanks,

Eric

Re: [PATCH 1/5] bond: use existing enslaved device queues

[Wodkowski, PawelX]

> 
> Pawel,
> 
> I generally test things I've just built using virtio devices and calling
> rte_eth_tx_queue_setup() more than once for a given queue id fails.
> However, it seems that most PMDs allow re-allocating device queues while
> virtio does not (xenvirt also seems to lack this functionality), so I
> don't think my approach here is right.  I'll remove this patch when I
> send the next version of this series.
> 
> Thanks,
> 
> Eric

Maybe you should rise this as separate issue maintainers of these drivers?

-- 
Pawel

[PATCH 2/5] bond mode 4: copy entire config structure

[Eric Kinzie]

  Copy all needed fields from the mode8023ad_private structure in
  bond_mode_8023ad_conf_get().

Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_bond/rte_eth_bond_8023ad.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
index 97a828e..1009d5b 100644
--- a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
+++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
@@ -1013,6 +1013,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
 	conf->aggregate_wait_timeout_ms = mode4->aggregate_wait_timeout / ms_ticks;
 	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;
 }
 
 void
-- 
1.7.10.4

Re: [PATCH 2/5] bond mode 4: copy entire config structure

[Pawel Wodkowski]

On 2015-04-06 19:01, Eric Kinzie wrote:
>    Copy all needed fields from the mode8023ad_private structure in
>    bond_mode_8023ad_conf_get().
>
> Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   lib/librte_pmd_bond/rte_eth_bond_8023ad.c |    1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> index 97a828e..1009d5b 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> @@ -1013,6 +1013,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
>   	conf->aggregate_wait_timeout_ms = mode4->aggregate_wait_timeout / ms_ticks;
>   	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;
>   }
>
>   void
>

This is bugfix.

Acked-by: Pawel Wodkowski <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

-- 
Pawel

Re: [PATCH 2/5] bond mode 4: copy entire config structure

[Thomas Monjalon]

2015-04-10 09:47, Pawel Wodkowski:
> On 2015-04-06 19:01, Eric Kinzie wrote:
> >    Copy all needed fields from the mode8023ad_private structure in
> >    bond_mode_8023ad_conf_get().
> >
> > Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >   lib/librte_pmd_bond/rte_eth_bond_8023ad.c |    1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> > index 97a828e..1009d5b 100644
> > --- a/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> > +++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad.c
> > @@ -1013,6 +1013,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
> >   	conf->aggregate_wait_timeout_ms = mode4->aggregate_wait_timeout / ms_ticks;
> >   	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;
> >   }
> >
> >   void
> >
> 
> This is bugfix.

When fixing a bug, it's better to explain what was wrong (i.e. behaviour impact)
and to add a tag "Fixes: <commit>".
Thanks

> Acked-by: Pawel Wodkowski <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[PATCH 3/5] bond mode 4: do not ignore multicast

[Eric Kinzie]

The bonding PMD in mode 4 puts all enslaved interfaces into promiscuous
mode in order to receive LACPDUs and must filter unwanted packets
after the traffic has been "collected".  Allow broadcast and multicast
through so that ARP and IPv6 neighbor discovery continue to work.

Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 app/test/test_link_bonding_mode4.c     |    7 +++++--
 lib/librte_pmd_bond/rte_eth_bond_pmd.c |    3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 02380f9..5a726af 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -755,8 +755,11 @@ test_mode4_rx(void)
 	rte_eth_macaddr_get(test_params.bonded_port_id, &bonded_mac);
 	ether_addr_copy(&bonded_mac, &dst_mac);
 
-	/* Assert that dst address is not bonding address */
-	dst_mac.addr_bytes[0]++;
+	/* Assert that dst address is not bonding address.  Do not set the
+	 * least significant bit of the zero byte as this would create a
+	 * multicast address.
+	 */
+	dst_mac.addr_bytes[0] += 2;
 
 	/* First try with promiscuous mode enabled.
 	 * Add 2 packets to each slave. First with bonding MAC address, second with
diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
index 4fd7d97..8631e12 100644
--- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
+++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
@@ -170,7 +170,8 @@ bond_ethdev_rx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
 			 * mode and packet address does not match. */
 			if (unlikely(hdr->ether_type == ether_type_slow_be ||
 				!collecting || (!promisc &&
-					!is_same_ether_addr(&bond_mac, &hdr->d_addr)))) {
+					(!is_multicast_ether_addr(&hdr->d_addr) &&
+					 !is_same_ether_addr(&bond_mac, &hdr->d_addr))))) {
 
 				if (hdr->ether_type == ether_type_slow_be) {
 					bond_mode_8023ad_handle_slow_pkt(internals, slaves[i],
-- 
1.7.10.4

Re: [PATCH 3/5] bond mode 4: do not ignore multicast

[Pawel Wodkowski]

On 2015-04-06 19:01, Eric Kinzie wrote:

>   			if (unlikely(hdr->ether_type == ether_type_slow_be ||
>   				!collecting || (!promisc &&
> -					!is_same_ether_addr(&bond_mac, &hdr->d_addr)))) {
> +					(!is_multicast_ether_addr(&hdr->d_addr) &&
> +					 !is_same_ether_addr(&bond_mac, &hdr->d_addr))))) {
>

You can drop extra parenthesis here, but beside that I think it is OK.

Should be marked as bugfix.

Acked-by: Pawel Wodkowski <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[PATCH 4/5] bond mode 4: allow external state machine

[Eric Kinzie]

  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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_bond/rte_eth_bond_8023ad.c         |  175 +++++++++++++++++++++
 lib/librte_pmd_bond/rte_eth_bond_8023ad.h         |   44 ++++++
 lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h |    2 +
 3 files changed, 221 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..29cd962 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,8 @@ 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;
+	conf->external_sm = mode4->external_sm;
 }
 
 void
@@ -1035,6 +1039,8 @@ 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;
+		conf->external_sm = 0;
 	}
 
 	mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks;
@@ -1045,6 +1051,8 @@ 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;
+	mode4->external_sm = conf->external_sm;
 }
 
 int
@@ -1062,6 +1070,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->external_sm)
+		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 +1084,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->external_sm) {
+		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 +1237,156 @@ 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 (valid_bonded_port_id(port_id) != 0 ||
+			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
+		return -EINVAL;
+
+	bond_dev = &rte_eth_devices[port_id];
+
+	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 || !mode4->external_sm)
+		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 (valid_bonded_port_id(port_id) != 0 ||
+			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
+		return -EINVAL;
+
+	bond_dev = &rte_eth_devices[port_id];
+
+	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 || !mode4->external_sm)
+		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 (valid_bonded_port_id(port_id) != 0 ||
+			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
+		return -EINVAL;
+
+	bond_dev = &rte_eth_devices[port_id];
+
+	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 || !mode4->external_sm)
+		return -EINVAL;
+
+	port = &mode_8023ad_ports[slave_id];
+
+	if (port->tx_ring == NULL)
+		return -EINVAL;
+
+	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) {
+		rte_pktmbuf_free(lacp_pkt);
+		return 0;
+	}
+
+	if (rte_ring_enqueue(port->tx_ring, lacp_pkt) == -ENOBUFS)
+		return -ENOBUFS;
+
+	MODE4_DEBUG("sending LACP frame\n");
+	return 0;
+}
+
+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;
+
+	if (mode4->slowrx_cb == NULL || !mode4->external_sm)
+		return;
+
+	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..c196584 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,8 @@ 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;
+	uint8_t external_sm;
 };
 
 struct rte_eth_bond_8023ad_slave_info {
@@ -219,4 +223,44 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
 }
 #endif
 
+/**
+ * Configure a slave port to start collecting.
+ *
+ * @param port_id	Bonding device id
+ * @param slave_id	Port id of valid slave.
+ * @param enabled	Non-zero when collection enabled.
+ * @return
+ *   0 - if ok
+ *   -EINVAL if slave is not valid.
+ */
+int
+rte_eth_bond_8023ad_ext_collect(uint8_t port_id, uint8_t slave_id, int enabled);
+
+/**
+ * Configure a slave port to start distributing.
+ *
+ * @param port_id	Bonding device id
+ * @param slave_id	Port id of valid slave.
+ * @param enabled	Non-zero when distribution enabled.
+ * @return
+ *   0 - if ok
+ *   -EINVAL if slave is not valid.
+ */
+int
+rte_eth_bond_8023ad_ext_distrib(uint8_t port_id, uint8_t slave_id, int enabled);
+
+/**
+ * LACPDU transmit path for external 802.3ad state machine
+ *
+ * @param port_id	Bonding device id
+ * @param slave_id	Port ID of valid slave device.
+ * @param lacp_pkt	mbuf containing LACPDU.
+ *
+ * @return
+ *   0 on success, negative value otherwise.
+ */
+int
+rte_eth_bond_8023ad_ext_slowtx(uint8_t port_id, uint8_t slave_id,
+		struct rte_mbuf *lacp_pkt);
+
 #endif /* RTE_ETH_BOND_8023AD_H_ */
diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h b/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h
index 8adee70..9e15ece 100644
--- a/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h
+++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h
@@ -173,6 +173,8 @@ struct mode8023ad_private {
 	uint64_t tx_period_timeout;
 	uint64_t rx_marker_timeout;
 	uint64_t update_timeout_us;
+	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
+	uint8_t external_sm;
 };
 
 /**
-- 
1.7.10.4

Re: [PATCH 4/5] bond mode 4: allow external state machine

[Pawel Wodkowski]

On 2015-04-06 19:01, Eric Kinzie wrote:

Interesting patch. I will closer look at this tomorrow.

For now I have first comments:

> +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,8 @@ 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;
> +	conf->external_sm = mode4->external_sm;

mode4->external_sm flag realy needed? Why do not use mode4->slowrx_cb as 
external state machine indicator?

>   }
>
>   void
> @@ -1035,6 +1039,8 @@ 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;
> +		conf->external_sm = 0;
>   	}
>
>   	mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks;
> @@ -1045,6 +1051,8 @@ 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;
> +	mode4->external_sm = conf->external_sm;
>   }
>
>   int
> @@ -1062,6 +1070,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->external_sm)
> +		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 +1084,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->external_sm) {

This is bad idea. If bond_mode_8023ad_setup will be called you might 
have two handlers running for while. You should stop mode 4 by invoking 
bond_mode_8023ad_stop() before you set mode4->external_sm and then, if 
mode 4 was running, start it again.

Also, maybe a renaming "external_sm" to "state_machine_cb", set it to 
against default one and using it without "if()" will simplify code. It 
is no crucial but will eliminate couple of if's. In 
rte_eth_bond_8023ad_ext_slowtx() you can compare it against default one.

> +		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 +1237,156 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
>   	info->agg_port_id = port->aggregator_port_id;
>   	return 0;
>   }

-- 
Pawel

Re: [PATCH 4/5] bond mode 4: allow external state machine

[Pawel Wodkowski]

On 2015-04-07 16:18, Pawel Wodkowski wrote:

>
> Also, maybe a renaming "external_sm" to "state_machine_cb", set it to
> against default one and using it without "if()" will simplify code. It
> is no crucial but will eliminate couple of if's. In
> rte_eth_bond_8023ad_ext_slowtx() you can compare it against default one.

Oh, I read what I wrote :)
Please ignore that.

-- 
Pawel

Re: [PATCH 4/5] bond mode 4: allow external state machine

[Eric Kinzie]

On Tue Apr 07 16:18:08 +0200 2015, Pawel Wodkowski wrote:
> On 2015-04-06 19:01, Eric Kinzie wrote:
> 
> Interesting patch. I will closer look at this tomorrow.
> 
> For now I have first comments:
> 
> >+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,8 @@ 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;
> >+	conf->external_sm = mode4->external_sm;
> 
> mode4->external_sm flag realy needed? Why do not use
> mode4->slowrx_cb as external state machine indicator?

I'll remove the external_sm flag.  You're correct, it's not needed.


> >  }
> >
> >  void
> >@@ -1035,6 +1039,8 @@ 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;
> >+		conf->external_sm = 0;
> >  	}
> >
> >  	mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks;
> >@@ -1045,6 +1051,8 @@ 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;
> >+	mode4->external_sm = conf->external_sm;
> >  }
> >
> >  int
> >@@ -1062,6 +1070,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->external_sm)
> >+		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 +1084,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->external_sm) {
> 
> This is bad idea. If bond_mode_8023ad_setup will be called you might
> have two handlers running for while. You should stop mode 4 by
> invoking bond_mode_8023ad_stop() before you set mode4->external_sm
> and then, if mode 4 was running, start it again.

Thanks, I'll make that change.



> Also, maybe a renaming "external_sm" to "state_machine_cb", set it
> to against default one and using it without "if()" will simplify
> code. It is no crucial but will eliminate couple of if's. In
> rte_eth_bond_8023ad_ext_slowtx() you can compare it against default
> one.
> 
> >+		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 +1237,156 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
> >  	info->agg_port_id = port->aggregator_port_id;
> >  	return 0;
> >  }
> 
> -- 
> Pawel

Re: [PATCH 4/5] bond mode 4: allow external state machine

[Pawel Wodkowski]

Hi Eric
Please see my comments.

On 2015-04-06 19:01, Eric Kinzie wrote:
>    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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   lib/librte_pmd_bond/rte_eth_bond_8023ad.c         |  175 +++++++++++++++++++++
>   lib/librte_pmd_bond/rte_eth_bond_8023ad.h         |   44 ++++++
>   lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h |    2 +
>   3 files changed, 221 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..29cd962 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,8 @@ 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;
> +	conf->external_sm = mode4->external_sm;
>   }
>
>   void
> @@ -1035,6 +1039,8 @@ 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;
> +		conf->external_sm = 0;
>   	}
>
>   	mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks;
> @@ -1045,6 +1051,8 @@ 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;
> +	mode4->external_sm = conf->external_sm;
>   }
>
>   int
> @@ -1062,6 +1070,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->external_sm)
> +		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 +1084,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->external_sm) {
> +		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 +1237,156 @@ 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 (valid_bonded_port_id(port_id) != 0 ||
> +			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)

The rte_eth_bond_mode_get() function already check if given port_id is 
valid bonded device so you can remove valid_bonded_port_id() here.

You should check here is port is started.

> +		return -EINVAL;
> +
> +	bond_dev = &rte_eth_devices[port_id];
> +
> +	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 || !mode4->external_sm)
> +		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 (valid_bonded_port_id(port_id) != 0 ||
> +			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)

The same as above.
You can remove call to valid_bonded_port_id().
You should check here if port is started.

> +		return -EINVAL;
> +
> +	bond_dev = &rte_eth_devices[port_id];
> +
> +	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 || !mode4->external_sm)
> +		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 (valid_bonded_port_id(port_id) != 0 ||
> +			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)

The same as above.
You can remove call to valid_bonded_port_id().
You should check here if port is started.

> +		return -EINVAL;
> +
> +	bond_dev = &rte_eth_devices[port_id];
> +
> +	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 || !mode4->external_sm)
> +		return -EINVAL;
> +
> +	port = &mode_8023ad_ports[slave_id];
> +
> +	if (port->tx_ring == NULL)
> +		return -EINVAL;

If bonded port is started and is in mode 4 the tx_ring might not be
NULL. If it is NULL then it is a bug, and should not be hidden by
returning error code.

> +
> +	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) {
> +		rte_pktmbuf_free(lacp_pkt);
> +		return 0;
> +	}

I would rather return here an error and do not free the packet.

> +
> +	if (rte_ring_enqueue(port->tx_ring, lacp_pkt) == -ENOBUFS)
> +		return -ENOBUFS;
> +
> +	MODE4_DEBUG("sending LACP frame\n");
> +	return 0;
> +}
> +
> +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;
> +
> +	if (mode4->slowrx_cb == NULL || !mode4->external_sm)
> +		return;

Please remove this check. It might not happen at runtime.
If you like you can place here RTE_VERIFY().

> +
> +	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..c196584 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,8 @@ 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;
> +	uint8_t external_sm;
>   };
>
>   struct rte_eth_bond_8023ad_slave_info {
> @@ -219,4 +223,44 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
>   }
>   #endif
>
> +/**
> + * Configure a slave port to start collecting.
> + *
> + * @param port_id	Bonding device id
> + * @param slave_id	Port id of valid slave.
> + * @param enabled	Non-zero when collection enabled.
> + * @return
> + *   0 - if ok
> + *   -EINVAL if slave is not valid.
> + */
> +int
> +rte_eth_bond_8023ad_ext_collect(uint8_t port_id, uint8_t slave_id, int enabled);
> +
> +/**
> + * Configure a slave port to start distributing.
> + *
> + * @param port_id	Bonding device id
> + * @param slave_id	Port id of valid slave.
> + * @param enabled	Non-zero when distribution enabled.
> + * @return
> + *   0 - if ok
> + *   -EINVAL if slave is not valid.
> + */
> +int
> +rte_eth_bond_8023ad_ext_distrib(uint8_t port_id, uint8_t slave_id, int enabled);
> +
> +/**
> + * LACPDU transmit path for external 802.3ad state machine
> + *
> + * @param port_id	Bonding device id
> + * @param slave_id	Port ID of valid slave device.
> + * @param lacp_pkt	mbuf containing LACPDU.
> + *
> + * @return
> + *   0 on success, negative value otherwise.

Please include informations when packet is given back to the caller and 
when its ownership is taken by bonding.

> + */
> +int
> +rte_eth_bond_8023ad_ext_slowtx(uint8_t port_id, uint8_t slave_id,
> +		struct rte_mbuf *lacp_pkt);
> +
>   #endif /* RTE_ETH_BOND_8023AD_H_ */
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h b/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h
> index 8adee70..9e15ece 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h
> +++ b/lib/librte_pmd_bond/rte_eth_bond_8023ad_private.h
> @@ -173,6 +173,8 @@ struct mode8023ad_private {
>   	uint64_t tx_period_timeout;
>   	uint64_t rx_marker_timeout;
>   	uint64_t update_timeout_us;
> +	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
> +	uint8_t external_sm;
>   };
>
>   /**
>


-- 
Pawel

[PATCH 5/5] bond mode 4: tests for external state machine

[Eric Kinzie]

  This adds test cases for exercising the external state machine API to
  the mode 4 autotest.

Signed-off-by: Eric Kinzie <ehkinzie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 app/test/test_link_bonding_mode4.c |  201 ++++++++++++++++++++++++++++++++++--
 1 file changed, 192 insertions(+), 9 deletions(-)

diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 5a726af..a37b59c 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -155,6 +155,8 @@ static struct rte_eth_conf default_pmd_conf = {
 	.lpbk_mode = 0,
 };
 
+static uint8_t lacpdu_rx_count[RTE_MAX_ETHPORTS] = {0, };
+
 #define FOR_EACH(_i, _item, _array, _size) \
 	for (_i = 0, _item = &_array[0]; _i < _size && (_item = &_array[_i]); _i++)
 
@@ -324,8 +326,16 @@ remove_slave(struct slave_conf *slave)
 	return 0;
 }
 
+static void
+lacp_recv_cb(uint8_t slave_id, struct rte_mbuf *lacp_pkt)
+{
+	lacpdu_rx_count[slave_id]++;
+	RTE_VERIFY(lacp_pkt != NULL);
+	rte_pktmbuf_free(lacp_pkt);
+}
+
 static int
-initialize_bonded_device_with_slaves(uint8_t slave_count, uint8_t start)
+initialize_bonded_device_with_slaves(uint8_t slave_count, uint8_t external_sm)
 {
 	uint8_t i;
 
@@ -341,9 +351,18 @@ initialize_bonded_device_with_slaves(uint8_t slave_count, uint8_t start)
 	rte_eth_bond_8023ad_setup(test_params.bonded_port_id, NULL);
 	rte_eth_promiscuous_disable(test_params.bonded_port_id);
 
-	if (start)
-		TEST_ASSERT_SUCCESS(rte_eth_dev_start(test_params.bonded_port_id),
-			"Failed to start bonded device");
+	if (external_sm) {
+		struct rte_eth_bond_8023ad_conf conf;
+
+		rte_eth_bond_8023ad_conf_get(test_params.bonded_port_id, &conf);
+		conf.external_sm = 1;
+		conf.slowrx_cb = lacp_recv_cb;
+		rte_eth_bond_8023ad_setup(test_params.bonded_port_id, &conf);
+
+	}
+
+	TEST_ASSERT_SUCCESS(rte_eth_dev_start(test_params.bonded_port_id),
+		"Failed to start bonded device");
 
 	return TEST_SUCCESS;
 }
@@ -648,7 +667,7 @@ test_mode4_lacp(void)
 {
 	int retval;
 
-	retval = initialize_bonded_device_with_slaves(TEST_LACP_SLAVE_COUT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_LACP_SLAVE_COUT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	/* Test LACP handshake function */
@@ -746,7 +765,7 @@ test_mode4_rx(void)
 	struct ether_addr dst_mac;
 	struct ether_addr bonded_mac;
 
-	retval = initialize_bonded_device_with_slaves(TEST_PROMISC_SLAVE_COUNT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_PROMISC_SLAVE_COUNT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	retval = bond_handshake();
@@ -923,7 +942,7 @@ test_mode4_tx_burst(void)
 	struct ether_addr dst_mac = { { 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 } };
 	struct ether_addr bonded_mac;
 
-	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	retval = bond_handshake();
@@ -1107,7 +1126,7 @@ test_mode4_marker(void)
 	uint8_t i, j;
 	const uint16_t ethtype_slow_be = rte_be_to_cpu_16(ETHER_TYPE_SLOW);
 
-	retval = initialize_bonded_device_with_slaves(TEST_MARKER_SLAVE_COUT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_MARKER_SLAVE_COUT, 0);
 	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
 
 	/* Test LACP handshake function */
@@ -1192,7 +1211,7 @@ test_mode4_expired(void)
 
 	struct rte_eth_bond_8023ad_conf conf;
 
-	retval = initialize_bonded_device_with_slaves(TEST_EXPIRED_SLAVE_COUNT, 1);
+	retval = initialize_bonded_device_with_slaves(TEST_EXPIRED_SLAVE_COUNT, 0);
 	/* Set custom timeouts to make test last shorter. */
 	rte_eth_bond_8023ad_conf_get(test_params.bonded_port_id, &conf);
 	conf.fast_periodic_ms = 100;
@@ -1274,6 +1293,156 @@ test_mode4_expired(void)
 }
 
 static int
+test_mode4_ext_ctrl(void)
+{
+	/*
+	 * configure bonded interface without the external sm enabled
+	 *   . try to transmit lacpdu (should fail)
+	 *   . try to set collecting and distributing flags (should fail)
+	 * reconfigure w/external sm
+	 *   . transmit one lacpdu on each slave using new api
+	 *   . make sure each slave receives one lacpdu using the callback api
+	 *   . transmit one data pdu on each slave (should fail)
+	 *   . enable distribution and collection, send one data pdu each again
+	 */
+
+	int retval;
+	struct slave_conf *slave = NULL;
+	uint8_t i;
+
+	struct rte_mbuf *lacp_tx_buf[SLAVE_COUNT];
+	struct ether_addr src_mac, dst_mac;
+	struct lacpdu_header lacpdu = {
+		.lacpdu = {
+			.subtype = SLOW_SUBTYPE_LACP,
+		},
+	};
+
+	ether_addr_copy(&parnter_system, &src_mac);
+	ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
+
+	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
+			      ETHER_TYPE_SLOW, 0, 0);
+
+	for (i = 0; i < SLAVE_COUNT; i++) {
+		lacp_tx_buf[i] = rte_pktmbuf_alloc(test_params.mbuf_pool);
+		rte_memcpy(rte_pktmbuf_mtod(lacp_tx_buf[i], char *),
+			   &lacpdu, sizeof(lacpdu));
+		rte_pktmbuf_pkt_len(lacp_tx_buf[i]) = sizeof(lacpdu);
+	}
+
+	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 0);
+	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
+
+	FOR_EACH_SLAVE(i, slave) {
+		TEST_ASSERT_FAIL(rte_eth_bond_8023ad_ext_slowtx(
+						test_params.bonded_port_id,
+						slave->port_id, lacp_tx_buf[i]),
+				 "Slave should not allow manual LACP xmit");
+		TEST_ASSERT_FAIL(rte_eth_bond_8023ad_ext_collect(
+						test_params.bonded_port_id,
+						slave->port_id, 1),
+				 "Slave should not allow external state controls");
+	}
+
+	free_pkts(lacp_tx_buf, RTE_DIM(lacp_tx_buf));
+
+	retval = remove_slaves_and_stop_bonded_device();
+	TEST_ASSERT_SUCCESS(retval, "Bonded device cleanup failed.");
+
+	return TEST_SUCCESS;
+}
+
+
+static int
+test_mode4_ext_lacp(void)
+{
+	int retval;
+	struct slave_conf *slave = NULL;
+	uint8_t all_slaves_done = 0, i;
+	uint16_t nb_pkts;
+	const unsigned delay = bond_get_update_timeout_ms();
+
+	struct rte_mbuf *lacp_tx_buf[SLAVE_COUNT];
+	struct rte_mbuf *buf[SLAVE_COUNT];
+	struct ether_addr src_mac, dst_mac;
+	struct lacpdu_header lacpdu = {
+		.lacpdu = {
+			.subtype = SLOW_SUBTYPE_LACP,
+		},
+	};
+
+	ether_addr_copy(&parnter_system, &src_mac);
+	ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
+
+	initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
+			      ETHER_TYPE_SLOW, 0, 0);
+
+	for (i = 0; i < SLAVE_COUNT; i++) {
+		lacp_tx_buf[i] = rte_pktmbuf_alloc(test_params.mbuf_pool);
+		rte_memcpy(rte_pktmbuf_mtod(lacp_tx_buf[i], char *),
+			   &lacpdu, sizeof(lacpdu));
+		rte_pktmbuf_pkt_len(lacp_tx_buf[i]) = sizeof(lacpdu);
+	}
+
+	retval = initialize_bonded_device_with_slaves(TEST_TX_SLAVE_COUNT, 1);
+	TEST_ASSERT_SUCCESS(retval, "Failed to initialize bonded device");
+
+	memset(lacpdu_rx_count, 0, sizeof(lacpdu_rx_count));
+
+	/* Wait for new settings to be applied. */
+	for (i = 0; i < 30; ++i)
+		rte_delay_ms(delay);
+
+	FOR_EACH_SLAVE(i, slave) {
+		retval = rte_eth_bond_8023ad_ext_slowtx(
+						test_params.bonded_port_id,
+						slave->port_id, lacp_tx_buf[i]);
+		TEST_ASSERT_SUCCESS(retval,
+				    "Slave should allow manual LACP xmit");
+	}
+
+	nb_pkts = bond_tx(NULL, 0);
+	TEST_ASSERT_EQUAL(nb_pkts, 0, "Packets transmitted unexpectedly");
+
+	FOR_EACH_SLAVE(i, slave) {
+		nb_pkts = slave_get_pkts(slave, buf, RTE_DIM(buf));
+		TEST_ASSERT_EQUAL(nb_pkts, 1, "found %u packets on slave %d\n",
+				  nb_pkts, i);
+		slave_put_pkts(slave, buf, nb_pkts);
+	}
+
+	nb_pkts = bond_rx(buf, RTE_DIM(buf));
+	free_pkts(buf, nb_pkts);
+	TEST_ASSERT_EQUAL(nb_pkts, 0, "Packets received unexpectedly");
+
+	/* wait for the periodic callback to run */
+	for (i = 0; i < 30 && all_slaves_done == 0; ++i) {
+		uint8_t s, total = 0;
+
+		rte_delay_ms(delay);
+		FOR_EACH_SLAVE(s, slave) {
+			total += lacpdu_rx_count[slave->port_id];
+		}
+
+		if (total >= SLAVE_COUNT)
+			all_slaves_done = 1;
+	}
+
+	FOR_EACH_SLAVE(i, slave) {
+		TEST_ASSERT_EQUAL(lacpdu_rx_count[slave->port_id], 1,
+				  "Slave port %u should have received 1 lacpdu (count=%u)",
+				  slave->port_id,
+				  lacpdu_rx_count[slave->port_id]);
+	}
+
+	retval = remove_slaves_and_stop_bonded_device();
+	TEST_ASSERT_SUCCESS(retval, "Test cleanup failed.");
+
+	return TEST_SUCCESS;
+}
+
+static int
 check_environment(void)
 {
 	struct slave_conf *port;
@@ -1389,6 +1558,18 @@ test_mode4_expired_wrapper(void)
 	return test_mode4_executor(&test_mode4_expired);
 }
 
+static int
+test_mode4_ext_ctrl_wrapper(void)
+{
+	return test_mode4_executor(&test_mode4_ext_ctrl);
+}
+
+static int
+test_mode4_ext_lacp_wrapper(void)
+{
+	return test_mode4_executor(&test_mode4_ext_lacp);
+}
+
 static struct unit_test_suite link_bonding_mode4_test_suite  = {
 	.suite_name = "Link Bonding mode 4 Unit Test Suite",
 	.setup = test_setup,
@@ -1399,6 +1580,8 @@ static struct unit_test_suite link_bonding_mode4_test_suite  = {
 		TEST_CASE_NAMED("test_mode4_tx_burst", test_mode4_tx_burst_wrapper),
 		TEST_CASE_NAMED("test_mode4_marker", test_mode4_marker_wrapper),
 		TEST_CASE_NAMED("test_mode4_expired", test_mode4_expired_wrapper),
+		TEST_CASE_NAMED("test_mode4_ext_ctrl", test_mode4_ext_ctrl_wrapper),
+		TEST_CASE_NAMED("test_mode4_ext_lacp", test_mode4_ext_lacp_wrapper),
 		{ NULL, NULL, NULL, NULL, NULL } /**< NULL terminate unit test array */
 	}
 };
-- 
1.7.10.4

Re: [PATCH 5/5] bond mode 4: tests for external state machine

[Pawel Wodkowski]

On 2015-04-06 19:01, Eric Kinzie wrote:

>
> +static void
> +lacp_recv_cb(uint8_t slave_id, struct rte_mbuf *lacp_pkt)
> +{
> +	lacpdu_rx_count[slave_id]++;
> +	RTE_VERIFY(lacp_pkt != NULL);
> +	rte_pktmbuf_free(lacp_pkt);
> +}
> +

Would be nice to check here if it is valid LACP packet.

-- 
Pawel