Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Fri, Mar 10, 2023 at 05:16:22AM +0000, Guo, Junfeng wrote:
> Thanks for the review! Comments inline.
> 
> > -----Original Message-----
> > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > Sent: Thursday, March 9, 2023 22:02
> > To: Guo, Junfeng <junfeng.guo@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org
> > Subject: Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check
> > for AVF FDIR
> > 
> > On Thu, Mar 09, 2023 at 01:10:11PM +0800, Junfeng Guo wrote:
> > > Add profile conflict check while adding some FDIR rules to aviod
> > > unexpected flow behavior, rules may have conflict including:
> > >         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
> > >         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> > >
> > > For example, when we create an FDIR rule for IPv4, this rule will work
> > > on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> > > then create an FDIR rule for IPv4_UDP and then destroy it, the first
> > > FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> > >
> > > To prevent this unexpected behavior, we add restriction in software
> > > when creating FDIR rules by adding necessary profile conflict check.
> > 
> > What about flow conflict when rule is added from host perspective (by
> > ethtool)? Do we also need to check for conflict? Maybe it is worth
> > create common code for this case.
> > >
> > > Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> > > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> > > ---
> > >  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 71 +++++++++++++++++++
> > >  1 file changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > > index e6ef6b303222..1431789c194e 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > > @@ -541,6 +541,71 @@ static void ice_vc_fdir_rem_prof_all(struct
> > ice_vf *vf)
> > >  	}
> > >  }
> > >
> > > +/**
> > > + * ice_vc_fdir_has_prof_conflict
> > > + * @vf: pointer to the VF structure
> > > + * @conf: FDIR configuration for each filter
> > > + *
> > > + * Check if @conf has conflicting profile with existing profiles
> > > + *
> > > + * Return: true on success, and false on error.
> > > + */
> > > +static bool
> > > +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> > > +			      struct virtchnl_fdir_fltr_conf *conf)
> > It isn't aligned.
> 
> I think here is just the display issue, the "+" at the beginning will occupy
> a character place. Once applied, it will show correctly.
> 

Oh, sure, sorry for pointing it.

> > 
> > > +{
> > > +	struct ice_fdir_fltr *desc;
> > > +
> > > +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> > > +		struct virtchnl_fdir_fltr_conf *existing_conf =
> > > +
> > 	to_fltr_conf_from_desc(desc);
> > > +		struct ice_fdir_fltr *a = &existing_conf->input;
> > > +		struct ice_fdir_fltr *b = &conf->input;
> > > +
> > > +		enum ice_fltr_ptype flow_type_a = a->flow_type;
> > > +		enum ice_fltr_ptype flow_type_b = b->flow_type;
> > I think You should folow RCT variable declaration here, and remove
> > empty
> > line.
> 
> Thanks for the advice!
> 
> Do you mean update the code order like this?
> 	{
> 
To follow RCT:
struct ice_fdir_fltr *a = &existing_conf->input;
enum ice_fltr_ptype flow_type_a, flow_type_b;
struct ice_fdir_fltr *b = &conf->input;

> 	flow_type_a = a->flow_type;
> 	flow_type_b = b->flow_type;
> 	}
> Or like this?
> 	{
> 	enum ice_fltr_ptype flow_type_a, flow_type_b;
> 	struct ice_fdir_fltr *a, *b;
This is also fine

Also fine will be:
struct ice_fdir_fltr *a = &existing_conf->input;
enum ice_fltr_ptype flow_type_a = a->flow_type;
enum ice_fltr_ptype flow_type_b = b->flow_type;
struct ice_fdir_fltr *b = &conf->input;

And it's look the best in my opinion, but it is only cosmetic.

> 
> 	a = &existing_conf->input;
> 	b = &conf->input;
> 	flow_type_a = a->flow_type;
> 	flow_type_b = b->flow_type;
> 	}
> 
> > 
> > > +
> > > +		/* No need to compare two rules with different tunnel
> > type */
> > > +		if (existing_conf->ttype != conf->ttype)
> > > +			continue;
> > > +
> > > +		/* No need to compare two rules with same protocol */
> > > +		if (flow_type_a == flow_type_b)
> > > +			continue;
> > This two ifs can be combined into one.
> 
> Sure, it could be updated in the coming version. Thanks!
> 
> > 
> > > +
> > > +		switch (flow_type_a) {
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> > > +				return true;
> > > +			break;
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> > > +				return true;
> > > +			break;
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> > > +				return true;
> > > +			break;
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> > > +				return true;
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  /**
> > >   * ice_vc_fdir_write_flow_prof
> > >   * @vf: pointer to the VF structure
> > > @@ -677,6 +742,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf,
> > struct virtchnl_fdir_add *fltr,
> > >  	enum ice_fltr_ptype flow;
> > >  	int ret;
> > >
> > > +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> > > +	if (ret) {
> > > +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf-
> > >vf_id);
> > > +		return ret;
> > > +	}
> > > +
> > >  	flow = input->flow_type;
> > >  	ret = ice_vc_fdir_alloc_prof(vf, flow);
> > >  	if (ret) {
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > Intel-wired-lan mailing list
> > > Intel-wired-lan@osuosl.org
> > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan