[ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control at netdev-init.

Chandran, Sugesh sugesh.chandran at intel.com
Fri Jul 20 22:26:02 UTC 2018



Regards
_Sugesh


> -----Original Message-----
> From: Stokes, Ian
> Sent: Friday, July 20, 2018 8:41 PM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>; ovs-
> dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control
> at netdev-init.
> 
> On 7/20/2018 7:24 PM, Chandran, Sugesh wrote:
> > Hi Ian,
> >
> > Thank you for testing it on VF. Please find my comments below,
> >
> > Regards
> > _Sugesh
> >
> >> -----Original Message-----
> >> From: Stokes, Ian
> >> Sent: Friday, July 20, 2018 6:30 PM
> >> To: Chandran, Sugesh <sugesh.chandran at intel.com>; ovs-
> >> dev at openvswitch.org
> >> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure
> >> flow control at netdev-init.
> >>
> >> On 7/13/2018 6:13 PM, Ian Stokes wrote:
> >>> On 7/10/2018 2:23 PM, Sugesh Chandran wrote:
> >>>> Configuring flow control at ixgbe netdev-init is throwing error in
> >>>> port start.
> >>>>
> >>>> For eg: without this fix, user cannot configure flow control on
> >>>> ixgbe dpdk port as below,
> >>>>
> >>>> "
> >>>>       ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> >>>> \
> >>>>           options:dpdk-devargs=0000:05:00.1
> >>>> options:rx-flow-ctrl=true "
> >>>>
> >>>> Instead,  it must be configured as two different commands,
> >>>>
> >>>> "
> >>>>       ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> >>>> \
> >>>>                  options:dpdk-devargs=0000:05:00.1
> >>>>       ovs-vsctl set Interface dpdk0 options:rx-flow-ctrl=true "
> >>>>
> >>>> The DPDK ixgbe driver is now validating all the 'rte_eth_fc_conf'
> >>>> fields before
> >>>> trying to configuring the dpdk ethdev. Hence OVS can no longer set
> >>>> the 'dont care' fields to just '0' as before. This commit make sure
> >>>> all the 'rte_eth_fc_conf' fields are populated with default values
> >>>> before the dev init.
> >>>>
> >>>
> >>> Thanks for the patch Sugesh, I've applied to dpdk_merge and it will
> >>> be part of this weeks pull request.
> >>>
> >>> I assume it should be backported to OVS 2.9 at least, do you know if
> >>> it should go to 2.8/2.7 also?
> >>
> >> Hi Sugesh,
> >>
> >> during further testing I found this breaks functionality for Virtual
> >> functions with OVS (specifically tested with igb_vf, i40e_vf and ixgbe_vf).
> >>
> >> The port itself will fail to initialize with the following error:
> >>
> >> netdev_dpdk|INFO|cannot get flow control parameters on port 2,
> >> err=-95
> >>
> >> And is not added. as such I think flow control should only be
> >> optional as there is no guarantee it will be available for a given
> >> dpdk device and if unavailable it should not stop the port from being
> initialized. > [Sugesh] I am not sure if we need to add this condition in OVS.
> > As a virtual switch, OVS should able to use these APIs irrespective of NIC/port
> types.
> > Actually we are masking this error , without the patch. It is possible
> > to hit this issue, when we configure the flow control on a VF port.
> 
> Ok, but now we have a situation that when adding a VF port, even without
> specifying the flow control option, it fails as now flow control parameters are
> being passed on init irregardless of whether the user intends to use it or not.
> 
> Now that I think of it I think this would probably break other port types as well
> that don't support flow control (such as vdevs).
> 
> > Adding a condition in OVS to check a VF port for similar parameters
> > doesn’t looks like a clean approach? What do you think?
> 
> We've already had to do this for a few items (HW CRC enablement, LSC, MTU
> support). It's not clean but until DPDK provides a way of checking capabilities
> before init it's the best we can do.
[Sugesh] We may need to feed this back to DPDK to see if we can get a standard 
way to know the capabilities.
The flow control is slightly different than other parameters that mentioned above.
It involved setting multiple parameters to enable . OVS doesn’t really configure
all of those parameters .Instead  It just only enable/disable the feature when user
is requested. Remaining parameters are left to its defaults(which is not zero).
So OVS must first query the flow control before trying to configure it.
Now the query will fail for unsupported hardware, and the error message will be reported as of now.
 AFAIK DPDK doesn’t have any way to check if a port can support flow control/not, so we cannot avoid the error.
But what we can do here is, Instead of calling the 'get' blindly,
limit it to call only when user trying to configure flow control in the system.
That will avoid the error in normal scenario.
Does it make sense?
I will send out the patch with those modifications.
> 
> >
> > This leads to another question , whats the impact of modifying other netdev
> parameters
> >   on a VF port. Does it error out/silently fail/work as normal netdev ports?
> 
> I think we can't fail completely based on an optional capability. And this is not
> just for VF ports, conceivably you can have HW ports that don't support a
> particular feature either supported by a netdev.
> 
> For the current patch you could warn a user that the feature is not supported
> and continue the initialization process (similar to the set mtu capability).
> 
> Ian
> >
> >
> >>
> >> I've pulled this patch from the merge request for master, I think the
> >> branch patches will have to be reworked also.
> >>
> >> Ian
> >>>
> >>> Ian
> >>>> Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
> >>>> ---
> >>>>    lib/netdev-dpdk.c | 15 +++++++--------
> >>>>    1 file changed, 7 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>>> bb4d60f26..023b80d3e 100644
> >>>> --- a/lib/netdev-dpdk.c
> >>>> +++ b/lib/netdev-dpdk.c
> >>>> @@ -1065,14 +1065,6 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> >>>>        mbp_priv = rte_mempool_get_priv(dev->dpdk_mp->mp);
> >>>>        dev->buf_size = mbp_priv->mbuf_data_room_size -
> >>>> RTE_PKTMBUF_HEADROOM;
> >>>> -
> >>>> -    /* Get the Flow control configuration for DPDK-ETH */
> >>>> -    diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
> >>>> -    if (diag) {
> >>>> -        VLOG_DBG("cannot get flow control parameters on port
> >>>> "DPDK_PORT_ID_FMT
> >>>> -                 ", err=%d", dev->port_id, diag);
> >>>> -    }
> >>>> -
> >>>>        return 0;
> >>>>    }
> >>>> @@ -1773,6 +1765,13 @@ netdev_dpdk_set_config(struct netdev
> >>>> *netdev, const struct smap *args,
> >>>>        autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
> >>>>        fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
> >>>> +    /* Get the Flow control configuration for DPDK-ETH */
> >>>> +    err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
> >>>> +    if (err) {
> >>>> +        VLOG_INFO("cannot get flow control parameters on port
> >>>> "DPDK_PORT_ID_FMT
> >>>> +                 ", err=%d", dev->port_id, err);
> >>>> +    }
> >>>> +
> >>>>        if (dev->fc_conf.mode != fc_mode || autoneg !=
> >>>> dev->fc_conf.autoneg) {
> >>>>            dev->fc_conf.mode = fc_mode;
> >>>>            dev->fc_conf.autoneg = autoneg;
> >>>>
> >>>
> >>> _______________________________________________
> >>> dev mailing list
> >>> dev at openvswitch.org
> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >



More information about the dev mailing list