[dpdk-dev] [RFC 0/4] Use Google Test as DPDK unit test framework

2016-08-03 Thread Doherty, Declan


> -Original Message-
...
> You are not advocating but the unit test must be written in C++.
> I don't think it is a good idea to force people to write and maintain the 
> tests
> in a different language than the code it tests.

I know where you are coming from on this point, and I general would agree if
it were not for the advantages you get from C++ test framework. Having worked 
with
multiple C and C++ frameworks, I've found that one of the biggest advantages of 
the
C++ frameworks is the amount of boilerplate code they can save you from 
writing. Also
nearly all of C frameworks I've used make use macros to the point that they 
look more like
objective C than C. In general I feel that even if the test code is written in 
C++ the code itself
should be simple enough that someone with even a passing knowledge of C++ could 
easily
understand the intent of the test code. 

> > Some of the major advantages of google test that I see over continuing to 
> > use
> the
> > current test include giving a consist feel to all tests, a powerful test
> > execution framework which allow individual test suites or tests to be 
> > specified
> > from the command line, support for a standard xunit output which can be
> integrated
> > into a continuous build systems, and a very powerful mocking library
> > which allows much more control over testing failure conditions.
> 
> It would be interesting to better describe in details what is missing 
> currently
> and what such a framework can bring.
> (I agree there is a huge room for improvements on unit tests)

Some of the things I've come across include:
No standard output format to integrated with continuous regression systems
No ability to specify specific unit tests or groups of tests to run from the 
command line
No standard set of test assertions used across the test suites.
No standard setup and teardown functions across test suites, state from 
previous test
suite can break current
Requirement to use a python script to orchestrate test runs.
No support for mocking functionality.

I know that none of the above couldn't be fixed in our current test 
application, but I would 
question if it is effort worthwhile when we take an off the shelf framework, 
which does all 
those things and a whole lot more, which has been test and used in a huge 
variety of
projects. 

I certainly willing to look at other frameworks both C and C++ but I yet to 
find a C framework
which come close to the usability and flexibility of the popular C++ ones.





[dpdk-dev] [PATCH] bonding: fix bond link detect in non-interrupt mode

2016-04-01 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Daley
> Sent: Saturday, March 26, 2016 12:45 AM
> To: dev at dpdk.org
> Cc: Nelson Escobar ; John Daley 
> Subject: [dpdk-dev] [PATCH] bonding: fix bond link detect in non-interrupt 
> mode
> 
> From: Nelson Escobar 
> 
> Stopping then re-starting a bond interface containing slaves that
> used polling for link detection caused the bond to think all slave
> links were down and inactive.
> 
> Move the start of the polling for link from slave_add() to
> bond_ethdev_start() and in bond_ethdev_stop() make sure we clear
> the last_link_status of the slaves.
> 
> Signed-off-by: Nelson Escobar 
> Signed-off-by: John Daley 
> ---
.
> --
> 2.7.0

Acked-by: Declan Doherty 


[dpdk-dev] [PATCH 0/2] bonding fixes

2016-02-22 Thread Doherty, Declan
> -Original Message-
> From: Iremonger, Bernard
> Sent: Wednesday, February 10, 2016 10:14 AM
> To: dev at dpdk.org
> Cc: Doherty, Declan ; Iremonger, Bernard
> 
> Subject: [PATCH 0/2] bonding fixes
> 
> These patches fix segmentation faults which were occurring when
> slave devices were detached before being removed from the bonding
> device.
> The slave devices must now be removed from the bonding device
> before they can be detached.
> The bonding device cannot be detached now until all slave devices
> have been removed from it.
> 
> Bernare Iremonger (2):
>   bonding: fix detach of bonded device
>   bonding: fix detach of bonded slave devices
> 
>  drivers/net/bonding/rte_eth_bond_api.c | 40 +++-
> --
>  lib/librte_ether/rte_ethdev.c  |  8 +--
>  lib/librte_ether/rte_ethdev.h  |  4 +++-
>  3 files changed, 26 insertions(+), 26 deletions(-)
> 
> --
> 2.6.3

Series Acked-by: Declan Doherty 


[dpdk-dev] [PATCH] cryptodev: mark experimental state

2015-11-26 Thread Doherty, Declan
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, November 26, 2015 10:09 AM
> To: Panu Matilainen; Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] cryptodev: mark experimental state
> 
> 2015-11-26 10:00, Panu Matilainen:
> > On 11/26/2015 09:39 AM, Panu Matilainen wrote:
> > > On 11/25/2015 07:38 PM, Thomas Monjalon wrote:
> > >> --- a/config/common_linuxapp
> > >> +++ b/config/common_linuxapp
> > >> @@ -319,6 +319,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
> > >>
> > >>   #
> > >>   # Compile generic crypto device library
> > >> +# EXPERIMENTAL: API may change without prior notice
> > >>   #
> > >>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
> > >>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
> > > [...]
> > >
> > > I think an experimental library which declares itself exempt from the
> > > ABI policy should not be compiled by default. That way anybody wanting
> > > to try it out will be forced to notice the experimental status.
> > >
> > > More generally / longer term, perhaps there should be a
> > > CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and
> > > defaults to off.
> >
> > On a related note, librte_mbuf_offload cannot be built if
> > CONFIG_RTE_LIBRTE_CRYPTODEV is disabled. Which seems to suggest its (at
> > least currently) so tightly couple to cryptodev that perhaps it too
> > should be marked experimental and default to off.
> 
> I think you are right.
> Declan, what is your opinion?


Hey Thomas, yes librte_mbuf_offload should also be set as experimental, it's 
probably one of the areas which will most likely change in the future.

On the issue of turning off experimental libraries in the build by default, my
preference would be not to turn them off unless the library has external
dependencies, otherwise the possibility of patches being submitted which 
could break an experimental library will be much higher. In my opinion the
fewer build configurations developers have to test against the better.



[dpdk-dev] [PATCH] bond: fix check initial link status of slave

2015-06-26 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tomasz Kulasek
> Sent: Wednesday, June 24, 2015 1:51 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] bond: fix check initial link status of slave
> 
> On Fortville NIC, link status change interrupt callback is not executed when
> slave in bonding is (re-)started. It causes that slave's NIC is inactive even
> if its link status is up on the start.
> 
> This patch invokes lsc callback, just after port's start, to check its initial
> link status and manage properly.
> 
> Signed-off-by: Tomasz Kulasek 
> ---
>  drivers/net/bonding/rte_eth_bond_pmd.c |5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 8bad2e1..277b310 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1357,6 +1357,11 @@ slave_configure(struct rte_eth_dev
> *bonded_eth_dev,
>   return -1;
>   }
> 
> + /* If lsc interrupt is set, check initial slave's link status */
> + if (slave_eth_dev->driver->pci_drv.drv_flags & RTE_PCI_DRV_INTR_LSC)
> + bond_ethdev_lsc_event_callback(slave_eth_dev->data->port_id,
> + RTE_ETH_EVENT_INTR_LSC, _eth_dev-
> >data->port_id);
> +
>   return 0;
>  }
> 
> --
> 1.7.9.5

Acked-by : Declan Doherty 


[dpdk-dev] [PATCH] Make bond_ethdev_stop iterate only over active slaves

