DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier MATZ <olivier.matz@6wind.com>
To: Jijiang Liu <jijiang.liu@intel.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
Date: Thu, 11 Dec 2014 11:17:46 +0100
Message-ID: <54896F4A.4070601@6wind.com> (raw)
In-Reply-To: <1418173403-30202-1-git-send-email-jijiang.liu@intel.com>

Hi Jijiang,

Sorry for the late review, I was very busy these last days. Please find
my comments below.

On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> In the current codes, the "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command is not easy to understand and extend, so the patch set enhances the tx_checksum command and reworks csum forwarding engine due to the change of tx_checksum command.
> The main changes of the tx_checksum command are listed below,
>
> 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
>
> The command is used to set/clear tunnel flag that is used to tell the NIC that the packetg is a tunneing packet and application want hardware TX checksum offload for outer layer, or inner layer, or both.

packetg -> packet
tunneing -> tunneling

I don't understand the description: this flag cannot be set to tell the
NIC that it's a tunnel packet because it's a configuration flag.
Whatever the value of this configuration option, the packets can be
either tunnel or non-tunnel packets. The real question is, what is
the behavior for each packet type for each value for this option.

> The 'none' option means that user treat tunneling packet as ordinary packet when using the csum forward engine.
> for example, let say we have a tunnel packet: eth_hdr_out/ipv4_hdr_out/udp_hdr_out/vxlan_hdr/ehtr_hdr_in/ipv4_hdr_in/tcp_hdr_in. one of several scenarios:
>
> 1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunnelled packet or not. So he sets:

tunnelled -> tunneled

>
> tx_checksum set tunnel none 0
>
> tx_checksum set ip hw 0
>
> So for such case we should set tx_tunnel to 'none'.
>
> 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
>
> The command is used to set/clear outer IP flag that is used to tell the NIC that application want hardware offload for outer layer.
>
> 3> remove the 'vxlan' option from the  "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
>
> The command is used to set IP, UDP, TCP and SCTP TX checksum flag. In the case of tunneling packet, the IP, UDP, TCP and SCTP flags always concern inner layer.
>
> Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag and add the TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag in test-pmd application.

You are mixing scenario descriptions with what you do in your patchset:
1/ is a scenario
2/ and 3/ are descriptions of added/removed commands

Let's first sumarize what was the behavior before this patch. This
is the description in csumonly code.

Receive a burst of packets, and for each packet:
  - parse packet, and try to recognize a supported packet type (1)
  - if it's not a supported packet type, don't touch the packet, else:
  - modify the IPs in inner headers and in outer headers if any
  - reprocess the checksum of all supported layers. This is done in SW
    or HW, depending on testpmd command line configuration
  - if TSO is enabled in testpmd command line, also flag the mbuf for TCP
    segmentation offload (this implies HW TCP checksum)
Then transmit packets on the output port.

(1) Supported packets are:
   Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .
   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
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).

 From this description, it is easy to deduct this table:

Packet type 1:
  Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .

Packet type 2
   Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
       UDP|TCP|SCTP

+---------------------------+-------------------+-------------------+
|test-pmd config \ packets  |Packet type 1      |Packet type 2      |
+---------------------------+-------------------+-------------------+
|whatever the flags         |IP addresses       |inner and outer IP |
|                           |incremented        |addresses          |
|                           |                   |incremented        |
+---------------------------+-------------------+-------------------+
|IP = sw                    |IP cksum is sw     |inner IP cksum is  |
|                           |                   |sw                 |
+---------------------------+-------------------+-------------------+
|IP = hw                    |IP cksum is hw     |inner IP cksum is  |
|                           |(using lX_len)     |hw (using lX_len)  |
+---------------------------+-------------------+-------------------+
|UDP or TCP or SCTP = sw    |L4 cksum is sw     |inner L4 cksum is  |
|                           |                   |sw                 |
+---------------------------+-------------------+-------------------+
|UDP or TCP or SCTP = hw    |L4 cksum is hw     |inner L4 cksum is  |
|                           |(using lX_len)     |hw (using lX_len)  |
+---------------------------+-------------------+-------------------+
|VxLAN = sw                 |N/A                |outer IP cksum is  |
|                           |                   |sw, outer UDP cksum|
|                           |                   |is sw              |
+---------------------------+-------------------+-------------------+
|VxLAN = hw                 |N/A                |outer IP cksum is  |
|                           |                   |hw (using          |
|                           |                   |outer_lX_len),     |
|                           |                   |outer UDP cksum is |
|                           |                   |sw (no hw support) |
+---------------------------+-------------------+-------------------+


