Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Dariusz Sosnowski]

Hi,

On Thu, 2 Dec 2021 10:24:40 +0800, Huichao Cai wrote:
> > Substituting options with NOOP might cause rte_ipv4_fragment_packet to produce more fragments than necessary, since options with copied flag unset will still occupy space in IPv4 header.
> --The "ip_options_fragment" just make a replacement and doesn't change the length of the IPv4 header.So I don't quite understand why it leads to produce more fragments.
If options with copied flag unset are not copied, then IPv4 headers in the fragments (despite 1st fragment) will be shorter. This leaves more byte space for the payload and in effect fragmentation might produce less fragments.

Best regards,
Dariusz Sosnowski



 

Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

If options with copied flag unset are not copied, then IPv4 headers in the fragments (despite 1st fragment) will be shorter. This leaves more byte space for the payload and in effect fragmentation might produce less fragments.
--Do I need to modify it this way?

[-- Attachment #2: Type: text/html, Size: 470 bytes --]

RE: Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Ananyev, Konstantin]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

I didn't look at it in detail yet,
just wonder would be real gain in terms of space?

From: Huichao Cai <chcchc88@163.com>
Sent: Thursday, December 2, 2021 11:39 AM
To: Dariusz Sosnowski <dsosnowski@nvidia.com>
Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
Subject: Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

If options with copied flag unset are not copied, then IPv4 headers in the fragments (despite 1st fragment) will be shorter. This leaves more byte space for the payload and in effect fragmentation might produce less fragments.
--Do I need to modify it this way?




[-- Attachment #2: Type: text/html, Size: 4278 bytes --]

Re:RE: Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 154 bytes --]

Perhaps performance is more important.This code comes from the linux kernel(5.10.9 and so on). :)
It is more performance-focused based on comments. :)




[-- Attachment #2: Type: text/html, Size: 715 bytes --]

Re:Re:RE: Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]







Hi everyone,


This patch hasn't changed status for a long time, I want to know what the current situation of this patch is?


Huichao Cai





















 

[-- Attachment #2: Type: text/html, Size: 2313 bytes --]

RE: Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Ananyev, Konstantin]

Hi Huichao

>>As a nit, why not 'uint8_t *', to keep style the same through all file?
>Yes,I can use 'uint8_t *.Thank you for your correction.
>
>>We already done such calculation in rte_ipv4_fragment_packet(),
>>so can re-use header_len value here.
>Yes,I can re-use header_len.Thank you for your correction.
>
>>Why optlen==1 is not considered as valid one?
>RFC791：
>        Case 2:  An option-type octet, an option-length octet, and the actual option-data octets.
>       The option-length octet counts the option-type octet and the option-length octet as well as the option-data octets.
>So for case 2, the value of optlen is at least 2.

Ok, thanks for explanation.

>>What means 'ANK' here? 
>Because this code comes from the Linux kernel and is licensed under the GPL, I kept the original comments, I looked up the Linux kernel code, and ‘ANK’ should be the name of a >developer.

AFAIK, we can't copy-paste code from Linux kernel.
As you noted it is under GPL, while DPDK is under BSD-3 license. 

>>I see two problems with that approach:
>>- you modify incoming packet's header - which is the change in behaviour,
>>  and doesn't look right at all.
>Because the incoming packet is fragmented, the incoming packet IP header is no longer used, so this behavior does not cause problems.At the same time, in order to consider >efficiency, modify the original IP header directly, rather than creating a separate IP header.Of course, if this method is not reasonable, I can create a separate IP header to modify.

Library routine has no idea would original IP packet will be used later or not.
In your particular case it might be not needed, but there might be other usages,
that do use it (logging, send un-fragmented via other port, etc.).
So I think we have to preserve original behaviour.

>>- you remove not-copied options from all fragments.
>>  As I can read RFC 791 - first fragment should have a copy of all options present
>>  in original packet, while other fragments need to have only those that have to be
>>  copied.  
>This function（ip_options_fragment） is called under that ‘__fill_ipv4hdr_frag’，so the first fragment have a copy of all options present in original packet.

Right, I missed the fact that you do modify original packet after making a first copy.
 

Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

A small problem.Why is the content of the email just sent to you not visible at Patchwork (this patch).

[-- Attachment #2: Type: text/html, Size: 348 bytes --]

Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

Hi,Konstantin


Thank you for your reply！


>As a nit, why not 'uint8_t *', to keep style the same through all file?
Yes,I can use 'uint8_t *.Thank you for your correction.


>We already done such calculation in rte_ipv4_fragment_packet(),
>so can re-use header_len value here.

Yes,I can re-use header_len.Thank you for your correction.


>Why optlen==1 is not considered as valid one?

RFC791：
        Case 2:  An option-type octet, an option-length octet, and the actual option-data octets.
        The option-length octet counts the option-type octet and the option-length octet as well as the option-data octets.

So for case 2, the value of optlen is at least 2.


>What means 'ANK' here?
Because this code comes from the Linux kernel and is licensed under the GPL, I kept the original comments, I looked up the Linux kernel code, and ‘ANK’ should be the name of a developer.


>I see two problems with that approach: >- you modify incoming packet's header - which is the change in behaviour,
> and doesn't look right at all.
Because the incoming packet is fragmented, the incoming packet IP header is no longer used, so this behavior does not cause problems.At the same time, in order to consider efficiency, modify the original IP header directly, rather than creating a separate IP header.Of course, if this method is not reasonable, I can create a separate IP header to modify.


>- you remove not-copied options from all fragments. > As I can read RFC 791 - first fragment should have a copy of all options present > in original packet, while other fragments need to have only those that have to be
> copied.
This function（ip_options_fragment） is called under that ‘__fill_ipv4hdr_frag’，so the first fragment have a copy of all options present in original packet.


Huichao Cai

[-- Attachment #2: Type: text/html, Size: 2480 bytes --]

Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hi Dariusz


Substituting options with NOOP might cause rte_ipv4_fragment_packet to produce more fragments than necessary, since options with copied flag unset will still occupy space in IPv4 header.


--The "ip_options_fragment" just make a replacement and doesn't change the length of the IPv4 header.So I don't quite understand why it leads to produce more fragments.


but maybe a better solution would be to prepare a separate IPv4 header for fragments without unnecessary options.
--Yes, we can do this, but it adds some extra work, such as generating a new IPv4 header and reassembling the data,which has some performance implications.

Huichao Cai

[-- Attachment #2: Type: text/html, Size: 2297 bytes --]