2015-06-26 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jan Blunck
> Sent: Wednesday, June 24, 2015 2:01 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] Make bond_ethdev_stop iterate only over active
> slaves
> 
> When stopping the bond device we don't need to try and free up the LACPDU's
> from deactivated devices since this is covered by
> bond_mode_8023ad_deactivate_slave().
> 
> This fixes the following:
> [0.100569] PANIC in bond_ethdev_stop():
> [0.100589] line 1172assert "port->rx_ring != NULL" failed
> 
> Signed-off-by: Jan Blunck 
> ---
>  drivers/net/bonding/rte_eth_bond_pmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 5a2fbef..1fd1321 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1520,8 +1520,8 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>   bond_mode_8023ad_stop(eth_dev);
> 
>   /* Discard all messages to/from mode 4 state machines */
> - for (i = 0; i < internals->slave_count; i++) {
> - port = _8023ad_ports[internals->slaves[i].port_id];
> + for (i = 0; i < internals->active_slave_count; i++) {
> + port = _8023ad_ports[internals->active_slaves[i]];
> 
>   RTE_VERIFY(port->rx_ring != NULL);
>   while (rte_ring_dequeue(port->rx_ring, ) != -
> ENOENT)
> --
> 2.1.4

Acked-by : Declan Doherty 


[dpdk-dev] [PATCH v2 0/8] Dynamic RSS Configuration for Bonding

2015-06-26 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tomasz Kulasek
> Sent: Friday, June 19, 2015 7:13 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v2 0/8] Dynamic RSS Configuration for Bonding
> 
> OVERVIEW
> 
> 1) Setting .rxmode.mq_mode for bonding device to ETH_MQ_RX_RSS makes
> bonding
> device fully RSS-capable, so all slaves are synchronized with its 
> configuration.
> This mode is intended to provide RSS configuration as known from "dynamic RSS
> configuration for one port" and made slaves transparent for client application
> implementation.
> 
> 2) If .rxmode.mq_mode for bonding device isn't ETH_MQ_RX_RSS, slaves are not
> synchronized. That provides an ability to configure them manually. This mode 
> may
> be useful when application wants to manage RSS in an unusual way and the
> consistency of RSS configuration for slaves isn't required.
> 
> Turning on/off RSS mode for slaves when bonding is started is not possible.
> Other RSS configuration is propagated over slaves, when bonding device API is
> used to do it.
> 
> v2 changes:
>  - added support for keys other than 40 bytes long,
>  - now, if RSS key is not set for bonding, it is not set also for slaves,
>  - fix - full initial RSS configuration before any slave is added was not
>possible due to the initially zeroed flow_type_rss_offloads for bonding,
>  - fix - changed error to warning when slave is synchronizing due to the
>bonding's initial configuration (to allow use slaves' drivers not 
> supporting
>dynamic RSS configuration in bonding),
>  - some code cleanups,
>  - updated documentation,
> 
> Tomasz Kulasek (8):
>   bond: rss dynamic configuration
>   ring: dynamic rss configuration
>   test: dynamic rss configuration
>   bond: queue stats mapping
>   ring: queue stats mapping set dummy implementation
>   examples: dynamic rss configuration for bonding
>   doc: fixed spellings and typos
>   doc: dynamic rss configuration for bonding
> 
>  app/test/Makefile  |1 +
>  app/test/test_link_bonding_rssconf.c   |  674 ++
>  .../prog_guide/link_bonding_poll_mode_drv_lib.rst  |   42 +-
>  drivers/net/bonding/rte_eth_bond_api.c |   27 +
>  drivers/net/bonding/rte_eth_bond_pmd.c |  239 -
>  drivers/net/bonding/rte_eth_bond_private.h |   12 +
>  drivers/net/ring/rte_eth_ring.c|  133 ++-
>  examples/bond_rss/Makefile |   59 ++
>  examples/bond_rss/bondrss.c|  293 +++
>  examples/bond_rss/bondrss.h|  163 
>  examples/bond_rss/config.c |  251 ++
>  examples/bond_rss/ui.c |  920 
> 
>  12 files changed, 2789 insertions(+), 25 deletions(-)
>  create mode 100644 app/test/test_link_bonding_rssconf.c
>  create mode 100644 examples/bond_rss/Makefile
>  create mode 100644 examples/bond_rss/bondrss.c
>  create mode 100644 examples/bond_rss/bondrss.h
>  create mode 100644 examples/bond_rss/config.c
>  create mode 100644 examples/bond_rss/ui.c
> 
> --
> 1.7.9.5 

Acked-by : Declan Doherty 


[dpdk-dev] [PATCH v2 0/3] PMD ring MAC management, fix initialization, link up/down

2015-01-28 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tomasz Kulasek
> Sent: Monday, January 19, 2015 11:57 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v2 0/3] PMD ring MAC management, fix 
> initialization,
> link up/down
> 
> Patch split into smaller parts to separate features from previous version.
> 
> Tomasz Kulasek (3):
>   PMD Ring - Add link up/down functions
>   PMD Ring - Add MAC addr add/remove functions
>   PMD Ring - Fix for per device management
> 
>  lib/librte_pmd_ring/rte_eth_ring.c |   62
> +---
>  1 file changed, 57 insertions(+), 5 deletions(-)
> 
> --
> 1.7.9.5

Acked-by: Declan Doherty 


[dpdk-dev] rte_eal_init() causes PANIC using latest DPDK code.

2015-01-19 Thread Doherty, Declan

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tetsuya Mukawa
> Sent: Monday, January 19, 2015 7:11 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] rte_eal_init() causes PANIC using latest DPDK code.
> 
> Hi,
> 
> I've got following error using latest DPDK code.
> Does someone face the same issue?
> In my environment, It seems almost all DPDK apps (at lease l2fwd and
> testpmd) cannot work.
> 
> 
> EAL: Support maximum 128 logical core(s) by configuration.
> EAL: Detected 8 lcore(s)
> EAL:   cannot open VFIO container, error 2 (No such file or directory)
> EAL: VFIO support could not be initialized
> EAL: Setting up memory...
> EAL: Ask a virtual area of 0x28000 bytes
> EAL: Virtual area found at 0x7ffd4000 (size = 0x28000)
> EAL: Requesting 10 pages of size 1024MB from socket 0
> RING: Cannot reserve memory for tailq
> EAL: rte_eal_common_log_init(): cannot create log_history mempool
> PANIC in rte_eal_init():
> Cannot init logs
> 6: [./build/l2fwd() [0x41cf53]]
> 5: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5)
> [0x77105ec5]]
> 4: [./build/l2fwd(main+0x1c) [0x41c52c]]
> 3: [./build/l2fwd(rte_eal_init+0x1073) [0x46a653]]
> 2: [./build/l2fwd(__rte_panic+0xc9) [0x41c3ff]]
> 1: [./build/l2fwd(rte_dump_stack+0x18) [0x471828]]
> 
> 
> I tried bisect, and it seems below patch causes this.
> "eal: fix check for power of 2 in 0 case".
> 
> 
> Thanks,
> Tetsuya
> 
> 

Hey Tetsuya, 

