[Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR
Michal Swiatkowski
michal.swiatkowski at linux.intel.com
Fri Mar 10 10:15:24 UTC 2023
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 at linux.intel.com>
> > Sent: Thursday, March 9, 2023 22:02
> > To: Guo, Junfeng <junfeng.guo at intel.com>
> > Cc: intel-wired-lan at 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 at 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 at osuosl.org
> > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
More information about the Intel-wired-lan
mailing list