diff mbox

[PATCHv4,1/2] Add odp helper function for udp checksum

Message ID 1399463020-14182-1-git-send-email-weilong.chen@linaro.org
State Accepted
Headers show

Commit Message

Weilong Chen May 7, 2014, 11:43 a.m. UTC
Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
---
 include/helper/odp_udp.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

Comments

Santosh Shukla May 7, 2014, 11:57 a.m. UTC | #1
On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>
> Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
> ---
>  include/helper/odp_udp.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
> index 738470e..9905d55 100644
> --- a/include/helper/odp_udp.h
> +++ b/include/helper/odp_udp.h
> @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>         uint16be_t chksum;   /**< UDP header and data checksum (0 if not used)*/
>  } odp_udphdr_t;
>
> +/**
> + * UDP checksum
> + *
> + * This function uses odp packet to calc checksum
> + *
> + * @param pkt  calculate chksum for pkt
> + * @return  checksum value
> + */
> +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
> +{
> +       unsigned long sum = 0;
> +       odp_udphdr_t *udph;
> +       odp_ipv4hdr_t *iph;
> +       uint8_t *buf;
> +       unsigned short udplen;
> +       uint16_t chksum;
> +
> +       if (!odp_packet_l3_offset(pkt))
> +               return 0;
> +
> +       if (!odp_packet_l4_offset(pkt))
> +               return 0;
> +
> +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
> +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
> +       buf = (uint8_t *)udph;
> +       udplen = odp_be_to_cpu_16(udph->length);
> +
> +       /* the source ip */
> +       sum += (iph->src_addr >> 16) & 0xFFFF;
> +       sum += (iph->src_addr) & 0xFFFF;
> +       /* the dest ip */
> +       sum += (iph->dst_addr >> 16) & 0xFFFF;
> +       sum += (iph->dst_addr) & 0xFFFF;
> +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
> +       /* the length */
> +       sum += udph->length;
> +
> +       while (udplen > 1) {
> +               sum += *buf++;
> +               udplen -= 1;
> +       }
> +       /* if any bytes left, pad the bytes and add */
> +       if (udplen > 0)
> +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
> +
> +       /* Fold sum to 16 bits: add carrier to result */
> +       while (sum >> 16)
> +               sum = (sum & 0xFFFF) + (sum >> 16);
> +       sum = ~sum;
> +       /* set computation result */
> +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
> +                         : (unsigned short)sum;

IIRC, chksum api already in odp/include/helper, why not use them for
chksum calculation.

Thx.
> +
> +       return chksum;
> +}
>
>  /** @internal Compile time assert */
>  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN, ODP_UDPHDR_T__SIZE_ERROR);
> --
> 1.7.9.5
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Weilong Chen May 7, 2014, 12:21 p.m. UTC | #2
Hi,

There's a function odp_chksum,
​UDP packet's chksum include "pseudo-header" and data,
I can't use the current chksum api directly.

Thanks,
Weilong


On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:

> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
> >
> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
> > ---
> >  include/helper/odp_udp.h |   56
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >
> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
> > index 738470e..9905d55 100644
> > --- a/include/helper/odp_udp.h
> > +++ b/include/helper/odp_udp.h
> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if not
> used)*/
> >  } odp_udphdr_t;
> >
> > +/**
> > + * UDP checksum
> > + *
> > + * This function uses odp packet to calc checksum
> > + *
> > + * @param pkt  calculate chksum for pkt
> > + * @return  checksum value
> > + */
> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
> > +{
> > +       unsigned long sum = 0;
> > +       odp_udphdr_t *udph;
> > +       odp_ipv4hdr_t *iph;
> > +       uint8_t *buf;
> > +       unsigned short udplen;
> > +       uint16_t chksum;
> > +
> > +       if (!odp_packet_l3_offset(pkt))
> > +               return 0;
> > +
> > +       if (!odp_packet_l4_offset(pkt))
> > +               return 0;
> > +
> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
> > +       buf = (uint8_t *)udph;
> > +       udplen = odp_be_to_cpu_16(udph->length);
> > +
> > +       /* the source ip */
> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
> > +       sum += (iph->src_addr) & 0xFFFF;
> > +       /* the dest ip */
> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
> > +       sum += (iph->dst_addr) & 0xFFFF;
> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
> > +       /* the length */
> > +       sum += udph->length;
> > +
> > +       while (udplen > 1) {
> > +               sum += *buf++;
> > +               udplen -= 1;
> > +       }
> > +       /* if any bytes left, pad the bytes and add */
> > +       if (udplen > 0)
> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
> > +
> > +       /* Fold sum to 16 bits: add carrier to result */
> > +       while (sum >> 16)
> > +               sum = (sum & 0xFFFF) + (sum >> 16);
> > +       sum = ~sum;
> > +       /* set computation result */
> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
> > +                         : (unsigned short)sum;
>
> IIRC, chksum api already in odp/include/helper, why not use them for
> chksum calculation.
>
> Thx.
> > +
> > +       return chksum;
> > +}
> >
> >  /** @internal Compile time assert */
> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
> ODP_UDPHDR_T__SIZE_ERROR);
> > --
> > 1.7.9.5
> >
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/lng-odp
>
Mike Holmes May 8, 2014, 7:02 p.m. UTC | #3
If you can't use the checksum directly as it is, can it be modified so that
would work in all cases or must it be a new function ?


On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:

> Hi,
>
> There's a function odp_chksum,
> ​UDP packet's chksum include "pseudo-header" and data,
> I can't use the current chksum api directly.
>
> Thanks,
> Weilong
>
>
> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>
>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>> >
>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>> > ---
>> >  include/helper/odp_udp.h |   56
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 56 insertions(+)
>> >
>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>> > index 738470e..9905d55 100644
>> > --- a/include/helper/odp_udp.h
>> > +++ b/include/helper/odp_udp.h
>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>> not used)*/
>> >  } odp_udphdr_t;
>> >
>> > +/**
>> > + * UDP checksum
>> > + *
>> > + * This function uses odp packet to calc checksum
>> > + *
>> > + * @param pkt  calculate chksum for pkt
>> > + * @return  checksum value
>> > + */
>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>> > +{
>> > +       unsigned long sum = 0;
>> > +       odp_udphdr_t *udph;
>> > +       odp_ipv4hdr_t *iph;
>> > +       uint8_t *buf;
>> > +       unsigned short udplen;
>> > +       uint16_t chksum;
>> > +
>> > +       if (!odp_packet_l3_offset(pkt))
>> > +               return 0;
>> > +
>> > +       if (!odp_packet_l4_offset(pkt))
>> > +               return 0;
>> > +
>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>> > +       buf = (uint8_t *)udph;
>> > +       udplen = odp_be_to_cpu_16(udph->length);
>> > +
>> > +       /* the source ip */
>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>> > +       sum += (iph->src_addr) & 0xFFFF;
>> > +       /* the dest ip */
>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>> > +       sum += (iph->dst_addr) & 0xFFFF;
>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>> > +       /* the length */
>> > +       sum += udph->length;
>> > +
>> > +       while (udplen > 1) {
>> > +               sum += *buf++;
>> > +               udplen -= 1;
>> > +       }
>> > +       /* if any bytes left, pad the bytes and add */
>> > +       if (udplen > 0)
>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>> > +
>> > +       /* Fold sum to 16 bits: add carrier to result */
>> > +       while (sum >> 16)
>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>> > +       sum = ~sum;
>> > +       /* set computation result */
>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>> > +                         : (unsigned short)sum;
>>
>> IIRC, chksum api already in odp/include/helper, why not use them for
>> chksum calculation.
>>
>> Thx.
>> > +
>> > +       return chksum;
>> > +}
>> >
>> >  /** @internal Compile time assert */
>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>> ODP_UDPHDR_T__SIZE_ERROR);
>> > --
>> > 1.7.9.5
>> >
>> >
>> > _______________________________________________
>> > lng-odp mailing list
>> > lng-odp@lists.linaro.org
>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Ola Liljedahl May 8, 2014, 7:41 p.m. UTC | #4
The UDP checksum function can use (call) the plain ones complement checksum
function when computing the checksum over the UDP header and payload.
The IPv4 hdr checksum can also use (call) the ones complement checksum
function where the size parameter comes from the IP.length field (length in
32-bit words).