This is the issue which my patch (http://dpdk.org/dev/patchwork/patch/2333/) 
submitted on Friday addresses.



[dpdk-dev] [PATCH] eal / malloc : alignment parameter check failing due to changes in rte_is_power_of_2

2015-01-18 Thread Doherty, Declan
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Friday, January 16, 2015 5:02 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org; De Lara Guarch, Pablo
> Subject: Re: [dpdk-dev] [PATCH] eal / malloc : alignment parameter check 
> failing
> due to changes in rte_is_power_of_2
> 
> 2015-01-16 15:10, Declan Doherty:
> > In commit 2fc8d6d the behaviour of function rte_is_power_of_2 was
> > changed to not return true for 0. memzone_reserve_aligned_thread_unsafe
> > and rte_malloc_socket both make the assumption that for align = 0
> > !rte_is_power_of_2(align) will return false. This patch adds a check
> > that align parameter is non-zero before doing the power of 2 check
> >
> > Signed-off-by: Declan Doherty 
> [...]
> > -   if (!rte_is_power_of_2(align)) {
> > +   if (align ? !rte_is_power_of_2(align) : 0) {
> [...]
> > -   if (size == 0 || !rte_is_power_of_2(align))
> > +   if (size == 0 || align ? !rte_is_power_of_2(align) : 0)
> 
> I don't understand why you write "align ? !rte_is_power_of_2(align) : 0"
> instead of the more readable "align && !rte_is_power_of_2(align)" ?
> 
> Pablo acked it so I guess there is something obvious I'm missing.
> 
> --
> Thomas

No there's nothing you're missing, this is just the way I saw the logic, if 
align is none
zero, then test the power of 2 condition otherwise return 0. I have no problem 
with
your suggestion in you prefer that, at the end of the day the logic test works 
out equivalent.


[dpdk-dev] [PATCH v2] bond: set {rx|tx}_offload_capa flags

2014-11-27 Thread Doherty, Declan
Thanks Thomas, was just about to send another version :)

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, November 27, 2014 9:53 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org; jyu at vmware.com
> Subject: Re: [PATCH v2] bond: set {rx|tx}_offload_capa flags
> 
> 2014-11-27 21:23, Declan Doherty:
> > v2:
> > rebased to HEAD
> >
> > From: Jia Yu 
> >
> > Before the fix, bond device's offload capabilities are unset. This fix
> > takes the minimum common set of slave devices' capabilities as bond
> > device's capabilities. For simplicity, we ensure all slave devices
> > to have a capability before bond device can claim this capability,
> > even if some slave devices are unused (i.e. linked down, standby).
> >
> > Signed-off-by: Jia Yu 
> > Signed-off-by: Declan Doherty 
> 
> Applied
> 
> Thanks
> --
> Thomas


[dpdk-dev] [PATCH v2] bond: set {rx|tx}_offload_capa flags

2014-11-27 Thread Doherty, Declan
Self nack, need to rebase again as mode 4 and 5 patches have been applied.

> -Original Message-
> From: Doherty, Declan
> Sent: Thursday, November 27, 2014 9:24 PM
> To: dev at dpdk.org
> Cc: jyu at vmware.com; thomas.monjalon at 6wind.com; Doherty, Declan
> Subject: [PATCH v2] bond: set {rx|tx}_offload_capa flags
> 
> v2:
> rebased to HEAD
> 
> From: Jia Yu 
> 
> Before the fix, bond device's offload capabilities are unset. This fix
> takes the minimum common set of slave devices' capabilities as bond
> device's capabilities. For simplicity, we ensure all slave devices
> to have a capability before bond device can claim this capability,
> even if some slave devices are unused (i.e. linked down, standby).
> 
> Signed-off-by: Jia Yu 
> Signed-off-by: Declan Doherty 
> ---
>  lib/librte_pmd_bond/rte_eth_bond_api.c | 18 +-
>  lib/librte_pmd_bond/rte_eth_bond_pmd.c |  5 +
>  lib/librte_pmd_bond/rte_eth_bond_private.h |  3 +++
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_api.c
> b/lib/librte_pmd_bond/rte_eth_bond_api.c
> index f146bda..5125b57 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_api.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_api.c
> @@ -238,6 +238,8 @@ rte_eth_bond_create(const char *name, uint8_t mode,
> uint8_t socket_id)
> 
>   internals->slave_count = 0;
>   internals->active_slave_count = 0;
> + internals->rx_offload_capa = 0;
> + internals->tx_offload_capa = 0;
> 
>   memset(internals->active_slaves, 0, sizeof(internals->active_slaves));
>   memset(internals->slaves, 0, sizeof(internals->slaves));
> @@ -265,6 +267,7 @@ __eth_bond_slave_add_lock_free(uint8_t
> bonded_port_id, uint8_t slave_port_id)
>   struct bond_dev_private *internals;
>   struct bond_dev_private *temp_internals;
>   struct rte_eth_link link_props;
> + struct rte_eth_dev_info dev_info;
> 
>   int i, j;
> 
> @@ -296,6 +299,9 @@ __eth_bond_slave_add_lock_free(uint8_t
> bonded_port_id, uint8_t slave_port_id)
>   /* Add slave details to bonded device */
>   slave_add(internals, slave_eth_dev);
> 
> + memset(_info, 0, sizeof(dev_info));
> + rte_eth_dev_info_get(slave_port_id, _info);
> +
>   if (internals->slave_count < 1) {
>   /* if MAC is not user defined then use MAC of first slave add to
>* bonded device */
> @@ -308,6 +314,11 @@ __eth_bond_slave_add_lock_free(uint8_t
> bonded_port_id, uint8_t slave_port_id)
> 
>   /* Make primary slave */
>   internals->primary_port = slave_port_id;
> +
> + /* Take the first dev's offload capabilities */
> + internals->rx_offload_capa = dev_info.rx_offload_capa;
> + internals->tx_offload_capa = dev_info.tx_offload_capa;
> +
>   } else {
>   /* Check slave link properties are supported if props are set,
>* all slaves must be the same */
> @@ -323,6 +334,8 @@ __eth_bond_slave_add_lock_free(uint8_t
> bonded_port_id, uint8_t slave_port_id)
>   link_properties_set(bonded_eth_dev,
>   &(slave_eth_dev->data->dev_link));
>   }
> + internals->rx_offload_capa &= dev_info.rx_offload_capa;
> + internals->tx_offload_capa &= dev_info.tx_offload_capa;
>   }
> 
>   internals->slave_count++;
> @@ -455,7 +468,10 @@ __eth_bond_slave_remove_lock_free(uint8_t
> bonded_port_id, uint8_t slave_port_id)
>   memset(rte_eth_devices[bonded_port_id].data-
> >mac_addrs, 0,
> 
>   sizeof(*(rte_eth_devices[bonded_port_id].data->mac_addrs)));
>   }
> -
> + if (internals->slave_count == 0) {
> + internals->rx_offload_capa = 0;
> + internals->tx_offload_capa = 0;
> + }
>   return 0;
>  }
> 
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> index cf2fbab..0d1a36b 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> @@ -788,6 +788,8 @@ static int bond_ethdev_configure(struct rte_eth_dev
> *dev);
>  static void
>  bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  {
> + struct bond_dev_private *internals = dev->data->dev_private;
> +
>   dev_info->driver_name = driver_name;
>   dev_info->max_mac_addrs = 1;
> 
> @@ -798,6 +800,9 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
> 
> 

[dpdk-dev] [PATCH v4 0/2] ADD mode 5(tlb) to link bonding pmd

2014-11-27 Thread Doherty, Declan
Sorry Thomas, this was my fault, I forgot that Daniel mentioned to me earlier 
today that he was going to 
submit a new revision of the patch set with updated patch descriptions, but no 
code changes, due to 
your comments relating to the link bonding mode 4. I will ack the v4 patchset 
when all patches for v4
are available.

Declan

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, November 27, 2014 4:42 PM
> To: Mrzyglod, DanielX T
> Cc: dev at dpdk.org; Doherty, Declan
> Subject: Re: [dpdk-dev] [PATCH v4 0/2] ADD mode 5(tlb) to link bonding pmd
> 
> 2014-11-27 16:33, Daniel Mrzyglod:
> > ADD mode 5(tlb) to link bonding pmd
> >
> > v4 change:
> > Change description of mode 5 in header.
> > Add description to cover letter.
> >
> > v3 change:
> > Rebase patch version to HEAD of orgin/master.
> > Unit tests moved to the separate patch v3 2/2.
> >
> > v2 change:
> > Add Unit Tests
> > Modification that updates obytes structure in virtualpmd driver.
> > change internals->slaves[i].last_obytes to have proper values.
> > Update codebase to Declan's patches.
> >
> > v1 change:
> > Add support for mode 5 (Transmit load balancing) into pmd driver
> >
> >
> > Daniel Mrzyglod (2):
> >   This patch add support of mode 5 to link bonding pmd
> >   Unit tests for Mode 5 of Bonding Transmit Load balancig.
> 
> Please help me to understand. Declan just acked the v3 and you are sending
> an incomplete v4 (patch 2 is missing).
> When a complete patchset will be sent, it will be applied.
> 
> --
> Thomas


[dpdk-dev] [PATCH v3 0/2] ADD mode 5(tlb) to link bonding pmd

2014-11-27 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Mrzyglod, DanielX T
> Sent: Wednesday, November 26, 2014 5:18 PM
> To: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/2] ADD mode 5(tlb) to link bonding pmd
> 
> v3 change:
> Rebase patch version to HEAD of orgin/master.
> Unit tests moved to the separate patch v3 2/2.
> 
> v2 change:
> Add Unit Tests
> Modification that updates obytes structure in virtualpmd driver.
> change internals->slaves[i].last_obytes to have proper values.
> Update codebase to Declan's patches.
> 
> v1 change:
> Add support for mode 5 (Transmit load balancing) into pmd driver
> 
> This mode provides an adaptive transmit load balancing.
> It dynamically changes the transmitting slave, according to the computed load.
> Statistics are collected in 100ms intervals and scheduled every 10ms.
> 
> > -Original Message-
> > From: Mrzyglod, DanielX T
> > Sent: Wednesday, November 26, 2014 6:13 PM
> > To: dev at dpdk.org
> > Cc: Mrzyglod, DanielX T
> > Subject: [PATCH v3 0/2] ADD mode 5(tlb) to link bonding pmd
> >
> > This mode provides an adaptive transmit load balancing.
> > It dynamically changes the transmitting slave, according to the computed 
> > load.
> > Statistics are collected in 100ms intervals and scheduled every 10ms.
> >
> > Daniel Mrzyglod (2):
> >   This patch add support of mode 5 to link bonding pmd
> >   Unit tests for Mode 5 of Bonding Transmit Load balancing.
> >
> >  app/test/test_link_bonding.c   | 499
> -
> >  app/test/virtual_pmd.c |   6 +-
> >  lib/librte_pmd_bond/rte_eth_bond.h |  11 +
> >  lib/librte_pmd_bond/rte_eth_bond_args.c|   1 +
> >  lib/librte_pmd_bond/rte_eth_bond_pmd.c | 160 -
> >  lib/librte_pmd_bond/rte_eth_bond_private.h |   2 +-
> >  6 files changed, 673 insertions(+), 6 deletions(-)
> >
> > --
> > 2.1.0

Acked by Declan Doherty 


[dpdk-dev] [PATCH v7 0/7] link bonding

2014-11-24 Thread Doherty, Declan
Hey, sorry about that, I submitted the patchset without the typo fix. I'll 
resubmit the patchset now.
What version of checkpatch are you using, I don't get any warning for the 
missing spaces for the
version of the script with kernel 3.15 on Fedora

Thanks
Declan


> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, November 24, 2014 3:36 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v7 0/7] link bonding
> 
> Hi Declan,
> 
> There are still 2 kind of errors reported by checkpatch:
> - 14 occurrences of this typo:
> WARNING:TYPO_SPELLING: 'interupt' may be misspelled - perhaps 'interrupt'?
> - 6 occurences of missing space:
> WARNING:MISSING_SPACE: break quoted strings at a space character
> 
> Example:
> #2016: FILE: lib/librte_pmd_bond/rte_eth_bond_pmd.c:1420:
> +   "LSC polling interval can be specified only 
> once for bonded"
> +   "device %s\n", name);
> You should put a space between "bonded" and "device".
> 
> --
> Thomas


