Re: [PATCH net 1/1] ice: identify aRFS flows using L3/L4 dissector info

[Leon Romanovsky <leon@kernel.org>]

On Thu, Apr 13, 2023 at 10:27:56AM -0700, Jacob Keller wrote:
> 
> 
> On 4/10/2023 11:54 AM, Ahmed Zaki wrote:
> > 
> > On 2023-04-09 04:45, Leon Romanovsky wrote:
> >> On Fri, Apr 07, 2023 at 02:08:20PM -0700, Tony Nguyen wrote:
> >>> From: Ahmed Zaki <ahmed.zaki@intel.com>
> >>>
> >>> The flow ID passed to ice_rx_flow_steer() is computed like this:
> >>>
> >>>      flow_id = skb_get_hash(skb) & flow_table->mask;
> >>>
> >>> With smaller aRFS tables (for example, size 256) and higher number of
> >>> flows, there is a good chance of flow ID collisions where two or more
> >>> different flows are using the same flow ID. This results in the aRFS
> >>> destination queue constantly changing for all flows sharing that ID.
> >>>
> >>> Use the full L3/L4 flow dissector info to identify the steered flow
> >>> instead of the passed flow ID.
> >>>
> >>> Fixes: 28bf26724fdb ("ice: Implement aRFS")
> >>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> >>> Tested-by: Arpana Arland <arpanax.arland@intel.com> (A Contingent worker at Intel)
> >>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >>> ---
> >>>   drivers/net/ethernet/intel/ice/ice_arfs.c | 44 +++++++++++++++++++++--
> >>>   1 file changed, 41 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/ice/ice_arfs.c b/drivers/net/ethernet/intel/ice/ice_arfs.c
> >>> index fba178e07600..d7ae64d21e01 100644
> >>> --- a/drivers/net/ethernet/intel/ice/ice_arfs.c
> >>> +++ b/drivers/net/ethernet/intel/ice/ice_arfs.c
> >>> @@ -345,6 +345,44 @@ ice_arfs_build_entry(struct ice_vsi *vsi, const struct flow_keys *fk,
> >>>   	return arfs_entry;
> >>>   }
> >>>   
> >>> +/**
> >>> + * ice_arfs_cmp - compare flow to a saved ARFS entry's filter info
> >>> + * @fltr_info: filter info of the saved ARFS entry
> >>> + * @fk: flow dissector keys
> >>> + *
> >>> + * Caller must hold arfs_lock if @fltr_info belongs to arfs_fltr_list
> >>> + */
> >>> +static bool
> >>> +ice_arfs_cmp(struct ice_fdir_fltr *fltr_info, const struct flow_keys *fk)
> >>> +{
> >>> +	bool is_ipv4;
> >>> +
> >>> +	if (!fltr_info || !fk)
> >>> +		return false;
> >>> +
> >>> +	is_ipv4 = (fltr_info->flow_type == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> >>> +		fltr_info->flow_type == ICE_FLTR_PTYPE_NONF_IPV4_TCP);
> >>> +
> >>> +	if (fk->basic.n_proto == htons(ETH_P_IP) && is_ipv4)
> >>> +		return (fltr_info->ip.v4.proto == fk->basic.ip_proto &&
> >>> +			fltr_info->ip.v4.src_port == fk->ports.src &&
> >>> +			fltr_info->ip.v4.dst_port == fk->ports.dst &&
> >>> +			fltr_info->ip.v4.src_ip == fk->addrs.v4addrs.src &&
> >>> +			fltr_info->ip.v4.dst_ip == fk->addrs.v4addrs.dst);
> >>> +	else if (fk->basic.n_proto == htons(ETH_P_IPV6) && !is_ipv4)
> >>> +		return (fltr_info->ip.v6.proto == fk->basic.ip_proto &&
> >>> +			fltr_info->ip.v6.src_port == fk->ports.src &&
> >>> +			fltr_info->ip.v6.dst_port == fk->ports.dst &&
> >>> +			!memcmp(&fltr_info->ip.v6.src_ip,
> >>> +				&fk->addrs.v6addrs.src,
> >>> +				sizeof(struct in6_addr)) &&
> >>> +			!memcmp(&fltr_info->ip.v6.dst_ip,
> >>> +				&fk->addrs.v6addrs.dst,
> >>> +				sizeof(struct in6_addr)));
> >> I'm confident that you can write this function more clear with
> >> comparisons in one "return ..." instruction.
> >>>> Thanks
> > 
> > Do you mean remove the "if condition"? how?
> > 
> > I wrote it this way to match how I'd think:
> > 
> > If (IPv4 and V4 flows), test IPv4 flow keys, else if (IPv6 and V6 
> > flows), test IPv6 keys, else false.
> > 
> 
> You can use a || chain, something like:
> 
> return (is_ipv4 && (<check ipv4 fields)) || (!is_ipv4 && (<check ip6
> fields>)
> 
> There might be other ways to simplify the conditional. You could
> possibly combine the n_proto check with the is_ipv4 check above as well.

Another possible option is to use variable to store intermediate result.

Thanks

> 
> 
> > I m not sure how can I make it more clearer.
> > 
> > Thanks.
> >