From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 192A7A0C43; Tue, 16 Nov 2021 06:20:48 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C1CD640141; Tue, 16 Nov 2021 06:20:47 +0100 (CET) Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) by mails.dpdk.org (Postfix) with ESMTP id A790640040 for ; Tue, 16 Nov 2021 06:20:46 +0100 (CET) Received: by mail-ed1-f48.google.com with SMTP id x15so82271822edv.1 for ; Mon, 15 Nov 2021 21:20:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=emumba-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=U/vQWWHpGrlF+e03/syGUziUd6HvLtbX6Q9JcrSWEp0=; b=WwBz6ly2ExiXS1wqSBxonTAfAALT0jPaidFinFbhEB8Nb8ocnWfRm5/Z1nnnKlSCKx hqykyApGBTKJCV+xPP02kT5w5yJ+XRdBmWKbVEuuZHQpfe02zeMMdimFRCNJudB6+yx2 Zoy5HdEn4oNkGkivzKV4fZ3cTCj2/GnSjfYaDUSPavvzgiNAUjBHFARJUiCq8umh3syn 715VxakASGXt1IHhexOwNvBgvlup9+PPi9acZC7mESFuw2r0nQrnFkKiRitpgJsAzK8T aN3al4OF5/KI+MckfQrnWiyr+29oul6Al1WhCiY35pVW2ejhAY4LxrwYEfzuQ6Hv3KrV cl9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=U/vQWWHpGrlF+e03/syGUziUd6HvLtbX6Q9JcrSWEp0=; b=zqzbiarvgWbZhSAXYhQA0Ybd6LGlf+a30XAcnvz4nNND6b0k0ZybbCTL4ukAD67d/n Xbr687ObrPA4FUYq54h8hnpCFggP96W3xGq0i+D+G4jeroGGy2zzLcEeVy0qDCwsIe78 8Qbq8bi6rydopG/iJFPFreJtsNnMvW4kd7gNAOCsmdvZTQaq+/pykX7TOobBLzW4wEeq 0b6/gl7Rl6BwQFnHwJ0OFUyiqsyeGLAeuoErgvmfZovQ8gGKrOKFf1eVsrN4/cDik7dp 2D2Z8D1Sgoa4uU2kVw9uyXSOv6Xv5q5nJWag9/IOUqX9z21TYJlToHuvxkWyCZskqQv/ pGZQ== X-Gm-Message-State: AOAM531HQoj6Vx2ci501MseLl6hGtjws6M+YlrGASvOZrAP3onm70iL5 T4yopvbU73lzHuUmnvZtDgMunshIR9gnjZyS2+8l X-Google-Smtp-Source: ABdhPJxfo2LM04fGxLUnzOsft5m1z3aRJG2wIW0XJtdAzrPwdB0zHnuoMHrLjfaRjXs50EGrD/5jcXvarbbf38L/kYU= X-Received: by 2002:a05:6402:348d:: with SMTP id v13mr6410128edc.337.1637040046453; Mon, 15 Nov 2021 21:20:46 -0800 (PST) MIME-Version: 1.0 References: <20211008155111.125786-1-usama.nadeem@emumba.com> <20211014184322.5148-1-usama.nadeem@emumba.com> In-Reply-To: From: Usama Nadeem Date: Tue, 16 Nov 2021 10:20:35 +0500 Message-ID: Subject: Re: [dpdk-dev] [PATCH v4] examples/l3fwd: ipv4 and udp/tcp cksum verification through software To: "Medvedkin, Vladimir" Cc: "Walsh, Conor" , "thomas@monjalon.net" , "dev@dpdk.org" Content-Type: multipart/alternative; boundary="00000000000073c6dd05d0e116e1" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --00000000000073c6dd05d0e116e1 Content-Type: text/plain; charset="UTF-8" Hi Medvedkin, Vladimir, Thank you for your suggestions. Two counters aren't really necessary. We also don't need the "dropped" variable. "continue" can also be used to implement logic. We will update the logic in the next patch version. This patch supports only IPV4 packets in LPM. It does not support other lookup methods, neither does it support IPV6 packets for now. If the current patch is satisfactory, we intend to begin work on those as well. Regarding perf drop, I will submit a new version of the patch, containing the callback. On Thu, Nov 4, 2021 at 9:19 PM Medvedkin, Vladimir < vladimir.medvedkin@intel.com> wrote: > Hi Usama, > > On 04/11/2021 12:11, Walsh, Conor wrote: > >> From: dev On Behalf Of Usama Nadeem > >> Sent: Thursday 14 October 2021 19:43 > >> To: thomas@monjalon.net > >> Cc: dev@dpdk.org; Usama Nadeem > >> Subject: [dpdk-dev] [PATCH v4] examples/l3fwd: ipv4 and udp/tcp cksum > >> verification through software > >> > >> checks if ipv4 and udptcp cksum offload capability available > >> If not available, cksum is verified through software > >> If cksum is corrupt, packet is dropped, rest of the packets > >> are forwarded back. > >> > >> Bugzilla ID:545 > >> Signed-off-by: Usama Nadeem > >> --- > > > > Hi Usama, > > > > This should be done in a generic way that allows all the lookup methods > to support it not just LPM. > > check_software_cksum should go in a common file and be called from LPM, > FIB and possibly EM. > > > > Thanks, > > Conor. > > > >> examples/l3fwd/l3fwd.h | 6 ++++ > >> examples/l3fwd/l3fwd_lpm.c | 72 > >> ++++++++++++++++++++++++++++++++++++-- > >> examples/l3fwd/main.c | 33 +++++++++++++++-- > >> 3 files changed, 105 insertions(+), 6 deletions(-) > >> > >> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h > >> index a808d60247..c2c21a91fb 100644 > >> --- a/examples/l3fwd/l3fwd.h > >> +++ b/examples/l3fwd/l3fwd.h > >> @@ -55,6 +55,8 @@ > >> #define L3FWD_HASH_ENTRIES (1024*1024*1) > >> #endif > >> #define HASH_ENTRY_NUMBER_DEFAULT 4 > >> +extern bool l3_sft_cksum; > >> +extern bool l4_sft_cksum; > >> > >> struct mbuf_table { > >> uint16_t len; > >> @@ -210,6 +212,10 @@ em_main_loop(__rte_unused void *dummy); > >> int > >> lpm_main_loop(__rte_unused void *dummy); > >> > >> +int > >> +check_software_cksum(struct rte_mbuf **pkts_burst, > >> +struct rte_mbuf **pkts_burst_to_send, int nb_rx); > >> + > >> int > >> fib_main_loop(__rte_unused void *dummy); > >> > >> diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c > >> index 232b606b54..ecaf323943 100644 > >> --- a/examples/l3fwd/l3fwd_lpm.c > >> +++ b/examples/l3fwd/l3fwd_lpm.c > >> @@ -26,6 +26,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include "l3fwd.h" > >> #include "l3fwd_event.h" > >> @@ -139,16 +140,65 @@ lpm_get_dst_port_with_ipv4(const struct > >> lcore_conf *qconf, struct rte_mbuf *pkt, > >> #include "l3fwd_lpm.h" > >> #endif > >> > >> + > >> +int check_software_cksum(struct rte_mbuf **pkts_burst, > >> +struct rte_mbuf **pkts_burst_to_send, int nb_rx) > >> +{ > >> + int j; > >> + int i = 0; > >> + struct rte_net_hdr_lens hdr_lens; > >> + struct rte_ipv4_hdr *ipv4_hdr; > >> + void *l3_hdr; > >> + void *l4_hdr; > >> + rte_be16_t prev_cksum; > >> + int dropped_pkts_udp_tcp = 0; > >> + int dropped_pkts_ipv4 = 0; > > Why do you need two separate counters if you eventually summing them up? > > >> + bool dropped; > >> + for (j = 0; j < nb_rx; j++) { > >> + dropped = false; > >> + rte_net_get_ptype(pkts_burst[j], &hdr_lens, > >> RTE_PTYPE_ALL_MASK); > >> + l3_hdr = rte_pktmbuf_mtod_offset(pkts_burst[j], > >> + void *, hdr_lens.l2_len); > >> + l4_hdr = rte_pktmbuf_mtod_offset(pkts_burst[j], > >> + void *, hdr_lens.l2_len + hdr_lens.l3_len); > > here hdr_lens.l3_len could be non initialized, for example in case of > MPLS packet. > > >> + ipv4_hdr = l3_hdr; > >> + prev_cksum = ipv4_hdr->hdr_checksum; > > it could be non IPv4 packet. > > >> + ipv4_hdr->hdr_checksum = 0; > >> + ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr); > > same here and below, it can be IPv6 for example. > > >> + > >> + if (l3_sft_cksum && prev_cksum != ipv4_hdr- > >>> hdr_checksum) { > >> + rte_pktmbuf_free(pkts_burst[j]); > >> + dropped_pkts_ipv4++; > >> + dropped = true; > > Do you need "dropped" value + the the final if statement at all? Maybe > it's better to just > ... > continue; > } > here... > > >> + } else if (l4_sft_cksum && > >> + rte_ipv4_udptcp_cksum_verify > >> + (l3_hdr, l4_hdr) != 0) { > >> + > >> + rte_pktmbuf_free(pkts_burst[j]); > >> + dropped_pkts_udp_tcp++; > >> + dropped = true; > >> + } > >> + if (dropped == false) { >> + > pkts_burst_to_send[i] = pkts_burst[j]; > >> + i++; > > ...and execute this code unconditionally? > > >> + } > >> + > >> + } > >> + return dropped_pkts_udp_tcp+dropped_pkts_ipv4; > >> +} > >> + > >> /* main processing loop */ > >> int > >> lpm_main_loop(__rte_unused void *dummy) > >> { > >> struct rte_mbuf *pkts_burst[MAX_PKT_BURST]; > >> + struct rte_mbuf *pkts_burst_to_send[MAX_PKT_BURST]; > >> unsigned lcore_id; > >> uint64_t prev_tsc, diff_tsc, cur_tsc; > >> int i, nb_rx; > >> uint16_t portid; > >> uint8_t queueid; > >> + int dropped; > >> struct lcore_conf *qconf; > >> const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) / > >> US_PER_S * BURST_TX_DRAIN_US; > >> @@ -209,19 +259,35 @@ lpm_main_loop(__rte_unused void *dummy) > >> if (nb_rx == 0) > >> continue; > >> > >> + if (l3_sft_cksum || l4_sft_cksum) { > >> + dropped = > >> check_software_cksum(pkts_burst, > > You are calling this function fight after rte_eth_rx_burst(), so > pkts_burst[] can have any possible packet proto, but current > check_software_cksum() implementation if purely IPv4. > > >> + pkts_burst_to_send, nb_rx); > >> + > >> + nb_rx = nb_rx-dropped; > >> + } > >> + > >> + > >> #if defined RTE_ARCH_X86 || defined __ARM_NEON \ > >> || defined RTE_ARCH_PPC_64 > >> + if (l3_sft_cksum == false && l4_sft_cksum == false) > >> l3fwd_lpm_send_packets(nb_rx, pkts_burst, > >> portid, qconf); > >> + else > >> + l3fwd_lpm_send_packets(nb_rx, > >> pkts_burst_to_send, > >> + portid, qconf); > >> + > >> #else > >> - l3fwd_lpm_no_opt_send_packets(nb_rx, > >> pkts_burst, > >> + if (l3_sft_cksum == false && l4_sft_cksum == false) > > While those if statements are perfectly predictable, it is still better > to avoid branching in hot path if possible. You can implement > check_software_cksum() to work with a single pkts_burst[] modifying it > in place and throw away pkts_burst_to_send[] and corresponding branches. > > >> + l3fwd_lpm_no_opt_send_packets(nb_rx, > >> pkts_burst, > >> portid, qconf); > >> + else > >> + l3fwd_lpm_no_opt_send_packets(nb_rx, > >> + pkts_burst_to_send, portid, qconf); > >> + > >> #endif /* X86 */ > >> } > >> - > >> cur_tsc = rte_rdtsc(); > >> } > >> - > >> return 0; > >> } > >> > >> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c > >> index 00ac267af1..a54aca070d 100644 > >> --- a/examples/l3fwd/main.c > >> +++ b/examples/l3fwd/main.c > >> @@ -61,6 +61,9 @@ static uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; > >> /**< Ports set in promiscuous mode off by default. */ > >> static int promiscuous_on; > >> > >> +bool l3_sft_cksum; > >> +bool l4_sft_cksum; > >> + > >> /* Select Longest-Prefix, Exact match or Forwarding Information Base. > */ > >> enum L3FWD_LOOKUP_MODE { > >> L3FWD_LOOKUP_DEFAULT, > >> @@ -123,7 +126,6 @@ static struct rte_eth_conf port_conf = { > >> .mq_mode = ETH_MQ_RX_RSS, > >> .max_rx_pkt_len = RTE_ETHER_MAX_LEN, > >> .split_hdr_size = 0, > >> - .offloads = DEV_RX_OFFLOAD_CHECKSUM, > >> }, > >> .rx_adv_conf = { > >> .rss_conf = { > >> @@ -981,6 +983,7 @@ prepare_ptype_parser(uint16_t portid, uint16_t > >> queueid) > >> return 0; > >> } > >> > >> + > >> static void > >> l3fwd_poll_resource_setup(void) > >> { > >> @@ -993,7 +996,8 @@ l3fwd_poll_resource_setup(void) > >> unsigned int nb_ports; > >> unsigned int lcore_id; > >> int ret; > >> - > >> + l3_sft_cksum = false; > >> + l4_sft_cksum = false; > >> if (check_lcore_params() < 0) > >> rte_exit(EXIT_FAILURE, "check_lcore_params failed\n"); > >> > >> @@ -1034,11 +1038,34 @@ l3fwd_poll_resource_setup(void) > >> rte_exit(EXIT_FAILURE, > >> "Error during getting device (port %u) > info: > >> %s\n", > >> portid, strerror(-ret)); > >> - > >> if (dev_info.tx_offload_capa & > >> DEV_TX_OFFLOAD_MBUF_FAST_FREE) > >> local_port_conf.txmode.offloads |= > >> DEV_TX_OFFLOAD_MBUF_FAST_FREE; > >> > >> + if (dev_info.rx_offload_capa & > >> DEV_RX_OFFLOAD_IPV4_CKSUM) > >> + local_port_conf.rxmode.offloads |= > >> + DEV_RX_OFFLOAD_IPV4_CKSUM; > >> + else { > >> + l3_sft_cksum = true; > >> + printf("WARNING: IPV4 checksum offload not > >> available.\n"); > >> + } > >> + > >> + if (dev_info.rx_offload_capa & > >> DEV_RX_OFFLOAD_UDP_CKSUM) > >> + local_port_conf.rxmode.offloads |= > >> + DEV_RX_OFFLOAD_UDP_CKSUM; > >> + else { > >> + l4_sft_cksum = true; > >> + printf("WARNING: UDP checksum offload not > >> available.\n"); > >> + } > >> + > >> + if (dev_info.rx_offload_capa & > >> DEV_RX_OFFLOAD_TCP_CKSUM) > >> + local_port_conf.rxmode.offloads |= > >> + DEV_RX_OFFLOAD_TCP_CKSUM; > >> + else { > >> + l4_sft_cksum = true; > >> + printf("WARNING: TCP checksum offload not > >> available.\n"); > >> + } > >> + > >> local_port_conf.rx_adv_conf.rss_conf.rss_hf &= > >> dev_info.flow_type_rss_offloads; > >> > >> -- > >> 2.25.1 > > > > generalizing: > 1. As Conor said earlier, make it generic, not only LPM and not only for > run-to-completion mode, it should be done for event mode as well. > 2. Function must work not only with IPv4. > 3. There should be no performance degradation if NIC supports CSUM offload. > > -- > Regards, > Vladimir > -- -Usama --00000000000073c6dd05d0e116e1 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Hi Medved= kin, Vladimir,