In which files these functions are declared and implemented is another
issue. Perhaps one header file for the ones complement checksum and other
generic stuff, one header files for IP definitions and one header file for
UDP definitions.

> +       unsigned short udplen;
No reason for udplen to be a short (even though the value would fit into an
unsigned short), might just complicate life for the compiler (at least this
was the case in the past as the compiler had to truncate the value into
16-bits every time the value was changed). Just use an (unsigned) int which
will map to a register.

> +       while (sum >> 16)
I would prefer this to be written
while ((sum >> 16) != 0)

I don't know if the below has been covered before.
> +       uint8_t *buf;
...
> +       while (udplen > 1) {
> +               sum += *buf++;
> +               udplen -= 1;
> +       }
> +       /* if any bytes left, pad the bytes and add */
> +       if (udplen > 0)
> +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
You probably want buf to be "uint16_t *buf" and decrease udplen by 2.
Decreasing by 1 will not leave anything left for the last if-statement.
Better to actually call the existing ones complement checksum function
which might actually do it right. This is needless duplication of code.

-- Ola



On 8 May 2014 21:02, Mike Holmes <mike.holmes@linaro.org> wrote:

> If you can't use the checksum directly as it is, can it be modified so
> that would work in all cases or must it be a new function ?
>
>
> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>
>> Hi,
>>
>> There's a function odp_chksum,
>> ​UDP packet's chksum include "pseudo-header" and data,
>> I can't use the current chksum api directly.
>>
>> Thanks,
>> Weilong
>>
>>
>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>
>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>> >
>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>> > ---
>>> >  include/helper/odp_udp.h |   56
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>> >  1 file changed, 56 insertions(+)
>>> >
>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>> > index 738470e..9905d55 100644
>>> > --- a/include/helper/odp_udp.h
>>> > +++ b/include/helper/odp_udp.h
>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>> not used)*/
>>> >  } odp_udphdr_t;
>>> >
>>> > +/**
>>> > + * UDP checksum
>>> > + *
>>> > + * This function uses odp packet to calc checksum
>>> > + *
>>> > + * @param pkt  calculate chksum for pkt
>>> > + * @return  checksum value
>>> > + */
>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>> > +{
>>> > +       unsigned long sum = 0;
>>> > +       odp_udphdr_t *udph;
>>> > +       odp_ipv4hdr_t *iph;
>>> > +       uint8_t *buf;
>>> > +       unsigned short udplen;
>>> > +       uint16_t chksum;
>>> > +
>>> > +       if (!odp_packet_l3_offset(pkt))
>>> > +               return 0;
>>> > +
>>> > +       if (!odp_packet_l4_offset(pkt))
>>> > +               return 0;
>>> > +
>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>> > +       buf = (uint8_t *)udph;
>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>> > +
>>> > +       /* the source ip */
>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>> > +       /* the dest ip */
>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>> > +       /* the length */
>>> > +       sum += udph->length;
>>> > +
>>> > +       while (udplen > 1) {
>>> > +               sum += *buf++;
>>> > +               udplen -= 1;
>>> > +       }
>>> > +       /* if any bytes left, pad the bytes and add */
>>> > +       if (udplen > 0)
>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>> > +
>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>> > +       while (sum >> 16)
>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>> > +       sum = ~sum;
>>> > +       /* set computation result */
>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>> > +                         : (unsigned short)sum;
>>>
>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>> chksum calculation.
>>>
>>> Thx.
>>> > +
>>> > +       return chksum;
>>> > +}
>>> >
>>> >  /** @internal Compile time assert */
>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>> ODP_UDPHDR_T__SIZE_ERROR);
>>> > --
>>> > 1.7.9.5
>>> >
>>> >
>>> > _______________________________________________
>>> > lng-odp mailing list
>>> > lng-odp@lists.linaro.org
>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Bill Fischofer May 8, 2014, 9:39 p.m. UTC | #5
IPv4, TCP, and UDP all use the same checksum algorithm (see RFC 1071 as
updated by RFC 1141 and RFC 1624 for details).  The difference is what is
input to the algorithm.  For both TCP and UDP a "pseudo header" is
prepended to the data to be checksummed that varies between IPv4 and IPv6.
 It's very important to follow the RFC algorithms exactly (they supply the
C code--just copy and paste it).  This is also an area that continues to
evolve (see RFC 6935 dated April 2013, for example) so we just want to
implement these standards, not re-interpret them.

Bill


On Thu, May 8, 2014 at 2:41 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote:

