Hi,
> I realize all of that is anecdotal, in that
> a TC-based fork of Linuxptp might have performed well also. Nevertheless,
I'm
> concerned that a fully TC-based implementation would move the code away
> from conformance with 802.1AS-2011, and interoperation would break as a
> result.
I'd like to know why the move towards BC based .1AS implementation, when
the section 7.5 in .1AS 2020 shows that the AVB profile is like a very
controlled BC, more like a P2P TC (+BMC) with its usage of Sync/Follow-up
and PDelay message set (instead of Delay_Req / Delay_Resp pairs). Also,
whether the syntonization of the bridge serves the purpose to send forward
the corrections (residence + link delay) and calculation of rate-ratios, or
a phase-sync is also required in the bridges implementing PTP relay, in
which case BC might make more sense?
Regards,
Jagmeet
On Thu, Oct 24, 2019 at 9:28 AM Y.b. Lu <yangbo...@nxp.com> wrote:
> Hi Richard,
> May I have your comments?
>
> Hi Rodney,
> Please see my comments inline.
>
> Thanks a lot.
>
> Best regards,
> Yangbo Lu
>
> > -----Original Message-----
> > From: Rodney Cummings <rodney.cummi...@ni.com>
> > Sent: Tuesday, October 15, 2019 1:12 AM
> > To: Y.b. Lu <yangbo...@nxp.com>; linuxptp-devel@lists.sourceforge.net;
> > Richard Cochran <richardcoch...@gmail.com>; Erik Hons <erik.h...@ni.com>
> > Subject: RE: [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge support
> >
> > Thanks Yangbo,
> >
> > I'd like to interject some overall comments/questions before digging into
> > comments on the code.
> >
> > In my opinion I prefer this v2 (BC) over the previous v1 (TC).
> >
> > To me, it isn't so much about the name. It is more about aligning the
> code with
> > the 802.1AS-2011 requirements ("shall"s). My employer (National
> Instruments)
> > has been shipping our own BC-based fork of Linuxptp for years now (e.g.
> uses
> > bc_dispatch() and bc_event() as is). That code has passed UNH-IOL
> > conformance for 802.1AS-2011, performed well in 802.1AS-2011 plugfests
> (i.e.
> > ICC, Avnu, ISPCS), and interoperated in customer applications with many
> other
> > 802.1AS-2011 end-stations and bridges. I realize all of that is
> anecdotal, in that
> > a TC-based fork of Linuxptp might have performed well also.
> Nevertheless, I'm
> > concerned that a fully TC-based implementation would move the code away
> > from conformance with 802.1AS-2011, and interoperation would break as a
> > result.
> >
>
> [Y.b. Lu] To me, I think BC is proper after you clarified that, although I
> thought it's TC before.
>
> > Assuming the group is okay with BC, there is probably one other major
> > question to answer before we go much further:
> > Are we okay implementing the 802.1AS-2011 BC specifics in clock.c,
> > or do we prefer to move most of that to a different file (bridge.c,
> > gptp_relay.c, or whatever)?
> >
>
> [Y.b. Lu] I'm ok with any method. We can have Richard's comment.
> From my patch, there were not too much changes to reuse BC.
>
> > There's a short list of items that make 802.1AS-2011 BC different from
> > 1588-2008 P2P Default BC:
> >
> > 1. Transfer Sync / FollowUp differences from Slave (receive) to Master
> > (transmit) ports.
> > 2. Support distinct sync intervals on Slave and Master ports (like a
> typical BC).
> >
> > The v2 patch covers item 1 and 2, which are the core differences.
> Nevertheless,
> > it might be good to keep other differences in mind (i.e. future patches)
> before
> > we decide the preceding question:
> >
>
> [Y.b. Lu] Yes. The patch had core changes, so that it could be reviewed to
> decide which method is good to add it in linuxptp.
>
> > 3. Transfer Pdelay differences from Slave to Master ports (cumulative
> rate
> > ratio).
>
> [Y.b. Lu] I have already done that in patch. But it's different with
> standard. I'm very confused on this, and doubt the standard.
> See below changes.
> + /* Update follow_up TLV */
> + gm_rr *= nrr;
> + fui->cumulativeScaledRateOffset = gm_rr * POW2_41 - POW2_41;
>
> I can't understand why standard gets rateRatio with,
> rateRatio += (neighborRateRatio - 1.0);
> not,
> rateRatio *= neighborRateRatio;
>
> As I understand, the rateRatio got from neighbor should be freq_gm /
> freq_neighbor.
> The neighborRateRato should be freq_neighbor / freq_cureent.
> So rateRatio for current device (freq_gm / freq_cureent) should be
> rateRatio * neighborRateRato.
>
> I will appreciate if you can help me to get it right.
>
> > 4. BMCA: Skip over UNCALIBRATED and PRE_MASTER states.
> > 5. BMCA: Remove foreign master qualification (i.e. change threshold from
> 2 to
> > 1).
> > 6. Check performance requirement in 802.1AS-2011 B.1.1 for neighbor rate
> > ratio computation.
> > 7. BMCA: FAULTY causes a State Decision Event. 802.1AS-2011's methodology
> > for faults is to avoid waiting in that FAULTY state in hopes that
> management
> > will notice. Instead, 802.1AS moves on to search for a valid non-faulty
> state
> > (SDE). If supported, the fault is logged so that management can notice
> later,
> > but that logging isn't a requirement.
>
> [Y.b. Lu] Thanks. No problem for me to generate patches to support these.
> I don't think these will introduce much changes either. I tend to not
> create new files for TAB/PRI.
>
> >
> > There's one other item in the upcoming revision (presumably 802.1AS-2020)
> > that is not explicitly stated in 802.1AS-2011:
> >
> > 8. If-and-only-if the sync interval is the same for a Slave and Master
> port pair,
> > "this PTP Port, when operating as a Master port, shall transmit a Sync
> as soon
> > as possible after the Slave port received a Sync (ignoring
> syncInterval)." To
> > Richard's credit, this is TC-like behavior. In the code, it presumably
> represents a
> > call to tc_fwd_sync() as was done in Yangbo's v1 patch. When we dig into
> the
> > details, there might be some 802.1AS-specifics, so maybe it will warrant
> a new
> > gptp_relay_fwd_sync() function.
> >
> > Since 802.1AS-2020 isn't published yet, I'd be perfectly happy ignoring
> item 8
> > for now. We can always revisit the issue later on.
> >
> > In the upcoming revision of 1588, all of items 1-8 are supported as
> options
> > that a PTP Profile (like 802.1AS) can list as required. For example, the
> 1588
> > revision allows UNCALIBRATED and PRE_MASTER to be skipped (whereas
> > 1588-2008 requires those states). Therefore, items 1-8 will be conformant
> > 1588 behavior that any profile can use. I mention this because if we
> want to
> > avoid "gptp" or "802.1AS" in the naming of items 1-8, that sounds fine,
> > because they are not necessarily exclusive to 802.1AS in the future.
>
> [Y.b. Lu] Thank you for sharing these information. That's much helpful :)
>
> >
> > Rodney
> > > -----Original Message-----
> > > From: Yangbo Lu <yangbo...@nxp.com>
> > > Sent: Monday, October 14, 2019 5:27 AM
> > > To: linuxptp-devel@lists.sourceforge.net; Rodney Cummings
> > > <rodney.cummi...@ni.com>; Richard Cochran
> > <richardcoch...@gmail.com>;
> > > Erik Hons <erik.h...@ni.com>
> > > Cc: Yangbo Lu <yangbo...@nxp.com>
> > > Subject: [EXTERNAL] [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge
> > > support
> > >
> > > This patch is to add IEEE 802.1AS-2011 time-aware bridge support based
> > > on current BC clock type. It implements only time information relay,
> > > and BMCA was not touched. To run it, the profile gPTP.cfg could be
> > > used with multiple interfaces specified using -i option.
> > >
> > > The main code changes are,
> > > - Create syfu_relay_info structure for time information relay.
> > > - Implement port_syfu_relay_info_insert() to update follow_up (with
> TLV)
> > > message with time information for relay.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo...@nxp.com>
> > > ---
> > > Changes for v2:
> > > - Implemented based on BC instead of TC.
> > > ---
> > > clock.c | 43 ++++++++++++++++++++++++++++++++++++-
> > > clock.h | 45 +++++++++++++++++++++++++++++++++++++++
> > > port.c | 75
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > > 3 files changed, 158 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/clock.c b/clock.c
> > > index 146576a..1865ecb 100644
> > > --- a/clock.c
> > > +++ b/clock.c
> > > @@ -45,7 +45,6 @@
> > > #include "util.h"
> > >
> > > #define N_CLOCK_PFD (N_POLLFD + 1) /* one extra per port, for the
> > > fault timer */ -#define POW2_41 ((double)(1ULL << 41))
> > >
> > > struct port {
> > > LIST_ENTRY(port) list;
> > > @@ -123,6 +122,7 @@ struct clock {
> > > int stats_interval;
> > > struct clockcheck *sanity_check;
> > > struct interface uds_interface;
> > > + struct syfu_relay_info syfu_relay;
> > > LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
> };
> > >
> > > @@ -1117,6 +1117,8 @@ struct clock *clock_create(enum clock_type type,
> > > struct config *config,
> > > }
> > > }
> > >
> > > + memset(&c->syfu_relay, 0, sizeof(struct syfu_relay_info));
> > > +
> > > /* Initialize the parentDS. */
> > > clock_update_grandmaster(c);
> > > c->dad.pds.parentStats = 0;
> > > @@ -1208,6 +1210,15 @@ void clock_follow_up_info(struct clock *c,
> > > struct follow_up_info_tlv *f)
> > > sizeof(c->status.lastGmPhaseChange));
> > > }
> > >
> > > +static void clock_get_follow_up_info(struct clock *c, struct
> > > +follow_up_info_tlv *f) {
> > > + f->cumulativeScaledRateOffset = c-
> > > >status.cumulativeScaledRateOffset;
> > > + f->scaledLastGmPhaseChange = c->status.scaledLastGmPhaseChange;
> > > + f->gmTimeBaseIndicator = c->status.gmTimeBaseIndicator;
> > > + memcpy(&f->lastGmPhaseChange, &c->status.lastGmPhaseChange,
> > > + sizeof(f->lastGmPhaseChange)); }
> > > +
> > > int clock_free_running(struct clock *c) {
> > > return c->free_running ? 1 : 0;
> > > @@ -1583,6 +1594,16 @@ void clock_peer_delay(struct clock *c, tmv_t
> > > ppd, tmv_t req, tmv_t rx,
> > > stats_add_value(c->stats.delay, tmv_dbl(ppd)); }
> > >
> > > +tmv_t clock_get_path_delay(struct clock *c) {
> > > + return c->path_delay;
> > > +}
> > > +
> > > +double clock_get_nrr(struct clock *c) {
> > > + return c->nrr;
> > > +}
> > > +
> > > int clock_slave_only(struct clock *c) {
> > > return c->dds.flags & DDS_SLAVE_ONLY; @@ -1776,6 +1797,7 @@ static
> > > void handle_state_decision_event(struct clock
> > > *c)
> > > c->path_delay = c->initial_delay;
> > > c->nrr = 1.0;
> > > fresh_best = 1;
> > > + clock_disable_syfu_relay(c);
> > > }
> > >
> > > c->best = best;
> > > @@ -1847,3 +1869,22 @@ enum servo_state clock_servo_state(struct clock
> > > *c) {
> > > return c->servo_state;
> > > }
> > > +
> > > +void clock_prepare_syfu_relay(struct clock *c, struct ptp_message
> *sync,
> > > + struct ptp_message *fup)
> > > +{
> > > + c->syfu_relay.precise_origin_ts = timestamp_to_tmv(fup->ts.pdu);
> > > + c->syfu_relay.correction = fup->header.correction;
> > > + clock_get_follow_up_info(c, &c->syfu_relay.fup_info_tlv);
> > > + c->syfu_relay.avail = 1;
> > > +}
> > > +
> > > +void clock_disable_syfu_relay(struct clock *c) {
> > > + c->syfu_relay.avail = 0;
> > > +}
> > > +
> > > +struct syfu_relay_info *clock_get_syfu_relay(struct clock *c) {
> > > + return &c->syfu_relay;
> > > +}
> > > diff --git a/clock.h b/clock.h
> > > index 9d3133a..8ff1181 100644
> > > --- a/clock.h
> > > +++ b/clock.h
> > > @@ -29,8 +29,18 @@
> > > #include "tmv.h"
> > > #include "transport.h"
> > >
> > > +#define POW2_41 ((double)(1ULL << 41))
> > > +
> > > struct ptp_message; /*forward declaration*/
> > >
> > > +struct syfu_relay_info {
> > > + tmv_t precise_origin_ts;
> > > + Integer64 correction;
> > > + struct follow_up_info_tlv fup_info_tlv;
> > > + /* Auxiliary info */
> > > + int avail;
> > > +};
> > > +
> > > /** Opaque type. */
> > > struct clock;
> > >
> > > @@ -240,6 +250,20 @@ void clock_peer_delay(struct clock *c, tmv_t ppd,
> > > tmv_t req, tmv_t rx,
> > > double nrr);
> > >
> > > /**
> > > + * Get the path delay as measured on a slave port.
> > > + * @param c The clock instance.
> > > + * @return The path delay as measured on a slave port.
> > > + */
> > > +tmv_t clock_get_path_delay(struct clock *c);
> > > +
> > > +/**
> > > + * Get the neighbor rate ratio as measured on a slave port.
> > > + * @param c The clock instance.
> > > + * @return The neighbor rate ratio as measured on a slave
> > > port.
> > > + */
> > > +double clock_get_nrr(struct clock *c);
> > > +
> > > +/**
> > > * Set clock sde
> > > * @param c A pointer to a clock instance obtained with
> > > clock_create().
> > > * @param sde Pass one (1) if need a decision event and zero if not.
> > > @@ -359,4 +383,25 @@ void clock_check_ts(struct clock *c, uint64_t ts);
> > > */
> > > double clock_rate_ratio(struct clock *c);
> > >
> > > +/**
> > > + * Prepare sync/follow_up relay.
> > > + * @param c The clock instance.
> > > + * @param sync The sync message.
> > > + * @param fup The follow_up message.
> > > + */
> > > +void clock_prepare_syfu_relay(struct clock *c, struct ptp_message
> *sync,
> > > + struct ptp_message *fup);
> > > +
> > > +/**
> > > + * Disable sync/follow_up relay.
> > > + * @param c The clock instance.
> > > + */
> > > +void clock_disable_syfu_relay(struct clock *c);
> > > +
> > > +/**
> > > + * Get sync/follow_up relay information.
> > > + * @param c The clock instance.
> > > + * @return The sync/follow_up relay information.
> > > + */
> > > +struct syfu_relay_info *clock_get_syfu_relay(struct clock *c);
> > > #endif
> > > diff --git a/port.c b/port.c
> > > index 07ad3f0..073e71a 100644
> > > --- a/port.c
> > > +++ b/port.c
> > > @@ -1241,6 +1241,10 @@ static void port_syfufsm(struct port *p, enum
> > > syfu_event event,
> > > break;
> > > case FUP_MATCH:
> > > syn = p->last_syncfup;
> > > + if (p->follow_up_info)
> > > + clock_prepare_syfu_relay(p->clock, syn, m);
> > > + else
> > > + clock_disable_syfu_relay(p->clock);
> > > port_synchronize(p, syn->hwts.ts, m->ts.pdu,
> > > syn->header.correction,
> > > m->header.correction,
> > > @@ -1261,6 +1265,10 @@ static void port_syfufsm(struct port *p, enum
> > > syfu_event event,
> > > break;
> > > case SYNC_MATCH:
> > > fup = p->last_syncfup;
> > > + if (p->follow_up_info)
> > > + clock_prepare_syfu_relay(p->clock, m, fup);
> > > + else
> > > + clock_disable_syfu_relay(p->clock);
> > > port_synchronize(p, m->hwts.ts, fup->ts.pdu,
> > > m->header.correction,
> > > fup->header.correction,
> > > @@ -1450,6 +1458,60 @@ int port_tx_announce(struct port *p, struct
> > > address
> > > *dst)
> > > return err;
> > > }
> > >
> > > +static void port_syfu_relay_info_insert(struct port *p,
> > > + struct ptp_message *sync,
> > > + struct ptp_message *fup)
> > > +{
> > > + struct syfu_relay_info *syfu_relay =
> clock_get_syfu_relay(p->clock);
> > > + struct follow_up_info_tlv *fui_relay = &syfu_relay->fup_info_tlv;
> > > + struct follow_up_info_tlv *fui = follow_up_info_extract(fup);
> > > + tmv_t ingress, egress, residence, path_delay;
> > > + double gm_rr, nrr, rr;
> > > + struct timestamp ts;
> > > +
> > > + if (syfu_relay->avail == 0)
> > > + return;
> > > +
> > > + fup->follow_up.preciseOriginTimestamp =
> > > + tmv_to_Timestamp(syfu_relay->precise_origin_ts);
> > > + fup->header.correction = syfu_relay->correction;
> > > +
> > > + /* Calculate residence time. */
> > > + ingress = clock_ingress_time(p->clock);
> > > + egress = sync->hwts.ts;
> > > + residence = tmv_sub(egress, ingress);
> > > + rr = clock_rate_ratio(p->clock);
> > > + if (rr != 1.0) {
> > > + residence = dbl_tmv(tmv_dbl(residence) * rr);
> > > + }
> > > +
> > > + gm_rr = 1.0 + (fui_relay->cumulativeScaledRateOffset + 0.0) /
> > > POW2_41;
> > > + nrr = clock_get_nrr(p->clock);
> > > +
> > > + /* Add corrected residence time into correction. */
> > > + fup->header.correction += tmv_to_TimeInterval(residence) * gm_rr *
> > > +nrr;
> > > +
> > > + /* Add corrected path delay into correction. */
> > > + path_delay = clock_get_path_delay(p->clock);
> > > + fup->header.correction += tmv_to_TimeInterval(path_delay) * gm_rr;
> > > +
> > > + /* Update follow_up TLV */
> > > + gm_rr *= nrr;
> > > + fui->cumulativeScaledRateOffset = gm_rr * POW2_41 - POW2_41;
> > > + fui->scaledLastGmPhaseChange = fui_relay->scaledLastGmPhaseChange;
> > > + fui->gmTimeBaseIndicator = fui_relay->gmTimeBaseIndicator;
> > > + memcpy(&fui->lastGmPhaseChange, &fui_relay->lastGmPhaseChange,
> > > + sizeof(fui->lastGmPhaseChange));
> > > +
> > > + ts.sec = fup->follow_up.preciseOriginTimestamp.seconds_msb;
> > > + ts.sec = ts.sec << 32 | fup-
> > > >follow_up.preciseOriginTimestamp.seconds_lsb;
> > > + ts.nsec = fup->follow_up.preciseOriginTimestamp.nanoseconds;
> > > + pr_debug("port %hu: syfu_relay info:", portnum(p));
> > > + pr_debug("port %hu: precise_origin_ts %" PRIu64 ".%u",
> portnum(p),
> > > ts.sec, ts.nsec);
> > > + pr_debug("port %hu: correction %" PRId64, portnum(p), fup-
> > > >header.correction >> 16);
> > > + pr_debug("port %hu: fup_info %.9f", portnum(p), gm_rr);
> > > +}
> > > +
> > > int port_tx_sync(struct port *p, struct address *dst) {
> > > struct ptp_message *msg, *fup;
> > > @@ -1543,10 +1605,15 @@ int port_tx_sync(struct port *p, struct
> > > address
> > > *dst)
> > > fup->address = *dst;
> > > fup->header.flagField[0] |= UNICAST;
> > > }
> > > - if (p->follow_up_info && follow_up_info_append(fup)) {
> > > - pr_err("port %hu: append fup info failed", portnum(p));
> > > - err = -1;
> > > - goto out;
> > > +
> > > + if (p->follow_up_info) {
> > > + if (follow_up_info_append(fup)) {
> > > + pr_err("port %hu: append fup info failed",
> portnum(p));
> > > + err = -1;
> > > + goto out;
> > > + }
> > > +
> > > + port_syfu_relay_info_insert(p, msg, fup);
> > > }
> > >
> > > err = port_prepare_and_send(p, fup, TRANS_GENERAL);
> > > --
> > > 2.7.4
>
>
>
> _______________________________________________
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
>
--
Best Regards,
~
Jagmeet Singh Hanspal
~
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