<= br>

Thank you fo= r your suggestions. Two counters aren't really necessary. We also don&#= 39;t need the "dropped" variable. "continue" can also b= e used to implement logic. We will update the logic in the next patch versi= on.


This pat= ch supports only IPV4 packets in LPM. It does not support other lookup meth= ods, neither does it support IPV6 packets for now. If the current patch is = satisfactory, we intend to begin work on those as well.


Regarding perf drop, I will sub= mit a new version of the patch, containing the callback.

<= br class=3D"gmail-Apple-interchange-newline">

On Thu, Nov 4, 2021 at 9:19 PM= Medvedkin, Vladimir <vl= adimir.medvedkin@intel.com> wrote:
Hi Usama,

On 04/11/2021 12:11, Walsh, Conor wrote:
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Usama Nadeem
>> Sent: Thursday 14 October 2021 19:43
>> To: thoma= s@monjalon.net
>> Cc: dev@dpdk.org= ; Usama Nadeem <usama.nadeem@emumba.com>
>> Subject: [dpdk-dev] [PATCH v4] examples/l3fwd: ipv4 and udp/tcp ck= sum
>> verification through software
>>
>> checks if ipv4 and udptcp cksum offload capability available
>> If not available, cksum is verified through software
>> If cksum is corrupt, packet is dropped, rest of the packets
>> are forwarded back.
>>
>> Bugzilla ID:545
>> Signed-off-by: Usama Nadeem <usama.nadeem@emumba.com>
>> ---
>
> Hi Usama,
>
> This should be done in a generic way that allows all the lookup method= s to support it not just LPM.
> check_software_cksum should go in a common file and be called from LPM= , FIB and possibly EM.
>
> Thanks,
> Conor.
>
>>=C2=A0 =C2=A0examples/l3fwd/l3fwd.h=C2=A0 =C2=A0 =C2=A0|=C2=A0 6 ++= ++
>>=C2=A0 =C2=A0examples/l3fwd/l3fwd_lpm.c | 72
>> ++++++++++++++++++++++++++++++++++++--
>>=C2=A0 =C2=A0examples/l3fwd/main.c=C2=A0 =C2=A0 =C2=A0 | 33 +++++++= ++++++++--
>>=C2=A0 =C2=A03 files changed, 105 insertions(+), 6 deletions(-)
>>
>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>> index a808d60247..c2c21a91fb 100644
>> --- a/examples/l3fwd/l3fwd.h
>> +++ b/examples/l3fwd/l3fwd.h
>> @@ -55,6 +55,8 @@
>>=C2=A0 =C2=A0#define L3FWD_HASH_ENTRIES=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0(1024*1024*1)
>>=C2=A0 =C2=A0#endif
>>=C2=A0 =C2=A0#define HASH_ENTRY_NUMBER_DEFAULT=C2=A0 4
>> +extern bool l3_sft_cksum;
>> +extern bool l4_sft_cksum;
>>
>>=C2=A0 =C2=A0struct mbuf_table {
>>=C2=A0 =C2=A0 =C2=A0 uint16_t len;
>> @@ -210,6 +212,10 @@ em_main_loop(__rte_unused void *dummy);
>>=C2=A0 =C2=A0int
>>=C2=A0 =C2=A0lpm_main_loop(__rte_unused void *dummy);
>>
>> +int
>> +check_software_cksum(struct rte_mbuf **pkts_burst,
>> +struct rte_mbuf **pkts_burst_to_send, int nb_rx);
>> +
>>=C2=A0 =C2=A0int
>>=C2=A0 =C2=A0fib_main_loop(__rte_unused void *dummy);
>>
>> diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm= .c
>> index 232b606b54..ecaf323943 100644
>> --- a/examples/l3fwd/l3fwd_lpm.c
>> +++ b/examples/l3fwd/l3fwd_lpm.c
>> @@ -26,6 +26,7 @@
>>=C2=A0 =C2=A0#include <rte_udp.h>
>>=C2=A0 =C2=A0#include <rte_lpm.h>
>>=C2=A0 =C2=A0#include <rte_lpm6.h>
>> +#include <rte_net.h>
>>
>>=C2=A0 =C2=A0#include "l3fwd.h"
>>=C2=A0 =C2=A0#include "l3fwd_event.h"
>> @@ -139,16 +140,65 @@ lpm_get_dst_port_with_ipv4(const struct
>> lcore_conf *qconf, struct rte_mbuf *pkt,
>>=C2=A0 =C2=A0#include "l3fwd_lpm.h"
>>=C2=A0 =C2=A0#endif
>>
>> +
>> +int check_software_cksum(struct rte_mbuf **pkts_burst,
>> +struct rte_mbuf **pkts_burst_to_send, int nb_rx)
>> +{
>> +=C2=A0 =C2=A0 int j;
>> +=C2=A0 =C2=A0 int i =3D 0;
>> +=C2=A0 =C2=A0 struct rte_net_hdr_lens hdr_lens;
>> +=C2=A0 =C2=A0 struct rte_ipv4_hdr *ipv4_hdr;
>> +=C2=A0 =C2=A0 void *l3_hdr;
>> +=C2=A0 =C2=A0 void *l4_hdr;
>> +=C2=A0 =C2=A0 rte_be16_t prev_cksum;
>> +=C2=A0 =C2=A0 int dropped_pkts_udp_tcp =3D 0;
>> +=C2=A0 =C2=A0 int dropped_pkts_ipv4 =3D 0;

Why do you need two separate counters if you eventually summing them up?
>> +=C2=A0 =C2=A0 bool dropped;
>> +=C2=A0 =C2=A0 for (j =3D 0; j < nb_rx; j++) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dropped =3D false;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rte_net_get_ptype(pkts_= burst[j], &hdr_lens,
>> RTE_PTYPE_ALL_MASK);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 l3_hdr =3D rte_pktmbuf_= mtod_offset(pkts_burst[j],
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 void *, hdr_lens.l2_len= );
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 l4_hdr =3D rte_pktmbuf_= mtod_offset(pkts_burst[j],
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 void *, hdr_lens.l2_len= + hdr_lens.l3_len);

here hdr_lens.l3_len could be non initialized, for example in case of
MPLS packet.

>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ipv4_hdr =3D l3_hdr; >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 prev_cksum =3D ipv4_hdr= ->hdr_checksum;

it could be non IPv4 packet.

>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ipv4_hdr->hdr_checks= um =3D 0;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ipv4_hdr->hdr_checks= um =3D rte_ipv4_cksum(ipv4_hdr);

same here and below, it can be IPv6 for example.

>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (l3_sft_cksum &&= amp; prev_cksum !=3D ipv4_hdr-
>>> hdr_checksum) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 rte_pktmbuf_free(pkts_burst[j]);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 dropped_pkts_ipv4++;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 dropped =3D true;

Do you need "dropped" value + the the final if statement at all? = Maybe
it's better to just
...
=C2=A0 =C2=A0 =C2=A0continue;
}
here...

>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (l4_sft_cksum= &&
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rte_ipv4_udptcp_cksum_verify
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (l3_hdr, l4_hdr) !=3D 0) {
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 rte_pktmbuf_free(pkts_burst[j]);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 dropped_pkts_udp_tcp++;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 dropped =3D true;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dropped =3D=3D fals= e) { >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 pkts_burst_to_send[i] =3D pkts_burst[j];
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 i++;

...and execute this code unconditionally?

>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> +
>> +=C2=A0 =C2=A0 }
>> +=C2=A0 =C2=A0 return dropped_pkts_udp_tcp+dropped_pkts_ipv4;
>> +}
>> +
>>=C2=A0 =C2=A0/* main processing loop */
>>=C2=A0 =C2=A0int
>>=C2=A0 =C2=A0lpm_main_loop(__rte_unused void *dummy)
>>=C2=A0 =C2=A0{
>>=C2=A0 =C2=A0 =C2=A0 struct rte_mbuf *pkts_burst[MAX_PKT_BURST]; >> +=C2=A0 =C2=A0 struct rte_mbuf *pkts_burst_to_send[MAX_PKT_BURST];=
>>=C2=A0 =C2=A0 =C2=A0 unsigned lcore_id;
>>=C2=A0 =C2=A0 =C2=A0 uint64_t prev_tsc, diff_tsc, cur_tsc;
>>=C2=A0 =C2=A0 =C2=A0 int i, nb_rx;
>>=C2=A0 =C2=A0 =C2=A0 uint16_t portid;
>>=C2=A0 =C2=A0 =C2=A0 uint8_t queueid;
>> +=C2=A0 =C2=A0 int dropped;
>>=C2=A0 =C2=A0 =C2=A0 struct lcore_conf *qconf;
>>=C2=A0 =C2=A0 =C2=A0 const uint64_t drain_tsc =3D (rte_get_tsc_hz()= + US_PER_S - 1) /
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 US_PER_S * BURST_T= X_DRAIN_US;
>> @@ -209,19 +259,35 @@ lpm_main_loop(__rte_unused void *dummy)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (nb_rx =3D=3D 0)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 continue;
>>
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 if (l3_sft_cksum || l4_sft_cksum) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dropped =3D
>> check_software_cksum(pkts_burst,

You are calling this function fight after rte_eth_rx_burst(), so
pkts_burst[] can have any possible packet proto, but current
check_software_cksum() implementation if purely IPv4.

>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pkts_burst_to_send,=C2=A0 =C2=A0 =C2=A0n= b_rx);
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nb_rx =3D nb_rx-dropped;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 }
>> +
>> +
>>=C2=A0 =C2=A0#if defined RTE_ARCH_X86 || defined __ARM_NEON \
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0|| defined RTE_ARCH_PPC_64
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (l3_sft_cksum =3D=3D= false && l4_sft_cksum =3D=3D false)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 l3fwd_lpm_send_packets(nb_rx, pkts_burst,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 portid, qconf);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 l3fwd_lpm_send_packets(nb_rx,
>> pkts_burst_to_send,
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 portid, qconf);
>> +
>>=C2=A0 =C2=A0#else
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 l3fwd_lpm_no_opt_send_packets(nb_rx,
>> pkts_burst,
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 if (l3_sft_cksum =3D=3D false && l4_sft_cksum =3D=3D false)<= br>
While those if statements are perfectly predictable, it is still better to avoid branching in hot path if possible. You can implement
check_software_cksum() to work with a single pkts_burst[] modifying it
in place and throw away pkts_burst_to_send[] and corresponding branches.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 l3fwd_lpm_no_opt_send_packets(nb_rx,
>> pkts_burst,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 portid, qconf);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 else
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 l3fwd_lpm_no_opt_send_packets(nb_rx,
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pkts_burst_to_send, portid, qconf);
>> +
>>=C2=A0 =C2=A0#endif /* X86 */
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> -
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cur_tsc =3D rte_rd= tsc();
>>=C2=A0 =C2=A0 =C2=A0 }
>> -
>>=C2=A0 =C2=A0 =C2=A0 return 0;
>>=C2=A0 =C2=A0}
>>
>> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
>> index 00ac267af1..a54aca070d 100644
>> --- a/examples/l3fwd/main.c
>> +++ b/examples/l3fwd/main.c
>> @@ -61,6 +61,9 @@ static uint16_t nb_txd =3D RTE_TEST_TX_DESC_DEFA= ULT;
>>=C2=A0 =C2=A0/**< Ports set in promiscuous mode off by default. = */
>>=C2=A0 =C2=A0static int promiscuous_on;
>>
>> +bool l3_sft_cksum;
>> +bool l4_sft_cksum;
>> +
>>=C2=A0 =C2=A0/* Select Longest-Prefix, Exact match or Forwarding In= formation Base. */
>>=C2=A0 =C2=A0enum L3FWD_LOOKUP_MODE {
>>=C2=A0 =C2=A0 =C2=A0 L3FWD_LOOKUP_DEFAULT,
>> @@ -123,7 +126,6 @@ static struct rte_eth_conf port_conf =3D {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .mq_mode =3D ETH_M= Q_RX_RSS,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .max_rx_pkt_len = =3D RTE_ETHER_MAX_LEN,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .split_hdr_size = =3D 0,
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .offloads =3D DEV_RX_OF= FLOAD_CHECKSUM,
>>=C2=A0 =C2=A0 =C2=A0 },
>>=C2=A0 =C2=A0 =C2=A0 .rx_adv_conf =3D {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .rss_conf =3D { >> @@ -981,6 +983,7 @@ prepare_ptype_parser(uint16_t portid, uint16_t=
>> queueid)
>>=C2=A0 =C2=A0 =C2=A0 return 0;
>>=C2=A0 =C2=A0}
>>
>> +
>>=C2=A0 =C2=A0static void
>>=C2=A0 =C2=A0l3fwd_poll_resource_setup(void)
>>=C2=A0 =C2=A0{
>> @@ -993,7 +996,8 @@ l3fwd_poll_resource_setup(void)
>>=C2=A0 =C2=A0 =C2=A0 unsigned int nb_ports;
>>=C2=A0 =C2=A0 =C2=A0 unsigned int lcore_id;
>>=C2=A0 =C2=A0 =C2=A0 int ret;
>> -
>> +=C2=A0 =C2=A0 l3_sft_cksum =3D false;
>> +=C2=A0 =C2=A0 l4_sft_cksum =3D false;
>>=C2=A0 =C2=A0 =C2=A0 if (check_lcore_params() < 0)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rte_exit(EXIT_FAIL= URE, "check_lcore_params failed\n");
>>
>> @@ -1034,11 +1038,34 @@ l3fwd_poll_resource_setup(void)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 rte_exit(EXIT_FAILURE,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Error during getting device (p= ort %u) info:
>> %s\n",
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 portid, strerror(-ret));
>> -
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dev_info.tx_of= fload_capa &
>> DEV_TX_OFFLOAD_MBUF_FAST_FREE)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 local_port_conf.txmode.offloads |=3D
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEV_TX_OFFLOAD_MBUF_FAST_FREE;
>>
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dev_info.rx_offload= _capa &
>> DEV_RX_OFFLOAD_IPV4_CKSUM)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 local_port_conf.rxmode.offloads |=3D
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 DEV_RX_OFFLOAD_IPV4_CKSUM;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 l3_sft_cksum =3D true;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 printf("WARNING: IPV4 checksum offload not
>> available.\n");
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 }
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dev_info.rx_offload= _capa &
>> DEV_RX_OFFLOAD_UDP_CKSUM)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 local_port_conf.rxmode.offloads |=3D
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEV_RX_OFFLOAD_UDP_CKSUM;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 l4_sft_cksum =3D true;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 printf("WARNING: UDP checksum offload not
>> available.\n");
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dev_info.rx_offload= _capa &
>> DEV_RX_OFFLOAD_TCP_CKSUM)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 local_port_conf.rxmode.offloads |=3D
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEV_RX_OFFLOAD_TCP_CKSUM;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 l4_sft_cksum =3D true;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 printf("WARNING: TCP checksum offload not
>> available.\n");
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> +
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 local_port_conf.rx= _adv_conf.rss_conf.rss_hf &=3D
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 dev_info.flow_type_rss_offloads;
>>
>> --
>> 2.25.1
>

generalizing:
1. As Conor said earlier, make it generic, not only LPM and not only for run-to-completion mode, it should be done for event mode as well.
2. Function must work not only with IPv4.
3. There should be no performance degradation if NIC supports CSUM offload.=

--
Regards,
Vladimir


--
-Usama
--00000000000073c6dd05d0e116e1--