> The UDP checksum function can use (call) the plain ones complement
> checksum function when computing the checksum over the UDP header and
> payload.
> The IPv4 hdr checksum can also use (call) the ones complement checksum
> function where the size parameter comes from the IP.length field (length in
> 32-bit words).
>
> In which files these functions are declared and implemented is another
> issue. Perhaps one header file for the ones complement checksum and other
> generic stuff, one header files for IP definitions and one header file for
> UDP definitions.
>
> > +       unsigned short udplen;
> No reason for udplen to be a short (even though the value would fit into
> an unsigned short), might just complicate life for the compiler (at least
> this was the case in the past as the compiler had to truncate the value
> into 16-bits every time the value was changed). Just use an (unsigned) int
> which will map to a register.
>
> > +       while (sum >> 16)
> I would prefer this to be written
> while ((sum >> 16) != 0)
>
> I don't know if the below has been covered before.
> > +       uint8_t *buf;
> ...
> > +       while (udplen > 1) {
> > +               sum += *buf++;
> > +               udplen -= 1;
> > +       }
> > +       /* if any bytes left, pad the bytes and add */
> > +       if (udplen > 0)
> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
> You probably want buf to be "uint16_t *buf" and decrease udplen by 2.
> Decreasing by 1 will not leave anything left for the last if-statement.
> Better to actually call the existing ones complement checksum function
> which might actually do it right. This is needless duplication of code.
>
> -- Ola
>
>
>
> On 8 May 2014 21:02, Mike Holmes <mike.holmes@linaro.org> wrote:
>
>> If you can't use the checksum directly as it is, can it be modified so
>> that would work in all cases or must it be a new function ?
>>
>>
>> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>>
>>> Hi,
>>>
>>> There's a function odp_chksum,
>>> ​UDP packet's chksum include "pseudo-header" and data,
>>> I can't use the current chksum api directly.
>>>
>>> Thanks,
>>> Weilong
>>>
>>>
>>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>>
>>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>> >
>>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>>> > ---
>>>> >  include/helper/odp_udp.h |   56
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>> >  1 file changed, 56 insertions(+)
>>>> >
>>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>>> > index 738470e..9905d55 100644
>>>> > --- a/include/helper/odp_udp.h
>>>> > +++ b/include/helper/odp_udp.h
>>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>>> not used)*/
>>>> >  } odp_udphdr_t;
>>>> >
>>>> > +/**
>>>> > + * UDP checksum
>>>> > + *
>>>> > + * This function uses odp packet to calc checksum
>>>> > + *
>>>> > + * @param pkt  calculate chksum for pkt
>>>> > + * @return  checksum value
>>>> > + */
>>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>>> > +{
>>>> > +       unsigned long sum = 0;
>>>> > +       odp_udphdr_t *udph;
>>>> > +       odp_ipv4hdr_t *iph;
>>>> > +       uint8_t *buf;
>>>> > +       unsigned short udplen;
>>>> > +       uint16_t chksum;
>>>> > +
>>>> > +       if (!odp_packet_l3_offset(pkt))
>>>> > +               return 0;
>>>> > +
>>>> > +       if (!odp_packet_l4_offset(pkt))
>>>> > +               return 0;
>>>> > +
>>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>>> > +       buf = (uint8_t *)udph;
>>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>>> > +
>>>> > +       /* the source ip */
>>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>>> > +       /* the dest ip */
>>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>>> > +       /* the length */
>>>> > +       sum += udph->length;
>>>> > +
>>>> > +       while (udplen > 1) {
>>>> > +               sum += *buf++;
>>>> > +               udplen -= 1;
>>>> > +       }
>>>> > +       /* if any bytes left, pad the bytes and add */
>>>> > +       if (udplen > 0)
>>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>>> > +
>>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>>> > +       while (sum >> 16)
>>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>>> > +       sum = ~sum;
>>>> > +       /* set computation result */
>>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>>> > +                         : (unsigned short)sum;
>>>>
>>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>>> chksum calculation.
>>>>
>>>> Thx.
>>>> > +
>>>> > +       return chksum;
>>>> > +}
>>>> >
>>>> >  /** @internal Compile time assert */
>>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>>> ODP_UDPHDR_T__SIZE_ERROR);
>>>> > --
>>>> > 1.7.9.5
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > lng-odp mailing list
>>>> > lng-odp@lists.linaro.org
>>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>>
>> --
>> *Mike Holmes*
>> Linaro Technical Manager / Lead
>> LNG - ODP
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Weilong Chen May 9, 2014, 8:44 a.m. UTC | #6
As odp_ipv4_csum_update/odp_ipv4_csum_valid function have already use
odp_chksum,
I think it's better not to modify odp_chksum.


On 9 May 2014 03:02, Mike Holmes <mike.holmes@linaro.org> wrote:

> If you can't use the checksum directly as it is, can it be modified so
> that would work in all cases or must it be a new function ?
>
>
> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>
>> Hi,
>>
>> There's a function odp_chksum,
>> ​UDP packet's chksum include "pseudo-header" and data,
>> I can't use the current chksum api directly.
>>
>> Thanks,
>> Weilong
>>
>>
>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>
>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>> >
>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>> > ---
>>> >  include/helper/odp_udp.h |   56
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>> >  1 file changed, 56 insertions(+)
>>> >
>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>> > index 738470e..9905d55 100644
>>> > --- a/include/helper/odp_udp.h
>>> > +++ b/include/helper/odp_udp.h
>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>> not used)*/
>>> >  } odp_udphdr_t;
>>> >
>>> > +/**
>>> > + * UDP checksum
>>> > + *
>>> > + * This function uses odp packet to calc checksum
>>> > + *
>>> > + * @param pkt  calculate chksum for pkt
>>> > + * @return  checksum value
>>> > + */
>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>> > +{
>>> > +       unsigned long sum = 0;
>>> > +       odp_udphdr_t *udph;
>>> > +       odp_ipv4hdr_t *iph;
>>> > +       uint8_t *buf;
>>> > +       unsigned short udplen;
>>> > +       uint16_t chksum;
>>> > +
>>> > +       if (!odp_packet_l3_offset(pkt))
>>> > +               return 0;
>>> > +
>>> > +       if (!odp_packet_l4_offset(pkt))
>>> > +               return 0;
>>> > +
>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>> > +       buf = (uint8_t *)udph;
>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>> > +
>>> > +       /* the source ip */
>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>> > +       /* the dest ip */
>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>> > +       /* the length */
>>> > +       sum += udph->length;
>>> > +
>>> > +       while (udplen > 1) {
>>> > +               sum += *buf++;
>>> > +               udplen -= 1;
>>> > +       }
>>> > +       /* if any bytes left, pad the bytes and add */
>>> > +       if (udplen > 0)
>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>> > +
>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>> > +       while (sum >> 16)
>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>> > +       sum = ~sum;
>>> > +       /* set computation result */
>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>> > +                         : (unsigned short)sum;
>>>
>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>> chksum calculation.
>>>
>>> Thx.
>>> > +
>>> > +       return chksum;
>>> > +}
>>> >
>>> >  /** @internal Compile time assert */
>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>> ODP_UDPHDR_T__SIZE_ERROR);
>>> > --
>>> > 1.7.9.5
>>> >
>>> >
>>> > _______________________________________________
>>> > lng-odp mailing list
>>> > lng-odp@lists.linaro.org
>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>
Ola Liljedahl May 9, 2014, 8:57 a.m. UTC | #7
No but you should be able to use odp_chksum() for the UDP header and
payload, then add the checksum for the pseudo header and finally convert to
positive 0 if needed and negate.
See it as a case of common subexpression elimination.

-- Ola


On 9 May 2014 10:44, Weilong Chen <weilong.chen@linaro.org> wrote:

