[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
Hi Konstantin,
> > > > On 05/05/16 18:12, Stephen Hemminger wrote:
> > > > > On Thu, 5 May 2016 16:14:56 +0100 Bernard Iremonger
> > > > > wrote:
> > > > >
> > > > >> Fixes: a45b288ef21a ("bond: support link status polling")
> > > > >> Signed-off-by: Bernard Iremonger
> > > > >
> > > > > You know an uncontested reader/writer lock is significantly
> > > > > slower than a spinlock.
> > > > >
> > > >
> > > > As we can have multiple readers of the active slave list / primary
> > > > slave, basically any tx/rx burst call needs to protect against a
> > > > device being removed/closed during it's operation now that we
> > > > support hotplugging, in the worst case this could mean we have
> > > > 2(rx+tx) * queues possibly using the active slave list
> > > > simultaneously, in that case I would have thought that a spinlock
> > > > would have a much more significant affect on performance?
> > >
> > > Right, but the window where the shared variable is accessed is very
> > > small, and it is actually faster to use spinlock for that.
> >
> > I don't think that window we hold the lock is that small, let say if
> > we have a burst of 32 packets * (let say) 50 cycles/pkt = ~1500 cycles -
> > each
> IO thread would stall.
> > For me that's long enough to justify rwlock usage here, especially
> > that DPDK rwlock price is not much bigger (as I remember) then
> > spinlock - it is basically 1 CAS operation.
>
> As another alternative we can have a spinlock per queue, then different IO
> threads doing RX/XTX over different queues will be uncontended at all.
> Though control thread would need to grab locks for all configured queues :)
>
> Konstantin
>
I am preparing a v2 patchset which uses a spinlock per queue.
Regards,
Bernard.
[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Friday, May 13, 2016 6:11 PM
> To: Stephen Hemminger; Doherty, Declan
> Cc: Iremonger, Bernard; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write
> lock
>
>
>
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Friday, May 06, 2016 4:56 PM
> > To: Doherty, Declan
> > Cc: Iremonger, Bernard; dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 1/5] bonding: replace spinlock with
> > read/write lock
> >
> > On Fri, 6 May 2016 11:32:19 +0100
> > Declan Doherty wrote:
> >
> > > On 05/05/16 18:12, Stephen Hemminger wrote:
> > > > On Thu, 5 May 2016 16:14:56 +0100
> > > > Bernard Iremonger wrote:
> > > >
> > > >> Fixes: a45b288ef21a ("bond: support link status polling")
> > > >> Signed-off-by: Bernard Iremonger
> > > >
> > > > You know an uncontested reader/writer lock is significantly slower
> > > > than a spinlock.
> > > >
> > >
> > > As we can have multiple readers of the active slave list / primary
> > > slave, basically any tx/rx burst call needs to protect against a device
> > > being removed/closed during it's operation now that we support
> > > hotplugging, in the worst case this could mean we have 2(rx+tx) * queues
> > > possibly using the active slave list simultaneously, in that case I
> > > would have thought that a spinlock would have a much more significant
> > > affect on performance?
> >
> > Right, but the window where the shared variable is accessed is very small,
> > and it is actually faster to use spinlock for that.
>
> I don't think that window we hold the lock is that small, let say if we have
> a burst of 32 packets * (let say) 50 cycles/pkt = ~1500 cycles - each IO
> thread would stall.
> For me that's long enough to justify rwlock usage here, especially that
> DPDK rwlock price is not much bigger (as I remember) then spinlock -
> it is basically 1 CAS operation.
As another alternative we can have a spinlock per queue, then different IO
threads
doing RX/XTX over different queues will be uncontended at all.
Though control thread would need to grab locks for all configured queues :)
Konstantin
>
> Konstantin
>
[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Friday, May 06, 2016 4:56 PM
> To: Doherty, Declan
> Cc: Iremonger, Bernard; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write
> lock
>
> On Fri, 6 May 2016 11:32:19 +0100
> Declan Doherty wrote:
>
> > On 05/05/16 18:12, Stephen Hemminger wrote:
> > > On Thu, 5 May 2016 16:14:56 +0100
> > > Bernard Iremonger wrote:
> > >
> > >> Fixes: a45b288ef21a ("bond: support link status polling")
> > >> Signed-off-by: Bernard Iremonger
> > >
> > > You know an uncontested reader/writer lock is significantly slower
> > > than a spinlock.
> > >
> >
> > As we can have multiple readers of the active slave list / primary
> > slave, basically any tx/rx burst call needs to protect against a device
> > being removed/closed during it's operation now that we support
> > hotplugging, in the worst case this could mean we have 2(rx+tx) * queues
> > possibly using the active slave list simultaneously, in that case I
> > would have thought that a spinlock would have a much more significant
> > affect on performance?
>
> Right, but the window where the shared variable is accessed is very small,
> and it is actually faster to use spinlock for that.
I don't think that window we hold the lock is that small, let say if we have
a burst of 32 packets * (let say) 50 cycles/pkt = ~1500 cycles - each IO thread
would stall.
For me that's long enough to justify rwlock usage here, especially that
DPDK rwlock price is not much bigger (as I remember) then spinlock -
it is basically 1 CAS operation.
Konstantin
[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
On 05/05/16 18:12, Stephen Hemminger wrote:
> On Thu, 5 May 2016 16:14:56 +0100
> Bernard Iremonger wrote:
>
>> Fixes: a45b288ef21a ("bond: support link status polling")
>> Signed-off-by: Bernard Iremonger
>
> You know an uncontested reader/writer lock is significantly slower
> than a spinlock.
>
As we can have multiple readers of the active slave list / primary
slave, basically any tx/rx burst call needs to protect against a device
being removed/closed during it's operation now that we support
hotplugging, in the worst case this could mean we have 2(rx+tx) * queues
possibly using the active slave list simultaneously, in that case I
would have thought that a spinlock would have a much more significant
affect on performance?
[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
On Fri, 6 May 2016 11:32:19 +0100
Declan Doherty wrote:
> On 05/05/16 18:12, Stephen Hemminger wrote:
> > On Thu, 5 May 2016 16:14:56 +0100
> > Bernard Iremonger wrote:
> >
> >> Fixes: a45b288ef21a ("bond: support link status polling")
> >> Signed-off-by: Bernard Iremonger
> >
> > You know an uncontested reader/writer lock is significantly slower
> > than a spinlock.
> >
>
> As we can have multiple readers of the active slave list / primary
> slave, basically any tx/rx burst call needs to protect against a device
> being removed/closed during it's operation now that we support
> hotplugging, in the worst case this could mean we have 2(rx+tx) * queues
> possibly using the active slave list simultaneously, in that case I
> would have thought that a spinlock would have a much more significant
> affect on performance?
Right, but the window where the shared variable is accessed is very small,
and it is actually faster to use spinlock for that.
[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
Fixes: a45b288ef21a ("bond: support link status polling")
Signed-off-by: Bernard Iremonger
---
drivers/net/bonding/rte_eth_bond_api.c | 10 +++---
drivers/net/bonding/rte_eth_bond_pmd.c | 57 +++---
drivers/net/bonding/rte_eth_bond_private.h | 6 ++--
3 files changed, 36 insertions(+), 37 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_api.c
b/drivers/net/bonding/rte_eth_bond_api.c
index d3bda35..c77626d 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -227,7 +227,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t
socket_id)
eth_dev->data->drv_name = pmd_bond_driver_name;
eth_dev->data->numa_node = socket_id;
- rte_spinlock_init(>lock);
+ rte_rwlock_init(>rwlock);
internals->port_id = eth_dev->data->port_id;
internals->mode = BONDING_MODE_INVALID;
@@ -451,11 +451,11 @@ rte_eth_bond_slave_add(uint8_t bonded_port_id, uint8_t
slave_port_id)
bonded_eth_dev = _eth_devices[bonded_port_id];
internals = bonded_eth_dev->data->dev_private;
- rte_spinlock_lock(>lock);
+ rte_rwlock_write_lock(>rwlock);
retval = __eth_bond_slave_add_lock_free(bonded_port_id, slave_port_id);
- rte_spinlock_unlock(>lock);
+ rte_rwlock_write_unlock(>rwlock);
return retval;
}
@@ -553,11 +553,11 @@ rte_eth_bond_slave_remove(uint8_t bonded_port_id, uint8_t
slave_port_id)
bonded_eth_dev = _eth_devices[bonded_port_id];
internals = bonded_eth_dev->data->dev_private;
- rte_spinlock_lock(>lock);
+ rte_rwlock_write_lock(>rwlock);
retval = __eth_bond_slave_remove_lock_free(bonded_port_id,
slave_port_id);
- rte_spinlock_unlock(>lock);
+ rte_rwlock_write_unlock(>rwlock);
return retval;
}
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
b/drivers/net/bonding/rte_eth_bond_pmd.c
index 129f04b..ed6245b 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -1750,37 +1750,36 @@ bond_ethdev_slave_link_status_change_monitor(void
*cb_arg)
!internals->link_status_polling_enabled)
return;
- /* If device is currently being configured then don't check slaves link
-* status, wait until next period */
- if (rte_spinlock_trylock(>lock)) {
- if (internals->slave_count > 0)
- polling_slave_found = 0;
+ rte_rwlock_read_lock(>rwlock);
+ if (internals->slave_count > 0)
+ polling_slave_found = 0;
- for (i = 0; i < internals->slave_count; i++) {
- if (!internals->slaves[i].link_status_poll_enabled)
- continue;
-
- slave_ethdev =
_eth_devices[internals->slaves[i].port_id];
- polling_slave_found = 1;
-
- /* Update slave link status */
- (*slave_ethdev->dev_ops->link_update)(slave_ethdev,
-
internals->slaves[i].link_status_wait_to_complete);
-
- /* if link status has changed since last checked then
call lsc
-* event callback */
- if (slave_ethdev->data->dev_link.link_status !=
- internals->slaves[i].last_link_status) {
- internals->slaves[i].last_link_status =
-
slave_ethdev->data->dev_link.link_status;
-
-
bond_ethdev_lsc_event_callback(internals->slaves[i].port_id,
- RTE_ETH_EVENT_INTR_LSC,
- _ethdev->data->port_id);
- }
+ for (i = 0; i < internals->slave_count; i++) {
+ if (!internals->slaves[i].link_status_poll_enabled)
+ continue;
+
+ slave_ethdev = _eth_devices[internals->slaves[i].port_id];
+ polling_slave_found = 1;
+
+ /* Update slave link status */
+ (*slave_ethdev->dev_ops->link_update)(slave_ethdev,
+ internals->slaves[i].link_status_wait_to_complete);
+
+ /* if link status has changed since last checked then call lsc
+* event callback
+*/
+ if (slave_ethdev->data->dev_link.link_status !=
+ internals->slaves[i].last_link_status) {
+ internals->slaves[i].last_link_status =
+
[dpdk-dev] [PATCH 1/5] bonding: replace spinlock with read/write lock
On Thu, 5 May 2016 16:14:56 +0100
Bernard Iremonger wrote:
> Fixes: a45b288ef21a ("bond: support link status polling")
> Signed-off-by: Bernard Iremonger
You know an uncontested reader/writer lock is significantly slower
than a spinlock.
