diff mbox series

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

Message ID 20230407210820.3046220-1-anthony.l.nguyen@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net,1/1] ice: identify aRFS flows using L3/L4 dissector info | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 64 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tony Nguyen April 7, 2023, 9:08 p.m. UTC 
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(-)

Comments

Leon Romanovsky April 9, 2023, 10:45 a.m. UTC | #1 
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

> +
> +	return false;
> +}
> +
>  /**
>   * ice_arfs_is_perfect_flow_set - Check to see if perfect flow is set
>   * @hw: pointer to HW structure
> @@ -436,17 +474,17 @@ ice_rx_flow_steer(struct net_device *netdev, const struct sk_buff *skb,
>  
>  	/* choose the aRFS list bucket based on skb hash */
>  	idx = skb_get_hash_raw(skb) & ICE_ARFS_LST_MASK;
> +
>  	/* search for entry in the bucket */
>  	spin_lock_bh(&vsi->arfs_lock);
>  	hlist_for_each_entry(arfs_entry, &vsi->arfs_fltr_list[idx],
>  			     list_entry) {
> -		struct ice_fdir_fltr *fltr_info;
> +		struct ice_fdir_fltr *fltr_info = &arfs_entry->fltr_info;
>  
>  		/* keep searching for the already existing arfs_entry flow */
> -		if (arfs_entry->flow_id != flow_id)
> +		if (!ice_arfs_cmp(fltr_info, &fk))
>  			continue;
>  
> -		fltr_info = &arfs_entry->fltr_info;
>  		ret = fltr_info->fltr_id;
>  
>  		if (fltr_info->q_index == rxq_idx ||
> -- 
> 2.38.1
>
Ahmed Zaki April 10, 2023, 6:54 p.m. UTC | #2 
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.

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

Thanks.
Keller, Jacob E April 13, 2023, 5:27 p.m. UTC | #3 
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.


> I m not sure how can I make it more clearer.
> 
> Thanks.
>
Leon Romanovsky April 14, 2023, 8:54 a.m. UTC | #4 
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.
> >
Alexander Lobakin April 14, 2023, 4:07 p.m. UTC | #5 
From: Leon Romanovsky <leon@kernel.org>
Date: Fri, 14 Apr 2023 11:54:05 +0300

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

@fltr_info can be const BTW.

>>>>> +{
>>>>> +	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);

	is_v4 = fk->basic.n_proto == htons(ETH_P_IP) &&
		(fltr_info->flow_type == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
		 fltr_info->flow_type == ICE_FLTR_PTYPE_NONF_IPV4_TCP);
	if (!is_v4 && fk->basic.n_proto != htons(ETH_P_IPV6))
		return;

That's -1 indent level.

(your statements have too many braces BTW, at least half of them are not
needed)

>>>>> +
>>>>> +	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);

	const struct ice_fdir_v4 *v4 = &fltr_info->ip.v4;

	return v4->proto == fk->basic.ip_proto && ...

That removes 13 chars from each comparison.
return with IP ver check would then look like:

	return (is_v4 && v4->proto == ...) ||
	       (!is_v4 && v6->proto == ...);

But honestly I would split those branches into separate small static
functions, compilers will combine them later as well:

	return is_v4 ? ice_arfs_cmp_v4(&fltr_info->ip.v4, fk) :
		       ice_arfs_cmp_v6(&fltr_info->ip.v6, fk);

>>>>> +	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)));

Or you can reorder src and dst IPs in &ice_fdir_v6 and then do that in
one memcmp():

	return ... &&
	       !memcmp(&v6->dst_ip, &fk->addrs.v6addrs.dst,
		       2 * sizeof(v6->dst_ip));

OR what I'd do is I'd use Flow Dissector's structures in ice_fdir_v{4,6}
so that it would be much easier to compare them. The layout won't even
change, not counting dst/src IP reorder:

struct ice_fdir_v6 {
	struct flow_dissector_key_ipv6_addrs addrs;
	struct flow_dissector_key_ports ports;
	__be32 l4_header;
	...
};

I know those structures probably come from OS-independent code or so,
but folks know I never sacrifice convenience in favor of some OOT
compatibility :p

Also, note that &flow_dissector_key_ports unionize src + dst ports into
one `__be32` nicely, so that they could be compared in one 32-bit value
cmp instruction. &ice_fdir_v{4,6} lack those and you need to use more
instructions and shorter types (even more instructions).

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

Billion of different options here to me <_<

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

Thanks,
Olek
diff mbox series

Patch

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)));
+
+	return false;
+}
+
 /**
  * ice_arfs_is_perfect_flow_set - Check to see if perfect flow is set
  * @hw: pointer to HW structure
@@ -436,17 +474,17 @@  ice_rx_flow_steer(struct net_device *netdev, const struct sk_buff *skb,
 
 	/* choose the aRFS list bucket based on skb hash */
 	idx = skb_get_hash_raw(skb) & ICE_ARFS_LST_MASK;
+
 	/* search for entry in the bucket */
 	spin_lock_bh(&vsi->arfs_lock);
 	hlist_for_each_entry(arfs_entry, &vsi->arfs_fltr_list[idx],
 			     list_entry) {
-		struct ice_fdir_fltr *fltr_info;
+		struct ice_fdir_fltr *fltr_info = &arfs_entry->fltr_info;
 
 		/* keep searching for the already existing arfs_entry flow */
-		if (arfs_entry->flow_id != flow_id)
+		if (!ice_arfs_cmp(fltr_info, &fk))
 			continue;
 
-		fltr_info = &arfs_entry->fltr_info;
 		ret = fltr_info->fltr_id;
 
 		if (fltr_info->q_index == rxq_idx ||