> As odp_ipv4_csum_update/odp_ipv4_csum_valid function have already use
> odp_chksum,
> I think it's better not to modify odp_chksum.
>
>
> On 9 May 2014 03:02, Mike Holmes <mike.holmes@linaro.org> wrote:
>
>> If you can't use the checksum directly as it is, can it be modified so
>> that would work in all cases or must it be a new function ?
>>
>>
>> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>>
>>> Hi,
>>>
>>> There's a function odp_chksum,
>>> ​UDP packet's chksum include "pseudo-header" and data,
>>> I can't use the current chksum api directly.
>>>
>>> Thanks,
>>> Weilong
>>>
>>>
>>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>>
>>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>> >
>>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>>> > ---
>>>> >  include/helper/odp_udp.h |   56
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>> >  1 file changed, 56 insertions(+)
>>>> >
>>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>>> > index 738470e..9905d55 100644
>>>> > --- a/include/helper/odp_udp.h
>>>> > +++ b/include/helper/odp_udp.h
>>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>>> not used)*/
>>>> >  } odp_udphdr_t;
>>>> >
>>>> > +/**
>>>> > + * UDP checksum
>>>> > + *
>>>> > + * This function uses odp packet to calc checksum
>>>> > + *
>>>> > + * @param pkt  calculate chksum for pkt
>>>> > + * @return  checksum value
>>>> > + */
>>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>>> > +{
>>>> > +       unsigned long sum = 0;
>>>> > +       odp_udphdr_t *udph;
>>>> > +       odp_ipv4hdr_t *iph;
>>>> > +       uint8_t *buf;
>>>> > +       unsigned short udplen;
>>>> > +       uint16_t chksum;
>>>> > +
>>>> > +       if (!odp_packet_l3_offset(pkt))
>>>> > +               return 0;
>>>> > +
>>>> > +       if (!odp_packet_l4_offset(pkt))
>>>> > +               return 0;
>>>> > +
>>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>>> > +       buf = (uint8_t *)udph;
>>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>>> > +
>>>> > +       /* the source ip */
>>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>>> > +       /* the dest ip */
>>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>>> > +       /* the length */
>>>> > +       sum += udph->length;
>>>> > +
>>>> > +       while (udplen > 1) {
>>>> > +               sum += *buf++;
>>>> > +               udplen -= 1;
>>>> > +       }
>>>> > +       /* if any bytes left, pad the bytes and add */
>>>> > +       if (udplen > 0)
>>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>>> > +
>>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>>> > +       while (sum >> 16)
>>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>>> > +       sum = ~sum;
>>>> > +       /* set computation result */
>>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>>> > +                         : (unsigned short)sum;
>>>>
>>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>>> chksum calculation.
>>>>
>>>> Thx.
>>>> > +
>>>> > +       return chksum;
>>>> > +}
>>>> >
>>>> >  /** @internal Compile time assert */
>>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>>> ODP_UDPHDR_T__SIZE_ERROR);
>>>> > --
>>>> > 1.7.9.5
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > lng-odp mailing list
>>>> > lng-odp@lists.linaro.org
>>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>>
>> --
>> *Mike Holmes*
>> Linaro Technical Manager / Lead
>> LNG - ODP
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Weilong Chen May 9, 2014, 9:10 a.m. UTC | #8
On 9 May 2014 03:41, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> The UDP checksum function can use (call) the plain ones complement
> checksum function when computing the checksum over the UDP header and
> payload.
> The IPv4 hdr checksum can also use (call) the ones complement checksum
> function where the size parameter comes from the IP.length field (length in
> 32-bit words).
>
> In which files these functions are declared and implemented is another
> issue. Perhaps one header file for the ones complement checksum and other
> generic stuff, one header files for IP definitions and one header file for
> UDP definitions.
>
> > +       unsigned short udplen;
> No reason for udplen to be a short (even though the value would fit into
> an unsigned short), might just complicate life for the compiler (at least
> this was the case in the past as the compiler had to truncate the value
> into 16-bits every time the value was changed). Just use an (unsigned) int
> which will map to a register.
>

​OK.​


> > +       while (sum >> 16)
> I would prefer this to be written
> while ((sum >> 16) != 0)
>

​OK.​


>
> I don't know if the below has been covered before.
> > +       uint8_t *buf;
> ...
> > +       while (udplen > 1) {
> > +               sum += *buf++;
> > +               udplen -= 1;
> > +       }
> > +       /* if any bytes left, pad the bytes and add */
> > +       if (udplen > 0)
> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
> You probably want buf to be "uint16_t *buf" and decrease udplen by 2.
> Decreasing by 1 will not leave anything left for the last if-statement.
> Better to actually call the existing ones complement checksum function
> which might actually do it right. This is needless duplication of code.
>

Thanks for pointing my mistake,
I planned to avoid "cast increases required alignment of target type"​
error by uint8_t replacing uint16_t.
It's a bad idea, I'll use "(uint16_t *)(void *)" to avoid it.

If use odp_chksum(void *buffer, int len) to replace the duplicate code,
I need to alloc a new buffer, then copy pseudo-header and udp data into it.
That brings down the performance.
Is there a better way?

Thanks,
Weilong



> -- Ola
>
>
>
> On 8 May 2014 21:02, Mike Holmes <mike.holmes@linaro.org> wrote:
>
>> If you can't use the checksum directly as it is, can it be modified so
>> that would work in all cases or must it be a new function ?
>>
>>
>> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>>
>>> Hi,
>>>
>>> There's a function odp_chksum,
>>> ​UDP packet's chksum include "pseudo-header" and data,
>>> I can't use the current chksum api directly.
>>>
>>> Thanks,
>>> Weilong
>>>
>>>
>>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>>
>>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>> >
>>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>>> > ---
>>>> >  include/helper/odp_udp.h |   56
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>> >  1 file changed, 56 insertions(+)
>>>> >
>>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>>> > index 738470e..9905d55 100644
>>>> > --- a/include/helper/odp_udp.h
>>>> > +++ b/include/helper/odp_udp.h
>>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>>> not used)*/
>>>> >  } odp_udphdr_t;
>>>> >
>>>> > +/**
>>>> > + * UDP checksum
>>>> > + *
>>>> > + * This function uses odp packet to calc checksum
>>>> > + *
>>>> > + * @param pkt  calculate chksum for pkt
>>>> > + * @return  checksum value
>>>> > + */
>>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>>> > +{
>>>> > +       unsigned long sum = 0;
>>>> > +       odp_udphdr_t *udph;
>>>> > +       odp_ipv4hdr_t *iph;
>>>> > +       uint8_t *buf;
>>>> > +       unsigned short udplen;
>>>> > +       uint16_t chksum;
>>>> > +
>>>> > +       if (!odp_packet_l3_offset(pkt))
>>>> > +               return 0;
>>>> > +
>>>> > +       if (!odp_packet_l4_offset(pkt))
>>>> > +               return 0;
>>>> > +
>>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>>> > +       buf = (uint8_t *)udph;
>>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>>> > +
>>>> > +       /* the source ip */
>>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>>> > +       /* the dest ip */
>>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>>> > +       /* the length */
>>>> > +       sum += udph->length;
>>>> > +
>>>> > +       while (udplen > 1) {
>>>> > +               sum += *buf++;
>>>> > +               udplen -= 1;
>>>> > +       }
>>>> > +       /* if any bytes left, pad the bytes and add */
>>>> > +       if (udplen > 0)
>>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>>> > +
>>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>>> > +       while (sum >> 16)
>>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>>> > +       sum = ~sum;
>>>> > +       /* set computation result */
>>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>>> > +                         : (unsigned short)sum;
>>>>
>>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>>> chksum calculation.
>>>>
>>>> Thx.
>>>> > +
>>>> > +       return chksum;
>>>> > +}
>>>> >
>>>> >  /** @internal Compile time assert */
>>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>>> ODP_UDPHDR_T__SIZE_ERROR);
>>>> > --
>>>> > 1.7.9.5
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > lng-odp mailing list
>>>> > lng-odp@lists.linaro.org
>>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>>
>> --
>> *Mike Holmes*
>> Linaro Technical Manager / Lead
>> LNG - ODP
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
Weilong Chen May 9, 2014, 9:23 a.m. UTC | #9
Thanks Bill!

