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

[Ian Stokes]

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.

> 
> 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
>