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

Eelco Chaudron echaudro at redhat.com
Fri Nov 9 11:40:40 UTC 2018



On 9 Nov 2018, at 12:16, Ilya Maximets wrote:

> On 09.11.2018 11:20, 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 at redhat.com>
>>
>> ---
>> * 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(+)
>
> Looks OK in general. One small comment inline.
>
>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 7e0a593..bfab857 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", dev->up.name);
>
> I think, we prefer 'netdev_get_name(&dev->up)' for a new code.

Thanks Ilya, I’ll sent out a v4 ;) Ian anything else before I sent it?


>> +            } else if (err < 0) {
>> +                dev->flags = *old_flagsp;
>> +                return -err;
>> +            }
>> +        }
>> +
>>          if (dev->flags & NETDEV_PROMISC) {
>>              rte_eth_promiscuous_enable(dev->port_id);
>>          }
>>
>>
>>


More information about the dev mailing list