date:20240211

[ovs-dev] [PATCH v4 4/4] ofproto-dpif-monitor: Remove unneeded calls to clear packets.

2024-02-11 Thread Mike Pattrick

Currently the monitor will call dp_packet_clear() on the dp_packet that
is shared amongst BFD, LLDP, and CFM. However, all of these packets are
created with eth_compose(), which already calls dp_packet_clear().

Reviewed-by: David Marchand 
Signed-off-by: Mike Pattrick 
---
 ofproto/ofproto-dpif-monitor.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-monitor.c b/ofproto/ofproto-dpif-monitor.c
index bb0e49091..5132f9c95 100644
--- a/ofproto/ofproto-dpif-monitor.c
+++ b/ofproto/ofproto-dpif-monitor.c
@@ -275,19 +275,16 @@ monitor_mport_run(struct mport *mport, struct dp_packet 
*packet)
 long long int lldp_wake_time = LLONG_MAX;
 
 if (mport->cfm && cfm_should_send_ccm(mport->cfm)) {
-dp_packet_clear(packet);
 cfm_compose_ccm(mport->cfm, packet, mport->hw_addr);
 ofproto_dpif_send_packet(mport->ofport, false, packet);
 }
 if (mport->bfd && bfd_should_send_packet(mport->bfd)) {
 bool oam;
 
-dp_packet_clear(packet);
 bfd_put_packet(mport->bfd, packet, mport->hw_addr, );
 ofproto_dpif_send_packet(mport->ofport, oam, packet);
 }
 if (mport->lldp && lldp_should_send_packet(mport->lldp)) {
-dp_packet_clear(packet);
 lldp_put_packet(mport->lldp, packet, mport->hw_addr);
 ofproto_dpif_send_packet(mport->ofport, false, packet);
 }
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 3/4] dp-packet: Include inner offsets in adjustments and checks.

2024-02-11 Thread Mike Pattrick

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 
---
v2:
 - Prints out new offsets in autovalidator
 - Extends resize_l2 change to avx512
v3:
 - Reordered fields in dp_packet_compare_offsets error print message
 - Updated and simplified comments in avx512_dp_packet_resize_l2()
v4:
 - Removed comment about three asserts
---
 lib/dp-packet.c  | 18 +-
 lib/odp-execute-avx512.c | 31 ---
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 0e23c766e..305822293 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(>l3_ofs, increment);
 dp_packet_adjust_layer_offset(>l4_ofs, increment);
+dp_packet_adjust_layer_offset(>inner_l3_ofs, increment);
+dp_packet_adjust_layer_offset(>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, l4_ofs %u,"
+  " inner_l3_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->l4_ofs,
+  b1->inner_l3_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, l4_ofs %u,"
+  " inner_l3_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->l4_ofs,
+  b2->inner_l3_ofs, b2->inner_l4_ofs);
 }
 return false;
 }
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index 747e04014..103ff2d0d 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -35,10 +35,10 @@
 
 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 build asserts make sure that the below fields remain in the same
+ * order and offset to l2_pad_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_pad_size offset. */
 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 +51,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) -
@@ -112,7 +120,7 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int 
resize_by_bytes)
 dp_packet_pull(b, -resize_by_bytes);
 }
 
-/* The next step is to update the l2_5_ofs, l3_ofs and l4_ofs fields which
+/* The next step is to update the l2_5_ofs to inner_l4_ofs fields which
  * the scalar implementation does with the  dp_packet_adjust_layer_offset()
  * function. */
 
