[ovs-dev,v4] netdev-dpdk: Bring link down when NETDEV_UP is not set

Message ID 20181109122432.99492.11001.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com
State New
Delegated to: Ian Stokes
Headers show
Series
  • [ovs-dev,v4] netdev-dpdk: Bring link down when NETDEV_UP is not set
Related show

Commit Message

Eelco Chaudron Nov. 9, 2018, 12:24 p.m.
When the netdev link flags are changed, !NETDEV_UP, the DPDK ports are not
actually going down. This is causing problems for people trying to bring
down a bond member. The bond link is no longer being used to receive or
transmit traffic, however, the other end keeps sending data as the link
remains up.

With OVS 2.6 the link was brought down, and this was changed with commit
3b1fb0779. In this commit, it's explicitly mentioned that the link down/up
DPDK APIs are not called as not all PMD devices support it.

However, this patch does call the appropriate DPDK APIs and ignoring
errors due to the PMD not supporting it. PMDs not supporting this should
be fixed in DPDK upstream.

I verified this patch is working correctly using the
ovs-appctl netdev-dpdk/set-admin-state <port> {up|down} and
ovs-ofctl mod-port <bridge> <port> {up|down} commands on a XL710
and 82599ES.

Fixes: 3b1fb0779b87 ("netdev-dpdk: Don't call rte_dev_stop() in update_flags().")
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

---
* V4:
  - Use netdev_get_name() instead of dev->up.name directly

* V3:
  - Update log message and changed the level to INFO

* V2:
  - Only call link state change function if the upstate changed
  - Restore original flags on error
  - Log a message when the NIC does not support the link state API

 lib/netdev-dpdk.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Ilya Maximets Nov. 9, 2018, 12:36 p.m. | #1
