> -----Original Message----- > From: Stokes, Ian <[email protected]> > Sent: Wednesday 12 January 2022 20:34 > To: Finn, Emma <[email protected]>; [email protected]; Van Haaren, > Harry <[email protected]>; Amber, Kumar > <[email protected]>; [email protected] > Subject: RE: [PATCH v5 7/8] odp-execute: Add ISA implementation of pop_vlan > action. > > > This commit adds the AVX512 implementation of the pop_vlan action. > > The implementation here is auto-validated by the miniflow extract > > autovalidator, hence its correctness can be easily tested and > > verified. > > > > Signed-off-by: Emma Finn <[email protected]> > > Hi Emma, some comments below. > Thanks Ian, Comments here are misleading so I will update them in v6 to make things clearer. > > --- > > lib/odp-execute-avx512.c | 77 > > ++++++++++++++++++++++++++++++++++++++- > > lib/odp-execute-private.c | 2 +- > > lib/odp-execute-private.h | 2 +- > > 3 files changed, 78 insertions(+), 3 deletions(-) > > > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index > > aa71faa1c..fcf27f070 100644 > > --- a/lib/odp-execute-avx512.c > > +++ b/lib/odp-execute-avx512.c > > @@ -14,6 +14,11 @@ > > * limitations under the License. > > */ > > > > +#ifdef __x86_64__ > > +/* Sparse cannot handle the AVX512 instructions. */ #if > > +!defined(__CHECKER__) > > + > > + > > #include <config.h> > > #include <errno.h> > > > > @@ -25,6 +30,71 @@ > > > > #include "immintrin.h" > > > > +VLOG_DEFINE_THIS_MODULE(odp_execute_avx512); > > +BUILD_ASSERT_DECL(offsetof(struct dp_packet, l2_5_ofs) + > > + MEMBER_SIZEOF(struct dp_packet, l2_5_ofs) == > > + offsetof(struct dp_packet, l3_ofs)); > > + > > +BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) + > > + MEMBER_SIZEOF(struct dp_packet, l3_ofs) == > > + offsetof(struct dp_packet, l4_ofs)); > > + > > +static inline void ALWAYS_INLINE > > +avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes) > > +{ > > + /* update packet size/data pointers */ > Minor, Capitalize start of comment, missing period (goes for a few of the > comments in the rest of this function also). > > > + dp_packet_set_data(b, (char *) dp_packet_data(b) - resize_by_bytes); > > + dp_packet_set_size(b, dp_packet_size(b) + resize_by_bytes); > > + > > + /* Increment u16 packet offset values */ > > + const __m128i v_zeros = _mm_setzero_si128(); > > + const __m128i v_u16_max = _mm_cmpeq_epi16(v_zeros, v_zeros); > > + > > + /* Only these lanes can be incremented for push-VLAN action. */
Only these lanes should be updated for L2 of a packet. > > + const uint8_t k_lanes = 0b1110; Scalar value of the VLAN_HEADER length is broadcasted across a whole SIMD register > > + __m128i v_offset = _mm_set1_epi16(VLAN_HEADER_LEN); > > Can you walk through the above logic, as this is the pop use cases, are you > saying you don't want to use these lanes as they should only be used for the > push vlan case? > > + > > + /* Load packet and compare with UINT16_MAX */ > > + void *adjust_ptr = &b->l2_pad_size; Load the 4 uint16 values from dp_packet into a SIMD register. And in this case that would be the 4 offset values in the packet mbuf. > > + __m128i v_adjust_src = _mm_loadu_si128(adjust_ptr); Generate a k mask to use for updating offset values of the packet buffer. > > + __mmask8 k_cmp = _mm_mask_cmpneq_epu16_mask(k_lanes, > > v_adjust_src, > > + v_u16_max); > > + > > + /* Add VLAN_HEADER_LEN using compare mask, store results. */ > Use a k mask subtract (for pop use case) to update the offset values of the packet buffer and store the updated values back > Again I'm confused here, if the operation being added is to pop_vlan why are > we > adding a vlan header? > > If you could give a general run down of the expected operations here and > logic it > would be appreciated. > > Thanks > Ian _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
