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

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

On 7/17/2018 2:04 PM, Ori Kam wrote:
> Hi,
> 
> PSB
> 
> Thanks,
> Ori
> 
>> -----Original Message-----
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Tuesday, July 17, 2018 12:57 PM
>> To: Ori Kam <orika@mellanox.com>; Xu, Rosen <rosen.xu@intel.com>;
>> dev@dpdk.org
>> Cc: stable@dpdk.org; Gilmore, Walter E <walter.e.gilmore@intel.com>; Qi
>> Zhang <qi.z.zhang@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] examples/flow_filtering: add rte_fdir_conf
>> initialization
>>
>> 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.
>>
> 
> Assuming that the setting for the fdir are fixed for all possible rte_flow rules
> I can agree for this workaround but we must add some comment in the code
> and also add this comment in the example documentation.
> 
> It will be a problem if other PMD will require different default setting.
> In this case we must find a better solution.

+1 for commenting code, and as far as I know fdir config only used by Intel PMDs
which we need to confirm all Intel PMDs are OK with change.

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