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

Eelco Chaudron echaudro at redhat.com
Fri Nov 9 08:09:33 UTC 2018



On 8 Nov 2018, at 16:09, Stokes, Ian wrote:

>> 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?
>
> Info would be fine with me, I'd just like it to be apparent to a user 
> when reading the logs. We do something similar for when 
> rte_eth_dev_set_mtu() is not supported, although that is a rarer 
> situation and in that case a warning is ok.

I did base the LOG level and message on the MTU one. Will send out a V3 
as Info.

>
>>
>>>> +                          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);
>>>>          }
>>>>
>>>>
>>>>


More information about the dev mailing list