[Linuxptp-devel] TLVs and Management messages

[Geoff Salmon]

I've been working on implementing all of the management messages. The
recently added changes supporting getting data sets made me realize I
had better get patches ready soon or risk painful merges. While I'm
working on that, I want to tell people what I'm doing and get some
advice on how to proceed.

My code so far contains a few departures from how things are implemented
in linuxptp currently. These are the things I think might be contentious
in getting my changes merged upstream. If anything looks wrong, or
doesn't make sense, let me know.

TLV structs not packed:
Unlike the other parts of the message some TLVs are not fixed length. So
instead of casting the suffix field in a message to the TLV struct, I
add a linked list of TLV structs to each message, and those structs
contain the TLV's data. The TLV data gets written to and read from the
message buffer during msg_pre_send and msg_post_recv.

Supporting multiple TLVs and inserting/removing TLVs:
A side affect of not storing the TLVs packed in the message buffer is
it's easier to implement hooks to modify the TLVs on outgoing messages.
This will be handy for implementing different PTP profiles. For example
the Power Profile adds 2 TLVs to Announce messages.

X Macros for TLV structs, pack/unpack functions:
To implement reading and writing the TLVs, I've used a modified version
of the def files and X Macros used in ptpd. I also use a python script
to generate the functions and structs that #include the def files. It
saved a lot of time upfront, but now I'm often tempted to use 'gcc -E'
and clean up the resulting functions and structs. Do you have strong
feelings on accepting generated code or X Macros and def files into
linuxptp?

Those are the big things I think. I'll continue plugging away at what I
have and try to get patches ready.

- Geoff

[Richard Cochran]

Post by Geoff Salmon
I've been working on implementing all of the management messages. The
recently added changes supporting getting data sets made me realize I
had better get patches ready soon or risk painful merges. While I'm
working on that, I want to tell people what I'm doing and get some
advice on how to proceed.

No need to hurry. I am going to release 1.0 with the management
support as it is now. A more complete management interface is something
that could appear in a 1.x or a 2.0 release.

Post by Geoff Salmon
My code so far contains a few departures from how things are implemented
in linuxptp currently. These are the things I think might be contentious
in getting my changes merged upstream. If anything looks wrong, or
doesn't make sense, let me know.

It would be best to just see the patches.

Post by Geoff Salmon
Unlike the other parts of the message some TLVs are not fixed length.

Most of the TLVs are fixed length. I think there are only two or three
exceptions. These can surely be treated individually.

Post by Geoff Salmon
So
instead of casting the suffix field in a message to the TLV struct, I
add a linked list of TLV structs to each message, and those structs
contain the TLV's data. The TLV data gets written to and read from the
message buffer during msg_pre_send and msg_post_recv.

Linked list? Management messages may only have one TLV.

Post by Geoff Salmon
A side affect of not storing the TLVs packed in the message buffer is
it's easier to implement hooks to modify the TLVs on outgoing messages.
This will be handy for implementing different PTP profiles. For example
the Power Profile adds 2 TLVs to Announce messages.

I imagine these two TLVs can be added together at the same time.

Post by Geoff Salmon
To implement reading and writing the TLVs, I've used a modified version
of the def files and X Macros used in ptpd. I also use a python script
to generate the functions and structs that #include the def files. It
saved a lot of time upfront, but now I'm often tempted to use 'gcc -E'
and clean up the resulting functions and structs. Do you have strong
feelings on accepting generated code or X Macros and def files into
linuxptp?

Okay, good that you asked. In principle, I am a big fan of model based
development and generated code. But in this case, I think this is
overkill, just to pack and unpack the message fields.

I had previously looked at the xmacro stuff in ptpd, and the result
really makes my eyes bleed. Besides being just ugly, this approach has
a bunch of drawbacks, including:

- it is really hard to read this code
- requires more LOC than the straightforward approach
- does a bunch of malloc/free for no good reason
- creates maintenance burden, keeping .c and .def in sync

And what are the advantages? I can't see any. The ptpd code looks
*less* maintainable to me. Even if it were easier to add or change the
message formats using this approach, in the real world we don't see
the PTP standard or the profiles changing very rapidly.

BTW, have you noticed that neither the ptpd daemon nor the management
clients work correctly WRT management messages?

Post by Geoff Salmon
Those are the big things I think. I'll continue plugging away at what I
have and try to get patches ready.

I hope I didn't totally discourage you with my xmacro reaction. I am
really not against generated code, especially if it results in greater
simplicity, correctness, and beauty.

I did consider using a generations scheme for the TLVs (but not using
xmacros), and I came to the conclusion that it just isn't worth the
effort. There really aren't that many different management messages,
and we already have the most important ones for the GET action.

The code pattern that I started will work quite well for the rest, I
think. For example, most of the remaining, piecemeal GET/SET items can
be handled with a single struct, like this:

struct management_tlv_datum {
uint8_t val;
uint8_t reserved;
} PACKED;

and the USER_DESCRIPTION can just be a special case.

BTW, if you are interested in the power profile, an area to tackle
would be the SNMP stuff. One idea I had was to have a separate program
linked to libsnmp that talks to ptp4l via the UDS interface.

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
- requires more LOC than the straightforward approach

And if the xmacro way is really more concise, one interesting exercise
would be to replace the existing management code (supporting the same
IDs and actions) with one based on xmacro, and then look at the
diffstat.

Thanks,
Richard

[Geoff Salmon]

I'd agree the xmacro stuff is ugly. The main benefit I saw was that
creating the struct, pack and unpack functions all from a single def
file avoided any mismatches between the three. However, as you say, the
layout of these things isn't changing rapidly, so I'll remove the
xmacros and clean up the generated structs and functions.

