Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
On Fri, Apr 15, 2016 at 2:33 AM, Panu Matilainen <pmati...@redhat.com> wrote: > On 04/13/2016 07:21 PM, Traynor, Kevin wrote: >>> >>> -Original Message- >>> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Panu >>> Matilainen >>> Sent: Wednesday, April 13, 2016 8:50 AM >>> To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org >>> Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support >>> for >>> DPDK 16.04. >>> >> >> [snip] >> >>> As an aside, I've been thinking maybe this is a case where OVS could >>> support both DPDK 2.2 and 16.04. I know its unprecedented but maybe that >>> could change, restricting OVS to just one DPDK version seems >>> unnecessarily strict when talking about differences this trivial. >> >> >> Judging by the ML, it's more commonly requested to use the current release >> of DPDK with the last release of OVS e.g. OVS 2.5 and DPDK 16.04, than >> people >> wanting OVS master with DPDK X-1. > > > That's actually just different angle of the same thing: people have > different needs. If DPDK 16.04 support was pulled into OVS 2.5, what about > those who were happy with DPDK 2.2 and dont want to (or cant) move? > >> Even for a trivial case like above - it would be ok now to add support for >> DPDK X-1 >> but if we then add OVS code to take advantage of new DPDK X features (e.g. >> vhost >> pmd) we'll end up with messy code. Also testing efforts would increase >> (double?), >> so I don't think we would get it without a cost. > > > Nothing is free of course. Note that I'm not suggesting OVS gets forever > burdened with ever increasing trail of ancient DPDK versions. Just that when > its trivial to support more than one, it perhaps should. When the older > versions get in the way of progress, just axe the support as necessary. > Perhaps also rephrasing "supported" towards something like "recommended > version is X.Y, but X.Y-1 is also known to work" along the way. > > Not that I have strong opinions on this, I'm just expecting distros will end > up/be required to forward-/backport bits and pieces *anyway* so the trouble > of doing so might as well go upstream to benefit everybody. I would really like to see DPDK to continue to work towards having a stable API. That's the only real long term solution to this problem and I think our effort would be best spent on helping out with that where possible. I'd like to keep OVS clean in the meantime since this is hopefully a relatively short term issue. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
On 04/13/2016 07:21 PM, Traynor, Kevin wrote: -Original Message- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Panu Matilainen Sent: Wednesday, April 13, 2016 8:50 AM To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04. [snip] As an aside, I've been thinking maybe this is a case where OVS could support both DPDK 2.2 and 16.04. I know its unprecedented but maybe that could change, restricting OVS to just one DPDK version seems unnecessarily strict when talking about differences this trivial. Judging by the ML, it's more commonly requested to use the current release of DPDK with the last release of OVS e.g. OVS 2.5 and DPDK 16.04, than people wanting OVS master with DPDK X-1. That's actually just different angle of the same thing: people have different needs. If DPDK 16.04 support was pulled into OVS 2.5, what about those who were happy with DPDK 2.2 and dont want to (or cant) move? Even for a trivial case like above - it would be ok now to add support for DPDK X-1 but if we then add OVS code to take advantage of new DPDK X features (e.g. vhost pmd) we'll end up with messy code. Also testing efforts would increase (double?), so I don't think we would get it without a cost. Nothing is free of course. Note that I'm not suggesting OVS gets forever burdened with ever increasing trail of ancient DPDK versions. Just that when its trivial to support more than one, it perhaps should. When the older versions get in the way of progress, just axe the support as necessary. Perhaps also rephrasing "supported" towards something like "recommended version is X.Y, but X.Y-1 is also known to work" along the way. Not that I have strong opinions on this, I'm just expecting distros will end up/be required to forward-/backport bits and pieces *anyway* so the trouble of doing so might as well go upstream to benefit everybody. - Panu - ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
2016-04-13 9:21 GMT-07:00 Traynor, Kevin <kevin.tray...@intel.com>: > > -Original Message- > > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Panu > Matilainen > > Sent: Wednesday, April 13, 2016 8:50 AM > > To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support > for > > DPDK 16.04. > > > > [snip] > > > As an aside, I've been thinking maybe this is a case where OVS could > > support both DPDK 2.2 and 16.04. I know its unprecedented but maybe that > > could change, restricting OVS to just one DPDK version seems > > unnecessarily strict when talking about differences this trivial. > > Judging by the ML, it's more commonly requested to use the current release > of DPDK with the last release of OVS e.g. OVS 2.5 and DPDK 16.04, than > people > wanting OVS master with DPDK X-1. > > Even for a trivial case like above - it would be ok now to add support for > DPDK X-1 > but if we then add OVS code to take advantage of new DPDK X features (e.g. > vhost > pmd) we'll end up with messy code. Also testing efforts would increase > (double?), > so I don't think we would get it without a cost. > I agree, I'd still prefer to support a single version > > Kevin. > > > > > - Panu - > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
> -Original Message- > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Panu Matilainen > Sent: Wednesday, April 13, 2016 8:50 AM > To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for > DPDK 16.04. > [snip] > As an aside, I've been thinking maybe this is a case where OVS could > support both DPDK 2.2 and 16.04. I know its unprecedented but maybe that > could change, restricting OVS to just one DPDK version seems > unnecessarily strict when talking about differences this trivial. Judging by the ML, it's more commonly requested to use the current release of DPDK with the last release of OVS e.g. OVS 2.5 and DPDK 16.04, than people wanting OVS master with DPDK X-1. Even for a trivial case like above - it would be ok now to add support for DPDK X-1 but if we then add OVS code to take advantage of new DPDK X features (e.g. vhost pmd) we'll end up with messy code. Also testing efforts would increase (double?), so I don't think we would get it without a cost. Kevin. > > - Panu - > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
On 04/13/2016 11:38 AM, Panu Matilainen wrote:
On 04/13/2016 11:33 AM, Panu Matilainen wrote:
On 04/13/2016 11:16 AM, Weglicki, MichalX wrote:
-Original Message-
From: Panu Matilainen [mailto:pmati...@redhat.com]
Sent: Wednesday, April 13, 2016 8:50 AM
To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add
support for DPDK 16.04.
On 04/12/2016 05:05 PM, mweglicx wrote:
[...]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e09b471..2295e53 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev
*netdev_,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
+if (link.link_duplex == ETH_LINK_AUTONEG) {
This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
autonegotiation end up with half-duplex these days isn't that likely.
[MW] Thank you, your're right, just to be sure, you mean that I should
check autoneg through
link_autoneg flag, like this:
if (link.link_autoneg) {
*current = NETDEV_F_AUTONEG;
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_HD;
}
} else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_10G) {
*current = NETDEV_F_10GB_FD;
}
}
Based on the comments in header files this should work. Thanks in
advance.
Yup, that's what I meant. However turns out that isn't right either.
Looking at lib/netdev-linux.c, *current is a bitfield with current speed
set, AND if its autonegotiated then NETDEV_F_AUTONEG is *also* set. I
think this code was never quite correct to begin with.
To clarify, I think the DPDK code for this was never quite correct to
begin with.
So I had a quick look whether 2.5 and earlier need some kind of fix in
this area. AFAICS the deal is that the autoneg case is a "can't happen"
situation here: when initializing a port, value of zero indicates
"autonegotiate this, please" but the driver then sets the fields to the
negotiated value and there's no way to tell whether it was
autonegotiated or not after the fact.
In other words, this could be applied to OVS 2.5 (and older) and nothing
would actually change:
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1736,11 +1736,7 @@ netdev_dpdk_get_features(const struct netdev *netdev,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
-if (link.link_speed == ETH_LINK_SPEED_AUTONEG) {
-*current = NETDEV_F_AUTONEG;
-}
-} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
+if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
if (link.link_speed == ETH_LINK_SPEED_10) {
*current = NETDEV_F_10MB_HD;
}
But since its just a NOP and now fixed for newer versions... probably
not worth the trouble.
- Panu -
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
-Original Message-
From: Panu Matilainen [mailto:pmati...@redhat.com]
Sent: Wednesday, April 13, 2016 9:38 AM
To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for
DPDK 16.04.
On 04/13/2016 11:33 AM, Panu Matilainen wrote:
> On 04/13/2016 11:16 AM, Weglicki, MichalX wrote:
>> -Original Message-
>> From: Panu Matilainen [mailto:pmati...@redhat.com]
>> Sent: Wednesday, April 13, 2016 8:50 AM
>> To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add
>> support for DPDK 16.04.
>>
>> On 04/12/2016 05:05 PM, mweglicx wrote:
>> [...]
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index e09b471..2295e53 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev
>>> *netdev_,
>>>link = dev->link;
>>>ovs_mutex_unlock(>mutex);
>>>
>>> -if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
>>> +if (link.link_duplex == ETH_LINK_AUTONEG) {
>>
>> This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
>> ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
>> ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
>> autonegotiation end up with half-duplex these days isn't that likely.
>>
>> [MW] Thank you, your're right, just to be sure, you mean that I should
>> check autoneg through
>> link_autoneg flag, like this:
>> if (link.link_autoneg) {
>> *current = NETDEV_F_AUTONEG;
>> } else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
>> if (link.link_speed == ETH_SPEED_NUM_10M) {
>> *current = NETDEV_F_10MB_HD;
>> }
>> if (link.link_speed == ETH_SPEED_NUM_100M) {
>> *current = NETDEV_F_100MB_HD;
>> }
>> if (link.link_speed == ETH_SPEED_NUM_1G) {
>> *current = NETDEV_F_1GB_HD;
>> }
>> } else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
>> if (link.link_speed == ETH_SPEED_NUM_10M) {
>> *current = NETDEV_F_10MB_FD;
>> }
>> if (link.link_speed == ETH_SPEED_NUM_100M) {
>> *current = NETDEV_F_100MB_FD;
>> }
>> if (link.link_speed == ETH_SPEED_NUM_1G) {
>> *current = NETDEV_F_1GB_FD;
>> }
>> if (link.link_speed == ETH_SPEED_NUM_10G) {
>> *current = NETDEV_F_10GB_FD;
>> }
>> }
>>
>> Based on the comments in header files this should work. Thanks in
>> advance.
>
> Yup, that's what I meant. However turns out that isn't right either.
>
> Looking at lib/netdev-linux.c, *current is a bitfield with current speed
> set, AND if its autonegotiated then NETDEV_F_AUTONEG is *also* set. I
> think this code was never quite correct to begin with.
To clarify, I think the DPDK code for this was never quite correct to
begin with.
- Panu -
Yes, you are right, I double checked netdev-linux and this is exactly how it is
implemented there.
Ok, I will apply v2 shortly, thank you.
Br,
Michal.
--
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
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.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
On 04/13/2016 11:33 AM, Panu Matilainen wrote:
On 04/13/2016 11:16 AM, Weglicki, MichalX wrote:
-Original Message-
From: Panu Matilainen [mailto:pmati...@redhat.com]
Sent: Wednesday, April 13, 2016 8:50 AM
To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add
support for DPDK 16.04.
On 04/12/2016 05:05 PM, mweglicx wrote:
[...]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e09b471..2295e53 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev
*netdev_,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
+if (link.link_duplex == ETH_LINK_AUTONEG) {
This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
autonegotiation end up with half-duplex these days isn't that likely.
[MW] Thank you, your're right, just to be sure, you mean that I should
check autoneg through
link_autoneg flag, like this:
if (link.link_autoneg) {
*current = NETDEV_F_AUTONEG;
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_HD;
}
} else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_10G) {
*current = NETDEV_F_10GB_FD;
}
}
Based on the comments in header files this should work. Thanks in
advance.
Yup, that's what I meant. However turns out that isn't right either.
Looking at lib/netdev-linux.c, *current is a bitfield with current speed
set, AND if its autonegotiated then NETDEV_F_AUTONEG is *also* set. I
think this code was never quite correct to begin with.
To clarify, I think the DPDK code for this was never quite correct to
begin with.
- Panu -
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
On 04/13/2016 11:16 AM, Weglicki, MichalX wrote:
-Original Message-
From: Panu Matilainen [mailto:pmati...@redhat.com]
Sent: Wednesday, April 13, 2016 8:50 AM
To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for
DPDK 16.04.
On 04/12/2016 05:05 PM, mweglicx wrote:
[...]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e09b471..2295e53 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev *netdev_,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
+if (link.link_duplex == ETH_LINK_AUTONEG) {
This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
autonegotiation end up with half-duplex these days isn't that likely.
[MW] Thank you, your're right, just to be sure, you mean that I should check
autoneg through
link_autoneg flag, like this:
if (link.link_autoneg) {
*current = NETDEV_F_AUTONEG;
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_HD;
}
} else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_10G) {
*current = NETDEV_F_10GB_FD;
}
}
Based on the comments in header files this should work. Thanks in advance.
Yup, that's what I meant. However turns out that isn't right either.
Looking at lib/netdev-linux.c, *current is a bitfield with current speed
set, AND if its autonegotiated then NETDEV_F_AUTONEG is *also* set. I
think this code was never quite correct to begin with.
So what the whole thing *really* should be is:
if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_HD;
}
} else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_10G) {
*current = NETDEV_F_10GB_FD;
}
}
if (link.autoneg == ETH_LINK_AUTONEG)
*current |= NETDEV_F_AUTONEG;
- Panu -
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
-Original Message-
From: Panu Matilainen [mailto:pmati...@redhat.com]
Sent: Wednesday, April 13, 2016 8:50 AM
To: Weglicki, MichalX <michalx.wegli...@intel.com>; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for
DPDK 16.04.
On 04/12/2016 05:05 PM, mweglicx wrote:
[...]
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e09b471..2295e53 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev *netdev_,
> link = dev->link;
> ovs_mutex_unlock(>mutex);
>
> -if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
> +if (link.link_duplex == ETH_LINK_AUTONEG) {
This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
autonegotiation end up with half-duplex these days isn't that likely.
[MW] Thank you, your're right, just to be sure, you mean that I should check
autoneg through
link_autoneg flag, like this:
if (link.link_autoneg) {
*current = NETDEV_F_AUTONEG;
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_HD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_HD;
}
} else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_FD;
}
if (link.link_speed == ETH_SPEED_NUM_10G) {
*current = NETDEV_F_10GB_FD;
}
}
Based on the comments in header files this should work. Thanks in advance.
> if (link.link_speed == ETH_LINK_SPEED_AUTONEG) {
> *current = NETDEV_F_AUTONEG;
> }
Additionally link_speed in DPDK 16.04 reflects the actual negotiated
speed and is never ETH_LINK_SPEED_AUTONEG, which is a bitmap flag
relevant to link_speeds field.
The autoneg case should be something like this:
@@ -1573,31 +1573,29 @@ netdev_dpdk_get_features(const struct netdev
*netdev_,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
-if (link.link_speed == ETH_LINK_SPEED_AUTONEG) {
-*current = NETDEV_F_AUTONEG;
-}
+if (link.link_autoneg) {
+ *current = NETDEV_F_AUTONEG;
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
> } else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
> -if (link.link_speed == ETH_LINK_SPEED_10) {
> +if (link.link_speed == ETH_SPEED_NUM_10M) {
> *current = NETDEV_F_10MB_HD;
> }
> -if (link.link_speed == ETH_LINK_SPEED_100) {
> +if (link.link_speed == ETH_SPEED_NUM_100M) {
> *current = NETDEV_F_100MB_HD;
> }
> -if (link.link_speed == ETH_LINK_SPEED_1000) {
> +if (link.link_speed == ETH_SPEED_NUM_1G) {
> *current = NETDEV_F_1GB_HD;
> }
> } else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
> -if (link.link_speed == ETH_LINK_SPEED_10) {
> +if (link.link_speed == ETH_SPEED_NUM_10M) {
> *current = NETDEV_F_10MB_FD;
> }
> -if (link.link_speed == ETH_LINK_SPEED_100) {
> +if (link.link_speed == ETH_SPEED_NUM_100M) {
> *current = NETDEV_F_100MB_FD;
> }
> -if (link.link_speed == ETH_LINK_SPEED_1000) {
> +if (link.link_speed == ETH_SPEED_NUM_1G) {
> *current = NETDEV_F_1GB_FD;
> }
> -if (link.link_speed == ETH_LINK_SPEED_1) {
> +if (link.link_speed == ETH_SPEED_NUM_10G) {
> *current = NETDEV_F_10GB_FD;
> }
> }
The rest looks ok.
As an aside, I've been thinking maybe this is a case where OVS could
support both DPDK 2.2 and 16.04. I know its unprecedented but maybe that
could change, restricting OVS to just one DPDK version seems
unnecessarily strict when talking about differences this trivial.
- Panu -
--
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
This e-mail and any attachments may contain confidential material for the sole
use of the intended
Re: [ovs-dev] [PATCH] Update relevant artifacts to add support for DPDK 16.04.
On 04/12/2016 05:05 PM, mweglicx wrote:
[...]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e09b471..2295e53 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1740,31 +1740,31 @@ netdev_dpdk_get_features(const struct netdev *netdev_,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
+if (link.link_duplex == ETH_LINK_AUTONEG) {
This isn't right, link_duplex is either ETH_LINK_HALF_DUPLEX or
ETH_LINK_FULL_DUPLEX. It sort of happens to work because both
ETH_LINK_AUTONEG and ETH_LINK_FULL_DUPLEX are defined to 1 and having
autonegotiation end up with half-duplex these days isn't that likely.
if (link.link_speed == ETH_LINK_SPEED_AUTONEG) {
*current = NETDEV_F_AUTONEG;
}
Additionally link_speed in DPDK 16.04 reflects the actual negotiated
speed and is never ETH_LINK_SPEED_AUTONEG, which is a bitmap flag
relevant to link_speeds field.
The autoneg case should be something like this:
@@ -1573,31 +1573,29 @@ netdev_dpdk_get_features(const struct netdev
*netdev_,
link = dev->link;
ovs_mutex_unlock(>mutex);
-if (link.link_duplex == ETH_LINK_AUTONEG_DUPLEX) {
-if (link.link_speed == ETH_LINK_SPEED_AUTONEG) {
-*current = NETDEV_F_AUTONEG;
-}
+if (link.link_autoneg) {
+ *current = NETDEV_F_AUTONEG;
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
} else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
-if (link.link_speed == ETH_LINK_SPEED_10) {
+if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_HD;
}
-if (link.link_speed == ETH_LINK_SPEED_100) {
+if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_HD;
}
-if (link.link_speed == ETH_LINK_SPEED_1000) {
+if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_HD;
}
} else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
-if (link.link_speed == ETH_LINK_SPEED_10) {
+if (link.link_speed == ETH_SPEED_NUM_10M) {
*current = NETDEV_F_10MB_FD;
}
-if (link.link_speed == ETH_LINK_SPEED_100) {
+if (link.link_speed == ETH_SPEED_NUM_100M) {
*current = NETDEV_F_100MB_FD;
}
-if (link.link_speed == ETH_LINK_SPEED_1000) {
+if (link.link_speed == ETH_SPEED_NUM_1G) {
*current = NETDEV_F_1GB_FD;
}
-if (link.link_speed == ETH_LINK_SPEED_1) {
+if (link.link_speed == ETH_SPEED_NUM_10G) {
*current = NETDEV_F_10GB_FD;
}
}
The rest looks ok.
As an aside, I've been thinking maybe this is a case where OVS could
support both DPDK 2.2 and 16.04. I know its unprecedented but maybe that
could change, restricting OVS to just one DPDK version seems
unnecessarily strict when talking about differences this trivial.
- Panu -
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
