Hi Amber,
As part of reviewing this patch series, I wanted to first run/test the code to
familiarize myself with it.
As part of this, I ran in to some problems where the IPv6 packets I use aren't
parsed by the AVX512 implementations unless they are 106B or larger.
I had a look at why this is and can see that some length checks are failing in
my case. Please see my points below about these length checks.
Hopefully my understanding is correct and makes sense. I wanted to reply with
this feedback now before looking at the series in more detail.
Thanks for the patches.
Cian
> -----Original Message-----
> From: Amber, Kumar <kumar.am...@intel.com>
> Sent: Tuesday 15 March 2022 04:12
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; Ferriter, Cian <cian.ferri...@intel.com>; Stokes, Ian
> <ian.sto...@intel.com>;
> f...@sysclose.org; echau...@redhat.com; Van Haaren, Harry
> <harry.van.haa...@intel.com>; Amber, Kumar
> <kumar.am...@intel.com>
> Subject: [PATCH v6 1/6] dpif-netdev/mfex: Add AVX512 ipv6 traffic profiles
>
> Add AVX512 Ipv6 optimized profile for vlan/IPv6/UDP and
> vlan/IPv6/TCP, IPv6/UDP and IPv6/TCP.
>
> MFEX autovalidaton test-case already has the IPv6 support for
> validating against the scalar mfex.
>
> Signed-off-by: Kumar Amber <kumar.am...@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
> Co-authored-by: Harry van Haaren <harry.van.haa...@intel.com>
>
<snip>
> +
> +/* IPv6 specific helper functions, for calculating offsets/lengths. */
> +static int
> +mfex_ipv6_set_l2_pad_size(struct dp_packet *pkt,
> + struct ovs_16aligned_ip6_hdr *nh,
> + uint32_t payload_size_ipv6,
> + uint32_t next_hdr_size)
> +{
> + /* Handle dynamic l2_pad_size. */
> + uint16_t p_len = ntohs(nh->plen);
> +
> + /* Error if IP total length is greater than remaining packet size. */
> + bool err_ipv6_len_too_high = p_len >= payload_size_ipv6;
I think this above check is wrong. p_len is the IPv6 payload length. For
example, for an ETH/IPv6/UDP/Payload(len=10B), the p_len will be 18B (8B for
UDP + 10B for payload).
payload_size_ipv6 is like the len_from_ipv4 or size_from_ipv4 field used below
in the mfex_ipv4_set_l2_pad_size() function. It's the actual packet size minus
sizeof(struct eth_header).
I think payload_size_ipv6 should be called len_from_ipv6, so it won't be
confused with the actual payload length IPv6 field.
What are we trying to accomplish with the below check? I think we want to check
if the p_len value in the packet is too big, right?
p_len is too big when:
p_len > payload_size_ipv6 - IPv6_HEADER_LEN
or
plen + IPV6_HEADER_LEN > payload_size_ipv6
This 2nd statement above is the same as what's in the ipv6_sanity_check()
function which carries out similar checks for the scalar MFEX.
Is my understanding correct here?
> +
> + /* Error if IP total length is less than the size of the IP header
> + * itself, and the size of the next-protocol this profile matches on.
> + */
> + bool err_ipv6_len_too_low = (IPV6_HEADER_LEN + next_hdr_size) > p_len;
I think this is wrong too. p_len doesn't include the actual IPv6 header in its
length (see my example above).
> +
> + /* Ensure the l2 pad size will not overflow. */
> + bool err_len_u16_overflow = (payload_size_ipv6 - p_len) > UINT16_MAX;
I think this overflow check is wrong, I think we should follow what's done in
ipv6_sanity_check() for overflow (the last check in the function).
> +
> + if (OVS_UNLIKELY(err_ipv6_len_too_high || err_ipv6_len_too_low ||
> + err_len_u16_overflow)) {
> + return -1;
> + }
> + dp_packet_set_l2_pad_size(pkt, payload_size_ipv6 - p_len);
> + return 0;
> +}
>
<snip>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
