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

Stokes, Ian ian.stokes at intel.com
Wed Nov 7 19:59:05 UTC 2018


> On 7 Nov 2018, at 15:04, Stokes, Ian 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>
> >>> ---
> >>>  lib/netdev-dpdk.c |   16 ++++++++++++++++
> >>>  1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>> 7e0a593..ee8296b 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -2942,10 +2942,26 @@ netdev_dpdk_update_flags__(struct
> >>> netdev_dpdk
> >> *dev,
> >>>      }
> >>>
> >>>      if (dev->type == DPDK_DEV_ETH) {
> >>> +        int err;
> >>> +
> >>> +        if (dev->flags & NETDEV_UP) {
> >>> +            err = rte_eth_dev_set_link_up(dev->port_id);
> >>> +            if (err < 0 && err != -ENOTSUP) {
> >>> +                return -err;
> >>
> >> Should we restore the flags before returning the error?
> >> Otherwise on the next equal call we'll return 0 without any real
> >> changes.
> >>
> 
> Yes I agree on error we should do dev->flags = *old_flags
> 
> > Yes, as well as that I think we should flag when the operation is not
> > supported and handle it accordingly.
> 
> I think if ENOTSUP is returned we should silently ignore the physical
> down, as from 2.7 till now it was not implemented. Clearing NETDEV_UP in
> this case still cause it to be administratively down (just external
> entities might not know due to the link staying up). Or do you suggest to
> log it as a warning that the physical link up/down is not supported by the
> PMD?

I think a warning would be welcome along with the state the device will remain in.

Ian


More information about the dev mailing list