[Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR
Guo, Junfeng
junfeng.guo at intel.com
Fri Mar 10 05:16:22 UTC 2023
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.
>
> > +{
> > + 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?
{
enum ice_fltr_ptype flow_type_a, flow_type_b;
struct ice_fdir_fltr *a = &existing_conf->input;
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;
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