DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier MATZ <olivier.matz@6wind.com>
To: "Liu, Jijiang" <jijiang.liu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
Date: Fri, 12 Dec 2014 17:33:13 +0100	[thread overview]
Message-ID: <548B18C9.3020408@6wind.com> (raw)
In-Reply-To: <1ED644BD7E0A5F4091CF203DAFB8E4CC01DA1B70@SHSMSX101.ccr.corp.intel.com>

Hello,

On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> The 'hw/sw' option  is used to set/clear the flag of enabling TX tunneling packet checksum hardware offload in testpmd application.

This is not clear at all.
In your command, there is (hw|sw|none).
Are you talking about inner or outer?
Is this command useful for any kind of packet?
How does it combine with "tx_checksum set outer-ip (hw|sw)"?

>> 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
> 
> No.
> Please note the symbols for command descriptions and  scenario descriptions.
> 
> The command descriptions with ">"  symbol.
>  1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
>  2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
> 3> (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
> 
> The scenario descriptions with ")"  symbol.
> 1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunneled packet or not. So he sets:

I read again your cover letter. You enumerate *one* scenario.
An enumeration starts with 2 items.
Then you mix 1> and 1) numbers, which does not make things readable.

>> 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
> 
> These details have already covered in http://dpdk.org/ml/archives/dev/2014-December/009213.html,
> if the patch set is applied, and we aslo have to update the some documents.

First, this link was not referenced in the cover letter or patchset.
I think it would help to first try to explain what problem is fixed,
what is the need, and why. I think in this case it should not require
lines and it could be done in a simple way.

Indeed, yesterday I spent more than an hour to review your patches
and read your descriptions. After that, I still don't understand how
the commands impact the behavior of the csumonly application. The
possible reasons are:

1) I am too dumb to understand. In this case, it would be better
   for you and the community to find another reviewer.

2) Your descriptions are not clear. In this case, you need to think
   about how to reword them so they can be understood, or even maybe
   think about rework your commands if they cannot be explained with
   simple words.

Note that 1) and 2) are not exclusive ;)

>> why not having the 2 following commands:
>>
> 
> I have talked about why we need the current 3 commands in another mail loop, let me explain it here again.

The community does not have this private thread.
And, that's right, I remember this thread: in it, I asked 3 times some
precisions about the commands without any clear answer.

> First. We  still think we need some command to enable/disable tunneling  support in testpmd, that's why the command 1 is needed.

What does enable/disable tunneling support mean?

> 1. tx_checksum set tunnel (hw|sw|none) (port-id) command
> 
> 2. tx_cksum set (outer-ip)  (hw|sw) (port_id)
> 
> 3. tx_cksum set (ip|l4) (hw|sw) (port_id)
> 
> Secondly, in most of cases,   user application use non-tunneling packet, so he just care how to use 3, don't need to care 1 and 2, don't you think  it becomes simpler?  
> If we mix tunneling packet command and non-tunneling packet together, and the commands will become more complicated and  not easy to understand.

Really no, it is not simpler. But if you are able to explain it
in few words what is done by csumonly, maybe I can change my mind.

>> tx_checksum set
>> (ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) <portid>
> 
> As far as I know, so far,  there is no a type of tunneling packet with outer-tcp and outer-sctp.

For TCP, there is STT, which is used in storage.
For SCTP, it could probably be removed.

>>    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
> 
> This command can't meet/match our previous discussions and current implementation.  In terms of 'inner' option, which can't meet the two following cases.
> 
> B) User is aware that it is a tunneled packet and requests HW offload for ipv4_hdr_in and tcp_hdr_in *only*.
> He doesn't care about outer IP checksum offload.
> In that case, for FVL  he has 2 choices:
>    1. Treat that packet as a 'proper' tunnelled packet, and fill all the fields:
>      mb->l2_len =  udp_hdr_len + vxlan_hdr_len + eth_hdr_in;
>      mb->l3_len = ipv4_hdr_in;
>      mb->outer_l2_len = eth_hdr_out;
>      mb->outer_l3_len = ipv4_hdr_out;
>      mb->ol_flags |= PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> 
>    2. As user doesn't care about outer IP hdr checksum, he can treat everything before ipv4_hdr_in as L2 header.
>    So he knows, that it is a tunneled packet, but makes HW to treat it as ordinary (non-tunneled) packet:
>      mb->l2_len = eth_hdr_out + ipv4_hdr_out + udp_hdr_out + vxlan_hdr + ehtr_hdr_in;
>      mb->l3_len = ipv4_hdr_in;
>      mb->ol_flags |= PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;

Indeed, there are 2 choices for fortville. I could argue that you also
have 2 choices to offload the IP checksum of Ether/IP/UDP/xxx:
 - use lX_len
 - use outer_lX_len

You have also the choice to use lX_len or outer_lX_len if you want
to offload the outer IP checksum of Ether/IP/UDP/vxlan/Ether/IP/UDP/xxx

That's exactly why I asked to first describe what problem you want
to solve in your patchset. You can extend the list I've send in my
first answer.

Maybe your solution supports all these cases, but as a user, I
have no idea how to configure it even after reading your desriptions
during a long time, so the result is the same than not supporting.

The question is: are all these casez useful? To me, it's equally
important that a user can use the application and understand what it
does. If they are required I'm sure we can find a good user API that
is able to test all these cases.

Regards,
Olivier

  reply	other threads:[~2014-12-12 16:33 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 ` [dpdk-dev] [PATCH v3 0/3] enhance TX checksum " Olivier MATZ
2014-12-12  3:48   ` Liu, Jijiang
2014-12-12 16:33     ` Olivier MATZ [this message]
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=548B18C9.3020408@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).