Message ID | 20240612104423.3285377-1-emma.finn@intel.com |
---|---|
State | Accepted |
Commit | 3f4df4c7bfe4ecd662a31a00a89eb990752c9879 |
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 > >
On 12 Jun 2024, at 12:44, Emma Finn 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> This patch is missing a fixes tag. Fixes: a879beb4dbee ("odp-execute: Add ISA implementation of set_masked IPv6 action") If no one else has comments on this patch, I can add this when I apply the patch (same for the nit below). The rest looks good to me. Cheers, Eelco > --- > 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); I thinks this could be rewritten as; 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); If you agree I can apply at commit. > + } > } > } > #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
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Thursday, June 20, 2024 9:41 AM > To: Finn, Emma <emma.finn@intel.com> > Cc: ovs-dev@openvswitch.org > Subject: Re: [PATCH] odp-execute: Set IPv6 traffic class in AVX > implementation. > > On 12 Jun 2024, at 12:44, Emma Finn 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> > > This patch is missing a fixes tag. > > Fixes: a879beb4dbee ("odp-execute: Add ISA implementation of set_masked > IPv6 action") > > If no one else has comments on this patch, I can add this when I apply the > patch (same for the nit below). > > The rest looks good to me. > > > Cheers, > > Eelco > > > --- > > 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); > > I thinks this could be rewritten as; > > 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); > > If you agree I can apply at commit. > Yes, looks good to me. 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
On 12 Jun 2024, at 12:44, Emma Finn 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> Thanks for the patch Emma, it has been applied to main and down to 3.2. Cheers, Eelco
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(-)