common linkstatus functions
Subsystems (2)
v8 cover common linkstatus functions
While writing hyper-v driver, noticed a lot of unnecessary duplication of code in drivers for handling the eth_dev link status information. While consolidating this, it also became obvious that some drivers behave differently for no good reason.
It also was a good chance to introduce atomic exchange primitives in EAL because there are other places using cmpset where not necessary (such as bonding).
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
v8
- fix typo in 32bit exchange
- fix build of dpaa2
- go back to same return value as original code
- reduce number of lines changed
v7
- add exchange functions for PPC64
- move linkstatus helpers to rte_ethdev_driver
- rebase to 18.02
Stephen Hemminger (15):
eal: introduce atomic exchange operation
ethdev: add linkstatus get/set helper functions
net/virtio: use eth_linkstatus_set
net/vmxnet3: use rte_eth_linkstatus_set
net/dpaa2: use rte_eth_linkstatus_set
net/nfp: use rte_eth_linkstatus functions
net/e1000: use rte_eth_linkstatus helpers
net/ixgbe: use rte_eth_linkstatus functions
net/sfc: use new rte_eth_linkstatus functions
net/i40e: use rte_eth_linkstatus functions
net/liquidio: use rte_eth_linkstatus_set
net/thunderx: use rte_eth_linkstatus_set
net/szedata: use _rte_eth_linkstatus_set
net/octeontx: use rte_eth_linkstatus_set
net/enic: use rte_eth_linkstatus_set
drivers/net/dpaa2/dpaa2_ethdev.c | 78 +++---------------- drivers/net/e1000/em_ethdev.c | 70 ++--------------- drivers/net/e1000/igb_ethdev.c | 71 ++--------------- drivers/net/enic/enic_ethdev.c | 5 +- drivers/net/enic/enic_main.c | 16 ++-- drivers/net/i40e/i40e_ethdev.c | 46 ++--------- drivers/net/i40e/i40e_ethdev_vf.c | 19 +---- drivers/net/ixgbe/ixgbe_ethdev.c | 90 ++++------------------ drivers/net/liquidio/lio_ethdev.c | 44 +---------- drivers/net/nfp/nfp_net.c | 72 ++--------------- drivers/net/octeontx/octeontx_ethdev.c | 16 +--- drivers/net/sfc/sfc_ethdev.c | 24 ++---- drivers/net/sfc/sfc_ev.c | 20 +---- drivers/net/szedata2/rte_eth_szedata2.c | 11 +-- drivers/net/thunderx/nicvf_ethdev.c | 45 +++++------ drivers/net/virtio/virtio_ethdev.c | 52 ++----------- drivers/net/vmxnet3/vmxnet3_ethdev.c | 63 +-------------- .../common/include/arch/ppc_64/rte_atomic.h | 21 ++++- .../common/include/arch/x86/rte_atomic.h | 24 ++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 +++ .../common/include/arch/x86/rte_atomic_64.h | 12 +++ lib/librte_eal/common/include/generic/rte_atomic.h | 78 +++++++++++++++++++ lib/librte_ether/rte_ethdev.c | 22 +----- lib/librte_ether/rte_ethdev_driver.h | 63 +++++++++++++++ 24 files changed, 313 insertions(+), 661 deletions(-)
Signature · 1 line
2.15.1
PATCH v8 1/15 eal: introduce atomic exchange operation
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Signed-off-by Stephen Hemminger <[email protected]>
--- .../common/include/arch/ppc_64/rte_atomic.h | 21 +++++- .../common/include/arch/x86/rte_atomic.h | 24 +++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 ++++ .../common/include/arch/x86/rte_atomic_64.h | 12 ++++ lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++++++ 5 files changed, 146 insertions(+), 1 deletion(-)
lib/librte_eal/common/include/arch/ppc_64/rte_atomic.h +20 -1
@@ -132,6 +132,12 @@ static inline int rte_atomic16_dec_and_test(rte_atomic16_t *v)return __atomic_sub_fetch (&v->cnt,1 , __ATOMIC_ACQUIRE) ==0 ; } +static inline uint16_t +rte_atomic16_exchange (volatile uint16_t *dst,uint16_t val) +{ +return __atomic_exchange_2 (dst, val, __ATOMIC_SEQ_CST); +} +/*------------------------- 32 bit atomic operations -------------------------*/ static inline int
@@ -233,6 +239,13 @@ static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v)return ret ==0 ; } + +static inline uint32_t +rte_atomic32_exchange (volatile uint32_t *dst,uint32_t val) +{ +return __atomic_exchange_4 (dst, val, __ATOMIC_SEQ_CST); +} +/*------------------------- 64 bit atomic operations -------------------------*/ static inline int
@@ -427,7 +440,6 @@ static inline int rte_atomic64_test_and_set(rte_atomic64_t *v) {return rte_atomic64_cmpset ((volatile uint64_t *)&v->cnt,0 ,1 ); } -/** * Atomically set a 64-bit counter to 0. *
@@ -438,6 +450,13 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v) { v->cnt =0 ; } + +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dst,uint64_t val) +{ +return __atomic_exchange_4 (dst, val, __ATOMIC_SEQ_CST); +} +#endif #ifdef __cplusplus
lib/librte_eal/common/include/arch/x86/rte_atomic.h +24 -0
@@ -60,6 +60,18 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)return res; } +static inline uint16_t +rte_atomic16_exchange (volatile uint16_t *dst,uint16_t val) +{ + asmvolatile ( +MPLOCKED +"xchgw %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline int rte_atomic16_test_and_set (rte_atomic16_t *v) {return rte_atomic16_cmpset ((volatile uint16_t *)&v->cnt,0 ,1 );
@@ -134,6 +146,18 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)return res; } +static inline uint32_t +rte_atomic32_exchange (volatile uint32_t *dst,uint32_t val) +{ + asmvolatile ( +MPLOCKED +"xchgl %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline int rte_atomic32_test_and_set (rte_atomic32_t *v) {return rte_atomic32_cmpset ((volatile uint32_t *)&v->cnt,0 ,1 );
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +12 -0
@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dest,uint64_t val) +{ +uint64_t old; + +do { + old = *dest; + }while (rte_atomic64_cmpset (dest, old, val)); + +return old; +} +static inline void rte_atomic64_init (rte_atomic64_t *v) {
lib/librte_eal/common/include/arch/x86/rte_atomic_64.h +12 -0
@@ -71,6 +71,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dst,uint64_t val) +{ + asmvolatile ( +MPLOCKED +"xchgq %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline void rte_atomic64_init (rte_atomic64_t *v) {
lib/librte_eal/common/include/generic/rte_atomic.h +78 -0
96 lines not on this page · raw message
Signature · 1 line
2.15.1
Re: [PATCH v8 01/15] eal: introduce atomic exchange operation
26/01/2018 03:01, Stephen Hemminger:
Quoted 5 lines
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Quoted 8 lines
Signed-off-by Stephen Hemminger <[email protected]>--- .../common/include/arch/ppc_64/rte_atomic.h | 21 +++++- .../common/include/arch/x86/rte_atomic.h | 24 +++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 ++++ .../common/include/arch/x86/rte_atomic_64.h | 12 ++++ lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++++++ 5 files changed, 146 insertions(+), 1 deletion(-)
Looks good, thanks.
It probably deserves a review by PPC experts.
Adding Chao, maintainer of this part.
+ Bruce and Konstantin for x86 part.
Re: [PATCH v8 01/15] eal: introduce atomic exchange operation
Quoted 14 lines · Thomas Monjalon
26/01/2018 03:01, Stephen Hemminger:
Quoted 4 lines
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset. Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Quoted 8 lines
Signed-off-by Stephen Hemminger <[email protected]> ---.../common/include/arch/ppc_64/rte_atomic.h | 21 +++++- .../common/include/arch/x86/rte_atomic.h | 24 +++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 ++++ .../common/include/arch/x86/rte_atomic_64.h | 12 ++++ lib/librte_eal/common/include/generic/rte_atomic.h | 78++++++++++++++++++++++ 5 files changed, 146 insertions(+), 1 deletion(-)
Quoted 5 lines
Looks good, thanks.
It probably deserves a review by PPC experts. Adding Chao, maintainer of this part. + Bruce and Konstantin for x86 part.
Would it not be simpler to use __sync_bool_compare_and_swap compiler built-in on all supported platforms? Do we really need the per-platform optimization of this?
/Bruce
Re: [PATCH v8 01/15] eal: introduce atomic exchange operation
Quoted 17 lines · On Fri, 26 Jan 2018 17:24:40 +0000 Bruce Richardson…
Quoted 16 lines · Thomas Monjalon
26/01/2018 03:01, Stephen Hemminger:
Quoted 4 lines
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset. Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Quoted 8 lines
Signed-off-by Stephen Hemminger <[email protected]> ---.../common/include/arch/ppc_64/rte_atomic.h | 21 +++++- .../common/include/arch/x86/rte_atomic.h | 24 +++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 ++++ .../common/include/arch/x86/rte_atomic_64.h | 12 ++++ lib/librte_eal/common/include/generic/rte_atomic.h | 78++++++++++++++++++++++ 5 files changed, 146 insertions(+), 1 deletion(-)
Looks good, thanks.
Quoted 8 lines
Quoted 3 lines
It probably deserves a review by PPC experts. Adding Chao, maintainer of this part. + Bruce and Konstantin for x86 part.
Would it not be simpler to use
__sync_bool_compare_and_swapcompiler built-in on all supported platforms? Do we really need the per-platform optimization of this?/Bruce
Exchange is different than compare and swap. The is nice atomic intrinsic in GCC. The x86 part is in Linux and BSD already.
Re: [PATCH v8 01/15] eal: introduce atomic exchange operation
Quoted 23 lines · Stephen Hemminger
Quoted 20 lines · On Fri, 26 Jan 2018 17:24:40 +0000 Bruce Richardson…
Quoted 19 lines · Thomas Monjalon
26/01/2018 03:01, Stephen Hemminger:
Quoted 4 lines
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset. Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Quoted 8 lines
Signed-off-by Stephen Hemminger <[email protected]> ---.../common/include/arch/ppc_64/rte_atomic.h | 21 +++++- .../common/include/arch/x86/rte_atomic.h | 24 +++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 ++++ .../common/include/arch/x86/rte_atomic_64.h | 12 ++++ lib/librte_eal/common/include/generic/rte_atomic.h | 78++++++++++++++++++++++ 5 files changed, 146 insertions(+), 1 deletion(-)
Looks good, thanks.
It probably deserves a review by PPC experts. Adding Chao, maintainer of this part. + Bruce and Konstantin for x86 part.
Quoted 8 lines
Quoted 5 lines
Would it not be simpler to use
__sync_bool_compare_and_swapcompiler built-in on all supported platforms? Do we really need the per-platform optimization of this?/Bruce
Exchange is different than compare and swap. The is nice atomic intrinsic in GCC. The x86 part is in Linux and BSD already.
Yes, sorry, I picked the wrong builtin. I should have referenced instead "__sync_lock_test_and_set", which is an xchg op according to the docs [https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html].
Re: [PATCH v8 01/15] eal: introduce atomic exchange operation
In reply to Stephen HemmingerQuoted 14 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Stephen Hemminger
Sent: Friday, January 26, 2018 2:02 AM
To: [email protected]
Cc: Stephen Hemminger <[email protected]>
Subject: [dpdk-dev] [PATCH v8 01/15] eal: introduce atomic exchange operationTo handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Quoted 7 lines
Signed-off-by Stephen Hemminger <[email protected]>--- .../common/include/arch/ppc_64/rte_atomic.h | 21 +++++- .../common/include/arch/x86/rte_atomic.h | 24 +++++++ .../common/include/arch/x86/rte_atomic_32.h | 12 ++++ .../common/include/arch/x86/rte_atomic_64.h | 12 ++++ lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++++++
PATCH v8 2/15 ethdev: add linkstatus get/set helper functions
In reply to Stephen HemmingerMany drivers are all doing copy/paste of the same code to atomically update the link status. Reduce duplication, and allow for future changes by having common function for this.
Signed-off-by Stephen Hemminger <[email protected]>
--- lib/librte_ether/rte_ethdev.c | 22 +++---------- lib/librte_ether/rte_ethdev_driver.h | 63 ++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 18 deletions(-)
lib/librte_ether/rte_ethdev.c +4 -18
@@ -1578,20 +1578,6 @@ rte_eth_allmulticast_get(uint16_t port_id)return dev->data->all_multicast; } -static inline int -rte_eth_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -void rte_eth_link_get (uint16_t port_id,struct rte_eth_link *eth_link) {
@@ -1600,8 +1586,8 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)RTE_ETH_VALID_PORTID_OR_RET (port_id); dev = &rte_eth_devices[port_id]; -if (dev->data->dev_conf.intr_conf.lsc !=0 ) -rte_eth_dev_atomic_read_link_status (dev, eth_link); +if (dev->data->dev_conf.intr_conf.lsc) +rte_eth_linkstatus_get (dev, eth_link);else {RTE_FUNC_PTR_OR_RET (*dev->dev_ops->link_update); (*dev->dev_ops->link_update)(dev,1 );
@@ -1617,8 +1603,8 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)RTE_ETH_VALID_PORTID_OR_RET (port_id); dev = &rte_eth_devices[port_id]; -if (dev->data->dev_conf.intr_conf.lsc !=0 ) -rte_eth_dev_atomic_read_link_status (dev, eth_link); +if (dev->data->dev_conf.intr_conf.lsc) +rte_eth_linkstatus_get (dev, eth_link);else {RTE_FUNC_PTR_OR_RET (*dev->dev_ops->link_update); (*dev->dev_ops->link_update)(dev,0 );
lib/librte_ether/rte_ethdev_driver.h +63 -0
69 lines not on this page · raw message
Signature · 1 line
2.15.1
PATCH v8 3/15 net/virtio: use eth_linkstatus_set
In reply to Stephen HemmingerUse the new comon code in ethdev to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/virtio/virtio_ethdev.c | 52 ++++---------------------------------- 1 file changed, 5 insertions(+), 47 deletions(-)
drivers/net/virtio/virtio_ethdev.c +5 -47
@@ -14,7 +14,6 @@#include <rte_string_fns.h> #include <rte_memzone.h> #include <rte_malloc.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_pci.h> #include <rte_bus_pci.h>
@@ -785,46 +784,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = { .mac_addr_set = virtio_mac_addr_set, }; -static inline int -virtio_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -virtio_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static void virtio_update_stats (struct rte_eth_dev *dev,struct rte_eth_stats *stats) {
@@ -2034,21 +1993,21 @@ virtio_dev_stop(struct rte_eth_dev *dev) hw->started =0 ;memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_write_link_status (dev, &link); +rte_eth_linkstatus_set (dev, &link);rte_spinlock_unlock (&hw->state_lock); }static int virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) { -struct rte_eth_link link, old; +struct rte_eth_link link;uint16_t status;struct virtio_hw *hw = dev->data->dev_private; +memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_read_link_status (dev, &link); - old = link; link.link_duplex =ETH_LINK_FULL_DUPLEX ; link.link_speed =ETH_SPEED_NUM_10G ; + link.link_autoneg =ETH_LINK_SPEED_FIXED ;if (hw->started ==0 ) { link.link_status =ETH_LINK_DOWN ;
@@ -2069,9 +2028,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet }else { link.link_status =ETH_LINK_UP ; } -virtio_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v8 4/15 net/vmxnet3: use rte_eth_linkstatus_set
In reply to Stephen HemmingerUse new rte_eth_link_update helper.
Also remove no longer necessary include of rte_atomic.h
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/vmxnet3/vmxnet3_ethdev.c | 63 ++---------------------------------- 1 file changed, 3 insertions(+), 60 deletions(-)
drivers/net/vmxnet3/vmxnet3_ethdev.c +3 -60
@@ -20,7 +20,6 @@#include <rte_debug.h> #include <rte_pci.h> #include <rte_bus_pci.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_memory.h> #include <rte_memzone.h>
@@ -160,59 +159,6 @@ gpa_zone_reserve(struct rte_eth_dev *dev, uint32_t size,return rte_memzone_reserve_aligned (z_name, size, socket_id,0 , align); } -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ - -static int -vmxnet3_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to write to. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static int -vmxnet3_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* * This function is based on vmxnet3_disable_intr() */
@@ -853,7 +799,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)/* Clear recorded link status */ memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_write_link_status (dev, &link); +rte_eth_linkstatus_set (dev, &link); }/*
@@ -1132,11 +1078,10 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) {struct vmxnet3_hw *hw = dev->data->dev_private; -struct rte_eth_link old = {0 }, link; +struct rte_eth_link link;uint32_t ret;memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_read_link_status (dev, &old);VMXNET3_WRITE_BAR1_REG (hw,VMXNET3_REG_CMD ,VMXNET3_CMD_GET_LINK ); ret =VMXNET3_READ_BAR1_REG (hw,VMXNET3_REG_CMD );
@@ -1148,9 +1093,7 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev, link.link_autoneg =ETH_LINK_AUTONEG ; } -vmxnet3_dev_atomic_write_link_status (dev, &link); - -return (old.link_status == link.link_status) ?-1 :0 ; +return rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v8 5/15 net/dpaa2: use rte_eth_linkstatus_set
In reply to Stephen HemmingerUse new helper function to update the link status.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/dpaa2/dpaa2_ethdev.c | 78 ++++++---------------------------------- 1 file changed, 10 insertions(+), 68 deletions(-)
drivers/net/dpaa2/dpaa2_ethdev.c +10 -68
@@ -57,58 +57,6 @@ static int dpaa2_dev_set_link_up(struct rte_eth_dev *dev);static int dpaa2_dev_set_link_down (struct rte_eth_dev *dev);static int dpaa2_dev_mtu_set (struct rte_eth_dev *dev,uint16_t mtu); -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -dpaa2_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &dev->data->dev_link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -dpaa2_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static int dpaa2_vlan_filter_set (struct rte_eth_dev *dev,uint16_t vlan_id,int on) {
@@ -852,7 +800,7 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)/* clear the recorded link status */ memset (&link,0 ,sizeof (link)); -dpaa2_dev_atomic_write_link_status (dev, &link); +rte_eth_linkstatus_set (dev, &link); } static void
@@ -884,7 +832,7 @@ dpaa2_dev_close(struct rte_eth_dev *dev) }memset (&link,0 ,sizeof (link)); -dpaa2_dev_atomic_write_link_status (dev, &link); +rte_eth_linkstatus_set (dev, &link); } static void
@@ -1335,15 +1283,13 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,int ret;struct dpaa2_dev_priv *priv = dev->data->dev_private;struct fsl_mc_io *dpni = (struct fsl_mc_io *)priv->hw; -struct rte_eth_link link, old; +struct rte_eth_link link;struct dpni_link_state state = {0 };if (dpni ==NULL ) {RTE_LOG (ERR ,PMD ,"dpni is NULL\n" );return 0 ; } -memset (&old,0 ,sizeof (old)); -dpaa2_dev_atomic_read_link_status (dev, &old); ret =dpni_get_link_state (dpni,CMD_PRI_LOW , priv->token, &state);if (ret <0 ) {
@@ -1351,11 +1297,6 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,return -1 ; } -if ((old.link_status == state.up) && (old.link_speed == state.rate)) { -RTE_LOG (DEBUG ,PMD ,"No change in status\n" ); -return -1 ; - } -memset (&link,0 ,sizeof (struct rte_eth_link )); link.link_status = state.up; link.link_speed = state.rate;
@@ -1365,13 +1306,14 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,else link.link_duplex =ETH_LINK_FULL_DUPLEX ; -dpaa2_dev_atomic_write_link_status (dev, &link); - -if (link.link_status) -PMD_DRV_LOG (INFO ,"Port %d Link is Up\n" , dev->data->port_id); + ret =rte_eth_linkstatus_set (dev, &link); +if (ret ==-1 ) +RTE_LOG (DEBUG ,PMD ,"No change in status\n" );else -PMD_DRV_LOG (INFO ,"Port %d Link is Down" , dev->data->port_id); -return 0 ; +PMD_DRV_LOG (INFO ,"Port %d Link is %s\n" , dev->data->port_id, + link.link_status ?"Up" :"Down" ); + +return ret; }/**
Signature · 1 line
2.15.1
PATCH v8 6/15 net/nfp: use rte_eth_linkstatus functions
In reply to Stephen HemmingerUse new rte_eth_linkstatus_get/set helper function.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/nfp/nfp_net.c | 72 +++++------------------------------------------ 1 file changed, 7 insertions(+), 65 deletions(-)
drivers/net/nfp/nfp_net.c +7 -65
@@ -213,57 +213,6 @@ nn_cfg_writeq(struct nfp_net_hw *hw, int off, uint64_t val)nn_writeq (rte_cpu_to_le_64 (val), hw->ctrl_bar + off); } -/* -* Atomically reads link status information from global structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -nfp_net_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &dev->data->dev_link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/* -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -nfp_net_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static void nfp_net_rx_queue_release_mbufs (struct nfp_net_rxq *rxq) {
@@ -999,8 +948,9 @@ static int nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) { struct nfp_net_hw *hw; -struct rte_eth_link link, old; +struct rte_eth_link link;uint32_t nn_link_status; +int ret; static const uint32_t ls_to_ethtool[] = { [NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = ETH_SPEED_NUM_NONE,
@@ -1017,9 +967,6 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) hw =NFP_NET_DEV_PRIVATE_TO_HW (dev->data->dev_private); -memset (&old,0 ,sizeof (old)); -nfp_net_dev_atomic_read_link_status (dev, &old); - nn_link_status =nn_cfg_readl (hw,NFP_NET_CFG_STS );memset (&link,0 ,sizeof (struct rte_eth_link ));
@@ -1037,16 +984,14 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)else link.link_speed = ls_to_ethtool[nn_link_status]; -if (old.link_status != link.link_status) { -nfp_net_dev_atomic_write_link_status (dev, &link); + ret =rte_eth_linkstatus_set (dev, &link); +if (ret ==0 ) {if (link.link_status)PMD_DRV_LOG (INFO ,"NIC Link is Up\n" );else PMD_DRV_LOG (INFO ,"NIC Link is Down\n" ); -return 0 ; } - -return -1 ; +return ret; } static int
@@ -1376,8 +1321,7 @@ nfp_net_dev_link_status_print(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_eth_link link; -memset (&link,0 ,sizeof (link)); -nfp_net_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link);if (link.link_status) RTE_LOG(INFO, PMD, "Port %d: Link Up - speed %u Mbps - %s\n", dev->data->port_id, link.link_speed,
@@ -1430,9 +1374,7 @@ nfp_net_dev_interrupt_handler(void *param)PMD_DRV_LOG (DEBUG ,"We got a LSC interrupt!!!\n" ); -/* get the link status */ -memset (&link,0 ,sizeof (link)); -nfp_net_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link);nfp_net_link_update (dev,0 );
Signature · 1 line
2.15.1
PATCH v8 7/15 net/e1000: use rte_eth_linkstatus helpers
In reply to Stephen HemmingerUse new rte_eth_linkstatus_get/set API.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/e1000/em_ethdev.c | 70 +++-------------------------------------- drivers/net/e1000/igb_ethdev.c | 71 +++--------------------------------------- 2 files changed, 10 insertions(+), 131 deletions(-)
drivers/net/e1000/em_ethdev.c +5 -65
@@ -20,7 +20,6 @@#include <rte_ethdev_pci.h> #include <rte_memory.h> #include <rte_eal.h> -#include <rte_atomic.h> #include <rte_malloc.h> #include <rte_dev.h>
@@ -197,57 +196,6 @@ static const struct eth_dev_ops eth_em_ops = { .txq_info_get = em_txq_info_get, }; -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_em_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_em_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -}/** * eth_em_dev_is_ich8 - Check for ICH8 device
@@ -802,7 +750,7 @@ eth_em_stop(struct rte_eth_dev *dev)/* clear the recorded link status */ memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_write_link_status (dev, &link); +rte_eth_linkstatus_set (dev, &link);if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
@@ -1160,7 +1108,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct e1000_hw *hw =E1000_DEV_PRIVATE_TO_HW (dev->data->dev_private); -struct rte_eth_link link, old; +struct rte_eth_link link;int link_check, count; link_check =0 ;
@@ -1195,8 +1143,6 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)rte_delay_ms (EM_LINK_UPDATE_CHECK_INTERVAL ); }memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_read_link_status (dev, &link); - old = link;/* Now we check if a transition has happened */ if (link_check && (link.link_status ==ETH_LINK_DOWN )) {
@@ -1215,14 +1161,8 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete) link.link_status =ETH_LINK_DOWN ; link.link_autoneg =ETH_LINK_FIXED ; } -rte_em_dev_atomic_write_link_status (dev, &link); - -/* not changed */ -if (old.link_status == link.link_status) -return -1 ; -/* changed */ -return 0 ; +return rte_eth_linkstatus_set (dev, &link); }/*
@@ -1631,8 +1571,8 @@ eth_em_interrupt_action(struct rte_eth_dev *dev,if (ret <0 )return 0 ; -memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link); +if (link.link_status) { PMD_INIT_LOG(INFO, " Port %d: Link Up - speed %u Mbps - %s", dev->data->port_id, link.link_speed,
drivers/net/e1000/igb_ethdev.c +5 -66
120 lines not on this page · raw message
Signature · 1 line
2.15.1
PATCH v8 8/15 net/ixgbe: use rte_eth_linkstatus functions
In reply to Stephen HemmingerUse the new helper functions from eth_dev for
handling atomic link_info update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/ixgbe/ixgbe_ethdev.c | 90 ++++++---------------------------------- 1 file changed, 13 insertions(+), 77 deletions(-)
drivers/net/ixgbe/ixgbe_ethdev.c +13 -77
@@ -20,7 +20,6 @@#include <rte_debug.h> #include <rte_pci.h> #include <rte_bus_pci.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_memory.h> #include <rte_eal.h>
@@ -787,58 +786,6 @@ static const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[] = {#define IXGBEVF_NB_XSTATS (sizeof(rte_ixgbevf_stats_strings) / \ sizeof(rte_ixgbevf_stats_strings[0])) -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_ixgbe_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_ixgbe_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* * This function is the same as ixgbe_is_sfp() in base/ixgbe.h. */
@@ -2758,7 +2705,7 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)/* Clear recorded link status */ memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_write_link_status (dev, &link); +rte_eth_linkstatus_set (dev, &link);if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
@@ -3943,7 +3890,7 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, int wait_to_complete, int vf) {struct ixgbe_hw *hw =IXGBE_DEV_PRIVATE_TO_HW (dev->data->dev_private); -struct rte_eth_link link, old; +struct rte_eth_link link;ixgbe_link_speed link_speed =IXGBE_LINK_SPEED_UNKNOWN ;struct ixgbe_interrupt *intr =IXGBE_DEV_PRIVATE_TO_INTR (dev->data->dev_private);
@@ -3953,12 +3900,11 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,int wait =1 ;bool autoneg = false; +memset (&link,0 ,sizeof (link)); link.link_status =ETH_LINK_DOWN ; link.link_speed =0 ; link.link_duplex =ETH_LINK_HALF_DUPLEX ; link.link_autoneg =ETH_LINK_AUTONEG ; -memset (&old,0 ,sizeof (old)); -rte_ixgbe_dev_atomic_read_link_status (dev, &old); hw->mac.get_link_status = true;
@@ -3982,19 +3928,14 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,if (diag !=0 ) { link.link_speed =ETH_SPEED_NUM_100M ; link.link_duplex =ETH_LINK_FULL_DUPLEX ; -rte_ixgbe_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return rte_eth_linkstatus_set (dev, &link); }if (link_up ==0 ) { -rte_ixgbe_dev_atomic_write_link_status (dev, &link); intr->flags |=IXGBE_FLAG_NEED_LINK_CONFIG ; -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return rte_eth_linkstatus_set (dev, &link); } + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG ; link.link_status =ETH_LINK_UP ; link.link_duplex =ETH_LINK_FULL_DUPLEX ;
@@ -4026,12 +3967,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, link.link_speed =ETH_SPEED_NUM_10G ;break ; } -rte_ixgbe_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; - -return 0 ; +return rte_eth_linkstatus_set (dev, &link); } static int
@@ -4230,8 +4167,8 @@ ixgbe_dev_link_status_print(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_eth_link link; -memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link); +if (link.link_status) { PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s", (int)(dev->data->port_id),
@@ -4266,7 +4203,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,struct ixgbe_interrupt *intr =IXGBE_DEV_PRIVATE_TO_INTR (dev->data->dev_private);int64_t timeout; -struct rte_eth_link link;struct ixgbe_hw *hw =IXGBE_DEV_PRIVATE_TO_HW (dev->data->dev_private);
@@ -4283,9 +4219,10 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev, }if (intr->flags &IXGBE_FLAG_NEED_LINK_UPDATE ) { +struct rte_eth_link link; +/* get the link status before link update, for predicting later */ -memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link);ixgbe_dev_link_update (dev,0 );
@@ -6804,9 +6741,8 @@ ixgbe_start_timecounters(struct rte_eth_dev *dev)uint32_t shift =0 ;/* Get current link speed. */ -memset (&link,0 ,sizeof (link));ixgbe_dev_link_update (dev,1 ); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link);switch (link.link_speed) {case ETH_SPEED_NUM_100M :
Signature · 1 line
2.15.1
PATCH v8 9/15 net/sfc: use new rte_eth_linkstatus functions
In reply to Stephen HemmingerUse the new API rte_eth_linkstatus_set to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/sfc/sfc_ethdev.c | 24 +++++------------------- drivers/net/sfc/sfc_ev.c | 20 ++------------------ 2 files changed, 7 insertions(+), 37 deletions(-)
drivers/net/sfc/sfc_ethdev.c +5 -19
@@ -236,22 +236,13 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link; +int ret;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;
@@ -259,21 +250,16 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status) + ret =rte_eth_linkstatus_set (dev, ¤t_link); +if (ret ==0 )sfc_info (sa,"Link status is %s" , current_link.link_status ?"UP" :"DOWN" ); - -return old_link.link_status == current_link.link_status ?0 :-1 ; +return ret; } static void
drivers/net/sfc/sfc_ev.c +2 -18
@@ -382,27 +382,11 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); - - new_link_u64 = *(uint64_t *)&new_link; -do { - old_link_u64 =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (old_link_u64 == new_link_u64) -break ; - -if (rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; -break ; - } - }while (B_TRUE ); +if (rte_eth_linkstatus_set (sa->eth_dev, &new_link)) + evq->sa->port.lsc_seq++;return B_FALSE ; }
Signature · 1 line
2.15.1
Re: [PATCH v8 09/15] net/sfc: use new rte_eth_linkstatus functions
Quoted 52 lines · Stephen Hemminger
Use the new API
rte_eth_linkstatus_setto handle link status update.Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/sfc/sfc_ethdev.c | 24 +++++------------------- drivers/net/sfc/sfc_ev.c | 20 ++------------------ 2 files changed, 7 insertions(+), 37 deletions(-)drivers/net/sfc/sfc_ethdev.c +2
-17@@ -236,22 +236,13 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link; +int ret;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;@@ -259,21 +250,16 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status)
drivers/net/sfc/sfc_ethdev.c sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) · line 265
Quoted 10 lines
drivers/net/sfc/sfc_ethdev.c +3
-2+ ret =rte_eth_linkstatus_set (dev, ¤t_link); +if (ret ==0 )sfc_info (sa,"Link status is %s" , current_link.link_status ?"UP" :"DOWN" ); - -return old_link.link_status == current_link.link_status ?0 :-1 ; +return ret; } static void
These changes are OK.
Quoted 24 lines
drivers/net/sfc/sfc_ev.c +0
-13@@ -382,27 +382,11 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; - struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; - uint64_t new_link_u64; - uint64_t old_link_u64; - - EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); - - new_link_u64 = *(uint64_t *)&new_link; - do { - old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link); - if (old_link_u64 == new_link_u64) - break; - - if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
drivers/net/sfc/sfc_ev.c sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) · line 392
Quoted 10 lines
drivers/net/sfc/sfc_ev.c +2
-5- old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; -break ; - } - }while (B_TRUE ); +if (rte_eth_linkstatus_set (sa->eth_dev, &new_link)) + evq->sa->port.lsc_seq++;return B_FALSE ; }
Let me repeat part of my review notes for v4:
Quoted 1 line
Quoted 1 line
lsc_seq should be incremented in the case of any changes in link, not up/down changes only and it should be preserved.
<<
and I'd like to keep it here. Thanks to rte_atomic64_exchange() the function can be simplified to:
- convert link mode to link info (as now)
- do exchange
- compare old and new value as uint64_t and increment lsc_seq if different
Thanks.
PATCH v8 10/15 net/i40e: use rte_eth_linkstatus functions
In reply to Stephen HemmingerUse new rte_linkstatus update API
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/i40e/i40e_ethdev.c | 46 ++++++--------------------------------- drivers/net/i40e/i40e_ethdev_vf.c | 19 ++-------------- 2 files changed, 9 insertions(+), 56 deletions(-)
drivers/net/i40e/i40e_ethdev.c +7 -39
@@ -628,34 +628,6 @@ static struct rte_pci_driver rte_i40e_pmd = { .remove = eth_i40e_pci_remove, }; -static inline int -rte_i40e_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -static inline int -rte_i40e_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -RTE_PMD_REGISTER_PCI (net_i40e, rte_i40e_pmd);RTE_PMD_REGISTER_PCI_TABLE (net_i40e, pci_id_i40e_map);RTE_PMD_REGISTER_KMOD_DEP (net_i40e,"* igb_uio | uio_pci_generic | vfio-pci" );
@@ -2347,17 +2319,17 @@ i40e_dev_link_update(struct rte_eth_dev *dev,#define MAX_REPEAT_TIME 10/* 1s (10 * 100ms) in total */ struct i40e_hw *hw =I40E_DEV_PRIVATE_TO_HW (dev->data->dev_private);struct i40e_link_status link_status; -struct rte_eth_link link, old; +struct rte_eth_link link;int status;unsigned rep_cnt =MAX_REPEAT_TIME ;bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false; +int ret;memset (&link,0 ,sizeof (link)); -memset (&old,0 ,sizeof (old)); -memset (&link_status,0 ,sizeof (link_status)); -rte_i40e_dev_atomic_read_link_status (dev, &old); do { + memset(&link_status, 0, sizeof(link_status)); + /* Get link status information from hardware */ status = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
@@ -2410,13 +2382,10 @@ i40e_dev_link_update(struct rte_eth_dev *dev, ETH_LINK_SPEED_FIXED); out: -rte_i40e_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; - + ret =rte_eth_linkstatus_set (dev, &link);i40e_notify_all_vfs_link_status (dev); -return 0 ; +return ret; }/* Get all the statistics of a VSI */
@@ -10034,9 +10003,8 @@ i40e_start_timecounters(struct rte_eth_dev *dev)uint32_t tsync_inc_h;/* Get current link speed. */ -memset (&link,0 ,sizeof (link));i40e_dev_link_update (dev,1 ); -rte_i40e_dev_atomic_read_link_status (dev, &link); +rte_eth_linkstatus_get (dev, &link);switch (link.link_speed) {case ETH_SPEED_NUM_40G :
drivers/net/i40e/i40e_ethdev_vf.c +2 -17
@@ -1036,20 +1036,6 @@ static const struct rte_pci_id pci_id_i40evf_map[] = { { .vendor_id = 0, /* sentinel */ }, }; -static inline int -i40evf_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* Disable IRQ0 */ static inline void i40evf_disable_irq0 (struct i40e_hw *hw)
@@ -2077,6 +2063,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,* while Linux driver does not */ +memset (&new_link,0 ,sizeof (new_link));/* Linux driver PF host */ switch (vf->link_speed) {case I40E_LINK_SPEED_100MB :
@@ -2108,9 +2095,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, new_link.link_autoneg = dev->data->dev_conf.link_speeds Ð_LINK_SPEED_FIXED ; -i40evf_dev_atomic_write_link_status (dev, &new_link); - -return 0 ; +return rte_eth_linkstatus_set (dev, &new_link); } static void
Signature · 1 line
2.15.1
PATCH v8 11/15 net/liquidio: use rte_eth_linkstatus_set
In reply to Stephen HemmingerUse the new link update API, and cleanup the logic in the the link update routine.
Tested-by Shijith Thotton <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/liquidio/lio_ethdev.c | 44 ++++----------------------------------- 1 file changed, 4 insertions(+), 40 deletions(-)
drivers/net/liquidio/lio_ethdev.c +4 -40
@@ -904,32 +904,6 @@ lio_dev_vlan_filter_set(struct rte_eth_dev *eth_dev, uint16_t vlan_id, int on)return 0 ; } -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param eth_dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -lio_dev_atomic_write_link_status (struct rte_eth_dev *eth_dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = ð_dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static uint64_t lio_hweight64 (uint64_t w) {
@@ -949,23 +923,19 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev, int wait_to_complete __rte_unused) {struct lio_device *lio_dev =LIO_DEV (eth_dev); -struct rte_eth_link link, old; +struct rte_eth_link link;/* Initialize */ +memset (&link,0 ,sizeof (link)); link.link_status =ETH_LINK_DOWN ; link.link_speed =ETH_SPEED_NUM_NONE ; link.link_duplex =ETH_LINK_HALF_DUPLEX ; link.link_autoneg =ETH_LINK_AUTONEG ; -memset (&old,0 ,sizeof (old));/* Return what we found */ if (lio_dev->linfo.link.s.link_up ==0 ) {/* Interface is down */ -if (lio_dev_atomic_write_link_status (eth_dev, &link)) -return -1 ; -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return rte_eth_linkstatus_set (eth_dev, &link); } link.link_status =ETH_LINK_UP ;/* Interface is up */
@@ -982,13 +952,7 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev, link.link_duplex =ETH_LINK_HALF_DUPLEX ; } -if (lio_dev_atomic_write_link_status (eth_dev, &link)) -return -1 ; - -if (link.link_status == old.link_status) -return -1 ; - -return 0 ; +return rte_eth_linkstatus_set (eth_dev, &link); }/**
Signature · 1 line
2.15.1
PATCH v8 12/15 net/thunderx: use rte_eth_linkstatus_set
In reply to Stephen HemmingerUse new helper function.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/thunderx/nicvf_ethdev.c | 45 +++++++++++++++---------------------- 1 file changed, 18 insertions(+), 27 deletions(-)
drivers/net/thunderx/nicvf_ethdev.c +18 -27
@@ -15,7 +15,6 @@#include <sys/queue.h> #include <rte_alarm.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_byteorder.h> #include <rte_common.h>
@@ -69,25 +68,14 @@ nicvf_init_log(void)rte_log_set_level (nicvf_logtype_driver,RTE_LOG_NOTICE ); } -static inline int -nicvf_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) +static void +nicvf_link_status_update (struct nicvf *nic, +struct rte_eth_link *link) { -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; +memset (link,0 ,sizeof (*link)); -return 0 ; -} + link->link_status = nic->link_up ?ETH_LINK_UP :ETH_LINK_DOWN ; -static inline void -nicvf_set_eth_link_status (struct nicvf *nic,struct rte_eth_link *link) -{ - link->link_status = nic->link_up; - link->link_duplex =ETH_LINK_AUTONEG ;if (nic->duplex ==NICVF_HALF_DUPLEX ) link->link_duplex =ETH_LINK_HALF_DUPLEX ;else if (nic->duplex ==NICVF_FULL_DUPLEX )
@@ -101,12 +89,16 @@ nicvf_interrupt(void *arg) {struct rte_eth_dev *dev = arg;struct nicvf *nic =nicvf_pmd_priv (dev); +struct rte_eth_link link;if (nicvf_reg_poll_interrupts (nic) ==NIC_MBOX_MSG_BGX_LINK_CHANGE ) { -if (dev->data->dev_conf.intr_conf.lsc) -nicvf_set_eth_link_status (nic, &dev->data->dev_link); -_rte_eth_dev_callback_process (dev,RTE_ETH_EVENT_INTR_LSC , -NULL ); +if (dev->data->dev_conf.intr_conf.lsc) { +nicvf_link_status_update (nic, &link); +rte_eth_linkstatus_set (dev, &link); + +_rte_eth_dev_callback_process (dev,RTE_ETH_EVENT_INTR_LSC , +NULL ); + } } rte_eal_alarm_set(NICVF_INTR_POLL_INTERVAL_MS * 1000,
@@ -153,17 +145,16 @@ nicvf_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)if (wait_to_complete) {/* rte_eth_link_get() might need to wait up to 9 seconds */ for (i =0 ; i <MAX_CHECK_TIME ; i++) { -memset (&link,0 ,sizeof (link)); -nicvf_set_eth_link_status (nic, &link); -if (link.link_status) +nicvf_link_status_update (nic, &link); +if (link.link_status ==ETH_LINK_UP )break ;rte_delay_ms (CHECK_INTERVAL ); } }else { -memset (&link,0 ,sizeof (link)); -nicvf_set_eth_link_status (nic, &link); +nicvf_link_status_update (nic, &link); } -return nicvf_atomic_write_link_status (dev, &link); + +return rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v8 13/15 net/szedata: use _rte_eth_linkstatus_set
In reply to Stephen HemmingerYet another driver which was not returing correct value on link change.
Since this driver can't be built on x86 could not even do a compile test.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/szedata2/rte_eth_szedata2.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/net/szedata2/rte_eth_szedata2.c +4 -7
@@ -50,7 +50,6 @@#include <rte_memcpy.h> #include <rte_kvargs.h> #include <rte_dev.h> -#include <rte_atomic.h> #include "rte_eth_szedata2.h" #include "szedata2_iobuf.h"
@@ -1174,14 +1173,14 @@ eth_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused) {struct rte_eth_link link; -struct rte_eth_link *link_ptr = &link; -struct rte_eth_link *dev_link = &dev->data->dev_link;struct pmd_internals *internals = (struct pmd_internals *) dev->data->dev_private;const volatile struct szedata2_ibuf *ibuf;uint32_t i;bool link_is_up = false; +memset (&link,0 ,sizeof (link)); +switch (get_link_speed (internals)) {case SZEDATA2_LINK_SPEED_10G : link.link_speed =ETH_SPEED_NUM_10G ;
@@ -1212,13 +1211,11 @@ eth_link_update(struct rte_eth_dev *dev, } } - link.link_status = (link_is_up) ?ETH_LINK_UP :ETH_LINK_DOWN ; + link.link_status = link_is_up ?ETH_LINK_UP :ETH_LINK_DOWN ; link.link_autoneg =ETH_LINK_FIXED ; -rte_atomic64_cmpset ((uint64_t *)dev_link, *(uint64_t *)dev_link, - *(uint64_t *)link_ptr); - +rte_eth_linkstatus_set (dev, &link);return 0 ; }
Signature · 1 line
2.15.1
PATCH v8 14/15 net/octeontx: use rte_eth_linkstatus_set
In reply to Stephen HemmingerCommon function matches this drivers usage.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/octeontx/octeontx_ethdev.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
drivers/net/octeontx/octeontx_ethdev.c +1 -15
@@ -487,20 +487,6 @@ octeontx_dev_promisc_disable(struct rte_eth_dev *dev)octeontx_port_promisc_set (nic,0 ); } -static inline int -octeontx_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static int octeontx_port_link_status (struct octeontx_nic *nic) {
@@ -572,7 +558,7 @@ octeontx_dev_link_update(struct rte_eth_dev *dev, link.link_duplex =ETH_LINK_FULL_DUPLEX ; link.link_autoneg =ETH_LINK_AUTONEG ; -return octeontx_atomic_write_link_status (dev, &link); +return rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v8 15/15 net/enic: use rte_eth_linkstatus_set
In reply to Stephen HemmingerThis driver was not doing atomic update of link status information. And the return value was different than others.
The hardware also does not do autonegotiation (at least on Linux).
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/enic/enic_ethdev.c | 5 ++--- drivers/net/enic/enic_main.c | 16 +++++++--------- 2 files changed, 9 insertions(+), 12 deletions(-)
drivers/net/enic/enic_ethdev.c +2 -3
@@ -426,10 +426,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)ENICPMD_FUNC_TRACE ();enic_disable (enic); +memset (&link,0 ,sizeof (link)); -rte_atomic64_cmpset ((uint64_t *)ð_dev->data->dev_link, - *(uint64_t *)ð_dev->data->dev_link, - *(uint64_t *)&link); +rte_eth_linkstatus_set (eth_dev, &link); }/*
drivers/net/enic/enic_main.c +7 -9
@@ -379,16 +379,14 @@ enic_free_consistent(void *priv,int enic_link_update (struct enic *enic) {struct rte_eth_dev *eth_dev = enic->rte_dev; -int ret; -int link_status =0 ; +struct rte_eth_link link; - link_status =enic_get_link_status (enic); - ret = (link_status == enic->link_status); - enic->link_status = link_status; - eth_dev->data->dev_link.link_status = link_status; - eth_dev->data->dev_link.link_duplex =ETH_LINK_FULL_DUPLEX ; - eth_dev->data->dev_link.link_speed =vnic_dev_port_speed (enic->vdev); -return ret; +memset (&link,0 ,sizeof (link)); + link.link_status =enic_get_link_status (enic); + link.link_duplex =ETH_LINK_FULL_DUPLEX ; + link.link_speed =vnic_dev_port_speed (enic->vdev); + +return rte_eth_linkstatus_set (eth_dev, &link); } static void
Signature · 1 line
2.15.1
Quoted 23 lines · Stephen Hemminger
While writing hyper-v driver, noticed a lot of unnecessary duplication of code in drivers for handling the
eth_devlink status information. While consolidating this, it also became obvious that some drivers behave differently for no good reason.It also was a good chance to introduce atomic exchange primitives in EAL because there are other places using cmpset where not necessary (such as bonding).
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
v8
- fix typo in 32bit exchange
- fix build of dpaa2
- go back to same return value as original code
- reduce number of lines changedv7
- add exchange functions for PPC64
- move linkstatus helpers torte_ethdev_driver
- rebase to 18.02
Quoted 16 lines
Stephen Hemminger (15):
eal: introduce atomic exchange operation
ethdev: add linkstatus get/set helper functions
net/virtio: useeth_linkstatus_set
net/vmxnet3: userte_eth_linkstatus_set
net/dpaa2: userte_eth_linkstatus_set
net/nfp: userte_eth_linkstatusfunctions
net/e1000: userte_eth_linkstatushelpers
net/ixgbe: userte_eth_linkstatusfunctions
net/sfc: use newrte_eth_linkstatusfunctions
net/i40e: userte_eth_linkstatusfunctions
net/liquidio: userte_eth_linkstatus_set
net/thunderx: userte_eth_linkstatus_set
net/szedata: use_rte_eth_linkstatus_set
net/octeontx: userte_eth_linkstatus_set
net/enic: userte_eth_linkstatus_set
For series
Reviewed-by Ferruh Yigit <[email protected]>
Quoted 41 lines · Ferruh Yigit
Quoted 23 lines · Stephen Hemminger
While writing hyper-v driver, noticed a lot of unnecessary duplication of code in drivers for handling the
eth_devlink status information. While consolidating this, it also became obvious that some drivers behave differently for no good reason.It also was a good chance to introduce atomic exchange primitives in EAL because there are other places using cmpset where not necessary (such as bonding).
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
v8
- fix typo in 32bit exchange
- fix build of dpaa2
- go back to same return value as original code
- reduce number of lines changedv7
- add exchange functions for PPC64
- move linkstatus helpers torte_ethdev_driver
- rebase to 18.02Quoted 16 lines
Stephen Hemminger (15):
eal: introduce atomic exchange operation
ethdev: add linkstatus get/set helper functions
net/virtio: useeth_linkstatus_set
net/vmxnet3: userte_eth_linkstatus_set
net/dpaa2: userte_eth_linkstatus_set
net/nfp: userte_eth_linkstatusfunctions
net/e1000: userte_eth_linkstatushelpers
net/ixgbe: userte_eth_linkstatusfunctions
net/sfc: use newrte_eth_linkstatusfunctions
net/i40e: userte_eth_linkstatusfunctions
net/liquidio: userte_eth_linkstatus_set
net/thunderx: userte_eth_linkstatus_set
net/szedata: use_rte_eth_linkstatus_set
net/octeontx: userte_eth_linkstatus_set
net/enic: userte_eth_linkstatus_set
Quoted 2 lines
For series
Reviewed-by Ferruh Yigit <[email protected]>
Series applied to dpdk-next-net/master, thanks.
(Need to resolve some conflicts, specially in i40e and vmxnet3, please verify final patches.)