One of the things I don't like about the current pattern of casting the
suffix field to a struct and using that to layout the messages is it
ties the layout of the linuxptp structs to PTP's wire representation.
This has a few negative consequences:

- TLVs that aren't fixed length have to be handled in some special way
- flags fields need to be packed together forcing the rest of linuxptp
to use bit masks and bit manipulation when dealing with them.
- odd-sized fields must be byte arrays making them hard to work with.
There's only one example of this is PTP spec, the Organization-specific
TLV's organizationId and organizationSubType, but profiles may introduce
more.
- can't keep additional useful info in the structs forcing nesting of
structs, ex. struct default_ds and struct defaultDS

By not tying the structs to the wire representation we'd have structs
that are less error-prone to use in the rest of linuxptp and we could
keep all the details of the wire representation inside tlv.c.

Post by Richard Cochran
Linked list? Management messages may only have one TLV.

True. Supporting a list of TLVs was for future profile support, but
that's premature. I'll change it a single pointer to a TLV, and I'll add
a single tlv allocated in the ptp_message to be used to hold a received
TLV. It won't be much effort to switch it back to a list if needed.

Post by Richard Cochran
BTW, if you are interested in the power profile, an area to tackle
would be the SNMP stuff. One idea I had was to have a separate program
linked to libsnmp that talks to ptp4l via the UDS interface.

Yeah, using a separate process for SNMP and the UDS interface seems like
the way to go.

- Geoff

[Richard Cochran]

Post by Geoff Salmon
One of the things I don't like about the current pattern of casting
the suffix field to a struct and using that to layout the messages
is it ties the layout of the linuxptp structs to PTP's wire

Casting a struct to a buffer is a common, useful pattern. For example,
the Linux networking stack is written this way.

Post by Geoff Salmon
- flags fields need to be packed together forcing the rest of
linuxptp to use bit masks and bit manipulation when dealing with
them.

Well, here is the consequence of using bit fields instead of some kind
of boolean.

clock.c: c->tds.flags = 0;
clock.c: c->tds.flags = PTP_TIMESCALE;
clock.c: c->tds.flags = msg->header.flagField[1];
clock.c: if (!(c->tds.flags & PTP_TIMESCALE)) {
clock.c: if (!(c->tds.flags & PTP_TIMESCALE))
clock.c: if (c->tds.flags & UTC_OFF_VALID && c->tds.flags & TIME_TRACEABLE) {
clock.c: return c->dds.flags & DDS_SLAVE_ONLY;

port.c: msg->header.flagField[1] = tp->flags;
port.c: msg->management.flags = RESPONSE;
port.c: msg->management.flags = ACKNOWLEDGE;

Doesn't look too bad to me.

Post by Geoff Salmon
- can't keep additional useful info in the structs forcing nesting
of structs, ex. struct default_ds and struct defaultDS

This nesting is only accidental/historical and can surely be removed
with changing the message parsing code.

Post by Geoff Salmon
By not tying the structs to the wire representation we'd have
structs that are less error-prone to use in the rest of linuxptp and
we could keep all the details of the wire representation inside
tlv.c.

We are not doing any internal storage or processing of the messages,
thus no special internal structs are needed. I really don't see a need
for duplicating the representation of the messages or of the TLVs.

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
Casting a struct to a buffer is a common, useful pattern. For example,
the Linux networking stack is written this way.

And this method has a nice advantage over blinding packing and
unpacking every field of every message: only those fields which are
actually used by the application are touched by the run time code.

Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Friday, December 14, 2012 1:06 AM
To: Geoff Salmon
Subject: Re: [Linuxptp-devel] TLVs and Management messages

Post by Geoff Salmon
One of the things I don't like about the current pattern of casting
the suffix field to a struct and using that to layout the messages
is it ties the layout of the linuxptp structs to PTP's wire

Casting a struct to a buffer is a common, useful pattern. For example,
the Linux networking stack is written this way.

Post by Geoff Salmon
- flags fields need to be packed together forcing the rest of
linuxptp to use bit masks and bit manipulation when dealing with
them.

Well, here is the consequence of using bit fields instead of some kind
of boolean.
clock.c: c->tds.flags = 0;
clock.c: c->tds.flags = PTP_TIMESCALE;
clock.c: c->tds.flags = msg->header.flagField[1];
clock.c: if (!(c->tds.flags & PTP_TIMESCALE)) {
clock.c: if (!(c->tds.flags & PTP_TIMESCALE))
clock.c: if (c->tds.flags & UTC_OFF_VALID && c->tds.flags &
TIME_TRACEABLE) {
clock.c: return c->dds.flags & DDS_SLAVE_ONLY;
port.c: msg->header.flagField[1] = tp->flags;
port.c: msg->management.flags = RESPONSE;
port.c: msg->management.flags = ACKNOWLEDGE;
Doesn't look too bad to me.

Post by Geoff Salmon
- can't keep additional useful info in the structs forcing nesting
of structs, ex. struct default_ds and struct defaultDS

This nesting is only accidental/historical and can surely be removed
with changing the message parsing code.

Post by Geoff Salmon
By not tying the structs to the wire representation we'd have
structs that are less error-prone to use in the rest of linuxptp and
we could keep all the details of the wire representation inside
tlv.c.

We are not doing any internal storage or processing of the messages,
thus no special internal structs are needed. I really don't see a need
for duplicating the representation of the messages or of the TLVs.
Thanks,
Richard

I agree with Richard.

- Jake