> -----Original Message-----
> From: Anders Selhammer [mailto:anders.selham...@est.tech]
> Sent: Tuesday, March 13, 2018 6:14 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH] Added TAILQ for sent delay_req
>
> In a ptp unaware network (like the telecom profile for frequency sync
> G.8265.1),
> both the RTD and the PDV can be substantially higher than in a ptp aware
> network. To achieve more accurate measurements, the rate may need to be
> configured higher to get more data and increase the chance of lucky packets.
> In a combination of a high configured rate of delay_req and high RTD/PDV in
> network, the risk that the response from the previously sent delay_req have
> not
> been received before a new delay_req is sent also become high. In that case,
> the
> need of storing more than the latest sent delay_req arise.
>
Hi,
This seems like a reasonable thing to want... However, a bit more explanation
on how the storage helps solve this problem, and possibly how you prevent the
list from growing too large in the worst case might be good. Also, the style {
} stuff would be easier to send as a preparatory patch first so that we can
more easily review the functional changes here.
> Signed-off-by: Anders Selhammer <anders.selham...@est.tech>
> ---
> msg.c | 1 +
> port.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
> 2 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/msg.c b/msg.c
> index a36d4d0..7c0a027 100644
> --- a/msg.c
> +++ b/msg.c
> @@ -368,6 +368,7 @@ int msg_post_recv(struct ptp_message *m, int cnt)
> break;
> case DELAY_RESP:
> timestamp_post_recv(m, &m->delay_resp.receiveTimestamp);
> + port_id_post_recv(&m->delay_resp.requestingPortIdentity);
> suffix = m->delay_resp.suffix;
> break;
> case PDELAY_RESP_FOLLOW_UP:
> diff --git a/port.c b/port.c
> index 25507e2..b9b35fc 100644
> --- a/port.c
> +++ b/port.c
> @@ -86,7 +86,7 @@ struct port {
> struct foreign_clock *best;
> enum syfu_state syfu;
> struct ptp_message *last_syncfup;
> - struct ptp_message *delay_req;
> + TAILQ_HEAD(delay_req, ptp_message) delay_req;
Is there some mechanism here to ensure that we don't grow too large of a tailq?
> struct ptp_message *peer_delay_req;
> struct ptp_message *peer_delay_resp;
> struct ptp_message *peer_delay_fup;
> @@ -143,6 +143,7 @@ struct port {
>
> #define NSEC2SEC 1000000000LL
>
> +static void flush_delay_req(struct port *p);
> static int port_capable(struct port *p);
> static int port_is_ieee8021as(struct port *p);
> static void port_nrate_initialize(struct port *p);
> @@ -1185,10 +1186,7 @@ static void port_synchronize(struct port *p,
> break;
> case SERVO_JUMP:
> port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
> - if (p->delay_req) {
> - msg_put(p->delay_req);
> - p->delay_req = NULL;
> - }
> + flush_delay_req(p);
Not sure why this wasn't already calling flush_delay_req.. seems like a good
change either way to reduce code duplication.
> if (p->peer_delay_req) {
> msg_put(p->peer_delay_req);
> p->peer_delay_req = NULL;
> @@ -1349,12 +1347,14 @@ static int port_delay_request(struct port *p)
> p->peer_delay_fup = NULL;
> }
>
> - if (p->delayMechanism == DM_P2P)
> + if (p->delayMechanism == DM_P2P) {
> return port_pdelay_request(p);
> + }
Style fix-ups might be easier to send as a separate patch.
>
> msg = msg_allocate();
> - if (!msg)
> + if (!msg) {
> return -1;
> + }
Same here, patches are easier to read if they are separate to what they concern.
>
> msg->hwts.type = p->timestamping;
>
> @@ -1383,10 +1383,8 @@ static int port_delay_request(struct port *p)
> goto out;
> }
>
> - if (p->delay_req)
> - msg_put(p->delay_req);
> + TAILQ_INSERT_HEAD(&p->delay_req, msg, list);
>
> - p->delay_req = msg;
> return 0;
> out:
> msg_put(msg);
> @@ -1559,9 +1557,10 @@ static void flush_last_sync(struct port *p)
>
> static void flush_delay_req(struct port *p)
> {
> - if (p->delay_req) {
> - msg_put(p->delay_req);
> - p->delay_req = NULL;
> + struct ptp_message *m;
> + while ((m = TAILQ_FIRST(&p->delay_req)) != NULL) {
> + TAILQ_REMOVE(&p->delay_req, m, list);
> + msg_put(m);
> }
> }
>
> @@ -1832,33 +1831,45 @@ out:
>
> static void process_delay_resp(struct port *p, struct ptp_message *m)
> {
> - struct delay_req_msg *req;
> struct delay_resp_msg *rsp = &m->delay_resp;
> + struct ptp_message *req, *obs;
> struct PortIdentity master;
> tmv_t c3, t3, t4, t4c;
>
> - if (!p->delay_req)
> - return;
> -
> master = clock_parent_identity(p->clock);
> - req = &p->delay_req->delay_req;
>
> - if (p->state != PS_UNCALIBRATED && p->state != PS_SLAVE)
> + if (p->state != PS_UNCALIBRATED && p->state != PS_SLAVE) {
> return;
> - if (!pid_eq(&rsp->requestingPortIdentity, &req-
> >hdr.sourcePortIdentity))
> + }
This (again, and sorry not really meaning to nitpick) is easier to read if we
had separated the style change from the functionality change.
> + if (!pid_eq(&rsp->requestingPortIdentity, &p->portIdentity)) {
> return;
> - if (rsp->hdr.sequenceId != ntohs(req->hdr.sequenceId))
> + }
> + if (!pid_eq(&master, &m->header.sourcePortIdentity)) {
> return;
> - if (!pid_eq(&master, &m->header.sourcePortIdentity))
> + }
> + TAILQ_FOREACH(req, &p->delay_req, list) {
> + if (rsp->hdr.sequenceId == ntohs(req-
> >delay_req.hdr.sequenceId)) {
> + break;
> + }
> + }
Ok so now we have to make sure that the sequence id matches one of the items in
the tailq.. makes sense.
> + if (!req) {
> return;
> + }
>
> c3 = correction_to_tmv(m->header.correction);
> - t3 = timespec_to_tmv(p->delay_req->hwts.ts);
> + t3 = timespec_to_tmv(req->hwts.ts);
> t4 = timestamp_to_tmv(m->ts.pdu);
> t4c = tmv_sub(t4, c3);
>
> clock_path_delay(p->clock, t3, t4c);
>
> + while ((obs = TAILQ_NEXT(req, list)) != NULL) {
> + TAILQ_REMOVE(&p->delay_req, obs, list);
> + msg_put(obs);
> + }
> + TAILQ_REMOVE(&p->delay_req, req, list);
> + msg_put(req);
> +
> if (p->logMinDelayReqInterval == rsp->hdr.logMessageInterval) {
> return;
> }
> --
> 1.8.3.1
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
