[dpdk-dev,RFC,02/14] virtio: use eth_link_read/write (and bug fix)
Checks
Commit Message
Use the new code in ethdev to handle link status update.
Also, virtio was not correctly setting the autoneg flags
since its speed should be marked as fixed.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/virtio/virtio_ethdev.c | 54 +++++---------------------------------
1 file changed, 6 insertions(+), 48 deletions(-)
Comments
On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> Use the new code in ethdev to handle link status update.
> Also, virtio was not correctly setting the autoneg flags
> since its speed should be marked as fixed.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 54 +++++---------------------------------
> 1 file changed, 6 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00a3122780ba..776ad4961a37 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -43,7 +43,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_ether.h>
> @@ -794,46 +793,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)
> {
> @@ -1829,20 +1788,20 @@ 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_link_update(dev, &link);
> }
>
> static int
> virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
> {
> - struct rte_eth_link link, old;
> - uint16_t status;
> struct virtio_hw *hw = dev->data->dev_private;
> + struct rte_eth_link link;
> + uint16_t status;
> +
> 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 = SPEED_10G;
> + link.link_autoneg = ETH_LINK_SPEED_FIXED;
As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
It looks like it has wrong comment:
uint16_t link_autoneg : 1; /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
since
#define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */
#define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed speed) */
whereas
#define ETH_LINK_FIXED 0 /**< No autonegotiation. */
#define ETH_LINK_AUTONEG 1 /**< Autonegotiated. */
In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
defines with opposite values.
> if (hw->started == 0) {
> link.link_status = ETH_LINK_DOWN;
> @@ -1863,9 +1822,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_link_update(dev, &link);
> }
>
> static void
On Sun, 16 Jul 2017 15:33:26 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> > + link.link_autoneg = ETH_LINK_SPEED_FIXED;
>
> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
> It looks like it has wrong comment:
> uint16_t link_autoneg : 1; /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
>
> since
> #define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */
> #define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed speed) */
>
> whereas
> #define ETH_LINK_FIXED 0 /**< No autonegotiation. */
> #define ETH_LINK_AUTONEG 1 /**< Autonegotiated. */
>
> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
> defines with opposite values.
Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
and others were not. Turns out it makes no difference
since FIXED == 0, the old code and new code have same effect.
On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> On Sun, 16 Jul 2017 15:33:26 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>>> + link.link_autoneg = ETH_LINK_SPEED_FIXED;
>> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
>> It looks like it has wrong comment:
>> uint16_t link_autoneg : 1; /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
>>
>> since
>> #define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */
>> #define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed speed) */
>>
>> whereas
>> #define ETH_LINK_FIXED 0 /**< No autonegotiation. */
>> #define ETH_LINK_AUTONEG 1 /**< Autonegotiated. */
>>
>> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
>> defines with opposite values.
> Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
> and others were not. Turns out it makes no difference
> since FIXED == 0, the old code and new code have same effect.
May be I miss something, but ETH_LINK_SPEED_FIXED==1, but
ETH_LINK_FIXED==0.
So, initially it was 0 (fixed speed), but now it is 1 (autoneg).
On Mon, 17 Jul 2017 19:14:16 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> > On Sun, 16 Jul 2017 15:33:26 +0300
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >
> >>> + link.link_autoneg = ETH_LINK_SPEED_FIXED;
> >> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
> >> It looks like it has wrong comment:
> >> uint16_t link_autoneg : 1; /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
> >>
> >> since
> >> #define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */
> >> #define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed speed) */
> >>
> >> whereas
> >> #define ETH_LINK_FIXED 0 /**< No autonegotiation. */
> >> #define ETH_LINK_AUTONEG 1 /**< Autonegotiated. */
> >>
> >> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
> >> defines with opposite values.
> > Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
> > and others were not. Turns out it makes no difference
> > since FIXED == 0, the old code and new code have same effect.
>
> May be I miss something, but ETH_LINK_SPEED_FIXED==1, but
> ETH_LINK_FIXED==0.
> So, initially it was 0 (fixed speed), but now it is 1 (autoneg).
My understanding is that ETH_SPEED_XXX and ETH_LINK_FIXED are for rte_eth_link
structure; and ETH_LINK_SPEED_XXX values are for dev_info->speed_capa
Would be good to use some kind of typedef for this, maybe introduce a bitfield
for speed_capa and get rid of the ETH_LINK_SPEED_XXX.
17/07/2017 18:28, Stephen Hemminger:
> On Mon, 17 Jul 2017 19:14:16 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
> > On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> > > On Sun, 16 Jul 2017 15:33:26 +0300
> > > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> > >
> > >>> + link.link_autoneg = ETH_LINK_SPEED_FIXED;
> > >> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
> > >> It looks like it has wrong comment:
> > >> uint16_t link_autoneg : 1; /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
> > >>
> > >> since
> > >> #define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) */
> > >> #define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed speed) */
> > >>
> > >> whereas
> > >> #define ETH_LINK_FIXED 0 /**< No autonegotiation. */
> > >> #define ETH_LINK_AUTONEG 1 /**< Autonegotiated. */
> > >>
> > >> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
> > >> defines with opposite values.
> > > Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
> > > and others were not. Turns out it makes no difference
> > > since FIXED == 0, the old code and new code have same effect.
> >
> > May be I miss something, but ETH_LINK_SPEED_FIXED==1, but
> > ETH_LINK_FIXED==0.
> > So, initially it was 0 (fixed speed), but now it is 1 (autoneg).
>
> My understanding is that ETH_SPEED_XXX and ETH_LINK_FIXED are for rte_eth_link
> structure; and ETH_LINK_SPEED_XXX values are for dev_info->speed_capa
Right.
The comment is wrong.
rte_eth_link.link_autoneg must uses ETH_LINK_[FIXED/AUTONEG]
ETH_LINK_SPEED_[AUTONEG/FIXED] is for rte_eth_conf.link_speeds
and for rte_eth_dev_info.speed_capa.
So in this patch, you should set link.link_autoneg = ETH_LINK_FIXED.
I will send a patch to fix the comment in ethdev.
@@ -43,7 +43,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_ether.h>
@@ -794,46 +793,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)
{
@@ -1829,20 +1788,20 @@ 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_link_update(dev, &link);
}
static int
virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
{
- struct rte_eth_link link, old;
- uint16_t status;
struct virtio_hw *hw = dev->data->dev_private;
+ struct rte_eth_link link;
+ uint16_t status;
+
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 = SPEED_10G;
+ link.link_autoneg = ETH_LINK_SPEED_FIXED;
if (hw->started == 0) {
link.link_status = ETH_LINK_DOWN;
@@ -1863,9 +1822,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_link_update(dev, &link);
}
static void