From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by dpdk.org (Postfix) with ESMTP id A0AB28062 for ; Thu, 11 Dec 2014 11:52:37 +0100 (CET) Received: by mail-wg0-f52.google.com with SMTP id x12so6011324wgg.25 for ; Thu, 11 Dec 2014 02:52:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=6MDGkQCQYTVNkOcg+17K1pt36A7/rmDR9dJtaBRXPM8=; b=NlI7lk7a2AfkrelBFxGehI2hwRGwHAprAJ0CiFP7D6noODgYIB7h4Bi1e4PdnI2GmU BnHo+nipy0i97qDs1uFdV3vSozdcaKdvJPBNrieWdxel5vi30H5VvfVATtES5GRXb3Dn fVJwpzA8DwynKNxUPgWbEtAqGIahzFoa64HXW4tDvE8WV2PFqeXBhay+SxTife5lTVZY /OCsFin3flNPKlPIXkhATr+aa8hF0UxINchlHZxqllFuPeUhKlgmnOHLofJdDVq3gHk7 p/iAtANIMq97OehoaEn7ugYo6F9LrDlsnBaYpFQWR0mjO8NlYcOoiD/NExrwRDfgSUxn 1SUQ== X-Gm-Message-State: ALoCoQkoiU87gHLM34V+L6/nfQz1XJul0o4heE6KxD7zl4L9mozXeGZzWt4goPjsjtJEr0zjuKo4 X-Received: by 10.194.92.37 with SMTP id cj5mr15508014wjb.81.1418295157376; Thu, 11 Dec 2014 02:52:37 -0800 (PST) Received: from [10.16.0.195] (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by mx.google.com with ESMTPSA id n5sm1840757wic.6.2014.12.11.02.52.36 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 11 Dec 2014 02:52:36 -0800 (PST) Message-ID: <54897774.6090107@6wind.com> Date: Thu, 11 Dec 2014 11:52:36 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 MIME-Version: 1.0 To: Jijiang Liu , dev@dpdk.org References: <1418173403-30202-1-git-send-email-jijiang.liu@intel.com> <1418173403-30202-4-git-send-email-jijiang.liu@intel.com> In-Reply-To: <1418173403-30202-4-git-send-email-jijiang.liu@intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 3/3] app/testpmd:change tx_checksum command and csum forwarding engine X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Dec 2014 10:52:37 -0000 Hi Jijiang, Some more comments, in addition to the one I've made in the cover letter. Reference link for patchwork readers: http://dpdk.org/ml/archives/dev/2014-December/009886.html On 12/10/2014 02:03 AM, Jijiang Liu wrote: > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -316,16 +316,30 @@ static void cmd_help_long_parsed(void *parsed_result, > " Disable hardware insertion of a VLAN header in" > " packets sent on a port.\n\n" > > - "tx_cksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port_id)\n" > + "tx_cksum set (ip|udp|tcp|sctp) (hw|sw) (port_id)\n" > " Select hardware or software calculation of the" > " checksum with when transmitting a packet using the" > " csum forward engine.\n" > - " ip|udp|tcp|sctp always concern the inner layer.\n" > - " vxlan concerns the outer IP and UDP layer (in" > - " case the packet is recognized as a vxlan packet by" > - " the forward engine)\n" > + " In the case of tunneling packet, ip|udp|tcp|sctp" > + " always concern the inner layer.\n\n" > + > + "tx_cksum set tunnel (hw|sw|none) (port_id)\n" > + " Select hardware or software calculation of the" > + " checksum with when transmitting a tunneling packet" > + " using the csum forward engine.\n" > + " The none option means treat tunneling packet as ordinary" > + " packet when using the csum forward engine\n." > + " Tunneling packet concerns the outer IP, inner IP" > + " and inner L4\n" > " Please check the NIC datasheet for HW limits.\n\n" > > + "tx_cksum set (outer-ip) (hw|sw) (port_id)\n" > + " Select hardware or software calculation of the" > + " checksum with when transmitting a packet using the" > + " csum forward engine.\n" > + " outer-ip always concern the outer layer of" > + " tunneling packet.\n\n" > + > "tx_checksum show (port_id)\n" > " Display tx checksum offload configuration\n\n" > not sure we need 2 different commands for tx_cksum set (outer-ip) and tx_cksum set (ip|udp|tcp|sctp). As the syntax is exactly the same, it may result in less code to have only one command. > --- a/app/test-pmd/csumonly.c > +++ b/app/test-pmd/csumonly.c > @@ -256,17 +256,16 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype, > struct udp_hdr *udp_hdr; > uint64_t ol_flags = 0; > > - if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) > - ol_flags |= PKT_TX_UDP_TUNNEL_PKT; > - > if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) { > ipv4_hdr->hdr_checksum = 0; > > - if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) > + if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) > ol_flags |= PKT_TX_OUTER_IP_CKSUM; > - else > + else { > ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr); > - } else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) > + ol_flags |= PKT_TX_OUTER_IPV4; > + } > + } else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) > ol_flags |= PKT_TX_OUTER_IPV6; > > udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + outer_l3_len); > @@ -300,11 +299,14 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype, > * Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 / > * UDP|TCP|SCTP > * > - * The testpmd command line for this forward engine sets the flags > + * These testpmd command lines for this forward engine sets the flags > * TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control > - * wether a checksum must be calculated in software or in hardware. The > - * IP, UDP, TCP and SCTP flags always concern the inner layer. The > - * VxLAN flag concerns the outer IP (if packet is recognized as a vxlan packet). > + * wether a checksum must be calculated in software or in hardware. > + * In the case of tunneling packet, the IP, UDP, TCP and SCTP flags > + * always concern the inner layer; the outer IP flag always concern > + * the outer layer; the tunnel flag is used to tell the NIC that it > + * is a tunneing packet, want hardware offload for outer layer, > + * or inner layer, or both. tunneing -> tunneling "the tunnel flag is used to tell the NIC that it is a tunneing packet, want hardware offload for outer layer, or inner layer, or both." what does that mean? > */ > static void > pkt_burst_checksum_forward(struct fwd_stream *fs) > @@ -376,7 +378,9 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) > l3_hdr = (char *)eth_hdr + l2_len; > > /* check if it's a supported tunnel (only vxlan for now) */ > - if (l4_proto == IPPROTO_UDP) { > + if (((testpmd_ol_flags & > + TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM) == 0) > + && (l4_proto == IPPROTO_UDP)) { > udp_hdr = (struct udp_hdr *)((char *)l3_hdr + l3_len); > > /* check udp destination port, 4789 is the default > @@ -386,17 +390,23 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) > tunnel = 1; > > /* currently, this flag is set by i40e only if the > - * packet is vxlan */ > + * packet is a tunneling packet */ > } else if (m->ol_flags & (PKT_RX_TUNNEL_IPV4_HDR | > PKT_RX_TUNNEL_IPV6_HDR)) > tunnel = 1; > > if (tunnel == 1) { > + > + if (testpmd_ol_flags > + & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) > + ol_flags |= PKT_TX_UDP_TUNNEL_PKT; > + > outer_ethertype = ethertype; > outer_l2_len = l2_len; > outer_l3_len = l3_len; > outer_l3_hdr = l3_hdr; > > + /* currently, only VXLAN packet is supported */ > eth_hdr = (struct ether_hdr *)((char *)udp_hdr + > sizeof(struct udp_hdr) + > sizeof(struct vxlan_hdr)); > @@ -434,7 +444,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) > /* step 4: fill the mbuf meta data (flags and header lengths) */ > > if (tunnel == 1) { > - if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) { > + if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) { > m->outer_l2_len = outer_l2_len; > m->outer_l3_len = outer_l3_len; > m->l2_len = l4_tun_len + l2_len; > @@ -505,7 +515,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) > "m->l4_len=%d\n", > m->l2_len, m->l3_len, m->l4_len); > if ((tunnel == 1) && > - (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)) > + (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)) > printf("tx: m->outer_l2_len=%d m->outer_l3_len=%d\n", > m->outer_l2_len, m->outer_l3_len); > if (tso_segsz != 0) > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > index f8b0740..09caa6a 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -125,10 +125,20 @@ struct fwd_stream { > #define TESTPMD_TX_OFFLOAD_TCP_CKSUM 0x0004 > /** Offload SCTP checksum in csum forward engine */ > #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM 0x0008 > -/** Offload VxLAN checksum in csum forward engine */ > -#define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM 0x0010 > +/** Offload tunneling packet checksum in csum forward engine */ > +#define TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM 0x0010 > /** Insert VLAN header in forward engine */ > #define TESTPMD_TX_OFFLOAD_INSERT_VLAN 0x0020 > +/** > + * Offload outer-IP checksum in csum forward engine > + * for tunneling packet > + */ > +#define TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM 0x0040 > +/** > + * For a tunneling packet, user requests HW offload for its outer > + * layer checksum, and don't care is it a tunneled packet or not. > + */ > +#define TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM 0x0080 > > /** > * The data structure associated with each port. > For now, I did not check the implementation of the patch. I'm not sure I understand the specifications, so I cannot check if the code conforms to it. How this patch is tested? I think a report similar to http://dpdk.org/ml/archives/dev/2014-November/007991.html would help to verify that it works (at least on one driver), and to understand the test-pmd API and the different use cases. Regards, Olivier