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

[Stokes, Ian]

> On 9 Nov 2018, at 13:07, Stokes, Ian wrote:
> 
> >> 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?
> >>
> >
> > LGTM, will be part of today's pull request.
> 
> Thanks I’ve sent out a V4, can you pull this for older versions also?

Sure, will add to 2.10 -> 2.7.

Ian
> >
> > Thanks
> > Ian