Hi Morten,
Thank you for the review.
<...>
On Tue, Apr 27, 2021 at 05:07:04PM +0200, Morten Brørup wrote:
> > +static inline uint16_t
> > +rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void
> > *l4_hdr)
> > +{
> > + uint16_t cksum = __rte_ipv4_udptcp_cksum(ipv4_hdr, l4_hdr);
> > +
> > + cksum = ~cksum;
> > +
> > /*
> > - * Per RFC 768:If the computed checksum is zero for UDP,
> > + * Per RFC 768: If the computed checksum is zero for UDP,
> > * it is transmitted as all ones
> > * (the equivalent in one's complement arithmetic).
> > */
> > if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
> > cksum = 0xffff;
> >
> > - return (uint16_t)cksum;
> > + return cksum;
> > +}
>
> The GCC static branch predictor treats the above comparison as likely.
> Playing around with Godbolt, I came up with this alternative:
>
> if (likely(cksum != 0)) return cksum;
> if (ipv4_hdr->next_proto_id == IPPROTO_UDP) return 0xffff;
> return 0;
Good idea, this is indeed an unlikely branch.
However this code was already present before this patch,
so I suggest to add it as a specific optimization patch.
> > +
> > +/**
> > + * Validate the IPv4 UDP or TCP checksum.
> > + *
> > + * @param ipv4_hdr
> > + * The pointer to the contiguous IPv4 header.
> > + * @param l4_hdr
> > + * The pointer to the beginning of the L4 header.
> > + * @return
> > + * Return 0 if the checksum is correct, else -1.
> > + */
> > +__rte_experimental
> > +static inline int
> > +rte_ipv4_udptcp_cksum_verify(const struct rte_ipv4_hdr *ipv4_hdr,
> > + const void *l4_hdr)
> > +{
> > + uint16_t cksum = __rte_ipv4_udptcp_cksum(ipv4_hdr, l4_hdr);
> > +
> > + if (cksum != 0xffff)
> > + return -1;
>
> The GCC static branch predictor treats the above comparison as likely, so I
> would prefer unlikely() around it.
For this one, I'm less convinced: should we decide here whether
the good or the bad checksum is more likely than the other?
Given it's a static inline function, wouldn't it be better to let
the application call it this way:
if (likely(rte_ipv4_udptcp_cksum_verify(...) == 0)) ?
Regards,
Olivier
