Re: [ovs-dev] [PATCH v3 10/11] ci: Allow make check-dpdk to run the MFEX tests.
On 15 Dec 2023, at 16:45, Simon Horman wrote: > On Tue, Dec 05, 2023 at 04:00:41PM +0100, Eelco Chaudron wrote: >> The testcase change will make sure the 'MFEX Configuration' test >> will run without the need for scapy and it's auto generated tests. >> In addition we exclude the traffic related MFEX tests from running >> on GitHub actions due to limited resources. >> >> Signed-off-by: Eelco Chaudron > > ... > >> diff --git a/python/test_requirements.txt b/python/test_requirements.txt >> index c85ce41ad..5043c71e2 100644 >> --- a/python/test_requirements.txt >> +++ b/python/test_requirements.txt >> @@ -2,4 +2,5 @@ netaddr >> pyftpdlib >> pyparsing >> pytest >> +scapy >> tftpy > > Hi Eelco, > > I'm having a bit of trouble reconciling the change above > with the patch description. Yes, this is for the remaining MFEX tests, which we exclude for Github, but they will not be run on your local system if you use the test_requermenrs file. I will update the commit message to mention this. Something like: Currently, if you use the python/test_requirements.txt file to set up your test environment the MFEX tests will be skipped due to the Scapy package not being included. This is fixed as part of this patch. The test case change will make sure the 'MFEX Configuration' test will run without the need for Scapy and its auto-generated tests. In addition, we exclude the traffic-related MFEX tests from running on GitHub actions due to limited resources. //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 16/16] northd: Add northd change handler for sync_to_sb_lb node.
On Mon, Nov 27, 2023 at 6:39 PM wrote:
>
> From: Numan Siddique
>
> Any changes to northd engine node due to load balancers
> are now handled in 'sync_to_sb_lb' node to sync the changed
> load balancers to SB load balancers.
>
> The logic to sync the SB load balancers is changed a bit and it
> now mimics the SB lflow sync.
>
> Below are the scale testing results done with all the patches applied
> in this series using ovn-heater. The test ran the scenario -
> ocp-500-density-heavy.yml [1].
>
> The resuts are:
>
>
---
> Min (s) Median (s) 90%ile (s)
99%ile (s) Max (s) Mean (s)Total (s) Count
Failed
>
---
> Iteration Total 0.1368831.1290161.192001
1.2041671.2127280.66501783.127099 125 0
> Namespace.add_ports 0.0052160.0057360.007034
0.0154860.0189780.0062110.776373125 0
> WorkerNode.bind_port0.0350300.0460820.052469
0.0582930.0603110.04597311.493259 250 0
> WorkerNode.ping_port0.0050570.0067271.047692
1.0692531.0713360.26689666.724094 250 0
>
---
>
> The results with the present main [2] are:
>
>
---
> Min (s) Median (s) 90%ile (s)
99%ile (s) Max (s) Mean (s)Total (s) Count
Failed
>
---
> Iteration Total 0.1354912.2238053.311270
3.3390783.3453461.729172216.146495 125 0
> Namespace.add_ports 0.0053800.0057440.006819
0.0187730.0208000.0062920.786532125 0
> WorkerNode.bind_port0.0341790.0460550.053488
0.0588010.0710430.04611711.529311 250 0
> WorkerNode.ping_port0.0049560.0069523.086952
3.1917433.1928070.791544197.886026 250 0
>
---
>
> [1] -
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
> [2] - 2a12cda890a7("controller, northd: Wait for cleanup before replying
to exit")
>
> Signed-off-by: Numan Siddique
> ---
> northd/en-sync-sb.c | 445 +--
> northd/inc-proc-northd.c | 1 +
> northd/lflow-mgr.c | 196 ++---
> northd/lflow-mgr.h | 23 +-
> northd/northd.c | 238 -
> northd/northd.h | 6 -
> tests/ovn-northd.at | 103 +
> 7 files changed, 585 insertions(+), 427 deletions(-)
>
> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> index 73b30272c4..62c5dbd20f 100644
> --- a/northd/en-sync-sb.c
> +++ b/northd/en-sync-sb.c
> @@ -30,6 +30,7 @@
> #include "lib/ovn-nb-idl.h"
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> +#include "lflow-mgr.h"
> #include "northd.h"
>
> #include "openvswitch/vlog.h"
> @@ -53,6 +54,38 @@ static void build_port_group_address_set(const struct
nbrec_port_group *,
> struct svec *ipv4_addrs,
> struct svec *ipv6_addrs);
>
> +struct sb_lb_table;
> +struct sb_lb_record;
> +static void sb_lb_table_init(struct sb_lb_table *);
> +static void sb_lb_table_clear(struct sb_lb_table *);
> +static struct sb_lb_record *sb_lb_table_find(struct hmap *sb_lbs,
> + const struct uuid *);
> +static void sb_lb_table_build_and_sync(struct sb_lb_table *,
> +struct ovsdb_idl_txn *ovnsb_txn,
> +const struct sbrec_load_balancer_table *,
> +const struct
sbrec_logical_dp_group_table *,
> +struct hmap *lb_dps_map,
> +struct ovn_datapaths *ls_datapaths,
> +struct ovn_datapaths *lr_datapaths,
> +struct chassis_features *);
> +static void
Re: [ovs-dev] [PATCH v3 2/2] netdev-dummy: Add support and test for tso.
Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: corrupt patch at line 159 error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 netdev-dummy: Add support and test for tso. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 1/2] dp-packet: Set checksum flags during software TSO.
When OVS needs to fallback on the software TSO implementation to segment
a packet, it currently doesn't guarantee that IP and TCP checksum
offload flags are set. However, it is possible that these is required.
This is true in the case of dp_netdev_upcall(), which clears these
flags.
This patch explicitly sets the appropriate flags when the segmentation
flag is removed, to guarantee that packets always end up with correct
checksums.
Signed-off-by: Mike Pattrick
---
v3: Moved logic from ofproto-dpif-upcall to dp-packet
Signed-off-by: Mike Pattrick
---
lib/dp-packet.h | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 11aa00723..f91b5e3fb 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -1131,11 +1131,23 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
*dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
}
-/* Resets TCP Segmentation flag in packet 'p'. */
+/* Resets TCP Segmentation in packet 'p' and adjust flags to indicate
+ * L3 and L4 checksumming is now required. */
static inline void
dp_packet_hwol_reset_tcp_seg(struct dp_packet *p)
{
-*dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_TX_TCP_SEG;
+uint64_t ol_flags = *dp_packet_ol_flags_ptr(p)
+| DP_PACKET_OL_TX_TCP_CKSUM;
+
+ol_flags = ol_flags & ~(DP_PACKET_OL_TX_TCP_SEG
+| DP_PACKET_OL_RX_L4_CKSUM_MASK
+| DP_PACKET_OL_RX_IP_CKSUM_GOOD);
+
+if (ol_flags & DP_PACKET_OL_TX_IPV4) {
+ ol_flags |= DP_PACKET_OL_TX_IP_CKSUM;
+}
+
+*dp_packet_ol_flags_ptr(p) = ol_flags;
}
/* Returns 'true' if the IP header has good integrity and the
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 2/2] netdev-dummy: Add support and test for tso.
Test that netdev-dummy is able to send and receive segment offloaded
packets.
Signed-off-by: Mike Pattrick
---
v2: Fix clang build error: mutex needed to access netdev_dummy members
v3: Remove use of tcpdump, hexdump, and otherwise clean up test
Note, somehow the v3 patch reported as corrupted in patchwork, but it
applied fine for me, trying to resubmit.
---
lib/netdev-dummy.c | 32 +++-
tests/dpif-netdev.at | 43 +++
2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 8c6e6d448..9d9a28892 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -44,6 +44,7 @@
#include "unaligned.h"
#include "timeval.h"
#include "unixctl.h"
+#include "userspace-tso.h"
#include "reconnect.h"
VLOG_DEFINE_THIS_MODULE(netdev_dummy);
@@ -152,6 +153,8 @@ struct netdev_dummy {
bool ol_ip_csum OVS_GUARDED;
/* Flag RX packet with good csum. */
bool ol_ip_csum_set_good OVS_GUARDED;
+/* Set the segment size for netdev TSO support. */
+int ol_tso_segsz OVS_GUARDED;
};
/* Max 'recv_queue_len' in struct netdev_dummy. */
@@ -806,6 +809,10 @@ netdev_dummy_get_config(const struct netdev *dev, struct
smap *args)
smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
}
+if (netdev->ol_tso_segsz && userspace_tso_enabled()) {
+smap_add_format(args, "ol_tso_segsz", "%d", netdev->ol_tso_segsz);
+}
+
/* 'dummy-pmd' specific config. */
if (!netdev_is_pmd(dev)) {
goto exit;
@@ -937,6 +944,14 @@ netdev_dummy_set_config(struct netdev *netdev_, const
struct smap *args,
netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
}
+if (userspace_tso_enabled()) {
+netdev->ol_tso_segsz = smap_get_int(args, "ol_tso_segsz", 0);
+if (netdev->ol_tso_segsz) {
+netdev_->ol_flags |= (NETDEV_TX_OFFLOAD_TCP_TSO
+ | NETDEV_TX_OFFLOAD_TCP_CKSUM);
+}
+}
+
netdev_change_seq_changed(netdev_);
/* 'dummy-pmd' specific config. */
@@ -1119,6 +1134,15 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct
dp_packet_batch *batch,
/* The netdev hardware sets the flag when the packet has good csum. */
dp_packet_ol_set_ip_csum_good(packet);
}
+
+if (userspace_tso_enabled() && netdev->ol_tso_segsz) {
+dp_packet_set_tso_segsz(packet, netdev->ol_tso_segsz);
+dp_packet_hwol_set_tcp_seg(packet);
+dp_packet_hwol_set_tx_ip_csum(packet);
+dp_packet_hwol_set_tx_ipv4(packet);
+dp_packet_hwol_set_csum_tcp(packet);
+}
+
ovs_mutex_unlock(>mutex);
dp_packet_batch_init_packet(batch, packet);
@@ -1174,6 +1198,12 @@ netdev_dummy_send(struct netdev *netdev, int qid,
DP_PACKET_BATCH_FOR_EACH(i, packet, batch) {
const void *buffer = dp_packet_data(packet);
size_t size = dp_packet_size(packet);
+bool is_tso;
+
+ovs_mutex_lock(>mutex);
+is_tso = userspace_tso_enabled() && dev->ol_tso_segsz &&
+ dp_packet_hwol_is_tso(packet);
+ovs_mutex_unlock(>mutex);
if (!dp_packet_is_eth(packet)) {
error = EPFNOSUPPORT;
@@ -1194,7 +1224,7 @@ netdev_dummy_send(struct netdev *netdev, int qid,
if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
max_size += VLAN_HEADER_LEN;
}
-if (size > max_size) {
+if (size > max_size && !is_tso) {
error = EMSGSIZE;
break;
}
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index d0359b5ea..dc6e209b1 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -810,6 +810,49 @@ AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0],
[${good_expected}
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([userspace offload - tso])
+OVS_VSWITCHD_START(
+ [set Open_vSwitch . other_config:userspace-tso-enable=true -- \
+ add-br br1 -- set bridge br1 datapath-type=dummy -- \
+ add-port br1 p1 -- \
+ set Interface p1 type=dummy -- \
+ add-port br1 p2 -- \
+ set Interface p2 type=dummy])
+
+dnl Simple passthrough rule.
+AT_CHECK([ovs-ofctl add-flow br1 in_port=p1,actions=output:p2])
+
+flow_s="in_port(1),eth(src=8a:bf:7e:2f:05:84,dst=0a:8f:39:4f:e0:73),eth_type(0x0800),
\
+
ipv4(src=192.168.123.2,dst=192.168.123.1,proto=6,tos=1,ttl=64,frag=no), \
+tcp(src=54392,dst=5201),tcp_flags(ack)"
+
+dnl Send from tso to no-tso.
+AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap -- \
+set Interface p1 options:ol_ip_csum=true -- \
+set Interface p1 options:ol_ip_csum_set_good=false -- \
+set Interface p1 options:ol_tso_segsz=500])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 "${flow_s}" --len 2054])
+
+dnl Send from tso to tso.
+AT_CHECK([ovs-vsctl set Interface p2 options:ol_ip_csum=true -- \
+
Re: [ovs-dev] [PATCH v3 2/2] netdev-dummy: Add support and test for tso.
These patches apply cleanly on the current master branch for me. I will try to resubmit. -M On Mon, Dec 18, 2023 at 4:21 PM 0-day Robot wrote: > > Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > error: corrupt patch at line 157 > error: could not build fake ancestor > hint: Use 'git am --show-current-patch=diff' to see the failed patch > Patch failed at 0001 netdev-dummy: Add support and test for tso. > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > > Patch skipped due to previous failure. > > Please check this out. If you feel there has been an error, please email > acon...@redhat.com > > Thanks, > 0-day Robot > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 15/16] northd: Add a noop handler for northd SB mac binding.
On Mon, Dec 18, 2023 at 12:22 PM Numan Siddique wrote: > > On Wed, Dec 13, 2023 at 9:57 AM Dumitru Ceara wrote: > > > > On 11/28/23 03:38, num...@ovn.org wrote: > > > From: Numan Siddique > > > > > > Signed-off-by: Numan Siddique > > > --- > > > northd/inc-proc-northd.c | 9 - > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > > index 7b1c6597e2..28f397ff39 100644 > > > --- a/northd/inc-proc-northd.c > > > +++ b/northd/inc-proc-northd.c > > > @@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > engine_add_input(_northd, _sb_mirror, NULL); > > > engine_add_input(_northd, _sb_meter, NULL); > > > engine_add_input(_northd, _sb_datapath_binding, NULL); > > > -engine_add_input(_northd, _sb_mac_binding, NULL); > > > engine_add_input(_northd, _sb_dns, NULL); > > > engine_add_input(_northd, _sb_ha_chassis_group, NULL); > > > engine_add_input(_northd, _sb_ip_multicast, NULL); > > > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > engine_add_input(_northd, _global_config, > > > northd_global_config_handler); > > > > > > +/* northd engine node uses the sb mac binding table to > > > + * cleanup mac binding entries for deleted logical ports > > > + * and datapaths. Any update to to SB mac binding doesn't > > > + * change the northd engine node state or data. Hence > > > + * it is ok to add a noop_handler here. */ > > > +engine_add_input(_northd, _sb_mac_binding, > > > + engine_noop_handler); > > > + > > > > Isn't this just a case of "ovn-northd" is not really interested in > > change tracking for SB.MAC_Binding? Can't we instead just disable > > alerting, ovsdb_idl_omit_alert(..), for all SBREC_MAC_BINDING columns > > like we do for other SB tables (lflow, multicast_group, meter, > > portt_group, logical_dp_group)? > > I thought about that. But mac_binding_ageing engine node also depends > on SB mac_binding and it results in full recompute (no handler for it). > This is a reasonable consideration. In this case I would put such consideration in the comment that explains why using the noop handler. > If @Ales Musil can confirm that the mac_binding_ageing node doesn't need > to handle SB mac_binding changes, then I'm fine with your suggestion. > I tend to believe that mac_binding_aging node doesn't really need to handle mac_binding changes. The aging node processing should be triggered solely by the aging timer. @Ales Musil may help confirm. Thanks, Han > Thanks > Numan > > > > > > engine_add_input(_northd, _sb_port_binding, > > > northd_sb_port_binding_handler); > > > engine_add_input(_northd, _nb_logical_switch, > > > > Regards, > > Dumitru > > > > ___ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 14/16] northd: Add I-P for NB_Global and SB_Global.
On Wed, Dec 13, 2023 at 6:47 AM Dumitru Ceara wrote: > > On 11/28/23 03:37, num...@ovn.org wrote: > > From: Numan Siddique > > > > A new engine node "global_config" is added which handles the changes > > to NB_Global an SB_Global tables. It also creates these rows if > > not present. > > > > Without the I-P, any changes to the options column of these tables > > result in recompute of 'northd' and 'lflow' engine nodes. This is not true. Common updates to NB_Global and SB_Global, such as nb_cfg and timestamps changes, or external_ids changes populated by ovn-k8s, will not trigger recompute. Could you be more specific what recompute are avoided by this patch? I can see for example, IPSec option change is handled with I-P, but these are really rare changes. It seems most other option changes and chassis feature changes would still trigger recompute with this patch. > > > > Signed-off-by: Numan Siddique > > --- > > northd/aging.c| 21 +- > > northd/automake.mk| 2 + > > northd/en-global-config.c | 588 ++ > > northd/en-global-config.h | 65 + > > northd/en-lflow.c | 11 +- > > northd/en-northd.c| 52 ++-- > > northd/en-northd.h| 2 +- > > northd/en-sync-sb.c | 22 +- > > northd/inc-proc-northd.c | 38 ++- > > northd/northd.c | 230 +++ > > northd/northd.h | 24 +- > > tests/ovn-northd.at | 256 +++-- > > 12 files changed, 1014 insertions(+), 297 deletions(-) > > I think most of the options values we interpret in the the NB_Global and > SB_Global tables don't usually change (or don't change often). > > Doesn't it make sense to not have "proper I-P" for these tables and > instead enhance northd_nb_nb_global_handler() to: > > - if one of the well known NB_Global/SB_Global options change trigger a > recompute of the northd node > - if one of the other options change then do a plain NB -> SB options sync > > I hope that can be done in way less than "1014 insertions(+), 297 > deletions(-)". > > What do you think? > +1. And I am even questioning "well known NB_Global/SB_Global options change". Are there really such changes that need to be done frequently in production? Besides, I have a comment to the function check_nb_options_out_of_sync(): why not simply check all options by comparing two SMAPs? Otherwise it would be easy to miss the check for a newly added option. The function name also looks like it checks all options instead of some selected ones. And the criteria for the selection is not clear. Thanks, Han > Thanks, > Dumitru > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 12/16] northd: Add lr_stateful handler for lflow engine node.
On Mon, Nov 27, 2023 at 6:38 PM wrote:
>
> From: Numan Siddique
>
> Signed-off-by: Numan Siddique
> ---
> northd/en-lflow.c| 29 +++
> northd/en-lflow.h| 1 +
> northd/en-lr-stateful.c | 39 -
> northd/en-lr-stateful.h | 26 +++
> northd/inc-proc-northd.c | 3 +-
> northd/northd.c | 370 ++-
> northd/northd.h | 10 ++
> tests/ovn-northd.at | 48 ++---
> 8 files changed, 336 insertions(+), 190 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 13284b5556..09748f570b 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -164,6 +164,35 @@ lflow_port_group_handler(struct engine_node *node,
void *data OVS_UNUSED)
> return true;
> }
>
> +bool
> +lflow_lr_stateful_handler(struct engine_node *node, void *data)
> +{
> +struct ed_type_lr_stateful *lr_sful_data =
> +engine_get_input_data("lr_stateful", node);
> +
> +if (!lr_stateful_has_tracked_data(_sful_data->trk_data)
> +|| lr_sful_data->trk_data.vip_nats_changed) {
> +return false;
> +}
> +
> +const struct engine_context *eng_ctx = engine_get_context();
> +struct lflow_data *lflow_data = data;
> +
> +struct lflow_input lflow_input;
> +lflow_get_input_data(node, _input);
> +
> +if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn,
> + _sful_data->trk_data,
> + _input,
> + lflow_data->lflow_table)) {
> +return false;
> +}
> +
> +engine_set_node_state(node, EN_UPDATED);
> +
> +return true;
> +}
> +
> void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index f7325c56b1..1d813a2a29 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -20,5 +20,6 @@ void *en_lflow_init(struct engine_node *node, struct
engine_arg *arg);
> void en_lflow_cleanup(void *data);
> bool lflow_northd_handler(struct engine_node *, void *data);
> bool lflow_port_group_handler(struct engine_node *, void *data);
> +bool lflow_lr_stateful_handler(struct engine_node *, void *data);
>
> #endif /* EN_LFLOW_H */
> diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> index a54749ad93..8e025f057e 100644
> --- a/northd/en-lr-stateful.c
> +++ b/northd/en-lr-stateful.c
> @@ -39,6 +39,7 @@
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> #include "lib/stopwatch-names.h"
> +#include "lflow-mgr.h"
> #include "northd.h"
>
> VLOG_DEFINE_THIS_MODULE(en_lr_stateful);
> @@ -81,7 +82,7 @@ static void remove_lrouter_lb_reachable_ips(struct
lr_stateful_record *,
> enum
lb_neighbor_responder_mode,
> const struct sset *lb_ips_v4,
> const struct sset
*lb_ips_v6);
> -static void lr_stateful_build_vip_nats(struct lr_stateful_record *);
> +static bool lr_stateful_build_vip_nats(struct lr_stateful_record *);
>
> /* 'lr_stateful' engine node manages the NB logical router LB data.
> */
> @@ -110,6 +111,7 @@ en_lr_stateful_clear_tracked_data(void *data_)
> struct ed_type_lr_stateful *data = (struct ed_type_lr_stateful *)
data_;
>
> hmapx_clear(>trk_data.crupdated);
> +data->trk_data.vip_nats_changed = false;
> }
>
> void
> @@ -190,6 +192,10 @@ lr_stateful_lb_data_handler(struct engine_node
*node, void *data_)
>
> /* Add the lr_sful_rec rec to the tracking data. */
> hmapx_add(>trk_data.crupdated, lr_sful_rec);
> +
> +if (!sset_is_empty(_sful_rec->vip_nats)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> continue;
> }
>
> @@ -298,7 +304,9 @@ lr_stateful_lb_data_handler(struct engine_node *node,
void *data_)
> * vip nats. */
> HMAPX_FOR_EACH (hmapx_node, >trk_data.crupdated) {
> lr_sful_rec = hmapx_node->data;
> -lr_stateful_build_vip_nats(lr_sful_rec);
> +if (lr_stateful_build_vip_nats(lr_sful_rec)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> lr_sful_rec->has_lb_vip = od_has_lb_vip(lr_sful_rec->od);
> }
>
> @@ -335,8 +343,13 @@ lr_stateful_lr_nat_handler(struct engine_node *node,
void *data_)
> lrnat_rec,
> input_data.lb_datapaths_map,
>
input_data.lbgrp_datapaths_map);
> +if (!sset_is_empty(_sful_rec->vip_nats)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> } else {
> -lr_stateful_build_vip_nats(lr_sful_rec);
> +if (lr_stateful_build_vip_nats(lr_sful_rec)) {
> +
Re: [ovs-dev] [PATCH v2] python: idl: Handle monitor_canceled
Bleep bloop. Greetings Terry Wilson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: The subject summary should end with a dot. Subject: python: idl: Handle monitor_canceled Lines checked: 76, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] python: idl: Handle monitor_canceled
Currently python-ovs claims to be "db change aware" but does not
parse the "monitor_canceled" notification. Transactions can continue
being made, but the monitor updates will not be sent. This handles
monitor_cancel similarly to how ovsdb-cs currently does.
Signed-off-by: Terry Wilson
---
python/ovs/db/idl.py | 32
1 file changed, 32 insertions(+)
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 9fc2159b0..be6ae2ca4 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -299,6 +299,7 @@ class Idl(object):
self._server_schema_request_id = None
self._server_monitor_request_id = None
self._db_change_aware_request_id = None
+self._monitor_cancel_request_id = None
self._server_db_name = '_Server'
self._server_db_table = 'Database'
self.server_tables = None
@@ -481,6 +482,10 @@ class Idl(object):
break
else:
self.__parse_update(msg.params[1], OVSDB_UPDATE)
+elif self.handle_monitor_canceled(msg):
+break
+elif self.handle_monitor_cancel_reply(msg):
+break
elif (msg.type == ovs.jsonrpc.Message.T_REPLY
and self._monitor_request_id is not None
and self._monitor_request_id == msg.id):
@@ -615,6 +620,33 @@ class Idl(object):
return initial_change_seqno != self.change_seqno
+def handle_monitor_canceled(self, msg):
+if msg.type != msg.T_NOTIFY:
+return False
+if msg.method != "monitor_canceled":
+return False
+
+if msg.params[0] == str(self.uuid):
+params = [str(self.server_monitor_uuid)]
+elif msg.params[0] == str(self.server_monitor_uuid):
+params = [str(self.uuid)]
+else:
+return False
+
+mc_msg = ovs.jsonrpc.Message.create_request("monitor_cancel", params)
+self._monitor_cancel_request_id = mc_msg.id
+self.send_request(mc_msg)
+self.restart_fsm()
+return True
+
+def handle_monitor_cancel_reply(self, msg):
+if msg.type != msg.T_REPLY:
+return False
+if msg.id != self._monitor_cancel_request_id:
+return False
+self._monitor_cancel_request_id = None
+return True
+
def compose_cond_change(self):
if not self.cond_changed:
return
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/2] netdev-dummy: Add support and test for tso.
Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: corrupt patch at line 157 error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 netdev-dummy: Add support and test for tso. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 2/2] netdev-dummy: Add support and test for tso.
Test that netdev-dummy is able to send and receive segment offloaded
packets.
Signed-off-by: Mike Pattrick
---
v2: Fix clang build error: mutex needed to access netdev_dummy members
v3: Remove use of tcpdump, hexdump, and otherwise clean up test
---
lib/netdev-dummy.c | 32 +++-
tests/dpif-netdev.at | 43 +++
2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 8c6e6d448..9d9a28892 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -44,6 +44,7 @@
#include "unaligned.h"
#include "timeval.h"
#include "unixctl.h"
+#include "userspace-tso.h"
#include "reconnect.h"
VLOG_DEFINE_THIS_MODULE(netdev_dummy);
@@ -152,6 +153,8 @@ struct netdev_dummy {
bool ol_ip_csum OVS_GUARDED;
/* Flag RX packet with good csum. */
bool ol_ip_csum_set_good OVS_GUARDED;
+/* Set the segment size for netdev TSO support. */
+int ol_tso_segsz OVS_GUARDED;
};
/* Max 'recv_queue_len' in struct netdev_dummy. */
@@ -806,6 +809,10 @@ netdev_dummy_get_config(const struct netdev *dev, struct
smap *args)
smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
}
+if (netdev->ol_tso_segsz && userspace_tso_enabled()) {
+smap_add_format(args, "ol_tso_segsz", "%d", netdev->ol_tso_segsz);
+}
+
/* 'dummy-pmd' specific config. */
if (!netdev_is_pmd(dev)) {
goto exit;
@@ -937,6 +944,14 @@ netdev_dummy_set_config(struct netdev *netdev_, const
struct smap *args,
netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
}
+if (userspace_tso_enabled()) {
+netdev->ol_tso_segsz = smap_get_int(args, "ol_tso_segsz", 0);
+if (netdev->ol_tso_segsz) {
+netdev_->ol_flags |= (NETDEV_TX_OFFLOAD_TCP_TSO
+ | NETDEV_TX_OFFLOAD_TCP_CKSUM);
+}
+}
+
netdev_change_seq_changed(netdev_);
/* 'dummy-pmd' specific config. */
@@ -1119,6 +1134,15 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct
dp_packet_batch *batch,
/* The netdev hardware sets the flag when the packet has good csum. */
dp_packet_ol_set_ip_csum_good(packet);
}
+
+if (userspace_tso_enabled() && netdev->ol_tso_segsz) {
+dp_packet_set_tso_segsz(packet, netdev->ol_tso_segsz);
+dp_packet_hwol_set_tcp_seg(packet);
+dp_packet_hwol_set_tx_ip_csum(packet);
+dp_packet_hwol_set_tx_ipv4(packet);
+dp_packet_hwol_set_csum_tcp(packet);
+}
+
ovs_mutex_unlock(>mutex);
dp_packet_batch_init_packet(batch, packet);
@@ -1174,6 +1198,12 @@ netdev_dummy_send(struct netdev *netdev, int qid,
DP_PACKET_BATCH_FOR_EACH(i, packet, batch) {
const void *buffer = dp_packet_data(packet);
size_t size = dp_packet_size(packet);
+bool is_tso;
+
+ovs_mutex_lock(>mutex);
+is_tso = userspace_tso_enabled() && dev->ol_tso_segsz &&
+ dp_packet_hwol_is_tso(packet);
+ovs_mutex_unlock(>mutex);
if (!dp_packet_is_eth(packet)) {
error = EPFNOSUPPORT;
@@ -1194,7 +1224,7 @@ netdev_dummy_send(struct netdev *netdev, int qid,
if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
max_size += VLAN_HEADER_LEN;
}
-if (size > max_size) {
+if (size > max_size && !is_tso) {
error = EMSGSIZE;
break;
}
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index d0359b5ea..dc6e209b1 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -810,6 +810,49 @@ AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0],
[${good_expected}
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([userspace offload - tso])
+OVS_VSWITCHD_START(
+ [set Open_vSwitch . other_config:userspace-tso-enable=true -- \
+ add-br br1 -- set bridge br1 datapath-type=dummy -- \
+ add-port br1 p1 -- \
+ set Interface p1 type=dummy -- \
+ add-port br1 p2 -- \
+ set Interface p2 type=dummy])
+
+dnl Simple passthrough rule.
+AT_CHECK([ovs-ofctl add-flow br1 in_port=p1,actions=output:p2])
+
+flow_s="in_port(1),eth(src=8a:bf:7e:2f:05:84,dst=0a:8f:39:4f:e0:73),eth_type(0x0800),
\
+
ipv4(src=192.168.123.2,dst=192.168.123.1,proto=6,tos=1,ttl=64,frag=no), \
+tcp(src=54392,dst=5201),tcp_flags(ack)"
+
+dnl Send from tso to no-tso.
+AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap -- \
+set Interface p1 options:ol_ip_csum=true -- \
+set Interface p1 options:ol_ip_csum_set_good=false -- \
+set Interface p1 options:ol_tso_segsz=500])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 "${flow_s}" --len 2054])
+
+dnl Send from tso to tso.
+AT_CHECK([ovs-vsctl set Interface p2 options:ol_ip_csum=true -- \
+set Interface p2 options:ol_ip_csum_set_good=false -- \
+set Interface p2
[ovs-dev] [PATCH v3 1/2] dp-packet: Set checksum flags during software TSO.
When OVS needs to fallback on the software TSO implementation to segment
a packet, it currently doesn't guarantee that IP and TCP checksum
offload flags are set. However, it is possible that these is required.
This is true in the case of dp_netdev_upcall(), which clears these
flags.
This patch explicitly sets the appropriate flags when the segmentation
flag is removed, to guarantee that packets always end up with correct
checksums.
Signed-off-by: Mike Pattrick
---
v3: Moved logic from ofproto-dpif-upcall to dp-packet
---
lib/dp-packet.h | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 11aa00723..f91b5e3fb 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -1131,11 +1131,23 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
*dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
}
-/* Resets TCP Segmentation flag in packet 'p'. */
+/* Resets TCP Segmentation in packet 'p' and adjust flags to indicate
+ * L3 and L4 checksumming is now required. */
static inline void
dp_packet_hwol_reset_tcp_seg(struct dp_packet *p)
{
-*dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_TX_TCP_SEG;
+uint64_t ol_flags = *dp_packet_ol_flags_ptr(p)
+| DP_PACKET_OL_TX_TCP_CKSUM;
+
+ol_flags = ol_flags & ~(DP_PACKET_OL_TX_TCP_SEG
+| DP_PACKET_OL_RX_L4_CKSUM_MASK
+| DP_PACKET_OL_RX_IP_CKSUM_GOOD);
+
+if (ol_flags & DP_PACKET_OL_TX_IPV4) {
+ ol_flags |= DP_PACKET_OL_TX_IP_CKSUM;
+}
+
+*dp_packet_ol_flags_ptr(p) = ol_flags;
}
/* Returns 'true' if the IP header has good integrity and the
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 15/16] northd: Add a noop handler for northd SB mac binding.
On Wed, Dec 13, 2023 at 9:57 AM Dumitru Ceara wrote: > > On 11/28/23 03:38, num...@ovn.org wrote: > > From: Numan Siddique > > > > Signed-off-by: Numan Siddique > > --- > > northd/inc-proc-northd.c | 9 - > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 7b1c6597e2..28f397ff39 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(_northd, _sb_mirror, NULL); > > engine_add_input(_northd, _sb_meter, NULL); > > engine_add_input(_northd, _sb_datapath_binding, NULL); > > -engine_add_input(_northd, _sb_mac_binding, NULL); > > engine_add_input(_northd, _sb_dns, NULL); > > engine_add_input(_northd, _sb_ha_chassis_group, NULL); > > engine_add_input(_northd, _sb_ip_multicast, NULL); > > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(_northd, _global_config, > > northd_global_config_handler); > > > > +/* northd engine node uses the sb mac binding table to > > + * cleanup mac binding entries for deleted logical ports > > + * and datapaths. Any update to to SB mac binding doesn't > > + * change the northd engine node state or data. Hence > > + * it is ok to add a noop_handler here. */ > > +engine_add_input(_northd, _sb_mac_binding, > > + engine_noop_handler); > > + > > Isn't this just a case of "ovn-northd" is not really interested in > change tracking for SB.MAC_Binding? Can't we instead just disable > alerting, ovsdb_idl_omit_alert(..), for all SBREC_MAC_BINDING columns > like we do for other SB tables (lflow, multicast_group, meter, > portt_group, logical_dp_group)? I thought about that. But mac_binding_ageing engine node also depends on SB mac_binding and it results in full recompute (no handler for it). If @Ales Musil can confirm that the mac_binding_ageing node doesn't need to handle SB mac_binding changes, then I'm fine with your suggestion. Thanks Numan > > > engine_add_input(_northd, _sb_port_binding, > > northd_sb_port_binding_handler); > > engine_add_input(_northd, _nb_logical_switch, > > Regards, > Dumitru > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Revert "ovn: add geneve PMTUD support"
On Mon, Dec 18, 2023 at 4:54 AM Dumitru Ceara wrote:
>
> On 12/18/23 10:52, Dumitru Ceara wrote:
> > On 12/16/23 04:48, num...@ovn.org wrote:
> >> From: Numan Siddique
> >>
> >> This reverts commit 450e41e783bfa69e4f9d6c80f6bcb01147d5cfe1.
> >>
> >> If a packet has to be tunnelled to another node and if the physical
> >> interface used for tunnelling has lower MTU than the packet or
> >> if there is a route exception with a lower MTU, then the geneve
> >> kernel module generates an ICMP need frag packet. This packet
> >> was getting dropped since the metadata had to be swapped.
> >> The commit [1] did exactly that and fixed the issue.
> >> But it has 2 issues -
> >> 1. It introduced a regression for the scenario when an ICMP need frag
> >> packet generated outside of OVN has to be tunnelled and delivered
> >> to the destination VM/pod. These ICMP need frag packets are now
> >> dropped.
> >> 2. If the logical switches has ACLs or load balancers configured then
> >> these icmp need frag packets are dropped as they are not sent to
> >> the correct zone.
> >>
> >> Its better to revert until we find a proper solution for the original
> >> issue.
> >
> > I agree, it sounds good to me.
> >
> >>
> >> [1] - 450e41e783bf("ovn: add geneve PMTUD support")
> >
> > This should actually be:
> >
> > Fixes: 450e41e783bf ("ovn: add geneve PMTUD support")
> >
> > With that fixed up:
> > Acked-by: Dumitru Ceara
> >
>
> I forgot, sorry. To simplify our downstream process it would be great
> if you could add:
>
> Reported-at: https://issues.redhat.com/browse/FDP-216
>
> Thanks again!
Thanks. I applied the patch to main by making the suggested changes
and backported to branch-23.09.
Running the branch-23.06 in CI before applying there.
Numan
>
> >>
> >> Signed-off-by: Numan Siddique
> >> ---
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 02/11] ci: Add make check-ovsdb-cluster tests to GitHub action ci.
On Mon, Dec 18, 2023 at 11:59:23AM +0100, Eelco Chaudron wrote:
>
>
> On 15 Dec 2023, at 16:23, Simon Horman wrote:
>
> > On Tue, Dec 05, 2023 at 03:59:31PM +0100, Eelco Chaudron wrote:
> >> This patch adds 'make check-ovsdb-cluster' tests to GitHub action ci.
> >> In addition, this patch also makes sure this test and 'make check' do
> >> not run as root.
> >>
> >> Signed-off-by: Eelco Chaudron
> >> ---
> >> .ci/linux-build.sh |5 -
> >> .github/workflows/build-and-test.yml |3 +++
> >> 2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> >> index 67c01a644..bb540703e 100755
> >> --- a/.ci/linux-build.sh
> >> +++ b/.ci/linux-build.sh
> >> @@ -129,11 +129,14 @@ else
> >> build_ovs
> >> for testsuite in $TESTSUITE; do
> >> run_as_root=
> >> +if [ "$testsuite" != "check" ] && \
> >> + [ "$testsuite" != "check-ovsdb-cluster" ] ; then
> >> +run_as_root="sudo -E PATH=$PATH"
> >> +fi
> >> if [ "${testsuite##*dpdk}" != "$testsuite" ]; then
> >> sudo sh -c 'echo 1024 > /proc/sys/vm/nr_hugepages' || true
> >> [ "$(cat /proc/sys/vm/nr_hugepages)" = '1024' ]
> >> export DPDK_EAL_OPTIONS="--lcores 0@1,1@1,2@1"
> >> -run_as_root="sudo -E PATH=$PATH"
> >> fi
> >> $run_as_root make $testsuite TESTSUITEFLAGS=${JOBS} RECHECK=yes
> >> done
> >
> > Hi Eelco,
> >
> > perhaps it is because it is Friday afternoon (although I did just have
> > a coffee), but I am a but confused by the change above.
> >
> > My reading is that, before this change:
> >
> >run_as_root is set if the $testsuite includes the string "dpdk"
> >
> > While after the change:
> >
> >run_as_root is set if $testsuite is neither "check"
> >nor check-ovsdb-cluster".
> >
> > In both cases:
> >
> >* Anything with "dpdk" results in run_as_root set; and
> >* "check" and "check-ovsdb-cluster" do not result in run_as_root being
> > set
>
> So the TESTSUITE value passed before this patchset is as a string “check
> check-dpdk” so it will loop through testsuite=check, and
> testsuite=check-dpdk. With the change in this patch the “check”, and
> “check-ovsdb-cluster” will not run as root, all others will.
>
> > Further, it seems to me that the other values of TESTSUITE are:
> > * "": which means the loop won't iterate
> > * "test": which is special cased and also means the loop won't iterated
>
> I guessed you missed ‘check check-dpdk’ and the new ‘check-ovsdb-cluster’
> below?
Yes, I missed that.
>
> > So I am a little puzzled regarding the motivation for this change.
>
> Your reply also puzzles me, but it’s Monday, and have not yet had my coffee :)
I think see my error now.
I still think that this patch does not change cases which exist
as of this patch. But I now see that it does change cases
which are added in subsequent patches in this series, f.e. check-kernel.
So I am now good with this change.
And I am sorry for the puzzling statements.
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/2] netdev-dummy: Add support and test for tso.
On Fri, Dec 15, 2023 at 8:05 PM Ilya Maximets wrote:
>
> On 12/11/23 16:39, Mike Pattrick wrote:
> > Test that netdev-dummy is able to send and recieve segment offloaded
> > packets.
> >
> > Signed-off-by: Mike Pattrick
> >
> > ---
> > v2: Fix clang build error: mutex needed to access netdev_dummy members
> >
> > Signed-off-by: Mike Pattrick
> > ---
> > lib/netdev-dummy.c | 32 +++-
> > tests/dpif-netdev.at | 50
> > 2 files changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> > index 8c6e6d448..9d9a28892 100644
> > --- a/lib/netdev-dummy.c
> > +++ b/lib/netdev-dummy.c
> > @@ -44,6 +44,7 @@
> > #include "unaligned.h"
> > #include "timeval.h"
> > #include "unixctl.h"
> > +#include "userspace-tso.h"
> > #include "reconnect.h"
> >
> > VLOG_DEFINE_THIS_MODULE(netdev_dummy);
> > @@ -152,6 +153,8 @@ struct netdev_dummy {
> > bool ol_ip_csum OVS_GUARDED;
> > /* Flag RX packet with good csum. */
> > bool ol_ip_csum_set_good OVS_GUARDED;
> > +/* Set the segment size for netdev TSO support. */
> > +int ol_tso_segsz OVS_GUARDED;
> > };
> >
> > /* Max 'recv_queue_len' in struct netdev_dummy. */
> > @@ -806,6 +809,10 @@ netdev_dummy_get_config(const struct netdev *dev,
> > struct smap *args)
> > smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
> > }
> >
> > +if (netdev->ol_tso_segsz && userspace_tso_enabled()) {
> > +smap_add_format(args, "ol_tso_segsz", "%d", netdev->ol_tso_segsz);
> > +}
> > +
> > /* 'dummy-pmd' specific config. */
> > if (!netdev_is_pmd(dev)) {
> > goto exit;
> > @@ -937,6 +944,14 @@ netdev_dummy_set_config(struct netdev *netdev_, const
> > struct smap *args,
> > netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
> > }
> >
> > +if (userspace_tso_enabled()) {
> > +netdev->ol_tso_segsz = smap_get_int(args, "ol_tso_segsz", 0);
> > +if (netdev->ol_tso_segsz) {
> > +netdev_->ol_flags |= (NETDEV_TX_OFFLOAD_TCP_TSO
> > + | NETDEV_TX_OFFLOAD_TCP_CKSUM);
> > +}
> > +}
> > +
> > netdev_change_seq_changed(netdev_);
> >
> > /* 'dummy-pmd' specific config. */
> > @@ -1119,6 +1134,15 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_,
> > struct dp_packet_batch *batch,
> > /* The netdev hardware sets the flag when the packet has good
> > csum. */
> > dp_packet_ol_set_ip_csum_good(packet);
> > }
> > +
> > +if (userspace_tso_enabled() && netdev->ol_tso_segsz) {
> > +dp_packet_set_tso_segsz(packet, netdev->ol_tso_segsz);
> > +dp_packet_hwol_set_tcp_seg(packet);
> > +dp_packet_hwol_set_tx_ip_csum(packet);
> > +dp_packet_hwol_set_tx_ipv4(packet);
> > +dp_packet_hwol_set_csum_tcp(packet);
> > +}
> > +
> > ovs_mutex_unlock(>mutex);
> >
> > dp_packet_batch_init_packet(batch, packet);
> > @@ -1174,6 +1198,12 @@ netdev_dummy_send(struct netdev *netdev, int qid,
> > DP_PACKET_BATCH_FOR_EACH(i, packet, batch) {
> > const void *buffer = dp_packet_data(packet);
> > size_t size = dp_packet_size(packet);
> > +bool is_tso;
> > +
> > +ovs_mutex_lock(>mutex);
> > +is_tso = userspace_tso_enabled() && dev->ol_tso_segsz &&
> > + dp_packet_hwol_is_tso(packet);
> > +ovs_mutex_unlock(>mutex);
> >
> > if (!dp_packet_is_eth(packet)) {
> > error = EPFNOSUPPORT;
> > @@ -1194,7 +1224,7 @@ netdev_dummy_send(struct netdev *netdev, int qid,
> > if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
> > max_size += VLAN_HEADER_LEN;
> > }
> > -if (size > max_size) {
> > +if (size > max_size && !is_tso) {
> > error = EMSGSIZE;
> > break;
> > }
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index d0359b5ea..2d9b8eb5f 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -810,6 +810,56 @@ AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0],
> > [${good_expected}
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([userspace offload - tso])
>
> Hi, Mike. I didn't review the code, but see a few comments
> for the test below.
>
> > +OVS_VSWITCHD_START(
> > + [set Open_vSwitch . other_config:userspace-tso-enable=true -- \
> > + add-br br1 -- set bridge br1 datapath-type=dummy -- \
> > + add-port br1 p1 -- \
> > + set Interface p1 type=dummy -- \
> > + add-port br1 p2 -- \
> > + set Interface p2 type=dummy --])
> > +
> > +dnl Modify the ip_dst addr to force changing the IP csum.
>
> This comment doesn't match the flow.
>
> > +AT_CHECK([ovs-ofctl add-flow br1 in_port=p1,actions=output:p2])
> > +
> > +flow_s="\
> > + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800,\
> > +
> >
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
On 12/18/23 16:17, Naveen Yerramneni wrote:
>
>
>> On 18-Dec-2023, at 7:26 PM, Dumitru Ceara wrote:
>>
>> On 11/30/23 16:32, Dumitru Ceara wrote:
>>> On 11/30/23 15:54, Naveen Yerramneni wrote:
> On 30-Nov-2023, at 6:06 PM, Dumitru Ceara wrote:
>
> On 11/30/23 09:45, Naveen Yerramneni wrote:
>>
>>
>>> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara wrote:
>>>
>>> On 11/29/23 07:45, naveen.yerramneni wrote:
This functionality can be enabled at the logical switch level:
- "other_config:fdb_local" can be used to enable/disable this
functionality, it is disabled by default.
- "other_config:fdb_local_idle_timeout" sepcifies idle timeout
for locally learned fdb flows, default timeout is 300 secs.
If enabled, below lflow is added for each port that has unknown addr
set.
- table=2 (ls_in_lookup_fdb), priority=100, match=(inport ==
),
action=(commit_fdb_local(timeout=); next;
New OVN action: "commit_fdb_local". This sets following OVS action.
-
learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
This is useful when OVN is managing VLAN network that has multiple
ports
set with unknown addr and localnet_learn_fdb is enabled. With this
config,
if there is east-west traffic flowing between VMs part of same VLAN
deployed on different hypervisors then, MAC addrs of the source and
destination VMs keeps flapping between VM port and localnet port in
Southbound FDB table. Enabling fdb_local config makes fdb table local
to
the chassis and avoids MAC flapping.
Signed-off-by: Naveen Yerramneni
---
>>>
>>> Hi Naveen,
>>>
>>> Thanks a lot for the patch!
>>>
>>> Just a note, we already have a fix for the east-west traffic that causes
>>> FDB flapping when localnet is used:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>>>
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>>>
>>>
>>> In general, however, I think it's a very good idea to move the FDB away
>>> from the Southbound and make it local to each hypervisor. That reduces
>>> load on the Southbound among other things.
>>>
>>
>> Hi Dumitru,
>>
>> Thanks for informing about the patches.
>> Yes, local FDB reduces load on southbound.
>>
>>
include/ovn/actions.h | 7 +++
lib/actions.c | 94
northd/northd.c | 26 ++
ovn-nb.xml| 14 ++
tests/ovn.at | 108 ++
utilities/ovn-trace.c | 2 +
6 files changed, 251 insertions(+)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 49cfe0624..85ac92cd3 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -127,6 +127,7 @@ struct collector_set_ids;
OVNACT(CHK_LB_AFF,ovnact_result) \
OVNACT(SAMPLE,ovnact_sample) \
OVNACT(MAC_CACHE_USE, ovnact_null)\
+OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
/* enum ovnact_type, with a member OVNACT_ for each action. */
enum OVS_PACKED_ENUM ovnact_type {
@@ -514,6 +515,12 @@ struct ovnact_commit_lb_aff {
uint16_t timeout;
};
+/* OVNACT_COMMIT_FBD_LOCAL. */
+struct ovnact_commit_fdb_local{
+struct ovnact ovnact;
+uint16_t timeout; /* fdb_local flow timeout */
+};
+
/* Internal use by the helpers below. */
void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
diff --git a/lib/actions.c b/lib/actions.c
index a73fe1a1e..f5aa78db1 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -5236,6 +5236,98 @@ format_MAC_CACHE_USE(const struct ovnact_null
*null OVS_UNUSED, struct ds *s)
ds_put_cstr(s,
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
> On 18-Dec-2023, at 7:26 PM, Dumitru Ceara wrote:
>
> On 11/30/23 16:32, Dumitru Ceara wrote:
>> On 11/30/23 15:54, Naveen Yerramneni wrote:
>>>
>>>
On 30-Nov-2023, at 6:06 PM, Dumitru Ceara wrote:
On 11/30/23 09:45, Naveen Yerramneni wrote:
>
>
>> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara wrote:
>>
>> On 11/29/23 07:45, naveen.yerramneni wrote:
>>> This functionality can be enabled at the logical switch level:
>>> - "other_config:fdb_local" can be used to enable/disable this
>>> functionality, it is disabled by default.
>>> - "other_config:fdb_local_idle_timeout" sepcifies idle timeout
>>> for locally learned fdb flows, default timeout is 300 secs.
>>>
>>> If enabled, below lflow is added for each port that has unknown addr
>>> set.
>>> - table=2 (ls_in_lookup_fdb), priority=100, match=(inport == ),
>>> action=(commit_fdb_local(timeout=); next;
>>>
>>> New OVN action: "commit_fdb_local". This sets following OVS action.
>>> -
>>> learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
>>>
>>> NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
>>>
>>> This is useful when OVN is managing VLAN network that has multiple ports
>>> set with unknown addr and localnet_learn_fdb is enabled. With this
>>> config,
>>> if there is east-west traffic flowing between VMs part of same VLAN
>>> deployed on different hypervisors then, MAC addrs of the source and
>>> destination VMs keeps flapping between VM port and localnet port in
>>> Southbound FDB table. Enabling fdb_local config makes fdb table local to
>>> the chassis and avoids MAC flapping.
>>>
>>> Signed-off-by: Naveen Yerramneni
>>> ---
>>
>> Hi Naveen,
>>
>> Thanks a lot for the patch!
>>
>> Just a note, we already have a fix for the east-west traffic that causes
>> FDB flapping when localnet is used:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>>
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>>
>>
>> In general, however, I think it's a very good idea to move the FDB away
>> from the Southbound and make it local to each hypervisor. That reduces
>> load on the Southbound among other things.
>>
>
> Hi Dumitru,
>
> Thanks for informing about the patches.
> Yes, local FDB reduces load on southbound.
>
>
>>> include/ovn/actions.h | 7 +++
>>> lib/actions.c | 94
>>> northd/northd.c | 26 ++
>>> ovn-nb.xml| 14 ++
>>> tests/ovn.at | 108 ++
>>> utilities/ovn-trace.c | 2 +
>>> 6 files changed, 251 insertions(+)
>>>
>>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>> index 49cfe0624..85ac92cd3 100644
>>> --- a/include/ovn/actions.h
>>> +++ b/include/ovn/actions.h
>>> @@ -127,6 +127,7 @@ struct collector_set_ids;
>>> OVNACT(CHK_LB_AFF,ovnact_result) \
>>> OVNACT(SAMPLE,ovnact_sample) \
>>> OVNACT(MAC_CACHE_USE, ovnact_null)\
>>> +OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
>>>
>>> /* enum ovnact_type, with a member OVNACT_ for each action. */
>>> enum OVS_PACKED_ENUM ovnact_type {
>>> @@ -514,6 +515,12 @@ struct ovnact_commit_lb_aff {
>>> uint16_t timeout;
>>> };
>>>
>>> +/* OVNACT_COMMIT_FBD_LOCAL. */
>>> +struct ovnact_commit_fdb_local{
>>> +struct ovnact ovnact;
>>> +uint16_t timeout; /* fdb_local flow timeout */
>>> +};
>>> +
>>> /* Internal use by the helpers below. */
>>> void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
>>> void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
>>> diff --git a/lib/actions.c b/lib/actions.c
>>> index a73fe1a1e..f5aa78db1 100644
>>> --- a/lib/actions.c
>>> +++ b/lib/actions.c
>>> @@ -5236,6 +5236,98 @@ format_MAC_CACHE_USE(const struct ovnact_null
>>> *null OVS_UNUSED, struct ds *s)
>>> ds_put_cstr(s, "mac_cache_use;");
>>> }
>>>
>>> +static void
>>> +parse_commit_fdb_local(struct action_context *ctx,
>>> + struct
Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif-upcall: Resolve checksums before controller upcall.
On Mon, Dec 18, 2023 at 5:04 AM Eelco Chaudron wrote:
>
>
>
> On 15 Dec 2023, at 19:22, Mike Pattrick wrote:
>
> > On Fri, Dec 15, 2023 at 8:01 AM Eelco Chaudron wrote:
> >>
> >>
> >>
> >> On 11 Dec 2023, at 16:39, Mike Pattrick wrote:
> >>
> >>> Currently dp_netdev_upcall() resolves checksums on all packets, but this
> >>> isn't strictly needed. The checksums will be resolved before
> >>> transmission. However, we do have to resolve the checksums before
> >>> sending a packet to the controller as offload flags aren't retained.
> >>>
> >>> Signed-off-by: Mike Pattrick
> >>> ---
> >>> lib/dpif-netdev.c | 2 --
> >>> ofproto/ofproto-dpif-upcall.c | 13 ++---
> >>> 2 files changed, 10 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >>> index 9a59a1b03..8f7aaffbb 100644
> >>> --- a/lib/dpif-netdev.c
> >>> +++ b/lib/dpif-netdev.c
> >>> @@ -7980,8 +7980,6 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd,
> >>> struct dp_packet *packet_,
> >>> ds_destroy();
> >>> }
> >>>
> >>> -dp_packet_ol_send_prepare(packet_, 0);
> >>> -
> >>> return dp->upcall_cb(packet_, flow, ufid, pmd->core_id, type,
> >>> userdata,
> >>> actions, wc, put_actions, dp->upcall_aux);
> >>> }
> >>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> >>> index cc10f57b5..e974d844a 100644
> >>> --- a/ofproto/ofproto-dpif-upcall.c
> >>> +++ b/ofproto/ofproto-dpif-upcall.c
> >>> @@ -1574,15 +1574,20 @@(struct udpif *udpif, struct upcall *upcall,
> >>> const struct frozen_state *state = _node->state;
> >>>
> >>> struct ofproto_async_msg *am = xmalloc(sizeof *am);
> >>> +
> >>> +/* Resolve offloaded checksums, if any. */
> >>> +struct dp_packet *packet_clone = dp_packet_clone(packet);
> >>> +dp_packet_ol_send_prepare(packet_clone, 0);
> >>
> >> Doing a clone (malloc/copy), and doing another malloc and copy below sound
> >> like a waste of resources.
> >>
> >> Can we maybe make this more smart in the dp_netdev_upcall()?
> >> Something like the below in dp_netdev_upcall():
> >
> > The reason for moving this out of dp_netdev_upcall is resolving the
> > checksum will modify certain flags that interfere with the software
> > gso implementation. I could reset the flags for all packets in
> > dp-packet-gso, but this would not be needed in most packets and would
> > mean that dp_netdev_upcall packets don't take advantage of checksum
> > offload at all.
>
> I assume only the MISS_UPCALL could get away with its checksum not being
> correctly calculated for upcall handling.
>
> So maybe you can handle this in dp_netdev_upcall() with something like:
>
> if (type != MISS_UPCALL) {
> //store flags / checkums
> dp_packet_ol_send_prepare(packet_, 0);
> }
>
> rc = dp->upcall_cb(...)
>
> if (type != MISS_UPCALL) {
> //restore flags / checksums
> }
>
> return rc;
>
> But this might become a complex piece of code, and your below solution might
> be cleaner.
>
> > Another option would be to add a new function that sets the checksum
> > in a different buffer than the dp-packet. This would only be used
> > here, but shouldn't be too complex to implement.
> >
> > Probably the first one would be far more straightforward, so unless
> > you have a preference I'll do that one.
>
> Your first solution sounds the more straightforward but you might lose a bit
> of performance on the upcall packets. If this is fine, this is probably the
> best one.
Initially I was worried about the performance in setting the flags,
but as we have to read/write the flags anyways while segmenting it
shouldn't noticeably impact things to do a second read/write. I'll
make that change for the v2.
And as Ilya points out, there could be other cases that I'm not
anticipating with upcall_cb().
-M
>
> >
> > -M
> >
> >>
> >> if (type == CONTROLLER_UPCALL)
> >> dp_packet_ol_send_prepare(packet_, 0);
> >>
> >>
> >>> *am = (struct ofproto_async_msg) {
> >>> .controller_id = cookie->controller.controller_id,
> >>> .oam = OAM_PACKET_IN,
> >>> .pin = {
> >>> .up = {
> >>> .base = {
> >>> -.packet = xmemdup(dp_packet_data(packet),
> >>> - dp_packet_size(packet)),
> >>> -.packet_len = dp_packet_size(packet),
> >>> +.packet =
> >>> xmemdup(dp_packet_data(packet_clone),
> >>> +
> >>> dp_packet_size(packet_clone)),
> >>> +.packet_len = dp_packet_size(packet_clone),
> >>> .reason = cookie->controller.reason,
> >>> .table_id = state->table_id,
> >>> .cookie =
[ovs-dev] [PATCH ovn] ovn-ic: handle NB:name updates properly
When the user updates the NB_GLOBAL.name after registering
to IC Databases if the user already has defined chassis as a gateway
that will cause ovn-ic instance to run in an infinity loop trying
to update the gateways and insert the current gateway to the SB.chassis
tables as a remote chassis (we match on the new AZ ) which will fail since
we already have this chassis with is-interconn in local SB.
This patch aims to fix the above issues by updating the AZ.name only
when the user updates the NB.name locally.
Signed-off-by: Mohammad Heib
---
ic/ovn-ic.c | 10 +++---
tests/ovn-ic.at | 23 +++
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 8ceb34d7c..12e2729ce 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -132,14 +132,18 @@ az_run(struct ic_context *ctx)
return NULL;
}
-/* Delete old AZ if name changes. Note: if name changed when ovn-ic
- * is not running, one has to manually delete the old AZ with:
+/* Update old AZ if name changes. Note: if name changed when ovn-ic
+ * is not running, one has to manually delete/update the old AZ with:
* "ovn-ic-sbctl destroy avail ". */
static char *az_name;
const struct icsbrec_availability_zone *az;
if (az_name && strcmp(az_name, nb_global->name)) {
ICSBREC_AVAILABILITY_ZONE_FOR_EACH (az, ctx->ovnisb_idl) {
-if (!strcmp(az->name, az_name)) {
+/* AZ name update locally need to update az in ISB. */
+if (nb_global->name[0] && !strcmp(az->name, az_name)) {
+icsbrec_availability_zone_set_name(az, nb_global->name);
+break;
+} else if (!nb_global->name[0] && !strcmp(az->name, az_name)) {
icsbrec_availability_zone_delete(az);
break;
}
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index d4c436f84..5cc504e17 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -28,7 +28,30 @@ availability-zone az3
])
OVN_CLEANUP_IC([az1], [az2])
+AT_CLEANUP
+])
+
+
+AT_SETUP([ovn-ic -- AZ update in GW])
+ovn_init_ic_db
+net_add n1
+ovn_start az1
+sim_add gw-az1
+as gw-az1
+
+check ovs-vsctl add-br br-phys
+ovn_az_attach az1 n1 br-phys 192.168.1.1
+check ovs-vsctl set open . external-ids:ovn-is-interconn=true
+
+az_uuid=$(fetch_column ic-sb:availability-zone _uuid name="az1")
+ovn_as az1 ovn-nbctl set NB_Global . name="az2"
+wait_column "$az_uuid" ic-sb:availability-zone _uuid name="az2"
+
+# make sure that gateway still point to the same AZ with new name
+wait_column "$az_uuid" ic-sb:gateway availability_zone name="gw-az1"
+
+OVN_CLEANUP_IC([az1])
AT_CLEANUP
])
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 16/16] northd: Add northd change handler for sync_to_sb_lb node.
On 11/28/23 03:38, num...@ovn.org wrote:
> From: Numan Siddique
>
> Any changes to northd engine node due to load balancers
> are now handled in 'sync_to_sb_lb' node to sync the changed
> load balancers to SB load balancers.
>
> The logic to sync the SB load balancers is changed a bit and it
> now mimics the SB lflow sync.
>
> Below are the scale testing results done with all the patches applied
> in this series using ovn-heater. The test ran the scenario -
> ocp-500-density-heavy.yml [1].
>
> The resuts are:
>
> ---
> Min (s) Median (s) 90%ile (s) 99%ile
> (s) Max (s) Mean (s)Total (s) Count Failed
> ---
> Iteration Total 0.1368831.1290161.192001
> 1.2041671.2127280.66501783.127099 125 0
> Namespace.add_ports 0.0052160.0057360.007034
> 0.0154860.0189780.0062110.776373125 0
> WorkerNode.bind_port 0.0350300.0460820.052469
> 0.0582930.0603110.04597311.493259 250 0
> WorkerNode.ping_port 0.0050570.0067271.047692
> 1.0692531.0713360.26689666.724094 250 0
> ---
>
> The results with the present main [2] are:
>
> ---
> Min (s) Median (s) 90%ile (s) 99%ile
> (s) Max (s) Mean (s)Total (s) Count Failed
> ---
> Iteration Total 0.1354912.2238053.311270
> 3.3390783.3453461.729172216.146495 125 0
> Namespace.add_ports 0.0053800.0057440.006819
> 0.0187730.0208000.0062920.786532125 0
> WorkerNode.bind_port 0.0341790.0460550.053488
> 0.0588010.0710430.04611711.529311 250 0
> WorkerNode.ping_port 0.0049560.0069523.086952
> 3.1917433.1928070.791544197.886026 250 0
> ---
>
> [1] -
> https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
> [2] - 2a12cda890a7("controller, northd: Wait for cleanup before replying to
> exit")
>
> Signed-off-by: Numan Siddique
> ---
> northd/en-sync-sb.c | 445 +--
> northd/inc-proc-northd.c | 1 +
> northd/lflow-mgr.c | 196 ++---
> northd/lflow-mgr.h | 23 +-
> northd/northd.c | 238 -
> northd/northd.h | 6 -
> tests/ovn-northd.at | 103 +
> 7 files changed, 585 insertions(+), 427 deletions(-)
>
Hi Numan,
The only reason why we need to not ignore SB.LB updates is because we
need to check for duplicates that under normal circumstances do not
exist. They can appear due to "spurious" [0] inserts because the LB
table doesn't have an index defined.
Would it be less of an effort to just have a periodic task (we do
similar work for mac binding aging) and in that task (I-P node run
function) check for SB load balancer duplicates, remove them and trigger
a full recompute?
Wouldn't that be less error prone than all the I-P code?
Thanks,
Dumitru
[0] https://github.com/ovn-org/ovn/commit/9deb000536e0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v4] northd: forward arp request to lrp snat on.
On 12/8/23 16:38, Daniel Ding wrote:
> If the router has a snat rule and it's external ip isn't lrp address,
> when the arp request from other router for this external ip, will
> be drop, because of this external ip use same mac address as lrp, so
> can not forward to MC_FLOOD.
>
> Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
> possible.")
> Reported-at: https://github.com/ovn-org/ovn/issues/209
>
> Signed-off-by: Daniel Ding
> ---
Thanks, Daniel, for working on this!
I applied the patch to main and backported it to all stable branches
down to 22.03.
I also added you to the AUTHORS list.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
On 11/30/23 16:32, Dumitru Ceara wrote:
> On 11/30/23 15:54, Naveen Yerramneni wrote:
>>
>>
>>> On 30-Nov-2023, at 6:06 PM, Dumitru Ceara wrote:
>>>
>>> On 11/30/23 09:45, Naveen Yerramneni wrote:
> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara wrote:
>
> On 11/29/23 07:45, naveen.yerramneni wrote:
>> This functionality can be enabled at the logical switch level:
>> - "other_config:fdb_local" can be used to enable/disable this
>> functionality, it is disabled by default.
>> - "other_config:fdb_local_idle_timeout" sepcifies idle timeout
>> for locally learned fdb flows, default timeout is 300 secs.
>>
>> If enabled, below lflow is added for each port that has unknown addr set.
>> - table=2 (ls_in_lookup_fdb), priority=100, match=(inport == ),
>> action=(commit_fdb_local(timeout=); next;
>>
>> New OVN action: "commit_fdb_local". This sets following OVS action.
>> - learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
>>
>> NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
>>
>> This is useful when OVN is managing VLAN network that has multiple ports
>> set with unknown addr and localnet_learn_fdb is enabled. With this
>> config,
>> if there is east-west traffic flowing between VMs part of same VLAN
>> deployed on different hypervisors then, MAC addrs of the source and
>> destination VMs keeps flapping between VM port and localnet port in
>> Southbound FDB table. Enabling fdb_local config makes fdb table local to
>> the chassis and avoids MAC flapping.
>>
>> Signed-off-by: Naveen Yerramneni
>> ---
>
> Hi Naveen,
>
> Thanks a lot for the patch!
>
> Just a note, we already have a fix for the east-west traffic that causes
> FDB flapping when localnet is used:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>
>
> In general, however, I think it's a very good idea to move the FDB away
> from the Southbound and make it local to each hypervisor. That reduces
> load on the Southbound among other things.
>
Hi Dumitru,
Thanks for informing about the patches.
Yes, local FDB reduces load on southbound.
>> include/ovn/actions.h | 7 +++
>> lib/actions.c | 94
>> northd/northd.c | 26 ++
>> ovn-nb.xml| 14 ++
>> tests/ovn.at | 108 ++
>> utilities/ovn-trace.c | 2 +
>> 6 files changed, 251 insertions(+)
>>
>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> index 49cfe0624..85ac92cd3 100644
>> --- a/include/ovn/actions.h
>> +++ b/include/ovn/actions.h
>> @@ -127,6 +127,7 @@ struct collector_set_ids;
>>OVNACT(CHK_LB_AFF,ovnact_result) \
>>OVNACT(SAMPLE,ovnact_sample) \
>>OVNACT(MAC_CACHE_USE, ovnact_null)\
>> +OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
>>
>> /* enum ovnact_type, with a member OVNACT_ for each action. */
>> enum OVS_PACKED_ENUM ovnact_type {
>> @@ -514,6 +515,12 @@ struct ovnact_commit_lb_aff {
>>uint16_t timeout;
>> };
>>
>> +/* OVNACT_COMMIT_FBD_LOCAL. */
>> +struct ovnact_commit_fdb_local{
>> +struct ovnact ovnact;
>> +uint16_t timeout; /* fdb_local flow timeout */
>> +};
>> +
>> /* Internal use by the helpers below. */
>> void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
>> void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
>> diff --git a/lib/actions.c b/lib/actions.c
>> index a73fe1a1e..f5aa78db1 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -5236,6 +5236,98 @@ format_MAC_CACHE_USE(const struct ovnact_null
>> *null OVS_UNUSED, struct ds *s)
>>ds_put_cstr(s, "mac_cache_use;");
>> }
>>
>> +static void
>> +parse_commit_fdb_local(struct action_context *ctx,
>> + struct ovnact_commit_fdb_local *fdb_local)
>> +{
>> +uint16_t timeout = 0;
>> +lexer_force_match(ctx->lexer, LEX_T_LPAREN); /* Skip '('. */
>> +if (!lexer_match_id(ctx->lexer, "timeout")) {
>> +
Re: [ovs-dev] [PATCH ovn] test: add dedicated test for garp-max-timeout
On 12/15/23 18:01, Lorenzo Bianconi wrote: > Introduce a dedicated test for garp-max-timeout knob > For context of the review, I had asked for this "standalone" test for garp-max-timeout so that we can backport the following patch to 21.12: https://mail.openvswitch.org/pipermail/ovs-dev/2023-December/410247.html > Signed-off-by: Lorenzo Bianconi > --- > tests/ovn.at | 82 > 1 file changed, 82 insertions(+) > > diff --git a/tests/ovn.at b/tests/ovn.at > index 918f97a9e..25b101a7a 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -37464,3 +37464,85 @@ wait_for_ports_up > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([gratuitous arp max timeout]) > +AT_KEYWORDS([garp-timeout]) > +AT_SKIP_IF([test $HAVE_TCPDUMP = no]) Nit: We don't really need tcpdump, we could use the ovs-pcap utility directly. But it's not mandatory I guess, we already have a bunch of tests using tcpdump. > +ovn_start > + > +ovn-nbctl ls-add ls0 > +ovn-nbctl lr-add lr0 > +ovn-nbctl lrp-add lr0 lr0-ls0 f0:00:00:00:00:01 192.168.0.1/24 > +ovn-nbctl lsp-add ls0 ls0-lr0 -- set Logical_Switch_Port ls0-lr0 \ > +type=router options:router-port=lr0-ls0 addresses='"f0:00:00:00:00:01"' > + > +ovn-nbctl lsp-add ls0 ln_port > +ovn-nbctl lsp-set-addresses ln_port unknown > +ovn-nbctl lsp-set-type ln_port localnet > +ovn-nbctl --wait=hv lsp-set-options ln_port network_name=physnet1 Missing "check .." for all these. > + > +# Prepare packets > +touch empty_expected > +echo > "f00108060001080006040001f001c0a80001c0a80001" > > arp_expected Please use fmt_pkt instead. > + > +net_add n1 > +sim_add hv1 > +as hv1 > +ovs-vsctl \ > +-- add-br br-phys \ > +-- add-br br-eth0 > + > +ovn_attach n1 br-phys 192.168.0.10 > + > +AT_CHECK([ovs-vsctl set Open_vSwitch . > external-ids:ovn-bridge-mappings=physnet1:br-eth0]) > +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif > options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap]) > + > +ovn-sbctl dump-flows > sbflows > +AT_CAPTURE_FILE([sbflows]) > + Why do we need this? > +# Wait until the patch ports are created in hv1 to connect br-int to br-eth0 > +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1]) > +OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv1"]) Using OVN_WAIT_PATCH_PORT_FLOWS makes the patch harder to backport to 21.12. Do we really need this? > +# Temporarily remove lr0 chassis > +# Wait for hv confirmation to make sure chassis is removed before we reset > pcap > +# Otherwise a garp might be sent after pcap have been reset but before > chassis is removed > +AT_CHECK([ovn-nbctl --wait=hv remove logical_router lr0 options chassis]) > + Why do we need to remove the "chassis" option on lr0? > +as hv1 reset_pcap_file snoopvif hv1/snoopvif > +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1) > +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1]) > +OVS_WAIT_UNTIL([ > +ls0_lr0=$(ovn-sbctl --bare --columns chassis list port_binding ls0-lr0) > +test "$ls0_lr0" = $hv1_uuid > +]) > + > +OVN_CHECK_PACKETS_CONTAIN([hv1/snoopvif-tx.pcap], [arp_expected]) This checks that one ARP packet is there, why? The test is about garp-max-timeout. > +# Temporarily remove lr0 chassis > +AT_CHECK([ovn-nbctl --wait=hv remove logical_router lr0 options chassis]) > + Why is this needed? > +as hv1 reset_pcap_file snoopvif hv1/snoopvif > +# set garp max timeout to 2s > +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch . > external-ids:garp-max-timeout-sec=2]) > +AT_CHECK([ovn-nbctl --wait=hv set logical_router lr0 options:chassis=hv1]) > + > +OVS_WAIT_UNTIL([ > +n_arp=$(tcpdump -c 10 -ner hv1/snoopvif-tx.pcap arp | wc -l) > +test "$n_arp" = 10 > +]) This doesn't actually test the timeout value. It will wait until 10 packets are received; they could be received at any intervals and the test will pass as long as it doesn't take more than OVS_TIMEOUT to receive all 10 packets. > + > +AT_CHECK([ovn-nbctl --wait=hv remove logical_router lr0 options chassis]) > +as hv1 reset_pcap_file snoopvif hv1/snoopvif > +# set garp max timeout to 2s > +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch . > external-ids:garp-max-timeout-sec=1]) The comment says "2s" but we set the value to 1 second. > +AT_CHECK([ovn-nbctl --wait=hv set logical_router lr0 options:chassis=hv1]) I'm still confused about why we keep changing the options:chassis value. > + > +OVS_WAIT_UNTIL([ > +n_arp=$(tcpdump -c 20 -ner hv1/snoopvif-tx.pcap arp | wc -l) > +test "$n_arp" = 20 > +]) > + Same comment as above about not really testing the timeout. > +OVN_CLEANUP([hv1]) > + > +AT_CLEANUP > +]) I'm not sure whether we can do better than: clear pcap file, configure "x seconds" timeout, sleep for "x * (y + 1)" seconds, check that the the pcap contains at least y arp packets. But that might be enough. We could also tag the test as
Re: [ovs-dev] [PATCH v3 02/11] ci: Add make check-ovsdb-cluster tests to GitHub action ci.
On 15 Dec 2023, at 16:23, Simon Horman wrote:
> On Tue, Dec 05, 2023 at 03:59:31PM +0100, Eelco Chaudron wrote:
>> This patch adds 'make check-ovsdb-cluster' tests to GitHub action ci.
>> In addition, this patch also makes sure this test and 'make check' do
>> not run as root.
>>
>> Signed-off-by: Eelco Chaudron
>> ---
>> .ci/linux-build.sh |5 -
>> .github/workflows/build-and-test.yml |3 +++
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
>> index 67c01a644..bb540703e 100755
>> --- a/.ci/linux-build.sh
>> +++ b/.ci/linux-build.sh
>> @@ -129,11 +129,14 @@ else
>> build_ovs
>> for testsuite in $TESTSUITE; do
>> run_as_root=
>> +if [ "$testsuite" != "check" ] && \
>> + [ "$testsuite" != "check-ovsdb-cluster" ] ; then
>> +run_as_root="sudo -E PATH=$PATH"
>> +fi
>> if [ "${testsuite##*dpdk}" != "$testsuite" ]; then
>> sudo sh -c 'echo 1024 > /proc/sys/vm/nr_hugepages' || true
>> [ "$(cat /proc/sys/vm/nr_hugepages)" = '1024' ]
>> export DPDK_EAL_OPTIONS="--lcores 0@1,1@1,2@1"
>> -run_as_root="sudo -E PATH=$PATH"
>> fi
>> $run_as_root make $testsuite TESTSUITEFLAGS=${JOBS} RECHECK=yes
>> done
>
> Hi Eelco,
>
> perhaps it is because it is Friday afternoon (although I did just have
> a coffee), but I am a but confused by the change above.
>
> My reading is that, before this change:
>
>run_as_root is set if the $testsuite includes the string "dpdk"
>
> While after the change:
>
>run_as_root is set if $testsuite is neither "check"
>nor check-ovsdb-cluster".
>
> In both cases:
>
>* Anything with "dpdk" results in run_as_root set; and
>* "check" and "check-ovsdb-cluster" do not result in run_as_root being set
So the TESTSUITE value passed before this patchset is as a string “check
check-dpdk” so it will loop through testsuite=check, and testsuite=check-dpdk.
With the change in this patch the “check”, and “check-ovsdb-cluster” will not
run as root, all others will.
> Further, it seems to me that the other values of TESTSUITE are:
> * "": which means the loop won't iterate
> * "test": which is special cased and also means the loop won't iterated
I guessed you missed ‘check check-dpdk’ and the new ‘check-ovsdb-cluster’ below?
> So I am a little puzzled regarding the motivation for this change.
Your reply also puzzles me, but it’s Monday, and have not yet had my coffee :)
>> diff --git a/.github/workflows/build-and-test.yml
>> b/.github/workflows/build-and-test.yml
>> index 09654205e..5d441157c 100644
>> --- a/.github/workflows/build-and-test.yml
>> +++ b/.github/workflows/build-and-test.yml
>> @@ -164,6 +164,9 @@ jobs:
>> m32: m32
>> opts: --disable-ssl
>>
>> + - compiler: gcc
>> +testsuite:check-ovsdb-cluster
>> +
>> steps:
>> - name: checkout
>>uses: actions/checkout@v3
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif-upcall: Resolve checksums before controller upcall.
On 15 Dec 2023, at 19:22, Mike Pattrick wrote:
> On Fri, Dec 15, 2023 at 8:01 AM Eelco Chaudron wrote:
>>
>>
>>
>> On 11 Dec 2023, at 16:39, Mike Pattrick wrote:
>>
>>> Currently dp_netdev_upcall() resolves checksums on all packets, but this
>>> isn't strictly needed. The checksums will be resolved before
>>> transmission. However, we do have to resolve the checksums before
>>> sending a packet to the controller as offload flags aren't retained.
>>>
>>> Signed-off-by: Mike Pattrick
>>> ---
>>> lib/dpif-netdev.c | 2 --
>>> ofproto/ofproto-dpif-upcall.c | 13 ++---
>>> 2 files changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 9a59a1b03..8f7aaffbb 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -7980,8 +7980,6 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd,
>>> struct dp_packet *packet_,
>>> ds_destroy();
>>> }
>>>
>>> -dp_packet_ol_send_prepare(packet_, 0);
>>> -
>>> return dp->upcall_cb(packet_, flow, ufid, pmd->core_id, type, userdata,
>>> actions, wc, put_actions, dp->upcall_aux);
>>> }
>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>>> index cc10f57b5..e974d844a 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -1574,15 +1574,20 @@(struct udpif *udpif, struct upcall *upcall,
>>> const struct frozen_state *state = _node->state;
>>>
>>> struct ofproto_async_msg *am = xmalloc(sizeof *am);
>>> +
>>> +/* Resolve offloaded checksums, if any. */
>>> +struct dp_packet *packet_clone = dp_packet_clone(packet);
>>> +dp_packet_ol_send_prepare(packet_clone, 0);
>>
>> Doing a clone (malloc/copy), and doing another malloc and copy below sound
>> like a waste of resources.
>>
>> Can we maybe make this more smart in the dp_netdev_upcall()?
>> Something like the below in dp_netdev_upcall():
>
> The reason for moving this out of dp_netdev_upcall is resolving the
> checksum will modify certain flags that interfere with the software
> gso implementation. I could reset the flags for all packets in
> dp-packet-gso, but this would not be needed in most packets and would
> mean that dp_netdev_upcall packets don't take advantage of checksum
> offload at all.
I assume only the MISS_UPCALL could get away with its checksum not being
correctly calculated for upcall handling.
So maybe you can handle this in dp_netdev_upcall() with something like:
if (type != MISS_UPCALL) {
//store flags / checkums
dp_packet_ol_send_prepare(packet_, 0);
}
rc = dp->upcall_cb(...)
if (type != MISS_UPCALL) {
//restore flags / checksums
}
return rc;
But this might become a complex piece of code, and your below solution might be
cleaner.
> Another option would be to add a new function that sets the checksum
> in a different buffer than the dp-packet. This would only be used
> here, but shouldn't be too complex to implement.
>
> Probably the first one would be far more straightforward, so unless
> you have a preference I'll do that one.
Your first solution sounds the more straightforward but you might lose a bit of
performance on the upcall packets. If this is fine, this is probably the best
one.
>
> -M
>
>>
>> if (type == CONTROLLER_UPCALL)
>> dp_packet_ol_send_prepare(packet_, 0);
>>
>>
>>> *am = (struct ofproto_async_msg) {
>>> .controller_id = cookie->controller.controller_id,
>>> .oam = OAM_PACKET_IN,
>>> .pin = {
>>> .up = {
>>> .base = {
>>> -.packet = xmemdup(dp_packet_data(packet),
>>> - dp_packet_size(packet)),
>>> -.packet_len = dp_packet_size(packet),
>>> +.packet = xmemdup(dp_packet_data(packet_clone),
>>> +
>>> dp_packet_size(packet_clone)),
>>> +.packet_len = dp_packet_size(packet_clone),
>>> .reason = cookie->controller.reason,
>>> .table_id = state->table_id,
>>> .cookie = get_32aligned_be64(
>>> @@ -1598,6 +1603,8 @@ process_upcall(struct udpif *udpif, struct upcall
>>> *upcall,
>>> },
>>> };
>>>
>>> +dp_packet_delete(packet_clone);
>>> +
>>> if (cookie->controller.continuation) {
>>> am->pin.up.stack = (state->stack_size
>>>? xmemdup(state->stack, state->stack_size)
>>> --
>>> 2.39.3
>>>
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
___
dev mailing
Re: [ovs-dev] [PATCH ovn] Revert "ovn: add geneve PMTUD support"
On 12/18/23 10:52, Dumitru Ceara wrote:
> On 12/16/23 04:48, num...@ovn.org wrote:
>> From: Numan Siddique
>>
>> This reverts commit 450e41e783bfa69e4f9d6c80f6bcb01147d5cfe1.
>>
>> If a packet has to be tunnelled to another node and if the physical
>> interface used for tunnelling has lower MTU than the packet or
>> if there is a route exception with a lower MTU, then the geneve
>> kernel module generates an ICMP need frag packet. This packet
>> was getting dropped since the metadata had to be swapped.
>> The commit [1] did exactly that and fixed the issue.
>> But it has 2 issues -
>> 1. It introduced a regression for the scenario when an ICMP need frag
>> packet generated outside of OVN has to be tunnelled and delivered
>> to the destination VM/pod. These ICMP need frag packets are now
>> dropped.
>> 2. If the logical switches has ACLs or load balancers configured then
>> these icmp need frag packets are dropped as they are not sent to
>> the correct zone.
>>
>> Its better to revert until we find a proper solution for the original
>> issue.
>
> I agree, it sounds good to me.
>
>>
>> [1] - 450e41e783bf("ovn: add geneve PMTUD support")
>
> This should actually be:
>
> Fixes: 450e41e783bf ("ovn: add geneve PMTUD support")
>
> With that fixed up:
> Acked-by: Dumitru Ceara
>
I forgot, sorry. To simplify our downstream process it would be great
if you could add:
Reported-at: https://issues.redhat.com/browse/FDP-216
Thanks again!
>>
>> Signed-off-by: Numan Siddique
>> ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Revert "ovn: add geneve PMTUD support"
On 12/16/23 04:48, num...@ovn.org wrote:
> From: Numan Siddique
>
> This reverts commit 450e41e783bfa69e4f9d6c80f6bcb01147d5cfe1.
>
> If a packet has to be tunnelled to another node and if the physical
> interface used for tunnelling has lower MTU than the packet or
> if there is a route exception with a lower MTU, then the geneve
> kernel module generates an ICMP need frag packet. This packet
> was getting dropped since the metadata had to be swapped.
> The commit [1] did exactly that and fixed the issue.
> But it has 2 issues -
> 1. It introduced a regression for the scenario when an ICMP need frag
> packet generated outside of OVN has to be tunnelled and delivered
> to the destination VM/pod. These ICMP need frag packets are now
> dropped.
> 2. If the logical switches has ACLs or load balancers configured then
> these icmp need frag packets are dropped as they are not sent to
> the correct zone.
>
> Its better to revert until we find a proper solution for the original
> issue.
I agree, it sounds good to me.
>
> [1] - 450e41e783bf("ovn: add geneve PMTUD support")
This should actually be:
Fixes: 450e41e783bf ("ovn: add geneve PMTUD support")
With that fixed up:
Acked-by: Dumitru Ceara
>
> Signed-off-by: Numan Siddique
> ---
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