It could be really helpful to have a table like this for what you
are implementing, because I feel there are too many options. Here
is an example of what could be done here (if options are not
independent like before, it can be presented in a different way
in the first column).

Another thing: you don't describe what you want to be able to do:

1/ packet type 1: compute L3 and/or L4 checksum using lX_len
2/ packet type 2: compute inner L3 and/or L4 checksum using lX_len
3/ packet type 2: compute outer L3 and/or L4 checksum using lX_len
4/ packet type 2: compute inner L3 and/or L4 checksum using lX_len
    and outer L3 and/or L4 checksum using outer_lX_len

why not having the 2 following commands:

tx_checksum set 
(ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) <portid>

   select if we use hw or sw calculation for each header type

tx_checksum tunnel (inner|outer|both)

   when a tunnel packet is received in csum only, control wether
   we want to process inner, outer or both headers

The resulting table would be:

+------------------+-----------+-------------------------------------+
|test-pmd \ packets|Packet type|Packet type 2                        |
|                  |1          +-----------+-----------+-------------+
|                  |           |tun = inner|tun = outer|tun = both   |
|                  |           |           |           |             |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|whatever the flags|IP         |inner IP   |outer IP   |inner and    |
|                  |addresses  |addresses  |addresses  |outer IP     |
|                  |incremented|incremented|incremented|addresses    |
|                  |           |           |           |incremented  |
+------------------+-----------+-----------+-----------+-------------+
|IP = sw           |IP cksum is|inner IP   |           |inner IP     |
|                  |sw         |cksum is sw|           |cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|IP = hw           |IP cksum is|inner IP   |           |inner IP     |
|                  |hw (using  |cksum is hw|           |cksum is hw  |
|                  |lX_len)    |(using     |           |(using       |
|                  |           |lX_len)    |           |lX_len)      |
+------------------+-----------+-----------+-----------+-------------+
|UDP or TCP or SCTP|L4 cksum is|inner L4   |           |inner L4     |
|= sw              |sw         |cksum is sw|           |cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|UDP or TCP or SCTP|L4 cksum is|inner L4   |           |inner L4     |
|= hw              |hw (using  |cksum is hw|           |cksum is hw  |
|                  |lX_len)    |(using     |           |(using       |
|                  |           |lX_len)    |           |lX_len)      |
+------------------+-----------+-----------+-----------+-------------+
|outer IP = sw     |N/A        |           |outer IP   |outer IP     |
|                  |           |           |cksum is sw|cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|outer IP = hw     |N/A        |           |outer IP   |outer IP     |
|                  |           |           |cksum is hw|cksum is hw  |
|                  |           |           |(using     |(using       |
|                  |           |           |lX_len)    |outer_lX_len)|
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|outer UDP or TCP  |N/A        |           |outer L4   |outer L4     |
|or SCTP = sw      |           |           |cksum is sw|cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|outer UDP or TCP  |N/A        |           |outer L4   |outer L4     |
|or SCTP = hw      |           |           |cksum is hw|cksum is hw  |
|                  |           |           |(using     |(using       |
|                  |           |           |lX_len)    |outer_lX_len,|
|                  |           |           |           |knowing that |
|                  |           |           |           |no hw support|
|                  |           |           |           |it today)    |
+------------------+-----------+-----------+-----------+-------------+