[dpdk-dev] [PATCH v6 0/8] link bonding

2014-11-21 Thread Doherty, Declan
Hey Thomas, have you had a chance to have a look at this patchset, if you have 
any comments or issues can you let me know asap, as the patch sets for link 
bonding modes 4 & 5 are dependent on this set and I don't want to be blocking 
them from the next release candidate.

Thanks 
Declan

> -Original Message-
> From: De Lara Guarch, Pablo
> Sent: Friday, November 7, 2014 4:41 PM
> To: Doherty, Declan; dev at dpdk.org
> Cc: thomas.monjalon at 6wind.com
> Subject: RE: [PATCH v6 0/8] link bonding
> 
> 
> 
> > -Original Message-
> > From: Doherty, Declan
> > Sent: Friday, November 07, 2014 12:23 PM
> > To: dev at dpdk.org
> > Cc: De Lara Guarch, Pablo; thomas.monjalon at 6wind.com; Doherty, Declan
> > Subject: [PATCH v6 0/8] link bonding
> >
> > v6:
> > - Re-based to dpdk.org addressing associated issues for MBUF_REFCNT
> > - Added details to testpmd user guide for new command to set link status
> > polling interval.
> >
> > v5:
> > - Fix uninitialized variable in broadcast_tx_burst function which caused a
> >   build error in 32-bit build
> > - Address unit test issue which is exposed by new test in mode 4/5 patch 
> > sets
> >
> > v4:
> > - Rebased to account for changes in master.
> > - Fix for rte_eth_bond_slaves_get() introduced in v3 patch set
> > - Addressed issue around disabling/enabling link status polling around
> > adding/
> >   removing slaves devices.
> >
> > v3 :
> > - Typo fix for the bond free mbufs patch.
> > - Rebased to account for changes in the mbuf patches.
> > - Add support for slave devices which don't support link status interrupts
> > - Tidy up the link bonding unit test so that all tests use the new test 
> > macros.
> >
> > v2 :
> > Addresses issues with the logic around the handling of fail transmissions.
> > In this version all modes behave in a manner similar to a standard PMD,
> > returning the number of successfully transmitted mbufs and with the failing
> > mbufs at the end of bufs array for freeing / retransmission by the
> > application software
> >
> > v1:
> >
> > This patch set adds support for link status interrupt in the link bonding
> > pmd. It also contains some patches to tidy up the code structure and to
> > of the link bonding code and to fix bugs relating to transmission
> > failures in the under lying slave pmd which could lead to leaked mbufs.
> >
> >
> > Declan Doherty (8):
> >   bond: link status interrupt support
> >   bond: removing switch statement from rx burst method
> >   bond: fix naming inconsistency in tx_burst_round_robin
> >   bond: free mbufs if transmission fails in bonding tx_burst functions
> >   test app: adding support for generating variable sized packet
> >   testpmd: adding parameter to reconfig method to set socket_id when
> > adding new port to portlist
> >   bond: lsc polling support
> >   bond: unit test test macro refactor
> >
> >  app/test-pmd/cmdline.c  |   65 +-
> >  app/test-pmd/testpmd.c  |3 +-
> >  app/test-pmd/testpmd.h  |2 +-
> >  app/test/packet_burst_generator.c   |   25 +-
> >  app/test/packet_burst_generator.h   |6 +-
> >  app/test/test.h |7 +-
> >  app/test/test_link_bonding.c| 3347 
> > ++-
> >  app/test/virtual_pmd.c  |   96 +-
> >  app/test/virtual_pmd.h  |   53 +-
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |   19 +
> >  lib/librte_pmd_bond/rte_eth_bond.h  |   80 +
> >  lib/librte_pmd_bond/rte_eth_bond_api.c  |  319 ++-
> >  lib/librte_pmd_bond/rte_eth_bond_args.c |   28 +-
> >  lib/librte_pmd_bond/rte_eth_bond_pmd.c  |  553 +++--
> >  lib/librte_pmd_bond/rte_eth_bond_private.h  |   71 +-
> >  15 files changed, 2715 insertions(+), 1959 deletions(-)
> >
> > --
> > 1.7.12.2
> 
> Acked-by: Pablo de Lara 


[dpdk-dev] [PATCH] Bond: set {rx|tx}_offload_capa flags

2014-11-11 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jia Yu
> Sent: Friday, November 7, 2014 5:36 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] Bond: set {rx|tx}_offload_capa flags
> 
> Before the fix, bond device's offload capabilities are unset. This fix
> takes the minimum common set of slave devices' capabilities as bond
> device's capabilities. For simplicity, we ensure all slave devices
> to have a capability before bond device can claim this capability,
> even if some slave devices are unused (i.e. linked down, standby).
> 
> Signed-off-by: Jia Yu 
> ---
>  lib/librte_pmd_bond/rte_eth_bond_api.c | 16 
> .
> --
> 1.9.1

Acked-by: Declan Doherty 




[dpdk-dev] [PATCH] ixgbe: Fix compilation issue in vpmd

2014-10-21 Thread Doherty, Declan
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, October 21, 2014 9:36 AM
> To: Ouyang, Changchun; De Lara Guarch, Pablo
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix compilation issue in vpmd
> 
> 2014-10-21 08:28, Ouyang, Changchun:
> > From: De Lara Guarch, Pablo
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> > > > 2014-10-21 14:59, Ouyang Changchun:
> > > > > Fix the compilation issue in vector PMD when macro RTE_MBUF_REFCNT
> > > > > is disabled.
> > > > >
> > > > > Signed-off-by: Changchun Ouyang 
> > > >
> > > > Acked-by: Thomas Monjalon 
> > > >
> > > > Applied
> > >
> > > I was checking this patch right now, and I come across a second 
> > > compilation
> > > issue,  because rte_mbuf_refcnt_update and rte_pktmbuf_attach are not
> > > declared, and Bond PMD and IP fragmentation libraries use those functions.
> > >
> > > I guess that it is late to NACK this :P, but we require a second patch to 
> > > fix
> > > completely this issue.
> >
> > As it fixes the compilation issue in vpmd, so no reason to NACK it,  :-)
> 
> Exact
> 
> > In my config, both BOND and IP fragment is disabled. So I don't come across 
> > your
> issues.
> > Yes, agree with you, we need another patch to fix compilation issue in other
> both places.
> 
> Yes, I'm aware of these limitations.
> Please, first explain why mbuf refcnt is needed for these features.
> Then we have 2 options: remove the dependency or add more ifdefs.
> 
> Thanks
> --
> Thomas

Thomas, 
for link bonding the refcnt is used in broadcast mode to allow the same
mbuf to be transmitted on multiple slaves. By increasing the refcnt, we 
can safely transmit the mbuf on multiple slaves without the danger of a 
mbuf being freed by one slave while in use by another. I can provide a patch to
disable the broadcast mode of operation if the bonding pmd is built with the 
macro 
RTE_MBUF_REFCNT disabled. I don't think there is any other option other than 
than to completely disable the library if the refcnt parameter isn't available
as we don't have access to the transmit mempool to allocate new mbufs to
make copies of the original. 

Declan


[dpdk-dev] [PATCH v2] ADD mode 5(tlb) to link bonding pmd

2014-10-07 Thread Doherty, Declan


> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Daniel Mrzyglod
> Sent: Friday, September 26, 2014 4:41 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v2] ADD mode 5(tlb) to link bonding pmd
> 
> 
> Signed-off-by: Daniel Mrzyglod 
> ---
>  app/test/test_link_bonding.c   |  501 
> +++-
>  app/test/virtual_pmd.c |6 +-
>  app/test/virtual_pmd.h |7 +
>  lib/librte_pmd_bond/rte_eth_bond.h |   23 ++
>  lib/librte_pmd_bond/rte_eth_bond_args.c|1 +
>  lib/librte_pmd_bond/rte_eth_bond_pmd.c |  161 -
>  lib/librte_pmd_bond/rte_eth_bond_private.h |3 +-
>  7 files changed, 696 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
> index c4fcaf7..77f791f 100644
> --- a/app/test/test_link_bonding.c
> +++ b/app/test/test_link_bonding.c
> @@ -41,7 +41,7 @@
>  #include 
>  #include 
>  #include 
> -
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -3845,6 +3845,500 @@ testsuite_teardown(void)
>   return remove_slaves_and_stop_bonded_device();
>  }
> 
> +#define NINETY_PERCENT_NUMERAL 90
> +#define ONE_HUNDRED_PERCENT_DENOMINATOR 100
> +#define ONE_HUNDRED_PERCENT_AND_TEN_NUMERAL 110
> +static int
> +test_tlb_tx_burst(void)
> +{
> + int i, burst_size, nb_tx;
> + uint64_t nb_tx2 = 0;
> + struct rte_mbuf *pkt_burst[MAX_PKT_BURST];
> + struct rte_eth_stats port_stats[32];
> + uint64_t sum_ports_opackets = 0, all_bond_opackets = 0,
> all_bond_obytes = 0;
> + uint16_t pktlen;
> +
> + TEST_ASSERT_SUCCESS(initialize_bonded_device_with_slaves
> +
>   (BONDING_MODE_ADAPTIVE_TRANSMIT_LOAD_BALANCING, 1, 3, 1),
> + "Failed to initialise bonded device");
> +
> + burst_size = 20 * test_params->bonded_slave_count;
> +
> + TEST_ASSERT(burst_size < MAX_PKT_BURST,
> + "Burst size specified is greater than supported.\n");
> +
> +
> + /* Generate 40 test bursts in 2s of packets to transmit  */
> + for (i = 0; i < 40; i++) {
> + /*test two types of mac src own(bonding) and others */
> + if (i % 2 == 0) {
> + initialize_eth_header(test_params->pkt_eth_hdr,
> + (struct ether_addr *)src_mac, (struct
> ether_addr *)dst_mac_0, 0, 0);
> + } else {
> + initialize_eth_header(test_params->pkt_eth_hdr,
> + (struct ether_addr *)test_params-
> >default_slave_mac,
> + (struct ether_addr *)dst_mac_0, 0, 0);
> + }
> + pktlen = initialize_udp_header(test_params->pkt_udp_hdr,
> src_port,
> + dst_port_0, 16);
> + pktlen = initialize_ipv4_header(test_params->pkt_ipv4_hdr,
> src_addr,
> + dst_addr_0, pktlen);
> + generate_packet_burst(test_params->mbuf_pool, pkt_burst,
> + test_params->pkt_eth_hdr, 0, test_params-
> >pkt_ipv4_hdr,
> + 1, test_params->pkt_udp_hdr, burst_size, 60, 1);
> + /* Send burst on bonded port */
> + nb_tx = rte_eth_tx_burst(test_params->bonded_port_id, 0,
> pkt_burst,
> + burst_size);
> + nb_tx2 += nb_tx;
> +
> + TEST_ASSERT_EQUAL(nb_tx, burst_size,
> + "number of packet not equal burst size");
> +
> + rte_delay_us(5);
> + }
> +
> +
> + /* Verify bonded port tx stats */
> + rte_eth_stats_get(test_params->bonded_port_id, _stats[0]);
> +
> + all_bond_opackets = port_stats[0].opackets;
> + all_bond_obytes = port_stats[0].obytes;
> +
> + TEST_ASSERT_EQUAL(port_stats[0].opackets, (uint64_t)nb_tx2,
> + "Bonded Port (%d) opackets value (%u) not as expected
> (%d)\n",
> + test_params->bonded_port_id, (unsigned
> int)port_stats[0].opackets,
> + burst_size);
> +
> +
> + /* Verify slave ports tx stats */
> + for (i = 0; i < test_params->bonded_slave_count; i++) {
> + rte_eth_stats_get(test_params->slave_port_ids[i], 
> _stats[i]);
> + sum_ports_opackets += port_stats[i].opackets;
> + }
> +
> + TEST_ASSERT_EQUAL(sum_ports_opackets, (uint64_t)all_bond_opackets,
> + "Total packets sent by slaves is not equalto packets 
> sent by
> bond interface");
> +
> + for (i = 0; i < test_params->bonded_slave_count; i++) {
> + printf("port stats:%"PRIu64"\n", port_stats[i].opackets);
> + /* distribution of packets on each slave within +/- 10% of the
> expected value. */
> + TEST_ASSERT(port_stats[i].obytes >=
> ((all_bond_obytes*NINETY_PERCENT_NUMERAL)/
> + (test_params-
> 

[dpdk-dev] [PATCH v2 4/6] bond: free mbufs if transmission fails in bonding tx_burst functions

2014-09-02 Thread Doherty, Declan
> -Original Message-
> From: Doherty, Declan
> Sent: Monday, September 1, 2014 9:31 AM
> To: dev at dpdk.org
> Cc: thomas.monjalon at 6wind.com; rsanford at akamai.com; Doherty, Declan
> Subject: [PATCH v2 4/6] bond: free mbufs if transmission fails in bonding 
> tx_burst
> functions
> 
> Fixing a number of corner cases that if transmission failed on slave devices 
> then
> this
> could lead to leaked mbufs
> 
> V2 addresses behaviouraly issues in the first version and packets are no 
> longer
> freed in
> the bonding layer, except in the case of broadcast mode where in failures 
> happen
> on
> more than a single slave then mbufs will be freed in all slaves except the one
> where
> the least errors occured. Also contains new unit tests to test the 
> transmission
>  failure case in slaves for
> round-robin, balance, and broadcast modes.
> 
> 
> Signed-off-by: Declan Doherty 
> ---
.
> --
> 1.7.0.7

I've just noticed that I inserted an extra character into the patch when I was 
annotating this
patch which will cause this a fail compilation.

> + TEST_BCAST_SLAVE_TX_FAIL_BURST_SIZE -s
> + TEST_BCAST_SLAVE_TX_FAIL_MIN_PACKETS_COUNT);

should have been 
> + TEST_BCAST_SLAVE_TX_FAIL_BURST_SIZE -
> + TEST_BCAST_SLAVE_TX_FAIL_MIN_PACKETS_COUNT);

Thomas, do you want me to submit a V3 of this?

Regards
Declan


[dpdk-dev] [PATCH v11 0/5] link bonding library

2014-06-30 Thread Doherty, Declan
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, June 30, 2014 10:21 AM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v11 0/5] link bonding library
> 
> Hi Declan,
> 
> 2014-06-29 18:49, Declan Doherty:
> > This patchset contains the name change fix in the link bonding library
> > makefile. I have also put a change into the unit test application makefile
> > so that the link bonding tests are not built if the library is also not
> > being built. It doesn't make sense to split the bonding libraries APIs into
> > a seperate library, as they are directly coupled to with the bonding
> > library implmentation
> 
> You fixed optional building but there is still an issue when building shared
> library.
> As you don't want to split the library to separate the PMD plugin from the
> specific API, I suggest to not consider it as a plugin (unlike other PMDs).
> 
> I've sent some patches about these 2 topics.
> If you agree, I'll apply them, merged with your serie.
> 
> Thanks
> --
> Thomas

Hi Thomas,
I'm happy for you to apply the makefile changes you have suggested and not 
consider the bonding PMD as a plugin, I  think this makes the most sense
considering it is a purely software library. 

Thanks
Declan


[dpdk-dev] [PATCH v7 6/6] Link Bonding Library doxygen additions

2014-06-25 Thread Doherty, Declan
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, June 25, 2014 2:44 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v7 6/6] Link Bonding Library doxygen additions
> 
> Hi Declan,
> 
> Since bonding library is a PMD, there should have no need of integrate it
> in the API documentation.
> I hadn't time to review it yet, so I would miss something.
> Do you mean it can be used directly by the application with a specific API?
> In this case, you could explain it in the commit log.
> 

Hi Thomas, the bonding library has a  API which supports creation and 
management of bonded devices directly from within applications, but more 
importantly provides the APIs required to dynamically add and remove slaves 
from a bonded device, this functionality will be required to support live 
migration of VMs using bonded ports in a future releases.


[dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library (librte_eal/librte_ether link bonding support changes)

2014-06-16 Thread Doherty, Declan
> -Original Message-
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Friday, June 13, 2014 8:38 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library
> (librte_eal/librte_ether link bonding support changes)
> 
> On Fri, Jun 13, 2014 at 06:34:04PM +, Doherty, Declan wrote:
> > > -Original Message-
> > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > Sent: Friday, June 13, 2014 5:08 PM
> > > To: Doherty, Declan
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library
> > > (librte_eal/librte_ether link bonding support changes)
> > >
> > > On Fri, Jun 13, 2014 at 03:41:59PM +0100, Declan Doherty wrote:
> > > > Updating functionality in EAL to support adding link bonding
> > > > devices via ?vdev option. Link bonding devices will be
> > > > initialized after all physical devices have been probed and
> > > > initialized.
> > > >
> > > > Signed-off-by: Declan Doherty 
> > > > ---
> > > >  lib/librte_eal/common/eal_common_dev.c  | 66
> > > +++--
> > > >  lib/librte_eal/common/eal_common_pci.c  |  6 +++
> > > >  lib/librte_eal/common/include/eal_private.h |  7 +++
> > > >  lib/librte_eal/common/include/rte_dev.h |  1 +
> > > >  lib/librte_ether/rte_ethdev.c   | 34 +--
> > > >  lib/librte_ether/rte_ethdev.h   |  7 ++-
> > > >  lib/librte_pmd_pcap/rte_eth_pcap.c  | 22 +-
> > > >  lib/librte_pmd_ring/rte_eth_ring.c  | 32 +++---
> > > >  lib/librte_pmd_ring/rte_eth_ring.h  |  3 +-
> > > >  lib/librte_pmd_xenvirt/rte_eth_xenvirt.c|  2 +-
> > > >  10 files changed, 144 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/lib/librte_eal/common/eal_common_dev.c
> > > b/lib/librte_eal/common/eal_common_dev.c
> > > > index eae5656..b50c908 100644
> > > > --- a/lib/librte_eal/common/eal_common_dev.c
> > > > +++ b/lib/librte_eal/common/eal_common_dev.c
> > > > @@ -75,14 +75,28 @@ rte_eal_dev_init(void)
> > > >
> > > > /* call the init function for each virtual device */
> > > > TAILQ_FOREACH(devargs, _list, next) {
> > > > +   uint8_t bdev = 0;
> > > >
> > > > if (devargs->type != RTE_DEVTYPE_VIRTUAL)
> > > > continue;
> > > >
> > > > TAILQ_FOREACH(driver, _driver_list, next) {
> > > > -   if (driver->type != PMD_VDEV)
> > > > +   /* RTE_DEVTYPE_VIRTUAL can only be a virtual or 
> > > > bonded
> > > device*/
> > > > +   if (driver->type != PMD_VDEV && driver->type !=
> > > PMD_BDEV)
> > > > continue;
> > > >
> > > > +   /*
> > > > +* Bonded devices are not initialize here, we 
> > > > do it later in
> > > > +* rte_eal_bonded_dev_init() after all physical 
> > > > devices
> > > have been
> > > > +* probed and initialized
> > > > +*/
> > > > +   if (driver->type == PMD_BDEV &&
> > > > +   !strncmp(driver->name, devargs-
> > > >virtual.drv_name,
> > > > +   
> > > > strlen(driver->name))) {
> > > > +   bdev = 1;
> > > > +   break;
> > > > +   }
> > > > +
> > > I really don't think you need to add a new device type for bonded devs.  
> > > Its got
> > > no specific hardware that it drives, and you configure it with a --vdev
> command,
> > > so treat it as one here.  I understand that you need to pass additional
> > > information about slaves to a bonded device, which is fine, but you can 
> > > do that
> > > with kvargs pretty easily, at which point its just another vdev. The only 
> > > other
> > > requirement is that you initilize the bonded vdev after the slave vdevs 
> > &

[dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library (librte_eal/librte_ether link bonding support changes)

2014-06-16 Thread Doherty, Declan
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, June 13, 2014 10:59 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library
> (librte_eal/librte_ether link bonding support changes)
> 
> On Fri, 13 Jun 2014 15:41:59 +0100
> Declan Doherty  wrote:
> 
> > Updating functionality in EAL to support adding link bonding
> > devices via ???vdev option. Link bonding devices will be
> > initialized after all physical devices have been probed and
> > initialized.
> >
> > Signed-off-by: Declan Doherty 
> 
> The DPDK is used by both static (demo style) applications and dynamic
> applications that need to be able to modify parameters while running.
> The tendency of the code has been to have values that can only be set
> at initialization, and this kind of inflexibility is hard to fix.
> 
> The whole dev args style of configuration is a poor design for those
> reasons.  How do you support creating additional bond devices at runtime?
> 
> Remember what works for a demo doesn't make a good API for real world.
> 
> 

Hi Stephen,

I totally agree but all of the dev argument parsing / static initialization 
here is based
on the rte_eth_bond.h public API. Actually testpmd has full support for dynamic 
initialization and management of bonding devices in run-time through the 
interactive command line options which pretty much have options corresponding
to all of the public link bonding APIs. 

It is my intent that this would main interface that this API would be used by 
user 
applications and that the static configuration options using --vdev interface 
would
only be used if a user wanted to use a bonded device in a existing application 
without having to make any changes to their code or add any explicit reference
to the link bonding library.


[dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library (librte_eal/librte_ether link bonding support changes)

2014-06-13 Thread Doherty, Declan
> -Original Message-
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Friday, June 13, 2014 5:08 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/5] Link Bonding PMD Library
> (librte_eal/librte_ether link bonding support changes)
> 
> On Fri, Jun 13, 2014 at 03:41:59PM +0100, Declan Doherty wrote:
> > Updating functionality in EAL to support adding link bonding
> > devices via ?vdev option. Link bonding devices will be
> > initialized after all physical devices have been probed and
> > initialized.
> >
> > Signed-off-by: Declan Doherty 
> > ---
> >  lib/librte_eal/common/eal_common_dev.c  | 66
> +++--
> >  lib/librte_eal/common/eal_common_pci.c  |  6 +++
> >  lib/librte_eal/common/include/eal_private.h |  7 +++
> >  lib/librte_eal/common/include/rte_dev.h |  1 +
> >  lib/librte_ether/rte_ethdev.c   | 34 +--
> >  lib/librte_ether/rte_ethdev.h   |  7 ++-
> >  lib/librte_pmd_pcap/rte_eth_pcap.c  | 22 +-
> >  lib/librte_pmd_ring/rte_eth_ring.c  | 32 +++---
> >  lib/librte_pmd_ring/rte_eth_ring.h  |  3 +-
> >  lib/librte_pmd_xenvirt/rte_eth_xenvirt.c|  2 +-
> >  10 files changed, 144 insertions(+), 36 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_dev.c
> b/lib/librte_eal/common/eal_common_dev.c
> > index eae5656..b50c908 100644
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -75,14 +75,28 @@ rte_eal_dev_init(void)
> >
> > /* call the init function for each virtual device */
> > TAILQ_FOREACH(devargs, _list, next) {
> > +   uint8_t bdev = 0;
> >
> > if (devargs->type != RTE_DEVTYPE_VIRTUAL)
> > continue;
> >
> > TAILQ_FOREACH(driver, _driver_list, next) {
> > -   if (driver->type != PMD_VDEV)
> > +   /* RTE_DEVTYPE_VIRTUAL can only be a virtual or bonded
> device*/
> > +   if (driver->type != PMD_VDEV && driver->type !=
> PMD_BDEV)
> > continue;
> >
> > +   /*
> > +* Bonded devices are not initialize here, we do it 
> > later in
> > +* rte_eal_bonded_dev_init() after all physical devices
> have been
> > +* probed and initialized
> > +*/
> > +   if (driver->type == PMD_BDEV &&
> > +   !strncmp(driver->name, devargs-
> >virtual.drv_name,
> > +   strlen(driver->name))) {
> > +   bdev = 1;
> > +   break;
> > +   }
> > +
> I really don't think you need to add a new device type for bonded devs.  Its 
> got
> no specific hardware that it drives, and you configure it with a --vdev 
> command,
> so treat it as one here.  I understand that you need to pass additional
> information about slaves to a bonded device, which is fine, but you can do 
> that
> with kvargs pretty easily, at which point its just another vdev. The only 
> other
> requirement is that you initilize the bonded vdev after the slave vdevs have
> been created, which you can do by any of several methods (a priority field to
> indicate that bonded drivers should be initilized last/later, a deferral 
> return
> code from the init routine, or by dead reckoning via the careful construction 
> of
> the application command line (placed the bonded --vdev option last on the
> command line argument list at run time).
> 
It was my initial intent to do as you have describe above, but the physical 
devices
cause a real issue here, physical devices don't call through to 
rte_eth_dev_allocate until 
during rte_eal_pci_probe call, so it's not possible to initialize the bonded 
device from
within rte_eal_dev_init as the physical devices have not been fully initialized 
at this
point, as a port_id has not been allocated and can't be added as bonding 
slaves. I don't 
see away around this without changing the EAL API, which I've tried to avoid 
with this solution.

Ordering isn't an issue, and can easily be solved if the above problem didn't 
exist, and although
a new device type isn't technically required, I think it's a cleaner solution 
than doing
string comparisons.

> 
> > /* search a driver prefix in virtual device name */
> > if (!strncmp

[dpdk-dev] [PATCH v2 0/4] Link Bonding Library

2014-06-13 Thread Doherty, Declan
> -Original Message-
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Friday, June 6, 2014 3:54 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 0/4] Link Bonding Library
> 
> On Fri, Jun 06, 2014 at 08:23:31AM +, Doherty, Declan wrote:
> > > -Original Message-
> > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > Sent: Thursday, May 29, 2014 12:34 PM
> > > To: Doherty, Declan
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 0/4] Link Bonding Library
> > >
> > > On Thu, May 29, 2014 at 10:33:00AM +, Doherty, Declan wrote:
> > > > -Original Message-
> > > > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > > > Sent: Wednesday, May 28, 2014 6:49 PM
> > > > > To: Doherty, Declan
> > > > > Cc: dev at dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH 0/4] Link Bonding Library
> > > > >
> > > > > On Wed, May 28, 2014 at 04:32:00PM +0100, declan.doherty at intel.com
> > > wrote:
> > > > > > From: Declan Doherty 
> > > > > >
> > > > > > Initial release of Link Bonding Library (lib/librte_bond) with
> > > > > > support for bonding modes :
> > > > > >  0 - Round Robin
> > > > > >  1 - Active Backup
> > > > > >  2 - Balance l2 / l23 / l34
> > > > > >  3 - Broadcast
> > > > > >
> > > > > Why make this a separate library?  That requires exposure of yet
> > > > > another
> > > API to applications.  Instead, why > not write a PMD that can enslave
> > > other PMD's and treat them all as a single interface?  That way this
> > > all >  > works with the existing API.
> > > > >
> > > > > Neil
> > > >
> > > > Hi Neil,
> > > > the link bonding device is essentially a software PMD, and as such
> > > > supports
> > > all the standard PMD APIs, the only new APIs which the link bonding
> > > library introduces  are for the control operations of the bonded
> > > device which are currently unsupported by the standard PMD API.
> > > Operations such as creating, adding/removing slaves, and configuring
> > > the modes of operation of the device have no analogous APIs in the
> > > current PMD API and required new ones to be created .
> > >
> > > Thats really only true in spirit, in the sense that this library
> > > transmits and receives frames like a PMD does.  In practice it doesn't
> > > work and isn't architected the same way.  You don't register the
> > > driver using the same method as the other PMDs, which means that using
> > > --vdev on the command line wont work for this type of device.  It also
> > > implies that applications have to be made specifically aware of the
> > > fact that they are using a bonded interface (i.e. they need to call
> > > the bonding setup routines to create the bond).  I would
> > > recommend:
> > >
> > > 1) Register the pmd using the PMD_DRIVER_REGISTER macro, like other
> > > PMD's
> > > 2) Use the kvargs library to support configuration via the --vdev
> > > command line option, so bonds can be created administratively, rather
> > > than just programatically
> > > 3) Separate the command api from the PMD functionality into separate
> > > libraries (use control mbufs to communicate configuration changes to
> > > the pmd).  This will allow users to dynamically load the pmd
> > > functionality (without compile or run time linking requirements), and
> > > then optionally use the programatic interface (or not if they want to
> > > save memory)
> > >
> > > Regards
> > > Neil
> > > -Original Message-
> > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > Sent: Thursday, June 5, 2014 12:04 PM
> > > To: Doherty, Declan
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 0/4] Link Bonding Library
> > >
> > > On Wed, Jun 04, 2014 at 04:18:01PM +0100, declan.doherty at intel.com
> > > wrote:
> > > > From: Declan Doherty 
> > > >
> > > > v2 patch additions,
> > > > fix for tx burst broadcast, incrementing the reference count on each
> > > > mbuf by the number of slaves - 1 add/remove slave behavior chnange
> > > > to f

[dpdk-dev] [PATCH v2 0/4] Link Bonding Library

2014-06-13 Thread Doherty, Declan
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, June 11, 2014 5:33 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 0/4] Link Bonding Library
> 
> Hi Declan,
> 
> Do you think you can send another version of your serie soon?
> Some comments need to be addressed.
> 
> 2 formatting comments:
> - I saw some strange alignments of comments
> - you should send your patches as children of the cover letter
> 
> Thanks
> --
> Thomas

Hi Thomas, 
I've just submitted v3 of the patch, which should address the majority of the 
comments and any formatting issues.

Regards
Declan


[dpdk-dev] [PATCH v2 1/4] Link Bonding Library

2014-06-13 Thread Doherty, Declan
> -Original Message-
> From: Eric Kinzie [mailto:ehkinzie at gmail.com]
> Sent: Monday, June 9, 2014 10:11 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] Link Bonding Library
> 
> On Wed Jun 04 16:18:53 +0100 2014, Declan Doherty wrote:
> > - Broadcast TX burst broadcast bug fix
> > - Add/remove slave behavior fix
> > - Checkpatch fixes
> 
> Declan, would you consider the following change to rte_bond.c?  The two
> header files from librte_cmdline don't seem to be necessary.
> 
>  Eric
> 
> 
> --- a/lib/librte_bond/rte_bond.c
> +++ b/lib/librte_bond/rte_bond.c
> @@ -44,9 +44,6 @@
>  #include 
>  #include 
> 
> -#include 
> -#include 
> -
>  #include "rte_bond.h"
> 
>  static const char *driver_name = "Link Bonding PMD";

Hi Eric, 
I've cleaned up the includes in v3 of the patch, but the those headers are now 
actually required for the kvarg parsing.

Regards
Declan


[dpdk-dev] [PATCH v2 1/4] Link Bonding Library

2014-06-06 Thread Doherty, Declan
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Thursday, June 5, 2014 4:16 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] Link Bonding Library
> 
> On Wed,  4 Jun 2014 16:18:02 +0100
> declan.doherty at intel.com wrote:
> 
> > From: Declan Doherty 
> >
> > - Broadcast TX burst broadcast bug fix
> > - Add/remove slave behavior fix
> > - Checkpatch fixes
> >
> > Signed-off-by: Declan Doherty 
> 
> There are some pretty weak hash functions in there.
> 
> What about using:
> .