@@ -122,13 +130,14 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int 
resize_by_bytes)
 /* Set the v_u16_max register to

[ovs-dev] [PATCH v4 2/4] bfd: Set proper offsets and flags in BFD packets.

2024-02-11 Thread Mike Pattrick

Previously the BFD packet creation code did not appropriately set
offsets or flags. This contributed to issues involving encapsulation and
the TSO code.

The transition to using standard functions also means some other
metadata like packet_type are set appropriately.

Fixes: ccc096898c46 ("bfd: Implement Bidirectional Forwarding Detection.")
Signed-off-by: Mike Pattrick 
---
v2: Corrected formatting, and just calculate checksum up front
v3: Extended patch comment
---
 lib/bfd.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lib/bfd.c b/lib/bfd.c
index 9698576d0..9af258917 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -586,7 +586,6 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
 {
 long long int min_tx, min_rx;
 struct udp_header *udp;
-struct eth_header *eth;
 struct ip_header *ip;
 struct msg *msg;
 
@@ -605,15 +604,13 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
  * set. */
 ovs_assert(!(bfd->flags & FLAG_POLL) || !(bfd->flags & FLAG_FINAL));
 
-dp_packet_reserve(p, 2); /* Properly align after the ethernet header. */
-eth = dp_packet_put_uninit(p, sizeof *eth);
-eth->eth_src = eth_addr_is_zero(bfd->local_eth_src)
-? eth_src : bfd->local_eth_src;
-eth->eth_dst = eth_addr_is_zero(bfd->local_eth_dst)
-? eth_addr_bfd : bfd->local_eth_dst;
-eth->eth_type = htons(ETH_TYPE_IP);
+ip = eth_compose(p,
+ eth_addr_is_zero(bfd->local_eth_dst)
+ ? eth_addr_bfd : bfd->local_eth_dst,
+ eth_addr_is_zero(bfd->local_eth_src)
+ ? eth_src : bfd->local_eth_src,
+ ETH_TYPE_IP, sizeof *ip + sizeof *udp + sizeof *msg);
 
-ip = dp_packet_put_zeros(p, sizeof *ip);
 ip->ip_ihl_ver = IP_IHL_VER(5, 4);
 ip->ip_tot_len = htons(sizeof *ip + sizeof *udp + sizeof *msg);
 ip->ip_ttl = MAXTTL;
@@ -621,15 +618,17 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
 ip->ip_proto = IPPROTO_UDP;
 put_16aligned_be32(>ip_src, bfd->ip_src);
 put_16aligned_be32(>ip_dst, bfd->ip_dst);
-/* Checksum has already been zeroed by put_zeros call. */
+/* Checksum has already been zeroed by eth_compose call. */
 ip->ip_csum = csum(ip, sizeof *ip);
+dp_packet_set_l4(p, ip + 1);
 
-udp = dp_packet_put_zeros(p, sizeof *udp);
+udp = dp_packet_l4(p);
 udp->udp_src = htons(bfd->udp_src);
 udp->udp_dst = htons(BFD_DEST_PORT);
 udp->udp_len = htons(sizeof *udp + sizeof *msg);
+/* Checksum already zero from eth_compose. */
 
-msg = dp_packet_put_uninit(p, sizeof *msg);
+msg = (struct msg *)(udp + 1);
 msg->vers_diag = (BFD_VERSION << 5) | bfd->diag;
 msg->flags = (bfd->state & STATE_MASK) | bfd->flags;
 
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 1/4] dp-packet: Validate correct offset for L4 inner size.

2024-02-11 Thread Mike Pattrick

This patch fixes the correctness of dp_packet_inner_l4_size() when
checking for the existence of an inner L4 header. Previously it checked
for the outer L4 header.

This function is currently only used when a packet is already flagged
for tunneling, so an incorrect determination isn't possible as long as
the flags of the packet are correct.

Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.")
Reviewed-by: David Marchand 
Signed-off-by: Mike Pattrick 
---
v2: Corrected patch subject
---
 lib/dp-packet.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index dceb701e8..802d3f385 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -540,7 +540,7 @@ dp_packet_inner_l4(const struct dp_packet *b)
 static inline size_t
 dp_packet_inner_l4_size(const struct dp_packet *b)
 {
-return OVS_LIKELY(b->l4_ofs != UINT16_MAX)
+return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX)
? (const char *) dp_packet_tail(b)
- (const char *) dp_packet_inner_l4(b)
- dp_packet_l2_pad_size(b)
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v3 1/4] tests: Remove hardcoded numbers from comments

