librte_net/rte_ip.h checksum functions question

librte_net/rte_ip.h checksum functions question

[Ido Barnea (ibarnea)]

Hi,
Is it intentional that rte_ipv4_phdr_cksum and others in this file assume that ipv4 header is sizeof(struct ipv4_hdr), actually assuming that there are no IPv4 options?
If not, I would like to submit a patch changing that.
Something in the line of:
sizeof(struct ipv4_hdr) changes to ipv4_hdr->version_ihl & IPV4_HDR_IHL_MASK) * IPV4_IHL_MULTIPLIER)

Thanks,
Ido

Re: librte_net/rte_ip.h checksum functions question

[Olivier Matz]

Hi Ido,

On Tue, 18 Jul 2017 12:18:20 +0000, "Ido Barnea (ibarnea)" <ibarnea@cisco.com> wrote:
> Hi,
> Is it intentional that rte_ipv4_phdr_cksum and others in this file assume that ipv4 header is sizeof(struct ipv4_hdr), actually assuming that there are no IPv4 options?
> If not, I would like to submit a patch changing that.
> Something in the line of:
> sizeof(struct ipv4_hdr) changes to ipv4_hdr->version_ihl & IPV4_HDR_IHL_MASK) * IPV4_IHL_MULTIPLIER)
> 

I agree we should have other functions that take the real ip header
len in account. What about the following:

static inline uint16_t
rte_ipv4ext_cksum(const struct ipv4_hdr *ipv4_hdr, size_t hdrlen)

static inline uint16_t
rte_ipv4ext_phdr_cksum(const struct ipv4_hdr *ipv4_hdr, size_t hdrlen,
	uint64_t ol_flags)
...

The current function rte_ipv4_cksum() can call rte_ipv4ext_cksum(.., 5)
without additional cost, since 5 will be a constant, resolved at
compilation time.

I also noticed that some functions are not properly documented (it should
be announced that options are not supported). Also, the api comment
of rte_ipv6_udptcp_cksum() talks about ipv4...


Thanks,
Olivier

Re: librte_net/rte_ip.h checksum functions question

[Ido Barnea (ibarnea)]

Sounds fine to me. Are you planning to add this, or you want me to do it?




On 20/07/2017, 12:09 PM, "Olivier Matz" <olivier.matz@6wind.com> wrote:

>Hi Ido,
>
>On Tue, 18 Jul 2017 12:18:20 +0000, "Ido Barnea (ibarnea)" <ibarnea@cisco.com> wrote:
>> Hi,
>> Is it intentional that rte_ipv4_phdr_cksum and others in this file assume that ipv4 header is sizeof(struct ipv4_hdr), actually assuming that there are no IPv4 options?
>> If not, I would like to submit a patch changing that.
>> Something in the line of:
>> sizeof(struct ipv4_hdr) changes to ipv4_hdr->version_ihl & IPV4_HDR_IHL_MASK) * IPV4_IHL_MULTIPLIER)
>> 
>
>I agree we should have other functions that take the real ip header
>len in account. What about the following:
>
>static inline uint16_t
>rte_ipv4ext_cksum(const struct ipv4_hdr *ipv4_hdr, size_t hdrlen)
>
>static inline uint16_t
>rte_ipv4ext_phdr_cksum(const struct ipv4_hdr *ipv4_hdr, size_t hdrlen,
>	uint64_t ol_flags)
>...
>
>The current function rte_ipv4_cksum() can call rte_ipv4ext_cksum(.., 5)
>without additional cost, since 5 will be a constant, resolved at
>compilation time.
>
>I also noticed that some functions are not properly documented (it should
>be announced that options are not supported). Also, the api comment
>of rte_ipv6_udptcp_cksum() talks about ipv4...
>
>
>Thanks,
>Olivier