| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <953fcf6d-0b50-01be-d592-8143791d2032@intel.com>
Date: Fri, 14 Apr 2023 18:07:37 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Ahmed Zaki <ahmed.zaki@...el.com>
CC: Leon Romanovsky <leon@...nel.org>,
Jacob Keller <jacob.e.keller@...el.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
<edumazet@...gle.com>, <netdev@...r.kernel.org>,
Arpana Arland <arpanax.arland@...el.com>
Subject: Re: [PATCH net 1/1] ice: identify aRFS flows using L3/L4 dissector
info
From: Leon Romanovsky <leon@...nel.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@...el.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@...el.com>
>>>>> Tested-by: Arpana Arland <arpanax.arland@...el.com> (A Contingent worker at Intel)
>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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
Powered by blists - more mailing lists