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

[Eelco Chaudron]

On 8 Nov 2018, at 15:34, Ilya Maximets wrote:

> On 08.11.2018 16:58, 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>
>>
>> ---
>> * 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 |   19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 7e0a593..382e287 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -2942,6 +2942,25 @@ 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_WARN("Interface %s does not support link state 
>> "
>> +                          "configuration, physical link will always 
>> be up.",
>
> This message is not fully correct. ENOTSUP means that we can't change 
> the state
> from the OVS side, but it could be changed physically by disconnecting 
> the cable
> or setting down the other side. IMHO, need to rephrase somehow. Maybe 
> just drop
> the second part about physical link?

I see the confusion here… I’ll drop the second part in the next 
re-spin.

> Also, as Ian said, ENOTSUP is a common case, maybe we can reduce log 
> level to INFO?
> Maybe even DBG to save some space in logs.

This message should not be so frequent, and I think the actual number of 
PMDs is not that big (mainly the virtual ones). But I’m fine changing 
it to INFO or DBG, Ian?

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