On Mon, Aug 17, 2020 at 11:58:59AM -0700, Richard Cochran wrote:
> Vladimir,
>
> On Tue, Aug 04, 2020 at 02:40:11PM +0300, Vladimir Oltean wrote:
> > The diff is large, yes, even though I tried to avoid making unnecessary
> > changes. I hope it isn't too difficult to review, it isn't as polished
> > as I typically like, because the idea itself was uncertain, so I
> > preferred not to spend that extra time at this point in the process.
> > Anyways, I'm looking forward to your feedback whenever you have the
> > time.
>
> I was going to begin my review, but first I ran my ts2phc smoke test.
> Valgrind found a memory leak.
>
> Thanks,
> Richard
>
> sudo valgrind ./ts2phc -f ts2phc-TC.cfg --free_running=1
> ...
> Control-C...
>
> ==9363== HEAP SUMMARY:
> ==9363== in use at exit: 1,022 bytes in 10 blocks
> ==9363== total heap usage: 279 allocs, 269 frees, 20,230 bytes allocated
> ==9363==
> ==9363== LEAK SUMMARY:
> ==9363== definitely lost: 568 bytes in 4 blocks
> ==9363== indirectly lost: 454 bytes in 6 blocks
> ==9363== possibly lost: 0 bytes in 0 blocks
> ==9363== still reachable: 0 bytes in 0 blocks
> ==9363== suppressed: 0 bytes in 0 blocks
> ==9363== Rerun with --leak-check=full to see details of leaked memory
>
> --- ts2phc-TC.cfg ---
> # for use with three i210 cards
> [global]
> use_syslog 0
> verbose 1
> logging_level 6
> ts2phc.pulsewidth 500000000
> [eth3]
> ts2phc.channel 0
> ts2phc.master 1
> ts2phc.pin_index 0
> [eth4]
> ts2phc.channel 0
> ts2phc.extts_polarity both
> ts2phc.pin_index 0
> [eth6]
> ts2phc.channel 0
> ts2phc.extts_polarity both
> ts2phc.pin_index 0
My bad. This is the sort of detail I haven't paid enough attention to.
FWIW, this fixes up the memory leakage for me:
diff --git a/ts2phc.c b/ts2phc.c
index 69cac305c791..cac1819d4060 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -29,6 +29,8 @@ struct interface {
static void ts2phc_cleanup(struct ts2phc_private *priv)
{
+ struct port *p, *tmp;
+
ts2phc_slave_cleanup(priv);
if (priv->master) {
ts2phc_master_destroy(priv->master);
@@ -37,6 +39,15 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
config_destroy(priv->cfg);
}
close_pmc_node(&priv->node);
+
+ /*
+ * Clocks are destroyed by the cleanup methods of the individual
+ * master and slave PHC modules.
+ */
+ LIST_FOREACH_SAFE(p, &priv->ports, list, tmp)
+ free(p);
+
+ msg_cleanup();
}
/* FIXME: Copied from phc2sys */
@@ -210,6 +221,14 @@ struct clock *clock_add(struct ts2phc_private *priv, const
char *device)
return c;
}
+void clock_destroy(struct clock *c)
+{
+ servo_destroy(c->servo);
+ posix_clock_close(c->clkid);
+ free(c->name);
+ free(c);
+}
+
/* FIXME: Copied from phc2sys */
static struct port *port_add(struct ts2phc_private *priv, unsigned int number,
char *device)
@@ -236,6 +255,7 @@ static struct port *port_add(struct ts2phc_private *priv,
unsigned int number,
p = malloc(sizeof(*p));
if (!p) {
pr_err("failed to allocate memory for a port");
+ clock_destroy(c);
return NULL;
}
p->number = number;
diff --git a/ts2phc.h b/ts2phc.h
index 9b5df9e25e38..45a5a730d722 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -70,6 +70,7 @@ struct ts2phc_private {
struct servo *servo_add(struct ts2phc_private *priv, struct clock *clock);
struct clock *clock_add(struct ts2phc_private *priv, const char *device);
void clock_add_tstamp(struct clock *clock, tmv_t ts);
+void clock_destroy(struct clock *clock);
#include "ts2phc_master.h"
#include "ts2phc_slave.h"
diff --git a/ts2phc_phc_master.c b/ts2phc_phc_master.c
index 3feaacfbfb39..43444e879d0a 100644
--- a/ts2phc_phc_master.c
+++ b/ts2phc_phc_master.c
@@ -87,7 +87,7 @@ static void ts2phc_phc_master_destroy(struct ts2phc_master
*master)
&perout_request)) {
pr_err(PTP_PEROUT_REQUEST_FAILED);
}
- posix_clock_close(m->clock->clkid);
+ clock_destroy(m->clock);
free(m);
}
diff --git a/ts2phc_slave.c b/ts2phc_slave.c
index 68fe355bdca7..6bb212865626 100644
--- a/ts2phc_slave.c
+++ b/ts2phc_slave.c
@@ -108,6 +108,7 @@ static void ts2phc_slave_array_destroy(struct
ts2phc_private *priv)
free(polling_array->slave);
free(polling_array->pfd);
+ free(polling_array->collected_events);
free(polling_array);
priv->polling_array = NULL;
}
@@ -191,12 +192,6 @@ static struct ts2phc_slave *ts2phc_slave_create(struct
ts2phc_private *priv,
pr_debug("PHC slave %s has ptp index %d", device,
slave->clock->phc_index);
- slave->clock->servo = servo_add(priv, slave->clock);
- if (!slave->clock->servo) {
- pr_err("failed to create servo");
- goto no_servo;
- }
-
if (phc_number_pins(slave->clock->clkid) > 0) {
err = phc_pin_setfunc(slave->clock->clkid, &slave->pin_desc);
if (err < 0) {
@@ -223,9 +218,7 @@ static struct ts2phc_slave *ts2phc_slave_create(struct
ts2phc_private *priv,
return slave;
no_ext_ts:
no_pin_func:
- servo_destroy(slave->clock->servo);
-no_servo:
- posix_clock_close(slave->clock->clkid);
+ clock_destroy(slave->clock);
no_posix_clock:
free(slave->name);
free(slave);
@@ -243,8 +236,7 @@ static void ts2phc_slave_destroy(struct ts2phc_slave *slave)
&extts)) {
pr_err(PTP_EXTTS_REQUEST_FAILED);
}
- servo_destroy(slave->clock->servo);
- posix_clock_close(slave->clock->clkid);
+ clock_destroy(slave->clock);
free(slave->name);
free(slave);
}
==2529==
==2529== HEAP SUMMARY:
==2529== in use at exit: 0 bytes in 0 blocks
==2529== total heap usage: 306 allocs, 306 frees, 105,297 bytes allocated
==2529==
==2529== All heap blocks were freed -- no leaks are possible
==2529==
==2529== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Do I need to fix up the patches now, or are you still going to review
them?
Thanks,
-Vladimir
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
