| Message ID | 20181109081951.107033.19797.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev,v3] netdev-dpdk: Bring link down when NETDEV_UP is not set | expand |
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@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. > + } else if (err < 0) { > + dev->flags = *old_flagsp; > + return -err; > + } > + } > + > if (dev->flags & NETDEV_PROMISC) { > rte_eth_promiscuous_enable(dev->port_id); > } > > >
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@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? >> + } else if (err < 0) { >> + dev->flags = *old_flagsp; >> + return -err; >> + } >> + } >> + >> if (dev->flags & NETDEV_PROMISC) { >> rte_eth_promiscuous_enable(dev->port_id); >> } >> >> >>
> 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@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 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@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? > > Thanks > 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@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
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); + } else if (err < 0) { + dev->flags = *old_flagsp; + return -err; + } + } + if (dev->flags & NETDEV_PROMISC) { rte_eth_promiscuous_enable(dev->port_id); }
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@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(+)