On Wed, Jan 31, 2024 at 10:04 AM David Marchand <[email protected]> wrote: > > On Tue, Jan 30, 2024 at 11:15 PM Mike Pattrick <[email protected]> wrote: > > > > Include inner offsets in functions where l3 and l4 offsets are either > > modified or checked. > > > > Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") > > Signed-off-by: Mike Pattrick <[email protected]> > > --- > > v2: > > > > - Prints out new offsets in autovalidator > > - Extends resize_l2 change to avx512 > > > > Signed-off-by: Mike Pattrick <[email protected]> > > --- > > lib/dp-packet.c | 18 +++++++++++++----- > > lib/odp-execute-avx512.c | 19 ++++++++++++++----- > > 2 files changed, 27 insertions(+), 10 deletions(-) > > > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > > index 0e23c766e..640b1dfeb 100644 > > --- a/lib/dp-packet.c > > +++ b/lib/dp-packet.c > > @@ -507,6 +507,8 @@ dp_packet_resize_l2_5(struct dp_packet *b, int > > increment) > > /* Adjust layer offsets after l2_5. */ > > dp_packet_adjust_layer_offset(&b->l3_ofs, increment); > > dp_packet_adjust_layer_offset(&b->l4_ofs, increment); > > + dp_packet_adjust_layer_offset(&b->inner_l3_ofs, increment); > > + dp_packet_adjust_layer_offset(&b->inner_l4_ofs, increment); > > > > return dp_packet_data(b); > > } > > @@ -529,17 +531,23 @@ dp_packet_compare_offsets(struct dp_packet *b1, > > struct dp_packet *b2, > > if ((b1->l2_pad_size != b2->l2_pad_size) || > > (b1->l2_5_ofs != b2->l2_5_ofs) || > > (b1->l3_ofs != b2->l3_ofs) || > > - (b1->l4_ofs != b2->l4_ofs)) { > > + (b1->l4_ofs != b2->l4_ofs) || > > + (b1->inner_l3_ofs != b2->inner_l3_ofs) || > > + (b1->inner_l4_ofs != b2->inner_l4_ofs)) { > > if (err_str) { > > ds_put_format(err_str, "Packet offset comparison failed\n"); > > ds_put_format(err_str, "Buffer 1 offsets: l2_pad_size %u," > > - " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n", > > + " l2_5_ofs : %u l3_ofs %u, inner_l3_ofs %u," > > + " l4_ofs %u, inner_l4_ofs %u\n", > > b1->l2_pad_size, b1->l2_5_ofs, > > - b1->l3_ofs, b1->l4_ofs); > > + b1->l3_ofs, b1->inner_l3_ofs, > > + b1->l4_ofs, b1->inner_l4_ofs); > > ds_put_format(err_str, "Buffer 2 offsets: l2_pad_size %u," > > - " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n", > > + " l2_5_ofs : %u l3_ofs %u, inner_l3_ofs %u," > > + " l4_ofs %u, inner_l4_ofs %u\n", > > b2->l2_pad_size, b2->l2_5_ofs, > > - b2->l3_ofs, b2->l4_ofs); > > + b2->l3_ofs, b2->inner_l3_ofs, > > + b2->l4_ofs, b2->inner_l4_ofs); > > } > > return false; > > } > > Not a strong opinion, but I prefer keeping those offsets in the same > order than a real packet layout, rather than mix l3 / l4 outer/inner > offsets. > IOW: > - " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n", > + " l2_5_ofs : %u l3_ofs %u, l4_ofs %u," > + " inner_l3_ofs : %u, inner_l4_ofs %u\n", > > > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c > > index 747e04014..7f9870669 100644 > > --- a/lib/odp-execute-avx512.c > > +++ b/lib/odp-execute-avx512.c > > @@ -35,10 +35,11 @@ > > > > VLOG_DEFINE_THIS_MODULE(odp_execute_avx512); > > > > -/* The below three build asserts make sure that l2_5_ofs, l3_ofs, and > > l4_ofs > > - * fields remain in the same order and offset to l2_padd_size. This is > > needed > > - * as the avx512_dp_packet_resize_l2() function will manipulate those > > fields at > > - * a fixed memory index based on the l2_padd_size offset. */ > > +/* The below three build asserts make sure that l2_5_ofs, l3_ofs, l4_ofs, > > Counting build asserts is useless in a comment.. and here it gets > wrong after the change. > I suggest a simple: "The below build asserts". > > > > + * inner_l3_ofs, and inner_l4_ofs fields remain in the same order and > > offset to > > + * l2_padd_size. This is needed as the avx512_dp_packet_resize_l2() > > function > > l2_pad_size* > > > > + * will manipulate those fields at a fixed memory index based on the > > + * l2_padd_size offset. */ > > Idem. > > > > BUILD_ASSERT_DECL(offsetof(struct dp_packet, l2_pad_size) + > > MEMBER_SIZEOF(struct dp_packet, l2_pad_size) == > > offsetof(struct dp_packet, l2_5_ofs)); > > @@ -51,6 +52,14 @@ BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) + > > MEMBER_SIZEOF(struct dp_packet, l3_ofs) == > > offsetof(struct dp_packet, l4_ofs)); > > > > +BUILD_ASSERT_DECL(offsetof(struct dp_packet, l4_ofs) + > > + MEMBER_SIZEOF(struct dp_packet, l4_ofs) == > > + offsetof(struct dp_packet, inner_l3_ofs)); > > + > > +BUILD_ASSERT_DECL(offsetof(struct dp_packet, inner_l3_ofs) + > > + MEMBER_SIZEOF(struct dp_packet, inner_l3_ofs) == > > + offsetof(struct dp_packet, inner_l4_ofs)); > > + > > /* The below build assert makes sure it's safe to read/write 128-bits > > starting > > * at the l2_pad_size location. */ > > BUILD_ASSERT_DECL(sizeof(struct dp_packet) - > > @@ -125,7 +134,7 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int > > resize_by_bytes) > > /* Each lane represents 16 bits in a 12-bit register. In this case the > > * first three 16-bit values, which will map to the l2_5_ofs, l3_ofs > > and > > * l4_ofs fields. */ > > - const uint8_t k_lanes = 0b1110; > > + const uint8_t k_lanes = 0b111110; > > > > /* Set all 16-bit words in the 128-bits v_offset register to the value > > we > > * need to add/substract from the l2_5_ofs, l3_ofs, and l4_ofs fields. > > */ > > Touching this part scares me. > I think some comments are wrong, and otherwise I hope Intel CI will be > enough to check nothing gets broken here :-).
A lot of thought and apprehension went into flipping two bits. One of the reasons that I didn't catch this before is because the normal machine I dev and test on doesn't have AVX512. However, I now have a VM on a newer CPU, this change satisfies the autovalidator with actual traffic flowing. But as you point out, various comments are incorrect. I'll send in a newer version with comments corrected. -M > > > > -- > David Marchand > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
