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

[Ori Kam <orika@mellanox.com>]

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