On 09.11.2018 15:24, Eelco Chaudron wrote:
> When the netdev link flags are changed, !NETDEV_UP, the DPDK ports are not
> actually going down. This is causing problems for people trying to bring
> down a bond member. The bond link is no longer being used to receive or
> transmit traffic, however, the other end keeps sending data as the link
> remains up.
> 
> With OVS 2.6 the link was brought down, and this was changed with commit
> 3b1fb0779. In this commit, it's explicitly mentioned that the link down/up
> DPDK APIs are not called as not all PMD devices support it.
> 
> However, this patch does call the appropriate DPDK APIs and ignoring
> errors due to the PMD not supporting it. PMDs not supporting this should
> be fixed in DPDK upstream.
> 
> I verified this patch is working correctly using the
> ovs-appctl netdev-dpdk/set-admin-state <port> {up|down} and
> ovs-ofctl mod-port <bridge> <port> {up|down} commands on a XL710
> and 82599ES.
> 
> Fixes: 3b1fb0779b87 ("netdev-dpdk: Don't call rte_dev_stop() in update_flags().")
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> 
> ---
> * V4:
>   - Use netdev_get_name() instead of dev->up.name directly
> 
> * V3:
>   - Update log message and changed the level to INFO
> 
> * V2:
>   - Only call link state change function if the upstate changed
>   - Restore original flags on error
>   - Log a message when the NIC does not support the link state API
> 
>  lib/netdev-dpdk.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Thanks,
Acked-by: Ilya Maximets <i.maximets@samsung.com>
Flavio Leitner Nov. 9, 2018, 4:19 p.m. | #2
On Fri, Nov 09, 2018 at 07:24:55AM -0500, Eelco Chaudron wrote:
> When the netdev link flags are changed, !NETDEV_UP, the DPDK ports are not
> actually going down. This is causing problems for people trying to bring
> down a bond member. The bond link is no longer being used to receive or
> transmit traffic, however, the other end keeps sending data as the link
> remains up.
> 
> With OVS 2.6 the link was brought down, and this was changed with commit
> 3b1fb0779. In this commit, it's explicitly mentioned that the link down/up
> DPDK APIs are not called as not all PMD devices support it.
> 
> However, this patch does call the appropriate DPDK APIs and ignoring
> errors due to the PMD not supporting it. PMDs not supporting this should
> be fixed in DPDK upstream.
> 
> I verified this patch is working correctly using the
> ovs-appctl netdev-dpdk/set-admin-state <port> {up|down} and
> ovs-ofctl mod-port <bridge> <port> {up|down} commands on a XL710
> and 82599ES.
> 
> Fixes: 3b1fb0779b87 ("netdev-dpdk: Don't call rte_dev_stop() in update_flags().")
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> 
> ---
> * V4:
>   - Use netdev_get_name() instead of dev->up.name directly
> 
> * V3:
>   - Update log message and changed the level to INFO
> 
> * V2:
>   - Only call link state change function if the upstate changed
>   - Restore original flags on error
>   - Log a message when the NIC does not support the link state API
> 
>  lib/netdev-dpdk.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 7e0a593..00f5acc 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2942,6 +2942,24 @@ netdev_dpdk_update_flags__(struct netdev_dpdk *dev,
>      }
>  
>      if (dev->type == DPDK_DEV_ETH) {
> +
> +        if ((dev->flags ^ *old_flagsp) & NETDEV_UP) {
> +            int err;
> +
> +            if (dev->flags & NETDEV_UP) {
> +                err = rte_eth_dev_set_link_up(dev->port_id);
> +            } else {
> +                err = rte_eth_dev_set_link_down(dev->port_id);
> +            }
> +            if (err == -ENOTSUP) {
> +                VLOG_INFO("Interface %s does not support link state "
> +                          "configuration", netdev_get_name(&dev->up));
> +            } else if (err < 0) {

The netdev_dpdk_set_admin_state__() ignores the return value so we
won't know if the there is a problem or not.
Maybe worth to fix the caller or add a msg here too?
(sorry to come up late)
fbl


> +                dev->flags = *old_flagsp;
> +                return -err;
> +            }
> +        }
> +
>          if (dev->flags & NETDEV_PROMISC) {
>              rte_eth_promiscuous_enable(dev->port_id);
>          }
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Nov. 12, 2018, 9:27 a.m. | #3
On 9 Nov 2018, at 17:19, Flavio Leitner wrote:

> On Fri, Nov 09, 2018 at 07:24:55AM -0500, Eelco Chaudron wrote:
>> When the netdev link flags are changed, !NETDEV_UP, the DPDK ports 
>> are not
>> actually going down. This is causing problems for people trying to 
>> bring
>> down a bond member. The bond link is no longer being used to receive 
>> or
>> transmit traffic, however, the other end keeps sending data as the 
>> link
>> remains up.
>>
>> With OVS 2.6 the link was brought down, and this was changed with 
>> commit
>> 3b1fb0779. In this commit, it's explicitly mentioned that the link 
>> down/up
>> DPDK APIs are not called as not all PMD devices support it.
>>
>> However, this patch does call the appropriate DPDK APIs and ignoring
>> errors due to the PMD not supporting it. PMDs not supporting this 
>> should
>> be fixed in DPDK upstream.
>>
>> I verified this patch is working correctly using the
>> ovs-appctl netdev-dpdk/set-admin-state <port> {up|down} and
>> ovs-ofctl mod-port <bridge> <port> {up|down} commands on a XL710
>> and 82599ES.
>>
>> Fixes: 3b1fb0779b87 ("netdev-dpdk: Don't call rte_dev_stop() in 
>> update_flags().")
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>
>> ---
>> * V4:
>>   - Use netdev_get_name() instead of dev->up.name directly
>>
>> * V3:
>>   - Update log message and changed the level to INFO
>>
>> * V2:
>>   - Only call link state change function if the upstate changed
>>   - Restore original flags on error
>>   - Log a message when the NIC does not support the link state API
>>
>>  lib/netdev-dpdk.c |   18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 7e0a593..00f5acc 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -2942,6 +2942,24 @@ netdev_dpdk_update_flags__(struct netdev_dpdk 
>> *dev,
>>      }
>>
>>      if (dev->type == DPDK_DEV_ETH) {
>> +
>> +        if ((dev->flags ^ *old_flagsp) & NETDEV_UP) {
>> +            int err;
>> +
>> +            if (dev->flags & NETDEV_UP) {
>> +                err = rte_eth_dev_set_link_up(dev->port_id);
>> +            } else {
>> +                err = rte_eth_dev_set_link_down(dev->port_id);
>> +            }
>> +            if (err == -ENOTSUP) {
>> +                VLOG_INFO("Interface %s does not support link state 
>> "
>> +                          "configuration", 
>> netdev_get_name(&dev->up));
>> +            } else if (err < 0) {
>
> The netdev_dpdk_set_admin_state__() ignores the return value so we
> won't know if the there is a problem or not.
> Maybe worth to fix the caller or add a msg here too?
> (sorry to come up late)
> fbl

Send out a v5, adding a log message

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 7e0a593..00f5acc 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2942,6 +2942,24 @@  netdev_dpdk_update_flags__(struct netdev_dpdk *dev,
     }
 
     if (dev->type == DPDK_DEV_ETH) {
+
+        if ((dev->flags ^ *old_flagsp) & NETDEV_UP) {
+            int err;
+
+            if (dev->flags & NETDEV_UP) {
+                err = rte_eth_dev_set_link_up(dev->port_id);
+            } else {
+                err = rte_eth_dev_set_link_down(dev->port_id);
+            }
+            if (err == -ENOTSUP) {
+                VLOG_INFO("Interface %s does not support link state "
+                          "configuration", netdev_get_name(&dev->up));
+            } else if (err < 0) {
+                dev->flags = *old_flagsp;
+                return -err;
+            }
+        }
+
         if (dev->flags & NETDEV_PROMISC) {
             rte_eth_promiscuous_enable(dev->port_id);
         }