Currently the generator only support IPv4 packets.
Should it support IPv6 now?


On 9 May 2014 05:39, Bill Fischofer <bill.fischofer@linaro.org> wrote:

> IPv4, TCP, and UDP all use the same checksum algorithm (see RFC 1071 as
> updated by RFC 1141 and RFC 1624 for details).  The difference is what is
> input to the algorithm.  For both TCP and UDP a "pseudo header" is
> prepended to the data to be checksummed that varies between IPv4 and IPv6.
>  It's very important to follow the RFC algorithms exactly (they supply the
> C code--just copy and paste it).  This is also an area that continues to
> evolve (see RFC 6935 dated April 2013, for example) so we just want to
> implement these standards, not re-interpret them.
>
> Bill
>
>
> On Thu, May 8, 2014 at 2:41 PM, Ola Liljedahl <ola.liljedahl@linaro.org>wrote:
>
>> The UDP checksum function can use (call) the plain ones complement
>> checksum function when computing the checksum over the UDP header and
>> payload.
>> The IPv4 hdr checksum can also use (call) the ones complement checksum
>> function where the size parameter comes from the IP.length field (length in
>> 32-bit words).
>>
>> In which files these functions are declared and implemented is another
>> issue. Perhaps one header file for the ones complement checksum and other
>> generic stuff, one header files for IP definitions and one header file for
>> UDP definitions.
>>
>> > +       unsigned short udplen;
>> No reason for udplen to be a short (even though the value would fit into
>> an unsigned short), might just complicate life for the compiler (at least
>> this was the case in the past as the compiler had to truncate the value
>> into 16-bits every time the value was changed). Just use an (unsigned) int
>> which will map to a register.
>>
>> > +       while (sum >> 16)
>> I would prefer this to be written
>> while ((sum >> 16) != 0)
>>
>> I don't know if the below has been covered before.
>> > +       uint8_t *buf;
>> ...
>>  > +       while (udplen > 1) {
>> > +               sum += *buf++;
>> > +               udplen -= 1;
>> > +       }
>> > +       /* if any bytes left, pad the bytes and add */
>> > +       if (udplen > 0)
>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>> You probably want buf to be "uint16_t *buf" and decrease udplen by 2.
>> Decreasing by 1 will not leave anything left for the last if-statement.
>> Better to actually call the existing ones complement checksum function
>> which might actually do it right. This is needless duplication of code.
>>
>> -- Ola
>>
>>
>>
>> On 8 May 2014 21:02, Mike Holmes <mike.holmes@linaro.org> wrote:
>>
>>> If you can't use the checksum directly as it is, can it be modified so
>>> that would work in all cases or must it be a new function ?
>>>
>>>
>>> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>
>>>> Hi,
>>>>
>>>> There's a function odp_chksum,
>>>> ​UDP packet's chksum include "pseudo-header" and data,
>>>> I can't use the current chksum api directly.
>>>>
>>>> Thanks,
>>>> Weilong
>>>>
>>>>
>>>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>>>
>>>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>>> >
>>>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>>>> > ---
>>>>> >  include/helper/odp_udp.h |   56
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>> >  1 file changed, 56 insertions(+)
>>>>> >
>>>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>>>> > index 738470e..9905d55 100644
>>>>> > --- a/include/helper/odp_udp.h
>>>>> > +++ b/include/helper/odp_udp.h
>>>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>>>> not used)*/
>>>>> >  } odp_udphdr_t;
>>>>> >
>>>>> > +/**
>>>>> > + * UDP checksum
>>>>> > + *
>>>>> > + * This function uses odp packet to calc checksum
>>>>> > + *
>>>>> > + * @param pkt  calculate chksum for pkt
>>>>> > + * @return  checksum value
>>>>> > + */
>>>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>>>> > +{
>>>>> > +       unsigned long sum = 0;
>>>>> > +       odp_udphdr_t *udph;
>>>>> > +       odp_ipv4hdr_t *iph;
>>>>> > +       uint8_t *buf;
>>>>> > +       unsigned short udplen;
>>>>> > +       uint16_t chksum;
>>>>> > +
>>>>> > +       if (!odp_packet_l3_offset(pkt))
>>>>> > +               return 0;
>>>>> > +
>>>>> > +       if (!odp_packet_l4_offset(pkt))
>>>>> > +               return 0;
>>>>> > +
>>>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>>>> > +       buf = (uint8_t *)udph;
>>>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>>>> > +
>>>>> > +       /* the source ip */
>>>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>>>> > +       /* the dest ip */
>>>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>>>> > +       /* the length */
>>>>> > +       sum += udph->length;
>>>>> > +
>>>>> > +       while (udplen > 1) {
>>>>> > +               sum += *buf++;
>>>>> > +               udplen -= 1;
>>>>> > +       }
>>>>> > +       /* if any bytes left, pad the bytes and add */
>>>>> > +       if (udplen > 0)
>>>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>>>> > +
>>>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>>>> > +       while (sum >> 16)
>>>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>>>> > +       sum = ~sum;
>>>>> > +       /* set computation result */
>>>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>>>> > +                         : (unsigned short)sum;
>>>>>
>>>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>>>> chksum calculation.
>>>>>
>>>>> Thx.
>>>>> > +
>>>>> > +       return chksum;
>>>>> > +}
>>>>> >
>>>>> >  /** @internal Compile time assert */
>>>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>>>> ODP_UDPHDR_T__SIZE_ERROR);
>>>>> > --
>>>>> > 1.7.9.5
>>>>> >
>>>>> >
>>>>> > _______________________________________________
>>>>> > lng-odp mailing list
>>>>> > lng-odp@lists.linaro.org
>>>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>>
>>>
>>> --
>>> *Mike Holmes*
>>> Linaro Technical Manager / Lead
>>> LNG - ODP
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Ola Liljedahl May 9, 2014, 9:40 a.m. UTC | #10
You don't need to copy all data to be checksummed into a new buffer. You
already have code that computes the partial checksum over the pseudo header
(I assume it is correct, I could compare with my own implementation that I
have somewhere). Then call odp_chksum() to compute the partial checksum
over the UDP header and payload. Combine these two checksums (it is an ones
complement *addition*, you can compute partial sums and combine them (i.e.
add) any way you want as long as you do it using proper ones complement
addition). Then the final post-processing to get a proper UDP checksum.