Hi Stephen,

I suppose the naming of these as hash functions is probably misleading, the aim 
is not to create a unique hash for each flow with minimal collisions but to  
help balance traffic flows across the slaves of the bonded device, and as we 
are doing a modulus calculation based on the number of slaves attached, which I 
believe will typically be in the 2-4 range, I'm not sure that a stronger 
hashing function will give a better balance of flows across the slaves. For 
example in the Linux kernel implementation of the link bonding driver the mac 
hashing only uses the last byte of src & dst mac addresses to calculate it's 
eth hash.

/* L2 hash helper */
static inline u32 bond_eth_hash(struct sk_buff *skb)
{
struct ethhdr *data = (struct ethhdr *)skb->data;

if (skb_headlen(skb) >= offsetof(struct ethhdr, h_proto))
return data->h_dest[5] ^ data->h_source[5];

return 0;
}

I'll investigate and see if a stronger hashing function will result in a better 
balancing of flows.
--
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

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.




[dpdk-dev] [PATCH v2 0/4] Link Bonding Library

2014-06-06 Thread Doherty, Declan
> -Original Message-
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Thursday, May 29, 2014 12:34 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/4] Link Bonding Library
> 
> On Thu, May 29, 2014 at 10:33:00AM +, Doherty, Declan wrote:
> > -Original Message-
> > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > Sent: Wednesday, May 28, 2014 6:49 PM
> > > To: Doherty, Declan
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 0/4] Link Bonding Library
> > >
> > > On Wed, May 28, 2014 at 04:32:00PM +0100, declan.doherty at intel.com
> wrote:
> > > > From: Declan Doherty 
> > > >
> > > > Initial release of Link Bonding Library (lib/librte_bond) with 
> > > > support for bonding modes :
> > > >  0 - Round Robin
> > > >  1 - Active Backup
> > > >  2 - Balance l2 / l23 / l34
> > > >  3 - Broadcast
> > > >
> > > Why make this a separate library?  That requires exposure of yet 
> > > another
> API to applications.  Instead, why > not write a PMD that can enslave 
> other PMD's and treat them all as a single interface?  That way this 
> all >  > works with the existing API.
> > >
> > > Neil
> >
> > Hi Neil,
> > the link bonding device is essentially a software PMD, and as such 
> > supports
> all the standard PMD APIs, the only new APIs which the link bonding 
> library introduces  are for the control operations of the bonded 
> device which are currently unsupported by the standard PMD API.
> Operations such as creating, adding/removing slaves, and configuring 
> the modes of operation of the device have no analogous APIs in the 
> current PMD API and required new ones to be created .
> 
> Thats really only true in spirit, in the sense that this library 
> transmits and receives frames like a PMD does.  In practice it doesn't 
> work and isn't architected the same way.  You don't register the 
> driver using the same method as the other PMDs, which means that using 
> --vdev on the command line wont work for this type of device.  It also 
> implies that applications have to be made specifically aware of the 
> fact that they are using a bonded interface (i.e. they need to call 
> the bonding setup routines to create the bond).  I would
> recommend:
> 
> 1) Register the pmd using the PMD_DRIVER_REGISTER macro, like other 
> PMD's
> 2) Use the kvargs library to support configuration via the --vdev 
> command line option, so bonds can be created administratively, rather 
> than just programatically
> 3) Separate the command api from the PMD functionality into separate 
> libraries (use control mbufs to communicate configuration changes to 
> the pmd).  This will allow users to dynamically load the pmd 
> functionality (without compile or run time linking requirements), and 
> then optionally use the programatic interface (or not if they want to 
> save memory)
> 
> Regards
> Neil
> -Original Message-
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Thursday, June 5, 2014 12:04 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 0/4] Link Bonding Library
> 
> On Wed, Jun 04, 2014 at 04:18:01PM +0100, declan.doherty at intel.com
> wrote:
> > From: Declan Doherty 
> >
> > v2 patch additions,
> > fix for tx burst broadcast, incrementing the reference count on each 
> > mbuf by the number of slaves - 1 add/remove slave behavior chnange 
> > to fix primary slave port assignment patchcheck code fixes
> >
> >
> >
> This doesn't address any of the comments I made previously.
> Neil


