> 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 <[email protected]>
> >>
> >> ---
> >> * 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.
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);
> >> }
> >>
> >>
> >>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
