From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A8C9C77B6E for ; Fri, 14 Apr 2023 08:54:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230046AbjDNIyN (ORCPT ); Fri, 14 Apr 2023 04:54:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33958 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229547AbjDNIyM (ORCPT ); Fri, 14 Apr 2023 04:54:12 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1A2315B98 for ; Fri, 14 Apr 2023 01:54:11 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A358C64579 for ; Fri, 14 Apr 2023 08:54:10 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1EBA9C4339B; Fri, 14 Apr 2023 08:54:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681462449; bh=Tnd57Pz7lgA5d+hcNhuN0ZFtEg3bLfSKhpwO81xJzIY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ty0l68kWhm7jwOJA+hmL2p2sgzmj+ZRBVGws0avkT/5YlAtsY4zp2fl8BF4MF3aEi aYEZTzsrnhDv0bmru7yuTd+1YgfBp75Kkb7Bu/leckHT71TYWWKERqefLQCxXw1iIY Aq9EL6LsfUWI89/tzyOla6jTQxdFLSiAR9n2aPNm22pLRZPDUgbshlisaBU9Q0kX1Q wb6sl6LylpXmdSSuMA4MHaeMq5T9EokO+amkozTug5b2IETPuuTO378jTTInL52cTU M2yIFfBEDluspAwwoVeM0YQ4vaQ/DzgOsi5rdDeDoOH4CqCmlridmdLv7dHFkvE2pE Z2UPz75vtz0GA== Date: Fri, 14 Apr 2023 11:54:05 +0300 From: Leon Romanovsky To: Jacob Keller Cc: Ahmed Zaki , Tony Nguyen , davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, edumazet@google.com, netdev@vger.kernel.org, Arpana Arland Subject: Re: [PATCH net 1/1] ice: identify aRFS flows using L3/L4 dissector info Message-ID: <20230414085405.GZ17993@unreal> References: <20230407210820.3046220-1-anthony.l.nguyen@intel.com> <20230409104529.GQ14869@unreal> <3de9c4a4-4fba-9837-962a-e3e78299ed3b@intel.com> <09ec7b55-5ec9-2abc-dbb8-cdb7e0b0c6a8@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <09ec7b55-5ec9-2abc-dbb8-cdb7e0b0c6a8@intel.com> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.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 > >>> > >>> 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 > >>> Tested-by: Arpana Arland (A Contingent worker at Intel) > >>> Signed-off-by: Tony Nguyen > >>> --- > >>> 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 && ( 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. > >