Hi Neil, 

sorry for not replying regarding this earlier, in relation to your 
recommendations 1 and 2, I'm currently working on the implementation of both 
and should have an additional patch within the next couple of days to add this 
functionality.

Regarding your third recommendation, I have no problem splitting the library 
into separate API and PMD libraries although this does seem to break convention 
with the other non hw based pmd's, such as pmd_pcap, so would prefer not to go 
down that route. Also, I don't see the ability to dynamically load the pmd as 
justification for the use of a control mbufs interface, this would require that 
either additional queues be created to handle control messages, or that the 
data TX queues have the ability to identify and process control mbufs, and 
either of these options will have a performance hit, for the core handling the 
separate control messages or directly to the tx burst performance. I am not 
curre

[dpdk-dev] [PATCH v2 0/4] Link Bonding Library

2014-06-04 Thread Doherty, Declan
Sorry for the double submission of the first 3 parts of this patch set. It was 
meant to be a test directed to my own email account.
Regards
Declan

> -Original Message-
> From: Doherty, Declan
> Sent: Wednesday, June 4, 2014 4:19 PM
> To: dev at dpdk.org; dev at dpdk.org
> Cc: Doherty, Declan
> Subject: [PATCH v2 0/4] Link Bonding Library
> 
> From: Declan Doherty 
> 
> v2 patch additions,
> fix for tx burst broadcast, incrementing the reference count on each mbuf by
> the number of slaves - 1
> add/remove slave behavior chnange to fix primary slave port assignment
> patchcheck code fixes
> 
> Initial release of Link Bonding Library (lib/librte_bond) with support for
> bonding modes :
>  0 - Round Robin
>  1 - Active Backup
>  2 - Balance l2 / l23 / l34
>  3 - Broadcast
> 
> patches split:
>  1 - library + makefile changes
>  2 - Unit test suite, including code to generate packet bursts for
> testing rx and tx functionality of bonded device and a
> virtual/stubbed out ethdev for use as slave ethdev in testing
>  3 - Link bonding integration into testpmd, including :
>  - Includes the ability to  create new bonded devices.
>  - Add /remove bonding slave devices.
>  - Interogate bonded device stats/configuration
>  - Change bonding modes and select balance transmit polices
>  4 - Add Link Bonding Library to Doxygen
> 
> 
>  app/test-pmd/cmdline.c|  570 ++
>  app/test-pmd/config.c |4 +-
>  app/test-pmd/parameters.c |4 +-
>  app/test-pmd/testpmd.c|   37 +-
>  app/test-pmd/testpmd.h|2 +
>  app/test/Makefile |3 +
>  app/test/commands.c   |3 +
>  app/test/packet_burst_generator.c |  289 +++
>  app/test/packet_burst_generator.h |   78 +
>  app/test/test.h   |1 +
>  app/test/test_link_bonding.c  | 3943
> +
>  app/test/virtual_pmd.c|  574 ++
>  app/test/virtual_pmd.h|   74 +
>  config/common_bsdapp  |5 +
>  config/common_linuxapp|5 +
>  doc/doxy-api-index.md |1 +
>  doc/doxy-api.conf |1 +
>  lib/Makefile  |1 +
>  lib/librte_bond/Makefile  |   28 +
>  lib/librte_bond/rte_bond.c| 1682 
>  lib/librte_bond/rte_bond.h|  228 +++
>  mk/rte.app.mk |5 +
>  22 files changed, 7531 insertions(+), 7 deletions(-)
>  create mode 100644 app/test/packet_burst_generator.c
>  create mode 100644 app/test/packet_burst_generator.h
>  create mode 100644 app/test/test_link_bonding.c
>  create mode 100644 app/test/virtual_pmd.c
>  create mode 100644 app/test/virtual_pmd.h
>  create mode 100644 lib/librte_bond/Makefile
>  create mode 100644 lib/librte_bond/rte_bond.c
>  create mode 100644 lib/librte_bond/rte_bond.h
> 
> --
> 1.8.5.3