You can see the checksum operation as summing (adding) N 16-bit words into
one 16-bit word. You can do this ((((W0 + W1) + W2) + W3) + ... or you can
do it (W0 + W1) + (W2 + W3) + ... the order of the partials sums don't
matter. Just remember that "+" is a ones complement addition and not the
normal twos complement addition.


-- Ola



On 9 May 2014 11:10, Weilong Chen <weilong.chen@linaro.org> wrote:

>
>
>
> On 9 May 2014 03:41, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
>
>> The UDP checksum function can use (call) the plain ones complement
>> checksum function when computing the checksum over the UDP header and
>> payload.
>> The IPv4 hdr checksum can also use (call) the ones complement checksum
>> function where the size parameter comes from the IP.length field (length in
>> 32-bit words).
>>
>> In which files these functions are declared and implemented is another
>> issue. Perhaps one header file for the ones complement checksum and other
>> generic stuff, one header files for IP definitions and one header file for
>> UDP definitions.
>>
>> > +       unsigned short udplen;
>> No reason for udplen to be a short (even though the value would fit into
>> an unsigned short), might just complicate life for the compiler (at least
>> this was the case in the past as the compiler had to truncate the value
>> into 16-bits every time the value was changed). Just use an (unsigned) int
>> which will map to a register.
>>
>
> ​OK.​
>
>
>> > +       while (sum >> 16)
>> I would prefer this to be written
>> while ((sum >> 16) != 0)
>>
>
> ​OK.​
>
>
>>
>> I don't know if the below has been covered before.
>> > +       uint8_t *buf;
>> ...
>>  > +       while (udplen > 1) {
>> > +               sum += *buf++;
>> > +               udplen -= 1;
>> > +       }
>> > +       /* if any bytes left, pad the bytes and add */
>> > +       if (udplen > 0)
>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>> You probably want buf to be "uint16_t *buf" and decrease udplen by 2.
>> Decreasing by 1 will not leave anything left for the last if-statement.
>> Better to actually call the existing ones complement checksum function
>> which might actually do it right. This is needless duplication of code.
>>
>
> Thanks for pointing my mistake,
> I planned to avoid "cast increases required alignment of target type"​
> error by uint8_t replacing uint16_t.
> It's a bad idea, I'll use "(uint16_t *)(void *)" to avoid it.
>
> If use odp_chksum(void *buffer, int len) to replace the duplicate code,
> I need to alloc a new buffer, then copy pseudo-header and udp data into
> it.
> That brings down the performance.
> Is there a better way?
>
> Thanks,
> Weilong
>
>
>
>> -- Ola
>>
>>
>>
>> On 8 May 2014 21:02, Mike Holmes <mike.holmes@linaro.org> wrote:
>>
>>> If you can't use the checksum directly as it is, can it be modified so
>>> that would work in all cases or must it be a new function ?
>>>
>>>
>>> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>
>>>> Hi,
>>>>
>>>> There's a function odp_chksum,
>>>> ​UDP packet's chksum include "pseudo-header" and data,
>>>> I can't use the current chksum api directly.
>>>>
>>>> Thanks,
>>>> Weilong
>>>>
>>>>
>>>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>>>
>>>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>>> >
>>>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>>>> > ---
>>>>> >  include/helper/odp_udp.h |   56
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>> >  1 file changed, 56 insertions(+)
>>>>> >
>>>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>>>> > index 738470e..9905d55 100644
>>>>> > --- a/include/helper/odp_udp.h
>>>>> > +++ b/include/helper/odp_udp.h
>>>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0 if
>>>>> not used)*/
>>>>> >  } odp_udphdr_t;
>>>>> >
>>>>> > +/**
>>>>> > + * UDP checksum
>>>>> > + *
>>>>> > + * This function uses odp packet to calc checksum
>>>>> > + *
>>>>> > + * @param pkt  calculate chksum for pkt
>>>>> > + * @return  checksum value
>>>>> > + */
>>>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>>>> > +{
>>>>> > +       unsigned long sum = 0;
>>>>> > +       odp_udphdr_t *udph;
>>>>> > +       odp_ipv4hdr_t *iph;
>>>>> > +       uint8_t *buf;
>>>>> > +       unsigned short udplen;
>>>>> > +       uint16_t chksum;
>>>>> > +
>>>>> > +       if (!odp_packet_l3_offset(pkt))
>>>>> > +               return 0;
>>>>> > +
>>>>> > +       if (!odp_packet_l4_offset(pkt))
>>>>> > +               return 0;
>>>>> > +
>>>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>>>> > +       buf = (uint8_t *)udph;
>>>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>>>> > +
>>>>> > +       /* the source ip */
>>>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>>>> > +       /* the dest ip */
>>>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>>>> > +       /* the length */
>>>>> > +       sum += udph->length;
>>>>> > +
>>>>> > +       while (udplen > 1) {
>>>>> > +               sum += *buf++;
>>>>> > +               udplen -= 1;
>>>>> > +       }
>>>>> > +       /* if any bytes left, pad the bytes and add */
>>>>> > +       if (udplen > 0)
>>>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>>>> > +
>>>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>>>> > +       while (sum >> 16)
>>>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>>>> > +       sum = ~sum;
>>>>> > +       /* set computation result */
>>>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>>>> > +                         : (unsigned short)sum;
>>>>>
>>>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>>>> chksum calculation.
>>>>>
>>>>> Thx.
>>>>> > +
>>>>> > +       return chksum;
>>>>> > +}
>>>>> >
>>>>> >  /** @internal Compile time assert */
>>>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>>>> ODP_UDPHDR_T__SIZE_ERROR);
>>>>> > --
>>>>> > 1.7.9.5
>>>>> >
>>>>> >
>>>>> > _______________________________________________
>>>>> > lng-odp mailing list
>>>>> > lng-odp@lists.linaro.org
>>>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>>
>>>
>>> --
>>> *Mike Holmes*
>>> Linaro Technical Manager / Lead
>>> LNG - ODP
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>
Weilong Chen May 9, 2014, 9:47 a.m. UTC | #11
Thanks very much!