> v2 change:
>       redefine the 'none' behaviour for "tx_checksum set tunnel (hw|sw|none) (port-id)" command.
> v3 change:
>       typo correction in cmdline help
>
> Jijiang Liu (3):
>    add outer IP offload capability in librte_ether.
>    add outer IP checksum capability in i40e PMD
>    testpmd command lines of the tx_checksum and csum forwarding rework
>
>   app/test-pmd/cmdline.c            |  201 +++++++++++++++++++++++++++++++++++--
>   app/test-pmd/csumonly.c           |   35 ++++---
>   app/test-pmd/testpmd.h            |    6 +-
>   lib/librte_ether/rte_ethdev.h     |    1 +
>   lib/librte_pmd_i40e/i40e_ethdev.c |    3 +-
>   5 files changed, 218 insertions(+), 28 deletions(-)
>

One more comment, which is not critical. I think the commit log would
be more readable if the lines are truncated at 72 columns, like
described here:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

Regards,
Olivier

  parent reply	other threads:[~2014-12-11 10:17 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-10  1:03 Jijiang Liu
2014-12-10  1:03 ` [dpdk-dev] [PATCH v3 1/3] librte_ether:add outer IP offload capability flag Jijiang Liu
2014-12-11 10:33   ` Olivier MATZ
2014-12-10  1:03 ` [dpdk-dev] [PATCH v3 2/3] i40e:support outer IPv4 checksum capability Jijiang Liu
2014-12-11 10:34   ` Olivier MATZ
2014-12-10  1:03 ` [dpdk-dev] [PATCH v3 3/3] app/testpmd:change tx_checksum command and csum forwarding engine Jijiang Liu
2014-12-11 10:52   ` Olivier MATZ
2014-12-12  4:06     ` Liu, Jijiang
2014-12-11 10:17 ` Olivier MATZ [this message]
2014-12-12  3:48   ` [dpdk-dev] [PATCH v3 0/3] enhance TX checksum " Liu, Jijiang
2014-12-12 16:33     ` Olivier MATZ
2015-01-07  2:03       ` Liu, Jijiang
2015-01-07  9:59         ` Ananyev, Konstantin
2015-01-07 11:39           ` Liu, Jijiang
2015-01-07 12:07             ` Ananyev, Konstantin
2015-01-08  8:51               ` Liu, Jijiang
2015-01-08 10:54                 ` Ananyev, Konstantin
2015-01-09 10:45                   ` Olivier MATZ
2015-01-12  3:41                     ` Liu, Jijiang
2015-01-12 11:43                       ` Olivier MATZ
2015-01-13  3:04                         ` Liu, Jijiang
2015-01-13  9:55                           ` Olivier MATZ
2015-01-14  3:01                             ` Liu, Jijiang
2015-01-15 13:31                               ` Ananyev, Konstantin
2015-01-16 17:27                                 ` Olivier MATZ
2015-01-19 13:04                                   ` Ananyev, Konstantin
2015-01-19 14:38                                     ` Olivier MATZ
2015-01-20  1:12                                       ` Ananyev, Konstantin
2015-01-20 12:39                                         ` Olivier MATZ
2015-01-20 15:18                                           ` Thomas Monjalon
2015-01-20 17:10                                             ` Stephen Hemminger
2015-01-20 17:23                                           ` Ananyev, Konstantin
2015-01-20 18:15                                             ` Olivier MATZ
2015-01-21  3:12                                               ` Liu, Jijiang
2015-01-21 15:25                                                 ` Olivier MATZ
2015-01-21 16:28                                                   ` Ananyev, Konstantin
2015-01-21 17:13                                                     ` Olivier MATZ
2015-01-26  4:13                                                   ` Ananyev, Konstantin
2015-01-26  6:02                                                     ` Liu, Jijiang
2015-01-26 14:07                                                       ` Olivier MATZ
2015-01-26 14:15                                                         ` Ananyev, Konstantin
2015-01-27  8:34                                                           ` Olivier MATZ
2015-01-27 15:26                                                             ` Ananyev, Konstantin
2015-01-21 19:44                                                 ` Stephen Hemminger
2015-01-22  1:40                                                   ` Liu, Jijiang
2015-01-21  8:01                                               ` Liu, Jijiang
2015-01-21  9:10                                                 ` Olivier MATZ
2015-01-21 11:52                                                   ` Ananyev, Konstantin
2015-01-07 13:06 ` Qiu, Michael

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54896F4A.4070601@6wind.com \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=jijiang.liu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git