Re: [ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization
> > Hi Matteo, > > > > Thanks for the patch. But I fail to understand why there is a memory leak. > > In fact when I tested in my setup, the test ran without any leak. > > The main part is: > '_REFILL_' loop removes packets from the original batch and > dp_execute_output_action() > doesn't free them if 'should_steal' equals to false. Since, > dp_execute_output_action() > returns success, '_REFILL_' doesn't put packet back to original batch > leaking > it. > > You need lb_output to not be the last action to reproduce. > Thanks Ilya. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization
Hi Matteo,
Thanks for the patch. But I fail to understand why there is a memory leak.
In fact when I tested in my setup, the test ran without any leak.
The functions dp_execute_lb_output_action and dp_execute_output_action, both
return 'false' if current batch is non-empty.
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;
And the caller to the function i.e. dp_execute_cb() will then execute
dp_packet_delete_batch(packets_, should_steal); which is last line of this
function.
case OVS_ACTION_ATTR_DROP:
case __OVS_ACTION_ATTR_MAX:
OVS_NOT_REACHED();
}
dp_packet_delete_batch(packets_, should_steal);
If the should_steal is correctly set to true (for e.g. in case this is the
last action), then packet buffer will be freed. Am I missing something here?
Warm Regards,
Vishal Ajmera
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization
On 4/6/20 3:03 PM, Vishal Deep Ajmera wrote: > Hi Matteo, > > Thanks for the patch. But I fail to understand why there is a memory leak. > In fact when I tested in my setup, the test ran without any leak. The main part is: '_REFILL_' loop removes packets from the original batch and dp_execute_output_action() doesn't free them if 'should_steal' equals to false. Since, dp_execute_output_action() returns success, '_REFILL_' doesn't put packet back to original batch leaking it. You need lb_output to not be the last action to reproduce. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization
On 3/31/20 12:28 PM, Matteo Croce wrote:
> On Wed, Mar 11, 2020 at 3:26 PM Vishal Deep Ajmera
> 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,
Re: [ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization
On Wed, Mar 11, 2020 at 3:26 PM Vishal Deep Ajmera
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:
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7354,15 +7354,17 @@ 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 tx_bond *p_bond = tx_bond_lookup(&pmd->tx_bonds, bond);
if (!p_bond) {
return false;
}
-DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
+
+struct dp_packet_batch del_pkts;
+dp_packet_batch_init(&del_pkts);
+
+struct dp_packet *packet;
+DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
/*
* Lookup the bond-hash table using hash to get the slave.
*/
@@ -7379,11 +7381,16 @@ dp_execute_lb_output_action(struct dp_netdev_pmd_thread
*pmd,
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);
+dp_packet_batch_add(&del_pkts, packet);
}
}
-return dp_packet_batch_is_empty(packets_);
+/* Delete packets that failed OUTPUT action */
+COVERAGE_ADD(datapath_drop_invalid_port,
+ dp_packet_batch_size(&del_pkts));
+dp_packet_delete_batch(&del_pkts, should_steal);
+
+return true;
}
static void
Regards,
--
Matteo Croce
per aspera ad upstream
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v12 0/1] Balance-tcp bond mode optimization
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. v10->v11: Addressed Ben and Ilya's comments. https://mail.openvswitch.org/pipermail/ovs-dev/2020-January/366626.html https://mail.openvswitch.org/pipermail/ovs-dev/2020-January/367306.html Using cmap instead of hmap and removed use of bond cache for pmds. v9->v10: Rebased to OVS master. v8->v9: Applied Ben's patch for fixing sparse and type errors. Addressed review comments from Ben. https://mail.openvswitch.org/pipermail/ovs-dev/2019-December/366101.html v7->v8: Removed hash action for balance-tcp mode. Removed bond-only pmd reload action. Rebased to OVS master. v6->v7: Fixed issue reported by Matteo for bond/show. v5->v6: Addressed comments from Ilya Maximets. https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362001.html Rebased to OVS master. v4->v5: Support for stats per hash bucket. Support for dynamic load balancing. Rebased to OVS Master. v3->v4: Addressed Ilya Maximets comments. https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360452.html v2->v3: Rebased to OVS master. Fixed git merge issue. v1->v2: Updated datapath action to hash + lb-output. Updated throughput test observations. Rebased to OVS master. Signed-off-by: Manohar Krishnappa Chidambaraswamy Co-authored-by: Manohar Krishnappa Chidambaraswamy Signed-off-by: Vishal Deep Ajmera CC: Jan Scheurich CC: Venkatesan Pradeep CC: Ben Pfaff CC: Ilya Maximets CC: Ian Stokes CC: David Marchand CC: Matteo Croce CC: Eelco Chaudron Vishal Deep Ajmera (1): Avoid dp_hash recirculation for balance-tcp bond selection mode datapath/linux/compat/include/linux/openvswitch.h | 1 + lib/dpif-netdev.c | 399 +++--- lib/dpif-netlink.c| 3 + lib/dpif-provider.h | 11 + lib/dpif.c| 49 +++ lib/dpif.h| 13 + lib/odp-execute.c | 2 + lib/odp-util.c| 4 + ofproto/bond.c| 68 +++- ofproto/bond.h| 5 + ofproto/ofproto-dpif-ipfix.c | 1 + ofproto/ofproto-dpif-sflow.c | 3 +- ofproto/ofproto-dpif-xlate.c | 32 +- ofproto/ofproto-dpif.c| 30 ++ ofproto/ofproto-dpif.h| 8 +- ofproto/ofproto-provider.h| 3 + ofproto/ofproto.c | 10 + ofproto/ofproto.h | 1 + tests/lacp.at | 9 + vswitchd/bridge.c | 13 + vswitchd/vswitch.xml | 23 ++ 21 files changed, 632 insertions(+), 56 deletions(-) -- 2.7.4 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