On 9 May 2014 17:40, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> You don't need to copy all data to be checksummed into a new buffer. You
> already have code that computes the partial checksum over the pseudo header
> (I assume it is correct, I could compare with my own implementation that I
> have somewhere). Then call odp_chksum() to compute the partial checksum
> over the UDP header and payload. Combine these two checksums (it is an ones
> complement *addition*, you can compute partial sums and combine them (i.e.
> add) any way you want as long as you do it using proper ones complement
> addition). Then the final post-processing to get a proper UDP checksum.
>
> You can see the checksum operation as summing (adding) N 16-bit words into
> one 16-bit word. You can do this ((((W0 + W1) + W2) + W3) + ... or you can
> do it (W0 + W1) + (W2 + W3) + ... the order of the partials sums don't
> matter. Just remember that "+" is a ones complement addition and not the
> normal twos complement addition.
>
>
> -- Ola
>
>
>
> On 9 May 2014 11:10, Weilong Chen <weilong.chen@linaro.org> wrote:
>
>>
>>
>>
>> On 9 May 2014 03:41, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
>>
>>> The UDP checksum function can use (call) the plain ones complement
>>> checksum function when computing the checksum over the UDP header and
>>> payload.
>>> The IPv4 hdr checksum can also use (call) the ones complement checksum
>>> function where the size parameter comes from the IP.length field (length in
>>> 32-bit words).
>>>
>>> In which files these functions are declared and implemented is another
>>> issue. Perhaps one header file for the ones complement checksum and other
>>> generic stuff, one header files for IP definitions and one header file for
>>> UDP definitions.
>>>
>>> > +       unsigned short udplen;
>>> No reason for udplen to be a short (even though the value would fit into
>>> an unsigned short), might just complicate life for the compiler (at least
>>> this was the case in the past as the compiler had to truncate the value
>>> into 16-bits every time the value was changed). Just use an (unsigned) int
>>> which will map to a register.
>>>
>>
>> ​OK.​
>>
>>
>>> > +       while (sum >> 16)
>>> I would prefer this to be written
>>> while ((sum >> 16) != 0)
>>>
>>
>> ​OK.​
>>
>>
>>>
>>> I don't know if the below has been covered before.
>>> > +       uint8_t *buf;
>>> ...
>>>  > +       while (udplen > 1) {
>>> > +               sum += *buf++;
>>> > +               udplen -= 1;
>>> > +       }
>>> > +       /* if any bytes left, pad the bytes and add */
>>> > +       if (udplen > 0)
>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>> You probably want buf to be "uint16_t *buf" and decrease udplen by 2.
>>> Decreasing by 1 will not leave anything left for the last if-statement.
>>> Better to actually call the existing ones complement checksum function
>>> which might actually do it right. This is needless duplication of code.
>>>
>>
>> Thanks for pointing my mistake,
>> I planned to avoid "cast increases required alignment of target type"​
>> error by uint8_t replacing uint16_t.
>> It's a bad idea, I'll use "(uint16_t *)(void *)" to avoid it.
>>
>> If use odp_chksum(void *buffer, int len) to replace the duplicate code,
>> I need to alloc a new buffer, then copy pseudo-header and udp data into
>> it.
>> That brings down the performance.
>> Is there a better way?
>>
>> Thanks,
>> Weilong
>>
>>
>>
>>> -- Ola
>>>
>>>
>>>
>>> On 8 May 2014 21:02, Mike Holmes <mike.holmes@linaro.org> wrote:
>>>
>>>> If you can't use the checksum directly as it is, can it be modified so
>>>> that would work in all cases or must it be a new function ?
>>>>
>>>>
>>>> On 7 May 2014 08:21, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> There's a function odp_chksum,
>>>>> ​UDP packet's chksum include "pseudo-header" and data,
>>>>> I can't use the current chksum api directly.
>>>>>
>>>>> Thanks,
>>>>> Weilong
>>>>>
>>>>>
>>>>> On 7 May 2014 19:57, Santosh Shukla <santosh.shukla@linaro.org> wrote:
>>>>>
>>>>>> On 7 May 2014 17:13, Weilong Chen <weilong.chen@linaro.org> wrote:
>>>>>> >
>>>>>> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>>>>>> > ---
>>>>>> >  include/helper/odp_udp.h |   56
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> >  1 file changed, 56 insertions(+)
>>>>>> >
>>>>>> > diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>>>>>> > index 738470e..9905d55 100644
>>>>>> > --- a/include/helper/odp_udp.h
>>>>>> > +++ b/include/helper/odp_udp.h
>>>>>> > @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>>>>> >         uint16be_t chksum;   /**< UDP header and data checksum (0
>>>>>> if not used)*/
>>>>>> >  } odp_udphdr_t;
>>>>>> >
>>>>>> > +/**
>>>>>> > + * UDP checksum
>>>>>> > + *
>>>>>> > + * This function uses odp packet to calc checksum
>>>>>> > + *
>>>>>> > + * @param pkt  calculate chksum for pkt
>>>>>> > + * @return  checksum value
>>>>>> > + */
>>>>>> > +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>>>>>> > +{
>>>>>> > +       unsigned long sum = 0;
>>>>>> > +       odp_udphdr_t *udph;
>>>>>> > +       odp_ipv4hdr_t *iph;
>>>>>> > +       uint8_t *buf;
>>>>>> > +       unsigned short udplen;
>>>>>> > +       uint16_t chksum;
>>>>>> > +
>>>>>> > +       if (!odp_packet_l3_offset(pkt))
>>>>>> > +               return 0;
>>>>>> > +
>>>>>> > +       if (!odp_packet_l4_offset(pkt))
>>>>>> > +               return 0;
>>>>>> > +
>>>>>> > +       iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>>>>>> > +       udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>>>>>> > +       buf = (uint8_t *)udph;
>>>>>> > +       udplen = odp_be_to_cpu_16(udph->length);
>>>>>> > +
>>>>>> > +       /* the source ip */
>>>>>> > +       sum += (iph->src_addr >> 16) & 0xFFFF;
>>>>>> > +       sum += (iph->src_addr) & 0xFFFF;
>>>>>> > +       /* the dest ip */
>>>>>> > +       sum += (iph->dst_addr >> 16) & 0xFFFF;
>>>>>> > +       sum += (iph->dst_addr) & 0xFFFF;
>>>>>> > +       sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>>>>>> > +       /* the length */
>>>>>> > +       sum += udph->length;
>>>>>> > +
>>>>>> > +       while (udplen > 1) {
>>>>>> > +               sum += *buf++;
>>>>>> > +               udplen -= 1;
>>>>>> > +       }
>>>>>> > +       /* if any bytes left, pad the bytes and add */
>>>>>> > +       if (udplen > 0)
>>>>>> > +               sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>>>>>> > +
>>>>>> > +       /* Fold sum to 16 bits: add carrier to result */
>>>>>> > +       while (sum >> 16)
>>>>>> > +               sum = (sum & 0xFFFF) + (sum >> 16);
>>>>>> > +       sum = ~sum;
>>>>>> > +       /* set computation result */
>>>>>> > +       chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>>>>>> > +                         : (unsigned short)sum;
>>>>>>
>>>>>> IIRC, chksum api already in odp/include/helper, why not use them for
>>>>>> chksum calculation.
>>>>>>
>>>>>> Thx.
>>>>>> > +
>>>>>> > +       return chksum;
>>>>>> > +}
>>>>>> >
>>>>>> >  /** @internal Compile time assert */
>>>>>> >  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN,
>>>>>> ODP_UDPHDR_T__SIZE_ERROR);
>>>>>> > --
>>>>>> > 1.7.9.5
>>>>>> >
>>>>>> >
>>>>>> > _______________________________________________
>>>>>> > lng-odp mailing list
>>>>>> > lng-odp@lists.linaro.org
>>>>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> lng-odp mailing list
>>>>> lng-odp@lists.linaro.org
>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> *Mike Holmes*
>>>> Linaro Technical Manager / Lead
>>>> LNG - ODP
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>>
>>
>
Anders Roxell May 9, 2014, 12:50 p.m. UTC | #12
On 2014-05-07 19:43, Weilong Chen wrote:
> 
> Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
> ---

I'm curious, why is this patch set pushed?
Are all the issues resolved and do we reuse the checksumming as
was suggested today?

Cheers,
Anders

