Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Sun, Dec 03, 2023 at 06:15:52PM -0800, Kishen Maloor wrote: > On 12/1/23 10:07 PM, Richard Cochran wrote: > > Does the standard require that either ALL the nodes in a network use > > CMLDS, or none do at all? > > > > When the time-aware system supports multiple domains, IEEE > 802.1as-2020/11.2.17 mandates CMLDS and the COMMON_P2P DM. Very narrow minded and short sighted. Oh well, I wouldn't expect 802.1as to actually be practical. Pass me the crazy sauce, please! Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On 12/1/23 10:07 PM, Richard Cochran wrote: On Thu, Nov 30, 2023 at 04:52:44PM -0800, Kishen Maloor wrote: CMLDS ptp4l process would be configured with transportSpecific/domain=0x2/0. Question about the spec: How on earth is this going to work? I mean, if the link partner is not also using CMLDS, then it won't reply because of the majorSdoId mismatch. Yes, as we understand, that is indeed the case. IEEE 1588-2019/16.6.3.1/Note 3 emphasizes this point and 16.6.3.2 specifies the wire protocol variations for CMLDS. So yeah, the link partner is expected to run an instance of the service and they would interoperate. Does the standard require that either ALL the nodes in a network use CMLDS, or none do at all? When the time-aware system supports multiple domains, IEEE 802.1as-2020/11.2.17 mandates CMLDS and the COMMON_P2P DM. Thanks, -Kishen. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Thu, Nov 30, 2023 at 04:52:44PM -0800, Kishen Maloor wrote: > CMLDS ptp4l process would be configured with > transportSpecific/domain=0x2/0. Question about the spec: How on earth is this going to work? I mean, if the link partner is not also using CMLDS, then it won't reply because of the majorSdoId mismatch. Does the standard require that either ALL the nodes in a network use CMLDS, or none do at all? This is messed up, IMO. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Thu, Nov 30, 2023 at 04:52:44PM -0800, Kishen Maloor wrote: > If serviceMeasurementValid=true, then we set both peer_portid_valid and > p->nrate.ratio_valid to true so that port_capable() could evaluate to true. Even simpler: include asCapable from the CMLDS Link Port in the TLV, and just assign asCapable directly in the cleint. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Thu, Nov 30, 2023 at 04:52:44PM -0800, Kishen Maloor wrote: > > + p->cmlds_pmc = pmc_create(cfg, TRANS_UDS, > > + config_get_string(cfg, p->name, > > "cmlds_client_address"), > > + config_get_string(cfg, p->name, > > "cmlds_server_address"), > > + hops, > > + config_get_int(cfg, NULL, "domainNumber"), > > + config_get_int(cfg, p->name, > > "transportSpecific") << 4, > > I haven't studied the flow here, but using the P2P_COMMON instance's > transportSpecific and domainNumber above might cause the CMLDS server's > port_ignore() to drop this message as the > CMLDS ptp4l process would be configured with > transportSpecific/domain=0x2/0. > > Perhaps just passing 0x2/0 above will fix things? This is assuming that > there are no transportSpecific/domainNumber checks on the receive path > for CMLDS events (which seemed to be the case looking at the code). Right, those two fields must match. So I think: - we'll need options for those two, and - this is another reason to use a separate pmc instance for cmlds rather than reusing the uds port. > > Right, so the next TODO is to enable FD_DELAY_TIMER for P2P_COMMON > > mode and let it 1) renew the push subscription and 2) catch the case > > when the push notification does not arrive on time. > > > > Yes, in our series CMLDS queries were performed synchronously at the Pdelay > request cadence. If serviceMeasurementValid=false then port->pdr_missing was > incremented and immediately followed by a call to port_capable(). This was > necessary as I recollect there was a related Avnu test case. > > If serviceMeasurementValid=true, then we set both peer_portid_valid and > p->nrate.ratio_valid to true so that port_capable() could evaluate to true. I think that logic can be implemented without any service Measurement Valid field in the TLV. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
Hi Richard,
Firstly, thanks so much for your reviews and efforts to get
this done.
I'll just rehash a couple of points also raised by Andrew.
On this change below:
> +static int port_cmlds_initialize(struct port *p)
> +{
> + struct config *cfg = clock_config(p->clock);
> + struct subscribe_events_np sen = {0};
> + const int zero_datalen = 1;
> + const UInteger8 hops = 0;
> +
> + p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
> + p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> +config_get_string(cfg, p->name,
"cmlds_client_address"),
> +config_get_string(cfg, p->name,
"cmlds_server_address"),
> +hops,
> +config_get_int(cfg, NULL, "domainNumber"),
> +config_get_int(cfg, p->name, "transportSpecific")
<< 4,
I haven't studied the flow here, but using the P2P_COMMON instance's
transportSpecific and domainNumber above might cause the CMLDS server's
port_ignore() to drop this message as the
CMLDS ptp4l process would be configured with
transportSpecific/domain=0x2/0.
Perhaps just passing 0x2/0 above will fix things? This is assuming that
there are no transportSpecific/domainNumber checks on the receive path
for CMLDS events (which seemed to be the case looking at the code).
On 11/30/23 11:28 AM, Richard Cochran wrote:
...
serviceMeasurementValid is implicit, because if you get a PUSH
notification, it is valid.
Right, serviceMeasurementValid == true is implicit but
serviceMeasurementValid == false is useful for the .port_id_valid /
.pdr_missing logic to work. The timeout logic is disabled with
P2P_COMMON.
Right, so the next TODO is to enable FD_DELAY_TIMER for P2P_COMMON
mode and let it 1) renew the push subscription and 2) catch the case
when the push notification does not arrive on time.
Yes, in our series CMLDS queries were performed synchronously at the
Pdelay request cadence. If serviceMeasurementValid=false then
port->pdr_missing was incremented and immediately followed by a call to
port_capable(). This was necessary as I recollect there was a related
Avnu test case.
If serviceMeasurementValid=true, then we set both peer_portid_valid and
p->nrate.ratio_valid to true so that port_capable() could evaluate to true.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Thu, Nov 30, 2023 at 07:12:54PM +0100, Andrew Zaborowski wrote:
> On Thu, 30 Nov 2023 at 17:44, Richard Cochran
> wrote:
> > On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:
> > > > + PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
> > >
> > > I assume the use of this is simply to set unique names for per-port
> > > sockets. Maybe something like tempnam() could be used instead.
> >
> > No, it has to be under user control, because the path might be under a
> > security policy.
>
> Do you mean that the user might want it to be under a security policy?
Yes.
> Otherwise it could be in /tmp.
The top down path has to be the user's choice,
/tmp/... or
/var/run/... or
/whatever/...
and so there must be a configuration option in any case. It seems
silly to restrict the option to the leading path and not include the
socket file name.
> > > > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int
> > > > count)
> > > > p->fda.fd[i] = -1;
> > > > }
> > > >
> > > > +static int port_cmlds_initialize(struct port *p)
> > > > +{
> > > > + struct config *cfg = clock_config(p->clock);
> > > > + struct subscribe_events_np sen = {0};
> > > > + const int zero_datalen = 1;
> > > > + const UInteger8 hops = 0;
> > > > +
> > > > + p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
> > >
> > > Should this fall back to port_number(p)?
> >
> > I don't see how, since there is always a default for each
> > configuration option.
>
> Right, it'd need to default to, say, -1 which would be checked here.
Okay, sure. That will reduce configuration burden for normal setups.
> > Also, I understood from you guys that the port
> > number _has_ to be different to pass compliance tests. For that, I
> > thought adding a "starting port number" option would do the trick
>
> The port identity (clock identity + port number) has to be unique. As
> long as the clock identity is unique, which it has to be, I don't
> think the port number *has* to be different.
Got it.
> > serviceMeasurementValid is implicit, because if you get a PUSH
> > notification, it is valid.
>
> Right, serviceMeasurementValid == true is implicit but
> serviceMeasurementValid == false is useful for the .port_id_valid /
> .pdr_missing logic to work. The timeout logic is disabled with
> P2P_COMMON.
Right, so the next TODO is to enable FD_DELAY_TIMER for P2P_COMMON
mode and let it 1) renew the push subscription and 2) catch the case
when the push notification does not arrive on time.
Thoughts?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Thu, 30 Nov 2023 at 17:44, Richard Cochran wrote:
> On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:
> > > + PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
> >
> > I assume the use of this is simply to set unique names for per-port
> > sockets. Maybe something like tempnam() could be used instead.
>
> No, it has to be under user control, because the path might be under a
> security policy.
Do you mean that the user might want it to be under a security policy?
Otherwise it could be in /tmp.
>
> > > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int
> > > count)
> > > p->fda.fd[i] = -1;
> > > }
> > >
> > > +static int port_cmlds_initialize(struct port *p)
> > > +{
> > > + struct config *cfg = clock_config(p->clock);
> > > + struct subscribe_events_np sen = {0};
> > > + const int zero_datalen = 1;
> > > + const UInteger8 hops = 0;
> > > +
> > > + p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
> >
> > Should this fall back to port_number(p)?
>
> I don't see how, since there is always a default for each
> configuration option.
Right, it'd need to default to, say, -1 which would be checked here.
> Also, I understood from you guys that the port
> number _has_ to be different to pass compliance tests. For that, I
> thought adding a "starting port number" option would do the trick
The port identity (clock identity + port number) has to be unique. As
long as the clock identity is unique, which it has to be, I don't
think the port number *has* to be different.
>
> > In Kishen's patch series struct cmlds_info_np also had a
> > .serviceMeasurementValid which enabled the port_capable() logic to
> > work and IIRC that is important at least for 802.1AS compliance. If
> > that was to be re-added, the event would need to be emitted in an
> > extra place in the CMLDS, perhaps where pdr_missing is incremented.
>
> serviceMeasurementValid is implicit, because if you get a PUSH
> notification, it is valid.
Right, serviceMeasurementValid == true is implicit but
serviceMeasurementValid == false is useful for the .port_id_valid /
.pdr_missing logic to work. The timeout logic is disabled with
P2P_COMMON.
Best regards
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:
> > @@ -249,6 +250,9 @@ struct config_item config_tab[] = {
> > GLOB_ITEM_INT("clock_class_threshold",
> > CLOCK_CLASS_THRESHOLD_DEFAULT, 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
> > GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
> > GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
> > + PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
>
> I assume the use of this is simply to set unique names for per-port
> sockets. Maybe something like tempnam() could be used instead.
No, it has to be under user control, because the path might be under a
security policy.
> > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
> > p->fda.fd[i] = -1;
> > }
> >
> > +static int port_cmlds_initialize(struct port *p)
> > +{
> > + struct config *cfg = clock_config(p->clock);
> > + struct subscribe_events_np sen = {0};
> > + const int zero_datalen = 1;
> > + const UInteger8 hops = 0;
> > +
> > + p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
>
> Should this fall back to port_number(p)?
I don't see how, since there is always a default for each
configuration option. Also, I understood from you guys that the port
number _has_ to be different to pass compliance tests. For that, I
thought adding a "starting port number" option would do the trick.
> > + p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> > + config_get_string(cfg, p->name,
> > "cmlds_client_address"),
> > + config_get_string(cfg, p->name,
> > "cmlds_server_address"),
> > + hops,
> > + config_get_int(cfg, NULL, "domainNumber"),
> > + config_get_int(cfg, p->name,
> > "transportSpecific") << 4,
>
> The sdoId is (by either IEEE 1588 or 802.1AS) different for the CMLDS
> than for a P2P_COMMON port, I think a check in port_ignore will
> discard the messages as a result. Maybe hardcode TS_CMLDS here? (with
> TS_CMLDS defined in msg.h next to TS_IEEE_8021AS)
>
> Using pmc here surely works but I wonder if it's not actually more
> work than reusing c->uds_port.
I considered that, but I invite you to try implementing that to see
which one is simpler.
> > + switch (mgt->id) {
> > + case MID_CMLDS_INFO_NP:
> > + if (msg->header.sourcePortIdentity.portNumber !=
> > p->cmlds_port) {
> > + break;
> > + }
> > + cmlds = (struct cmlds_info_np *) mgt->data;
> > + p->peer_delay = nanoseconds_to_tmv(cmlds->meanLinkDelay >>
> > 16);
> > + p->peerMeanPathDelay = tmv_to_TimeInterval(p->peer_delay);
>
> cmlds->meanLinkDelay is already a TimeInterval in case you want to use that.
good point
> > +
> > + if (p->state == PS_UNCALIBRATED || p->state == PS_SLAVE) {
> > + const struct timestamp dummy = {0};
> > + const tmv_t tx = timestamp_to_tmv(dummy);
>
> tmv_zero() could also be used.
yeah
> > + clock_peer_delay(p->clock, p->peer_delay, tx, tx,
> > +p->nrate.ratio);
>
> p->nrate.ratio hasn't been set yet, I think a line similar to this is missing:
>
> p->nrate.ratio = 1.0 + (double) cmlds->scaledNeighborRateRatio / POW2_41;
Right
> The CMLDS port will also need to ensure that the NRR is calculated
> because now it's done conditionally.
Huh, I didn't change that part?
> In Kishen's patch series struct cmlds_info_np also had a
> .serviceMeasurementValid which enabled the port_capable() logic to
> work and IIRC that is important at least for 802.1AS compliance. If
> that was to be re-added, the event would need to be emitted in an
> extra place in the CMLDS, perhaps where pdr_missing is incremented.
serviceMeasurementValid is implicit, because if you get a PUSH
notification, it is valid.
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.
Hi Richard,
I haven't tested this but here are some comments.
On Thu, 30 Nov 2023 at 08:58, Richard Cochran wrote:
> From: Kishen Maloor
>
> Signed-off-by: Richard Cochran
> ---
> config.c | 4 ++
> dm.h | 3 +
> fd.h | 1 +
> makefile | 4 +-
> port.c | 166 +++--
> port_private.h | 3 +
> 6 files changed, 174 insertions(+), 7 deletions(-)
>
> diff --git a/config.c b/config.c
> index fe65b76..05ffd3c 100644
> --- a/config.c
> +++ b/config.c
> @@ -171,6 +171,7 @@ static struct config_enum delay_filter_enu[] = {
>
> static struct config_enum delay_mech_enu[] = {
> { "Auto", DM_AUTO },
> + { "COMMON_P2P", DM_COMMON_P2P },
> { "E2E", DM_E2E },
> { "P2P", DM_P2P },
> { "NONE", DM_NO_MECHANISM },
> @@ -249,6 +250,9 @@ struct config_item config_tab[] = {
> GLOB_ITEM_INT("clock_class_threshold", CLOCK_CLASS_THRESHOLD_DEFAULT,
> 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
> GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
> GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
> + PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
I assume the use of this is simply to set unique names for per-port
sockets. Maybe something like tempnam() could be used instead.
> + PORT_ITEM_INT("cmlds_port", 1, 1, UINT16_MAX),
> + PORT_ITEM_STR("cmlds_server_address", "/var/run/cmlds_server"),
> GLOB_ITEM_ENU("dataset_comparison", DS_CMP_IEEE1588,
> dataset_comp_enu),
> PORT_ITEM_INT("delayAsymmetry", 0, INT_MIN, INT_MAX),
> PORT_ITEM_ENU("delay_filter", FILTER_MOVING_MEDIAN, delay_filter_enu),
> diff --git a/dm.h b/dm.h
> index 47bd847..80d1ce5 100644
> --- a/dm.h
> +++ b/dm.h
> @@ -34,6 +34,9 @@ enum delay_mechanism {
> /** Peer delay mechanism. */
> DM_P2P,
>
> + /** Peer delay as measured by CMLDS. */
> + DM_COMMON_P2P,
> +
> /** No Delay Mechanism. */
> DM_NO_MECHANISM = 0xFE,
> };
> diff --git a/fd.h b/fd.h
> index 16420d7..9a072ab 100644
> --- a/fd.h
> +++ b/fd.h
> @@ -39,6 +39,7 @@ enum {
> FD_SYNC_TX_TIMER,
> FD_UNICAST_REQ_TIMER,
> FD_UNICAST_SRV_TIMER,
> + FD_CMLDS,
> FD_RTNL,
> N_POLLFD,
> };
> diff --git a/makefile b/makefile
> index 7fc5f6f..e9c1ccc 100644
> --- a/makefile
> +++ b/makefile
> @@ -30,8 +30,8 @@ TS2PHC= ts2phc.o lstab.o nmea.o serial.o sock.o
> ts2phc_generic_pps_source.o \
> ts2phc_nmea_pps_source.o ts2phc_phc_pps_source.o ts2phc_pps_sink.o
> ts2phc_pps_source.o
> OBJ= bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
> e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o monitor.o msg.o phc.o \
> - port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) \
> - sk.o stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
> + pmc_common.o port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o
> rtnl.o \
> + $(SERVOS) sk.o stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o
> unicast_client.o \
> unicast_fsm.o unicast_service.o util.o version.o
>
> OBJECTS= $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o
> pmc_agent.o \
> diff --git a/port.c b/port.c
> index 8afe1b2..75b4dd8 100644
> --- a/port.c
> +++ b/port.c
> @@ -963,7 +963,8 @@ static int port_management_fill_response(struct port
> *target,
> ptp_text_copy(cd->userDescription, >userDescription);
> buf += sizeof(struct PTPText) + cd->userDescription->length;
>
> - if (target->delayMechanism == DM_P2P) {
> + if (target->delayMechanism == DM_P2P ||
> + target->delayMechanism == DM_COMMON_P2P) {
> memcpy(buf, profile_id_p2p, PROFILE_ID_LEN);
> } else {
> struct config *cfg = clock_config(target->clock);
> @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
> p->fda.fd[i] = -1;
> }
>
> +static int port_cmlds_initialize(struct port *p)
> +{
> + struct config *cfg = clock_config(p->clock);
> + struct subscribe_events_np sen = {0};
> + const int zero_datalen = 1;
> + const UInteger8 hops = 0;
> +
> + p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
Should this fall back to port_number(p)?
> + p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> + config_get_string(cfg, p->name,
> "cmlds_client_address"),
> + config_get_string(cfg, p->name,
> "cmlds_server_address"),
> + hops,
> + config_get_int(cfg, NULL, "domainNumber"),
> + config_get_int(cfg, p->name,
> "transportSpecific") << 4,
The sdoId is (by either IEEE 1588 or 802.1AS) different for the CMLDS
than for a
