Re: [dpdk-dev] [PATCH] examples/flow_filtering: add rte_fdir_conf initialization

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 7/17/2018 6:15 AM, Ori Kam wrote:
> Sorry for the late response,
> 
>> -----Original Message-----
>> From: Xu, Rosen [mailto:rosen.xu@intel.com]
>> Sent: Thursday, July 12, 2018 9:23 AM
>> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org; Gilmore, Walter
>> E <walter.e.gilmore@intel.com>
>> Subject: RE: [dpdk-dev] [PATCH] examples/flow_filtering: add rte_fdir_conf
>> initialization
>>
>> Hi Ori,
>>
>> Pls see my reply.
>>
>> Hi Walter and Ferruh,
>>
>> I need your voice :)
>>
>>> -----Original Message-----
>>> From: Ori Kam [mailto:orika@mellanox.com]
>>> Sent: Thursday, July 12, 2018 13:58
>>> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org
>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
>>> Subject: RE: [dpdk-dev] [PATCH] examples/flow_filtering: add
>> rte_fdir_conf
>>> initialization
>>>
>>> Hi,
>>>
>>> PSB
>>>
>>>> -----Original Message-----
>>>> From: Xu, Rosen [mailto:rosen.xu@intel.com]
>>>> Sent: Thursday, July 12, 2018 8:27 AM
>>>> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
>>>> Subject: RE: [dpdk-dev] [PATCH] examples/flow_filtering: add
>>>> rte_fdir_conf initialization
>>>>
>>>> Hi Ori,
>>>>
>>>> examples/flow_filtering sample app fails on i40e [1] because i40e
>>>> requires explicit FDIR configuration.
>>>>
>>>> But rte_flow in and hardware independent ways of describing
>>>> flow-action, it shouldn't require specific config options for specific
>>> hardware.
>>>>
>>>
>>> I don't understand why using rte flow require the use of fdir.
>>> it doesn't make sense to me, that  new API will need old one.
>>
>> It's a good question, I also have this question about Mellanox NIC Driver
>> mlx5_flow.c.
>> In this file many flow functions call fdir. :)
> 
> The only functions that are calling fdir are fdir function,
> and you can see that inside of the create function we convert the fdir 
> Into rte flow.
> 
>>
>>>> Is there any chance driver select the FDIR config automatically based
>>>> on rte_flow rule, unless explicitly a FDIR config set by user?
>>>
>>> I don't know how the i40e driver is implemented but I know that Mellanox
>>> convert the other way around, if fdir is given it is converted to rte_flow.
>>
>> Firstly, rte_fdir_conf is part of rte_eth_conf definition.
>> 	struct rte_eth_conf {
>> 		......
>> 		struct rte_fdir_conf fdir_conf; /**< FDIR configuration. */
>> 		......
>> 	};
>> Secondly, default value of rte_eth_conf.fdir_conf.mode is
>> RTE_FDIR_MODE_NONE, which means Disable FDIR support.
>> Thirdly, flow_filtering should align with test-pmd, in test-pmd all fdir_conf is
>> initialized.
>>
> 
> This sounds to me correct we don't want to enable fdir.
> Why should the example app for rte flow use fdir? And align to 
> testpmd which support everything in in all modes?

In i40e fdir is used to implement filters, that is why rte_flow rules
requires/depends some fdir configurations.

In long term I agree it is better if driver doesn't require any fdir
configuration for rte_flow programing, although not sure if this is completely
possible, cc'ed Qi for more comment.

For short term I am for getting this patch so that sample app can run on i40e
too, and fdir configuration shouldn't effect others. Perhaps it can be good to
add a comment to say why that config option is added and it is a temporary
workaround.

> 
> 
>>>
>>>>
>>>> [1]
>>>> Flow can't be created 1 message: Check the mode in fdir_conf.
>>>> EAL: Error - exiting with code: 1
>>>>
>>>>> -----Original Message-----
>>>>> From: Ori Kam [mailto:orika@mellanox.com]
>>>>> Sent: Thursday, July 12, 2018 13:17
>>>>> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org
>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org; Ori Kam
>>>>> <orika@mellanox.com>
>>>>> Subject: RE: [dpdk-dev] [PATCH] examples/flow_filtering: add
>>>> rte_fdir_conf
>>>>> initialization
>>>>>
>>>>> Hi Rosen,
>>>>>
>>>>> Why do the fdir_conf must be initialized?
>>>>>
>>>>> What is the issue you are seeing?
>>>>>
>>>>> Best,
>>>>> Ori
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rosen Xu
>>>>>> Sent: Thursday, July 12, 2018 5:10 AM
>>>>>> To: dev@dpdk.org
>>>>>> Cc: rosen.xu@intel.com; ferruh.yigit@intel.com; Ori Kam
>>>>>> <orika@mellanox.com>; stable@dpdk.org
>>>>>> Subject: [dpdk-dev] [PATCH] examples/flow_filtering: add
>> rte_fdir_conf
>>>>>> initialization
>>>>>>
>>>>>> Rte_fdir_conf of rte_eth_conf should be initialized before port
>>>>>> initialization.
>>>>>>
>>>>>> Fixes: 4a3ef59a10c8 ("examples/flow_filtering: add simple demo of
>>> flow
>>>>>> API")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
>>>>>> ---
>>>>>>  examples/flow_filtering/main.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/examples/flow_filtering/main.c
>>>>>> b/examples/flow_filtering/main.c index f595034..aa03e23 100644
>>>>>> --- a/examples/flow_filtering/main.c
>>>>>> +++ b/examples/flow_filtering/main.c
>>>>>> @@ -132,6 +132,12 @@
>>>>>>  				DEV_TX_OFFLOAD_SCTP_CKSUM  |
>>>>>>  				DEV_TX_OFFLOAD_TCP_TSO,
>>>>>>  		},
>>>>>> +		.fdir_conf = {
>>>>>> +			.mode = RTE_FDIR_MODE_PERFECT,
>>>>>> +			.pballoc = RTE_FDIR_PBALLOC_64K,
>>>>>> +			.status = RTE_FDIR_REPORT_STATUS,
>>>>>> +			.drop_queue = 127,
>>>>>> +		},
>>>>>>  	};
>>>>>>  	struct rte_eth_txconf txq_conf;
>>>>>>  	struct rte_eth_rxconf rxq_conf;
>>>>>> --
>>>>>> 1.8.3.1
> 
> 
> Best,
> Ori
>