| Message ID | 20240612104423.3285377-1-emma.finn@intel.com |
|---|---|
| State | New |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev] odp-execute: Set IPv6 traffic class in AVX implementation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Wed, Jun 12, 2024 at 6:44 AM Emma Finn <emma.finn@intel.com> wrote: > > The AVX implementation for the IPv6 action did not set > traffic class field. Adding support for this field to > the AVX implementation. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Reported-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/odp-execute-avx512.c | 8 ++++++++ > lib/packets.c | 2 +- > lib/packets.h | 1 + > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c > index a74a85dc1..569ea789e 100644 > --- a/lib/odp-execute-avx512.c > +++ b/lib/odp-execute-avx512.c > @@ -741,6 +741,14 @@ action_avx512_set_ipv6(struct dp_packet_batch *batch, const struct nlattr *a) > } > /* Write back the modified IPv6 addresses. */ > _mm512_mask_storeu_epi64((void *) nh, 0x1F, v_new_hdr); > + > + /* Scalar method for setting IPv6 tclass field. */ > + if (key->ipv6_tclass) { > + uint8_t old_tc = ntohl(get_16aligned_be32(&nh->ip6_flow)) >> 20; > + uint8_t key_tc = (key->ipv6_tclass | > + (old_tc & ~mask->ipv6_tclass)); > + packet_set_ipv6_tc(&nh->ip6_flow, key_tc); > + } Hello, I'm wondering if we also need to set the flow label? Thanks, M > } > } > #endif /* HAVE_AVX512VBMI */ > diff --git a/lib/packets.c b/lib/packets.c > index ebf516d67..91c28daf0 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -1299,7 +1299,7 @@ packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, ovs_be32 flow_key) > put_16aligned_be32(flow_label, new_label); > } > > -static void > +void > packet_set_ipv6_tc(ovs_16aligned_be32 *flow_label, uint8_t tc) > { > ovs_be32 old_label = get_16aligned_be32(flow_label); > diff --git a/lib/packets.h b/lib/packets.h > index 8b6994809..a102f8163 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -1635,6 +1635,7 @@ void packet_set_ipv6_addr(struct dp_packet *packet, uint8_t proto, > bool recalculate_csum); > void packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, > ovs_be32 flow_key); > +void packet_set_ipv6_tc(ovs_16aligned_be32 *flow_label, uint8_t tc); > void packet_set_tcp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); > void packet_set_udp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); > void packet_set_sctp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); > -- > 2.34.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> -----Original Message----- > From: Mike Pattrick <mkp@redhat.com> > Sent: Thursday, June 13, 2024 6:53 PM > To: Finn, Emma <emma.finn@intel.com> > Cc: ovs-dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] odp-execute: Set IPv6 traffic class in AVX > implementation. > > On Wed, Jun 12, 2024 at 6:44 AM Emma Finn <emma.finn@intel.com> wrote: > > > > The AVX implementation for the IPv6 action did not set traffic class > > field. Adding support for this field to the AVX implementation. > > > > Signed-off-by: Emma Finn <emma.finn@intel.com> > > Reported-by: Eelco Chaudron <echaudro@redhat.com> > > --- > > lib/odp-execute-avx512.c | 8 ++++++++ > > lib/packets.c | 2 +- > > lib/packets.h | 1 + > > 3 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index > > a74a85dc1..569ea789e 100644 > > --- a/lib/odp-execute-avx512.c > > +++ b/lib/odp-execute-avx512.c > > @@ -741,6 +741,14 @@ action_avx512_set_ipv6(struct dp_packet_batch > *batch, const struct nlattr *a) > > } > > /* Write back the modified IPv6 addresses. */ > > _mm512_mask_storeu_epi64((void *) nh, 0x1F, v_new_hdr); > > + > > + /* Scalar method for setting IPv6 tclass field. */ > > + if (key->ipv6_tclass) { > > + uint8_t old_tc = ntohl(get_16aligned_be32(&nh->ip6_flow)) >> 20; > > + uint8_t key_tc = (key->ipv6_tclass | > > + (old_tc & ~mask->ipv6_tclass)); > > + packet_set_ipv6_tc(&nh->ip6_flow, key_tc); > > + } > > Hello, > > I'm wondering if we also need to set the flow label? > > Thanks, > M > Flow label is being handled okay by the AVX implementation. It was only the traffic class field that was causing issues. The shuffle mask was ignoring the traffic class field. And since the traffic class is not byte aligned, it was too difficult to reorder the shuffle mask. Hence, after the AVX implementation has stored back the ipv6 entire header, we can use the scalar method at the end to update the traffic class only. Thanks, Emma > > } > > } > > #endif /* HAVE_AVX512VBMI */ > > diff --git a/lib/packets.c b/lib/packets.c index ebf516d67..91c28daf0 > > 100644 > > --- a/lib/packets.c > > +++ b/lib/packets.c > > @@ -1299,7 +1299,7 @@ packet_set_ipv6_flow_label(ovs_16aligned_be32 > *flow_label, ovs_be32 flow_key) > > put_16aligned_be32(flow_label, new_label); } > > > > -static void > > +void > > packet_set_ipv6_tc(ovs_16aligned_be32 *flow_label, uint8_t tc) { > > ovs_be32 old_label = get_16aligned_be32(flow_label); diff --git > > a/lib/packets.h b/lib/packets.h index 8b6994809..a102f8163 100644 > > --- a/lib/packets.h > > +++ b/lib/packets.h > > @@ -1635,6 +1635,7 @@ void packet_set_ipv6_addr(struct dp_packet > *packet, uint8_t proto, > > bool recalculate_csum); void > > packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, > > ovs_be32 flow_key); > > +void packet_set_ipv6_tc(ovs_16aligned_be32 *flow_label, uint8_t tc); > > void packet_set_tcp_port(struct dp_packet *, ovs_be16 src, ovs_be16 > > dst); void packet_set_udp_port(struct dp_packet *, ovs_be16 src, > > ovs_be16 dst); void packet_set_sctp_port(struct dp_packet *, ovs_be16 > > src, ovs_be16 dst); > > -- > > 2.34.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index a74a85dc1..569ea789e 100644 --- a/lib/odp-execute-avx512.c +++ b/lib/odp-execute-avx512.c @@ -741,6 +741,14 @@ action_avx512_set_ipv6(struct dp_packet_batch *batch, const struct nlattr *a) } /* Write back the modified IPv6 addresses. */ _mm512_mask_storeu_epi64((void *) nh, 0x1F, v_new_hdr); + + /* Scalar method for setting IPv6 tclass field. */ + if (key->ipv6_tclass) { + uint8_t old_tc = ntohl(get_16aligned_be32(&nh->ip6_flow)) >> 20; + uint8_t key_tc = (key->ipv6_tclass | + (old_tc & ~mask->ipv6_tclass)); + packet_set_ipv6_tc(&nh->ip6_flow, key_tc); + } } } #endif /* HAVE_AVX512VBMI */ diff --git a/lib/packets.c b/lib/packets.c index ebf516d67..91c28daf0 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -1299,7 +1299,7 @@ packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, ovs_be32 flow_key) put_16aligned_be32(flow_label, new_label); } -static void +void packet_set_ipv6_tc(ovs_16aligned_be32 *flow_label, uint8_t tc) { ovs_be32 old_label = get_16aligned_be32(flow_label); diff --git a/lib/packets.h b/lib/packets.h index 8b6994809..a102f8163 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -1635,6 +1635,7 @@ void packet_set_ipv6_addr(struct dp_packet *packet, uint8_t proto, bool recalculate_csum); void packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, ovs_be32 flow_key); +void packet_set_ipv6_tc(ovs_16aligned_be32 *flow_label, uint8_t tc); void packet_set_tcp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); void packet_set_udp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); void packet_set_sctp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
The AVX implementation for the IPv6 action did not set traffic class field. Adding support for this field to the AVX implementation. Signed-off-by: Emma Finn <emma.finn@intel.com> Reported-by: Eelco Chaudron <echaudro@redhat.com> --- lib/odp-execute-avx512.c | 8 ++++++++ lib/packets.c | 2 +- lib/packets.h | 1 + 3 files changed, 10 insertions(+), 1 deletion(-)