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

Message ID 20180710132337.18211-1-sugesh.chandran@intel.com
State Superseded
Headers show
Series
  • [ovs-dev] netdev-dpdk: Fix failure to configure flow control at netdev-init.
Related show

Commit Message

Chandran, Sugesh July 10, 2018, 1:23 p.m.
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.

Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
---
 lib/netdev-dpdk.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Stokes, Ian July 13, 2018, 5:13 p.m. | #1
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?

Ian
> Signed-off-by: Sugesh Chandran <sugesh.chandran@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;
>
Chandran, Sugesh July 16, 2018, 12:19 p.m. | #2
Hi Ian,

Regards
_Sugesh

> -----Original Message-----
...
> > 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?
> 
[Sugesh]Yes, that’s right. It need to backported to 2.8 and 2.7 too.
Should I send separate patch for those branches as well?

> Ian
> > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
...
Stokes, Ian July 16, 2018, 12:51 p.m. | #3
> Hi Ian,
> 
> Regards
> _Sugesh
> 
> > -----Original Message-----
> ...
> > > 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?
> >
> [Sugesh]Yes, that’s right. It need to backported to 2.8 and 2.7 too.
> Should I send separate patch for those branches as well?

Yes, it doesn't apply cleanly for 2.8/2.7 so a patch for those branches would be needed.

Thanks
Ian
> 
> > Ian
> > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> ...
Chandran, Sugesh July 16, 2018, 1:07 p.m. | #4
Regards
_Sugesh


> -----Original Message-----
> From: Stokes, Ian
> Sent: Monday, July 16, 2018 1:52 PM
> To: Chandran, Sugesh <sugesh.chandran@intel.com>; ovs-
> dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control
> at netdev-init.
> 
> > Hi Ian,
> >
> > Regards
> > _Sugesh
> >
> > > -----Original Message-----
> > ...
> > > > 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?
> > >
> > [Sugesh]Yes, that’s right. It need to backported to 2.8 and 2.7 too.
> > Should I send separate patch for those branches as well?
> 
> Yes, it doesn't apply cleanly for 2.8/2.7 so a patch for those branches would be
> needed.
[Sugesh] Sure, I will send patches for those versions.

> 
> Thanks
> Ian
> >
> > > Ian
> > > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > ...
Stokes, Ian July 20, 2018, 5:30 p.m. | #5
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.

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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Chandran, Sugesh July 20, 2018, 6:24 p.m. | #6
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@intel.com>; ovs-
> dev@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.
Adding a condition in OVS to check a VF port for similar parameters doesn’t looks like
a clean approach? What do you think?

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'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@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@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian July 20, 2018, 7:41 p.m. | #7
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@intel.com>; ovs-
>> dev@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@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@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Chandran, Sugesh July 20, 2018, 10:26 p.m. | #8
Regards
_Sugesh


> -----Original Message-----
> From: Stokes, Ian
> Sent: Friday, July 20, 2018 8:41 PM
> To: Chandran, Sugesh <sugesh.chandran@intel.com>; ovs-
> dev@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@intel.com>; ovs-
> >> dev@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@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@openvswitch.org
> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

Patch

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;