[ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization

Ilya Maximets i.maximets at ovn.org
Tue Mar 31 13:00:00 UTC 2020


On 3/31/20 12:28 PM, Matteo Croce wrote:
> On Wed, Mar 11, 2020 at 3:26 PM Vishal Deep Ajmera
> <vishal.deep.ajmera at ericsson.com> wrote:
>>
>> v11->v12:
>>  Addressed most of comments from Ilya and Eelco.
>>  https://mail.openvswitch.org/pipermail/ovs-dev/2020-February/367832.html
>>  https://mail.openvswitch.org/pipermail/ovs-dev/2020-February/367842.html
>>  Rebased to OVS master.
>>
> 
> Hi Vishal,
> 
> I tested your patch, but it seems there is a memory leak which depletes
> all the buffers in a very short time.
> I've found that reverting a chunk fixed it, this is the change I made:

Thanks, Matteo, for testing and finding a root cause!

I agree that it fixes the leak, but I still have few questions to this part
of code, because it doesn't look clean.

One new thing that came to mind is that both implemented "output" functions
has 'should_steal' argument, but doesn't respect it in terms that they doesn't
really take ownership of packets.  And that is controversial to semantics of
this argument.  We had a lot of troubles with such bad code in the past and
should be very careful.  Basically that is the main reason of dp-packet leaks
now and in the past.

Here is my suggestion on how this part of code should look like:

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 9d7a329cd..076ec3c8d 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -110,6 +110,7 @@ COVERAGE_DEFINE(datapath_drop_tunnel_push_error);
 COVERAGE_DEFINE(datapath_drop_tunnel_pop_error);
 COVERAGE_DEFINE(datapath_drop_recirc_error);
 COVERAGE_DEFINE(datapath_drop_invalid_port);
+COVERAGE_DEFINE(datapath_drop_invalid_bond);
 COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
 COVERAGE_DEFINE(datapath_drop_rx_invalid_packet);
 
@@ -7319,6 +7320,9 @@ dp_execute_output_action(struct dp_netdev_pmd_thread *pmd,
 {
     struct tx_port *p = pmd_send_port_cache_lookup(pmd, port_no);
     if (!OVS_LIKELY(p)) {
+        COVERAGE_ADD(datapath_drop_invalid_port,
+                     dp_packet_batch_size(packets_));
+        dp_packet_delete_batch(packets_, should_steal);
         return false;
     }
 
@@ -7357,20 +7361,30 @@ dp_execute_output_action(struct dp_netdev_pmd_thread *pmd,
     return true;
 }
 
-static bool
+static void
 dp_execute_lb_output_action(struct dp_netdev_pmd_thread *pmd,
                             struct dp_packet_batch *packets_,
                             bool should_steal, uint32_t bond)
 {
     struct dp_packet *packet;
-    uint32_t i;
-    const uint32_t cnt = dp_packet_batch_size(packets_);
+    struct dp_packet_batch out;
     struct tx_bond *p_bond = tx_bond_lookup(&pmd->tx_bonds, bond);
 
     if (!p_bond) {
-        return false;
+        COVERAGE_ADD(datapath_drop_invalid_bond,
+                     dp_packet_batch_size(packets_));
+        dp_packet_delete_batch(packets_, should_steal);
+        return;
     }
-    DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
+
+    if (!should_steal) {
+        dp_packet_batch_clone(&out, packets_);
+        dp_packet_batch_reset_cutlen(packets_);
+        packets_ = &out;
+    }
+    dp_packet_batch_apply_cutlen(packets_);
+
+    DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
         /*
          * Lookup the bond-hash table using hash to get the slave.
          */
@@ -7381,17 +7395,13 @@ dp_execute_lb_output_action(struct dp_netdev_pmd_thread *pmd,
 
         struct dp_packet_batch output_pkt;
         dp_packet_batch_init_packet(&output_pkt, packet);
-        if (OVS_LIKELY(dp_execute_output_action(pmd, &output_pkt, should_steal,
+        if (OVS_LIKELY(dp_execute_output_action(pmd, &output_pkt, true,
                                                 bond_member))) {
             /* Update slave stats. */
             non_atomic_ullong_add(&s_entry->n_packets, 1);
             non_atomic_ullong_add(&s_entry->n_bytes, size);
-        } else {
-            dp_packet_batch_refill(packets_, packet, i);
         }
     }
-
-    return dp_packet_batch_is_empty(packets_);
 }
 
 static void
@@ -7409,24 +7419,14 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
 
     switch ((enum ovs_action_attr)type) {
     case OVS_ACTION_ATTR_OUTPUT:
-        if (dp_execute_output_action(pmd, packets_, should_steal,
-                                     nl_attr_get_odp_port(a))) {
-            return;
-        } else {
-            COVERAGE_ADD(datapath_drop_invalid_port,
-                         dp_packet_batch_size(packets_));
-        }
-        break;
+        dp_execute_output_action(pmd, packets_, should_steal,
+                                 nl_attr_get_odp_port(a));
+        return;
 
     case OVS_ACTION_ATTR_LB_OUTPUT:
-        if (dp_execute_lb_output_action(pmd, packets_, should_steal,
-                                        nl_attr_get_u32(a))) {
-            return;
-        } else {
-            COVERAGE_ADD(datapath_drop_invalid_port,
-                         dp_packet_batch_size(packets_));
-        }
-        break;
+        dp_execute_lb_output_action(pmd, packets_, should_steal,
+                                    nl_attr_get_u32(a));
+        return;
 
     case OVS_ACTION_ATTR_TUNNEL_PUSH:
         if (should_steal) {
---


It additionally fixes double usage of the same drop counter.
Also, this version might be slightly faster because it uses batched packet
copy instead of copying packets one by one in the should_steal=false case.

P.S. These are not all the changes I'd like to make in this patch.  Only this
particular part.  Will make a usual review separately.

Best regards, Ilya Maximets.


More information about the dev mailing list