Hi Bruce,

-----Original Message-----
From: Richardson, Bruce
Sent: Friday, June 10, 2016 3:46 PM
To: Iremonger, Bernard <redacted>
Cc: dev@dpdk.org; Doherty, Declan <redacted>; Ananyev,
Konstantin [off-list ref]
Subject: Re: [dpdk-dev] [PATCH v2 0/6] bonding: locks

On Thu, May 26, 2016 at 05:38:41PM +0100, Bernard Iremonger wrote:

quoted

Add spinlock to bonding rx and tx queues.
Take spinlock in rx and tx burst functions.
Take all spinlocks in slave add and remove functions.
With spinlocks in place remove memcpy of slaves.

Changes in v2:
Replace patch 1.
Add patch 2 and reorder patches.
Add spinlock to bonding rx and tx queues.
Take all spinlocks in slave add and remove functions.
Replace readlocks with spinlocks.

Bernard Iremonger (6):
  bonding: add spinlock to rx and tx queues
  bonding: grab queue spinlocks in slave add and remove
  bonding: take queue spinlock in rx/tx burst functions
  bonding: add spinlock to stop function
  bonding: add spinlock to link update function
  bonding: remove memcpy from burst functions

 drivers/net/bonding/rte_eth_bond_api.c     |  52 +++++++-
 drivers/net/bonding/rte_eth_bond_pmd.c     | 196

++++++++++++++++++-----------

quoted

 drivers/net/bonding/rte_eth_bond_private.h |   4 +-
 3 files changed, 173 insertions(+), 79 deletions(-)

--

The patches in this set are missing any explanation for the reasons behind
each patch. The commit messages are empty for every patch.

I'm also concerned at the fact that this patchset is adding lock operations all
over the bonding PMD. While other PMDs need synchronization between
control plane and data plane threads so that e.g. you don't do IO on a
stopped port, they don't use locks so as to get max performance. Nowhere
in the patchset is there an explanation as to why bonding is so different that
it needs locks where other PMDs can do without them. This should also be
explained in each individual patch as to why the area covered by the patch
needs locks in this PMD (again, compared to other PMDs)

Thanks,
/Bruce

I will be sending a v3 for this patchset.
The empty commit messages were an oversight on my part, this will be corrected in the v3.
I will also try to explain why the locks are needed.

Regards,

Bernard.