>  include/helper/odp_udp.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
> index 738470e..9905d55 100644
> --- a/include/helper/odp_udp.h
> +++ b/include/helper/odp_udp.h
> @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>  	uint16be_t chksum;   /**< UDP header and data checksum (0 if not used)*/
>  } odp_udphdr_t;
>  
> +/**
> + * UDP checksum
> + *
> + * This function uses odp packet to calc checksum
> + *
> + * @param pkt  calculate chksum for pkt
> + * @return  checksum value
> + */
> +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
> +{
> +	unsigned long sum = 0;
> +	odp_udphdr_t *udph;
> +	odp_ipv4hdr_t *iph;
> +	uint8_t *buf;
> +	unsigned short udplen;
> +	uint16_t chksum;
> +
> +	if (!odp_packet_l3_offset(pkt))
> +		return 0;
> +
> +	if (!odp_packet_l4_offset(pkt))
> +		return 0;
> +
> +	iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
> +	udph = (odp_udphdr_t *)odp_packet_l4(pkt);
> +	buf = (uint8_t *)udph;
> +	udplen = odp_be_to_cpu_16(udph->length);
> +
> +	/* the source ip */
> +	sum += (iph->src_addr >> 16) & 0xFFFF;
> +	sum += (iph->src_addr) & 0xFFFF;
> +	/* the dest ip */
> +	sum += (iph->dst_addr >> 16) & 0xFFFF;
> +	sum += (iph->dst_addr) & 0xFFFF;
> +	sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
> +	/* the length */
> +	sum += udph->length;
> +
> +	while (udplen > 1) {
> +		sum += *buf++;
> +		udplen -= 1;
> +	}
> +	/* if any bytes left, pad the bytes and add */
> +	if (udplen > 0)
> +		sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
> +
> +	/* Fold sum to 16 bits: add carrier to result */
> +	while (sum >> 16)
> +		sum = (sum & 0xFFFF) + (sum >> 16);
> +	sum = ~sum;
> +	/* set computation result */
> +	chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
> +			  : (unsigned short)sum;
> +
> +	return chksum;
> +}
>  
>  /** @internal Compile time assert */
>  ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN, ODP_UDPHDR_T__SIZE_ERROR);
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Maxim Uvarov May 9, 2014, 4:47 p.m. UTC | #13
On 05/09/2014 04:50 PM, Anders Roxell wrote:
> On 2014-05-07 19:43, Weilong Chen wrote:
>> Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
>> ---
> I'm curious, why is this patch set pushed?
> Are all the issues resolved and do we reuse the checksumming as
> was suggested today?
>
> Cheers,
> Anders
Discussion started after patch reached v4. Before I and Santosh used 
checksum in our
code so checksum itself works. After discussion it can be refined and 
should go in separate
patch.

Thanks,
Maxim.
>>   include/helper/odp_udp.h |   56 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 56 insertions(+)
>>
>> diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
>> index 738470e..9905d55 100644
>> --- a/include/helper/odp_udp.h
>> +++ b/include/helper/odp_udp.h
>> @@ -33,6 +33,62 @@ typedef struct ODP_PACKED {
>>   	uint16be_t chksum;   /**< UDP header and data checksum (0 if not used)*/
>>   } odp_udphdr_t;
>>   
>> +/**
>> + * UDP checksum
>> + *
>> + * This function uses odp packet to calc checksum
>> + *
>> + * @param pkt  calculate chksum for pkt
>> + * @return  checksum value
>> + */
>> +static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
>> +{
>> +	unsigned long sum = 0;
>> +	odp_udphdr_t *udph;
>> +	odp_ipv4hdr_t *iph;
>> +	uint8_t *buf;
>> +	unsigned short udplen;
>> +	uint16_t chksum;
>> +
>> +	if (!odp_packet_l3_offset(pkt))
>> +		return 0;
>> +
>> +	if (!odp_packet_l4_offset(pkt))
>> +		return 0;
>> +
>> +	iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
>> +	udph = (odp_udphdr_t *)odp_packet_l4(pkt);
>> +	buf = (uint8_t *)udph;
>> +	udplen = odp_be_to_cpu_16(udph->length);
>> +
>> +	/* the source ip */
>> +	sum += (iph->src_addr >> 16) & 0xFFFF;
>> +	sum += (iph->src_addr) & 0xFFFF;
>> +	/* the dest ip */
>> +	sum += (iph->dst_addr >> 16) & 0xFFFF;
>> +	sum += (iph->dst_addr) & 0xFFFF;
>> +	sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
>> +	/* the length */
>> +	sum += udph->length;
>> +
>> +	while (udplen > 1) {
>> +		sum += *buf++;
>> +		udplen -= 1;
>> +	}
>> +	/* if any bytes left, pad the bytes and add */
>> +	if (udplen > 0)
>> +		sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
>> +
>> +	/* Fold sum to 16 bits: add carrier to result */
>> +	while (sum >> 16)
>> +		sum = (sum & 0xFFFF) + (sum >> 16);
>> +	sum = ~sum;
>> +	/* set computation result */
>> +	chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
>> +			  : (unsigned short)sum;
>> +
>> +	return chksum;
>> +}
>>   
>>   /** @internal Compile time assert */
>>   ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN, ODP_UDPHDR_T__SIZE_ERROR);
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/include/helper/odp_udp.h b/include/helper/odp_udp.h
index 738470e..9905d55 100644
--- a/include/helper/odp_udp.h
+++ b/include/helper/odp_udp.h
@@ -33,6 +33,62 @@  typedef struct ODP_PACKED {
 	uint16be_t chksum;   /**< UDP header and data checksum (0 if not used)*/
 } odp_udphdr_t;
 
+/**
+ * UDP checksum
+ *
+ * This function uses odp packet to calc checksum
+ *
+ * @param pkt  calculate chksum for pkt
+ * @return  checksum value
+ */
+static inline uint16_t odp_ipv4_udp_chksum(odp_packet_t pkt)
+{
+	unsigned long sum = 0;
+	odp_udphdr_t *udph;
+	odp_ipv4hdr_t *iph;
+	uint8_t *buf;
+	unsigned short udplen;
+	uint16_t chksum;
+
+	if (!odp_packet_l3_offset(pkt))
+		return 0;
+
+	if (!odp_packet_l4_offset(pkt))
+		return 0;
+
+	iph = (odp_ipv4hdr_t *)odp_packet_l3(pkt);
+	udph = (odp_udphdr_t *)odp_packet_l4(pkt);
+	buf = (uint8_t *)udph;
+	udplen = odp_be_to_cpu_16(udph->length);
+
+	/* the source ip */
+	sum += (iph->src_addr >> 16) & 0xFFFF;
+	sum += (iph->src_addr) & 0xFFFF;
+	/* the dest ip */
+	sum += (iph->dst_addr >> 16) & 0xFFFF;
+	sum += (iph->dst_addr) & 0xFFFF;
+	sum += odp_cpu_to_be_16(ODP_IPPROTO_UDP);
+	/* the length */
+	sum += udph->length;
+
+	while (udplen > 1) {
+		sum += *buf++;
+		udplen -= 1;
+	}
+	/* if any bytes left, pad the bytes and add */
+	if (udplen > 0)
+		sum += ((*buf)&odp_cpu_to_be_16(0xFF00));
+
+	/* Fold sum to 16 bits: add carrier to result */
+	while (sum >> 16)
+		sum = (sum & 0xFFFF) + (sum >> 16);
+	sum = ~sum;
+	/* set computation result */
+	chksum = ((unsigned short)sum == 0x0) ? 0xFFFF
+			  : (unsigned short)sum;
+
+	return chksum;
+}
 
 /** @internal Compile time assert */
 ODP_ASSERT(sizeof(odp_udphdr_t) == ODP_UDPHDR_LEN, ODP_UDPHDR_T__SIZE_ERROR);