From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mail.toke.dk (Postfix) with ESMTPS id C956E9EA2E0 for ; Thu, 16 Feb 2023 17:47:01 +0100 (CET) Authentication-Results: mail.toke.dk; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eiUmXRnz DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676566020; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=59uNXmnY+5xpfv8KNp1rUASo+YONUDHrOKaND1BYqkc=; b=eiUmXRnzB0Zj2oENtIy9Bl7xbrfCzH/CuXptchXYKmtVB0aC+yKBqjvUZ7I4awis69eVVI qwfCP6p4SCx76ogV1dRDMVRTjAdajziP2GzC6ezPooq8HgeCqs83IPoDl3JJX11BOi92b2 kBpVFEc2bftq4LwO3ZJVA/B0rk5at0M= Received: from mail-ed1-f72.google.com (mail-ed1-f72.google.com [209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-112-PC0UvVJFP6uxK14PCAJNtQ-1; Thu, 16 Feb 2023 11:46:57 -0500 X-MC-Unique: PC0UvVJFP6uxK14PCAJNtQ-1 Received: by mail-ed1-f72.google.com with SMTP id y17-20020a056402359100b004acc4f8aa3fso2317026edc.3 for ; Thu, 16 Feb 2023 08:46:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:to :content-language:subject:cc:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=59uNXmnY+5xpfv8KNp1rUASo+YONUDHrOKaND1BYqkc=; b=gtMEZWZvp/plm9Aht1Vz9K+aWVr0lME0N3h7nxsV279fS5IsUOaAsbEY8AG1JxKHy4 BrhpC9HhgYm4526hFo3khHqvZ0IltZfhIGVcuPC2gRCqEnS9PswIu9RSepSG1XFraL/s HhwqWyq7nUt9i3fpPJFkMxhGHd54E9h4HVMfLN+2F6EGpX/o2tbAu1cEiCaSlaTA/4W5 n8ZKOgoLtSNmCOz+oEbyr07JuQDz//TvijEYMYc4M6vab8KwFCiuGnxAoC3IbjrYJuMp UePl+dq2z6UIDclA4STTO8gyTr4thU6x4L7h9SEZqibPnlRg/89lnVQlQKUhWMi1wpvu NZsg== X-Gm-Message-State: AO0yUKVmC0pYSVQexC8nGtz2Vy3XQs9s6CKungNE8yq4oxCjincRh9t9 XhaQVI7aJg4tB9Iy6d+3YeSdQQ3/CLIwbl98StF7T+vszX7QxvlRZ0zxQ2KChmEJ36Xi2yfbCUE M6i4ln26iyylJ0mvXMVkU X-Received: by 2002:aa7:d28d:0:b0:4ac:bcef:505a with SMTP id w13-20020aa7d28d000000b004acbcef505amr6881337edq.38.1676566016259; Thu, 16 Feb 2023 08:46:56 -0800 (PST) X-Google-Smtp-Source: AK7set80Fzbld5F2gDEWeKiKO6SZW4LBZxJ3poPT7SamcTW7fAXxk1srziilHzzaelgpB+gTsNh7sw== X-Received: by 2002:aa7:d28d:0:b0:4ac:bcef:505a with SMTP id w13-20020aa7d28d000000b004acbcef505amr6881312edq.38.1676566015902; Thu, 16 Feb 2023 08:46:55 -0800 (PST) Received: from [192.168.42.100] (nat-cgn9-185-107-15-52.static.kviknet.net. [185.107.15.52]) by smtp.gmail.com with ESMTPSA id v14-20020a50c40e000000b004acaa4d51bdsm1136242edf.32.2023.02.16.08.46.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Feb 2023 08:46:55 -0800 (PST) From: Jesper Dangaard Brouer X-Google-Original-From: Jesper Dangaard Brouer Message-ID: Date: Thu, 16 Feb 2023 17:46:53 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 To: Alexander Lobakin References: <167604167956.1726972.7266620647404438534.stgit@firesoul> In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Message-ID-Hash: J5FZJP4KYTLL3RVC5QR4XU7DMFHQQJLM X-Message-ID-Hash: J5FZJP4KYTLL3RVC5QR4XU7DMFHQQJLM X-MailFrom: jbrouer@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: brouer@redhat.com, bpf@vger.kernel.org, netdev@vger.kernel.org, Stanislav Fomichev , martin.lau@kernel.org, ast@kernel.org, daniel@iogearbox.net, yoong.siang.song@intel.com, anthony.l.nguyen@intel.com, intel-wired-lan@lists.osuosl.org, xdp-hints@xdp-project.net X-Mailman-Version: 3.3.8 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next V1] igc: enable and fix RX hash usage by netstack List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 14/02/2023 14.21, Alexander Lobakin wrote: > From: Jesper Dangaard Brouer > Date: Fri, 10 Feb 2023 16:07:59 +0100 > >> When function igc_rx_hash() was introduced in v4.20 via commit 0507ef8a0372 >> ("igc: Add transmit and receive fastpath and interrupt handlers"), the >> hardware wasn't configured to provide RSS hash, thus it made sense to not >> enable net_device NETIF_F_RXHASH feature bit. > > [...] > >> @@ -311,6 +311,58 @@ extern char igc_driver_name[]; >> #define IGC_MRQC_RSS_FIELD_IPV4_UDP 0x00400000 >> #define IGC_MRQC_RSS_FIELD_IPV6_UDP 0x00800000 >> >> +/* RX-desc Write-Back format RSS Type's */ >> +enum igc_rss_type_num { >> + IGC_RSS_TYPE_NO_HASH = 0, >> + IGC_RSS_TYPE_HASH_TCP_IPV4 = 1, >> + IGC_RSS_TYPE_HASH_IPV4 = 2, >> + IGC_RSS_TYPE_HASH_TCP_IPV6 = 3, >> + IGC_RSS_TYPE_HASH_IPV6_EX = 4, >> + IGC_RSS_TYPE_HASH_IPV6 = 5, >> + IGC_RSS_TYPE_HASH_TCP_IPV6_EX = 6, >> + IGC_RSS_TYPE_HASH_UDP_IPV4 = 7, >> + IGC_RSS_TYPE_HASH_UDP_IPV6 = 8, >> + IGC_RSS_TYPE_HASH_UDP_IPV6_EX = 9, >> + IGC_RSS_TYPE_MAX = 10, >> +}; >> +#define IGC_RSS_TYPE_MAX_TABLE 16 >> +#define IGC_RSS_TYPE_MASK 0xF > > GENMASK()? > hmm... GENMASK(3,0) looks more confusing to me. The mask we need here is so simple that I prefer not to complicate this with GENMASK. >> + >> +/* igc_rss_type - Rx descriptor RSS type field */ >> +static inline u8 igc_rss_type(union igc_adv_rx_desc *rx_desc) > > Why use types shorter than u32 on the stack? Changing to u32 in V2 > Why this union is not const here, since there are no modifications? Sure >> +{ >> + /* RSS Type 4-bit number: 0-9 (above 9 is reserved) */ >> + return rx_desc->wb.lower.lo_dword.hs_rss.pkt_info & IGC_RSS_TYPE_MASK; > > The most important I wanted to mention: doesn't this function make the > CPU read the uncached field again, while you could just read it once > onto the stack and then extract all such data from there? I really don't think this is an issues here. The igc_adv_rx_desc is only 16 bytes and it should be hot in CPU cache by now. To avoid the movzx I have changed this to do a u32 read instead. >> +} >> + >> +/* Packet header type identified by hardware (when BIT(11) is zero). >> + * Even when UDP ports are not part of RSS hash HW still parse and mark UDP bits >> + */ >> +enum igc_pkt_type_bits { >> + IGC_PKT_TYPE_HDR_IPV4 = BIT(0), >> + IGC_PKT_TYPE_HDR_IPV4_WITH_OPT= BIT(1), /* IPv4 Hdr includes IP options */ >> + IGC_PKT_TYPE_HDR_IPV6 = BIT(2), >> + IGC_PKT_TYPE_HDR_IPV6_WITH_EXT= BIT(3), /* IPv6 Hdr includes extensions */ >> + IGC_PKT_TYPE_HDR_L4_TCP = BIT(4), >> + IGC_PKT_TYPE_HDR_L4_UDP = BIT(5), >> + IGC_PKT_TYPE_HDR_L4_SCTP= BIT(6), >> + IGC_PKT_TYPE_HDR_NFS = BIT(7), >> + /* Above only valid when BIT(11) is zero */ >> + IGC_PKT_TYPE_L2 = BIT(11), >> + IGC_PKT_TYPE_VLAN = BIT(12), >> + IGC_PKT_TYPE_MASK = 0x1FFF, /* 13-bits */ > > Also GENMASK(). GENMASK would make more sense here. >> +}; >> + >> +/* igc_pkt_type - Rx descriptor Packet type field */ >> +static inline u16 igc_pkt_type(union igc_adv_rx_desc *rx_desc) > > Also short types and consts. > Fixed in V2 >> +{ >> + u32 data = le32_to_cpu(rx_desc->wb.lower.lo_dword.data); >> + /* Packet type is 13-bits - as bits (16:4) in lower.lo_dword*/ >> + u16 pkt_type = (data >> 4) & IGC_PKT_TYPE_MASK; > > Perfect candidate for FIELD_GET(). No, even for le32_get_bits(). I adjusted this, but I could not find a central define for FIELD_GET (but many drivers open code this). > Also my note above about excessive expensive reads. > >> + >> + return pkt_type; >> +} >> + >> /* Interrupt defines */ >> #define IGC_START_ITR 648 /* ~6000 ints/sec */ >> #define IGC_4K_ITR 980 >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 8b572cd2c350..42a072509d2a 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -1677,14 +1677,40 @@ static void igc_rx_checksum(struct igc_ring *ring, >> le32_to_cpu(rx_desc->wb.upper.status_error)); >> } >> >> +/* Mapping HW RSS Type to enum pkt_hash_types */ >> +struct igc_rss_type { >> + u8 hash_type; /* can contain enum pkt_hash_types */ > > Why make a struct for one field? + short type note > >> +} igc_rss_type_table[IGC_RSS_TYPE_MAX_TABLE] = { >> + [IGC_RSS_TYPE_NO_HASH].hash_type = PKT_HASH_TYPE_L2, >> + [IGC_RSS_TYPE_HASH_TCP_IPV4].hash_type = PKT_HASH_TYPE_L4, >> + [IGC_RSS_TYPE_HASH_IPV4].hash_type = PKT_HASH_TYPE_L3, >> + [IGC_RSS_TYPE_HASH_TCP_IPV6].hash_type = PKT_HASH_TYPE_L4, >> + [IGC_RSS_TYPE_HASH_IPV6_EX].hash_type = PKT_HASH_TYPE_L3, >> + [IGC_RSS_TYPE_HASH_IPV6].hash_type = PKT_HASH_TYPE_L3, >> + [IGC_RSS_TYPE_HASH_TCP_IPV6_EX].hash_type = PKT_HASH_TYPE_L4, >> + [IGC_RSS_TYPE_HASH_UDP_IPV4].hash_type = PKT_HASH_TYPE_L4, >> + [IGC_RSS_TYPE_HASH_UDP_IPV6].hash_type = PKT_HASH_TYPE_L4, >> + [IGC_RSS_TYPE_HASH_UDP_IPV6_EX].hash_type = PKT_HASH_TYPE_L4, >> + [10].hash_type = PKT_HASH_TYPE_L2, /* RSS Type above 9 "Reserved" by HW */ >> + [11].hash_type = PKT_HASH_TYPE_L2, >> + [12].hash_type = PKT_HASH_TYPE_L2, >> + [13].hash_type = PKT_HASH_TYPE_L2, >> + [14].hash_type = PKT_HASH_TYPE_L2, >> + [15].hash_type = PKT_HASH_TYPE_L2, > > Why define those empty if you could do a bound check in the code > instead? E.g. `if (unlikely(bigger_than_9)) return PKT_HASH_TYPE_L2`. Having a branch for this is likely slower. On godbolt I see that this generates suboptimal and larger code. >> +}; >> + >> static inline void igc_rx_hash(struct igc_ring *ring, >> union igc_adv_rx_desc *rx_desc, >> struct sk_buff *skb) >> { >> - if (ring->netdev->features & NETIF_F_RXHASH) >> - skb_set_hash(skb, >> - le32_to_cpu(rx_desc->wb.lower.hi_dword.rss), >> - PKT_HASH_TYPE_L3); >> + if (ring->netdev->features & NETIF_F_RXHASH) { > > if (!(feature & HASH)) > return; > > and -1 indent level? Usually, yes, I also prefer early return style code. For one I just followed the existing style. Second, I tried to code it up, but it looks ugly in this case, as the variable defines need to get moved outside the if statement. >> + u32 rss_hash = le32_to_cpu(rx_desc->wb.lower.hi_dword.rss); >> + u8 rss_type = igc_rss_type(rx_desc); >> + enum pkt_hash_types hash_type; >> + >> + hash_type = igc_rss_type_table[rss_type].hash_type; >> + skb_set_hash(skb, rss_hash, hash_type); >> + } >> } > > [...] > > Thanks, > Olek >