--
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

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.




[dpdk-dev] [PATCH 1/4] Link Bonding Library

2014-05-29 Thread Doherty, Declan


> -Original Message-
> From: Shaw, Jeffrey B
> Sent: Wednesday, May 28, 2014 5:55 PM
> To: Doherty, Declan; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/4] Link Bonding Library
> 
> Hi Declan,
> I'm worried about one thing in "bond_ethdev_tx_broadcast()" related to
> freeing of the broadcasted packets.
> 
> > +static uint16_t
> > +bond_ethdev_tx_broadcast(void *queue, struct rte_mbuf **bufs,
> > +uint16_t nb_pkts) {
> > +   struct bond_dev_private *internals;
> > +   struct bond_tx_queue *bd_tx_q;
> > +
> > +   uint8_t num_of_slaves;
> > +   uint8_t slaves[RTE_MAX_ETHPORTS];
> > +
> > +   uint16_t num_tx_total = 0;
> > +
> > +   int i;
> > +
> > +   bd_tx_q = (struct bond_tx_queue *)queue;
> > +   internals = bd_tx_q->dev_private;
> > +
> > +   /* Copy slave list to protect against slave up/down changes during tx
> > +* bursting */
> > +   num_of_slaves = internals->active_slave_count;
> > +   memcpy(slaves, internals->active_slaves,
> > +   sizeof(internals->active_slaves[0]) * num_of_slaves);
> > +
> > +   if (num_of_slaves < 1)
> > +   return 0;
> > +
> > +
> > +   for (i = 0; i < num_of_slaves; i++) {
> > +   num_tx_total += rte_eth_tx_burst(slaves[i], bd_tx_q-
> >queue_id,
> > +   bufs, nb_pkts);
> > +   }
> > +
> > +   return num_tx_total;
> > +}
> > +
> 
> Transmitting the same buffers on all slaves will cause problems when the PMD
> frees the mbufs for previously transmitted packets.
> So if you broadcast 1 packet on 4 slaves, each of the 4 slaves will 
> (eventually)
> have to free the same mbuf.  Without updating the refcnt, you will end up
> incorrectly freeing the same mbuf 3 extra times.
> 
> Your test application does not catch this case since the max packets that are
> tested is 320, which is less than the size of the Tx descriptor rings (512).  
> Try
> testing with more packets and you should see some latent segmentation
> faults.  You should also see this with testpmd, if you try broadcasting enough
> packets.
> 
> 
> Thanks,
> Jeff

Hey Jeff, I'm not sure why I didn't see this in testing using on the test-pmd 
app, but it's obvious that this will cause issues. I'll add code to increment 
the refcnt in each mbuf by (num_of_slaves -1) in the bond_ethdev_tx_broadcast() 
function and also add a unit test to validate this in the next version of the 
patch.

Thanks,
Declan
--
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

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.




[dpdk-dev] [PATCH 0/4] Link Bonding Library

2014-05-29 Thread Doherty, Declan
> From: Cao, Waterman 
> Sent: Thursday, May 29, 2014 4:23 AM
> To: Doherty, Declan; dev at dpdk.org; dev at dpdk.org
> Cc: Cao, Waterman
> Subject: RE: [dpdk-dev] [PATCH 0/4] Link Bonding Library
>
> Hi declan,
>
>   Do you send out Patch 1,2, 3 for link bonding?
>   Only see patch 0 and 4.
>
> Thanks
> Waterman 

Hi Waterman, 

As far as I can see all the patches have been submitted, and are visible on the 
mailing list archives.

Declan
--
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

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.




[dpdk-dev] [PATCH 0/4] Link Bonding Library

2014-05-29 Thread Doherty, Declan
-Original Message-
> From: Neil Horman [mailto:nhorman at tuxdriver.com] 
> Sent: Wednesday, May 28, 2014 6:49 PM
> To: Doherty, Declan
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/4] Link Bonding Library
>
> On Wed, May 28, 2014 at 04:32:00PM +0100, declan.doherty at intel.com wrote:
> > From: Declan Doherty 
> > 
> > Initial release of Link Bonding Library (lib/librte_bond) with support 
> > for bonding modes :
> >  0 - Round Robin
> >  1 - Active Backup
> >  2 - Balance l2 / l23 / l34
> >  3 - Broadcast
> > 
> Why make this a separate library?  That requires exposure of yet another API 
> to applications.  Instead, why > not write a PMD that can enslave other PMD's 
> and treat them all as a single interface?  That way this all >  > works with 
> the existing API.
>
> Neil

Hi Neil,
the link bonding device is essentially a software PMD, and as such supports all 
the standard PMD APIs, the only new APIs which the link bonding library 
introduces  are for the control operations of the bonded device which are 
currently unsupported by the standard PMD API. Operations such as creating, 
adding/removing slaves, and configuring the modes of operation of the device 
have no analogous APIs in the current PMD API and required new ones to be 
created .

Declan
--
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

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.