2024-02-11 Thread Ales Musil

On Fri, Feb 9, 2024 at 5:51 PM Mark Michelson  wrote:

> Hi Ales,
>
> I have one comment below, but it's small enough it can be fixed during
> merge.
>
> Acked-by: Mark Michelson 
>
> On 2/8/24 13:17, Ales Musil wrote:
> > There were some comments left with hardcoded numbers. Even if it
> > wouldn't break any test table shift/change it would just left the
> > comment outdated.
> >
> > Signed-off-by: Ales Musil 
> > ---
> >   tests/ovn-northd.at | 2 +-
> >   tests/ovn.at| 8 
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 591ad5aad..bb5e1f958 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2150,7 +2150,7 @@ AT_CLEANUP
> >
> >   # This test case tests that when a logical switch has load balancers
> associated
> >   # (with VIPs configured), the below logical flow is added by
> ovn-northd.
> > -# table=1 (ls_out_pre_lb  ), priority=100  , match=(ip),
> action=(reg0[[0]] = 1; next;)
> > +# (ls_out_pre_lb  ), priority=100  , match=(ip), action=(reg0[[0]]
> = 1; next;)
>
> The other changes in this patch are formatted differently than this one.
> If this were modeled after the other changes, it would be
>
> # table=ls_out_pr_lb, priority=100...
>
> Should this comment be formatted the same as the others in this change?
>

We can follow the same "convention" as for the others.


> >   # This test case is added for the BZ -
> >   # https://bugzilla.redhat.com/show_bug.cgi?id=1849162
> >   #
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 2cf335cf4..0af60f893 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -18372,8 +18372,8 @@ AT_CHECK([cat 2.packets], [0], [expout])
> >
> >   # There should be total of 9 flows present with conjunction action and
> 2 flows
> >   # with conj match. Eg.
> > -# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45)
> > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
> > +# table=ls_out_acl_eval, priority=2001,conj_id=2,metadata=0x1
> actions=resubmit(,ls_out_acl_action)
> > +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1
> actions=drop
> >   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(2,2/2)
> >   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,2/2)
> >   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5
> actions=conjunction(2,2/2)
> > @@ -18413,7 +18413,7 @@ AT_CHECK([cat 2.packets], [0], [])
> >   # properly.
> >   # There should be total of 6 flows present with conjunction action and
> 1 flow
> >   # with conj match. Eg.
> > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
> > +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1
> actions=drop
> >   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(4,2/2)
> >   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(4,2/2)
> >   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
> actions=conjunction(4,2/2)
> > @@ -34296,7 +34296,7 @@ in_port_sec=OFTABLE_CHK_IN_PORT_SEC
> >   in_port_sec_nd=OFTABLE_CHK_IN_PORT_SEC_ND
> >   out_port_sec=OFTABLE_CHK_OUT_PORT_SEC
> >
> > -# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 74 and 75
> in hv1 and hv2
> > +# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC,
> OFTABLE_CHK_IN_PORT_SEC_ND and OFTABLE_CHK_OUT_PORT_SEC in hv1 and hv2
> >   > hv1_t${in_port_sec}_flows.expected
> >   > hv1_t${in_port_sec_nd}_flows.expected
> >   > hv1_t${out_port_sec}_flows.expected
>
>
Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 4/4] ofproto-dpif-monitor: Remove unneeded calls to clear packets.

[ovs-dev] [PATCH v4 3/4] dp-packet: Include inner offsets in adjustments and checks.

[ovs-dev] [PATCH v4 2/4] bfd: Set proper offsets and flags in BFD packets.

[ovs-dev] [PATCH v4 1/4] dp-packet: Validate correct offset for L4 inner size.

Re: [ovs-dev] [PATCH ovn v3 1/4] tests: Remove hardcoded numbers from comments

5 matches

Site Navigation

Mail list logo

Footer information