NFD

I like short titles and the word "signaling" :)

The design sounds good to me. Can someone else have a look?

I agree with the protocol changes proposed in note-3. I don't think adding BaseCongestionMarkingInterval and DefaultCongestionThreshold to FaceEventNotification is necessary or useful, but I'm not against it either.

In addition to note-3, I was considering whether adding an nfd.conf setting to globally enable or disable congestion marking would be useful. I think it would, because NFD's management API cannot manipulate Unix faces, and is not practical for on-demand faces in general. Flags specified on nfdc command line would still override the global setting.

Davide Pesavento wrote:

In addition to note-3, I was considering whether adding an nfd.conf setting to globally enable or disable congestion marking would be useful. I think it would, because NFD's management API cannot manipulate Unix faces, and is not practical for on-demand faces in general. Flags specified on nfdc command line would still override the global setting.

Yeah, that's a great idea.

Klaus Schneider wrote:

Davide Pesavento wrote:

In addition to note-3, I was considering whether adding an nfd.conf setting to globally enable or disable congestion marking would be useful. I think it would, because NFD's management API cannot manipulate Unix faces, and is not practical for on-demand faces in general. Flags specified on nfdc command line would still override the global setting.

Yeah, that's a great idea.

I like this idea as well. I assume it would be in addition to the management interface above?

Also, I have a few minor corrections/changes to the design that I'll add in a minute.

Eric Newberry wrote:

I like this idea as well. I assume it would be in addition to the management interface above?

yes

The design in note 3 has been updated. Here is a summary of the changes:

• Allow BaseCongestionMarkingInterval and DefaultCongestionThreshold to be set even if marking is disabled or disallowed on the transport type
• Only allow CongestionMarkingEnabled to be "yes" (value=1) when using a TCP, UDP, or Unix transport. (Unix is included to allow it to be displayed as enabled when this is done through nfd.conf).

By default, this feature will be disabled (value=0).

What does "by default" mean in this context? There needn't be a default for a face attribute.

A value of yes is only valid when the remote FaceUri scheme is "udp4", "udp6", "tcp4", "tcp6", or "unix". (Unix transports cannot be controlled by management, so they are excluded from this list).

The second sentence contradicts the first one. Also note that you're mixing protocol definition with implementation-specific behavior. The protocol doesn't need to say which attribute values are valid on which face types; the fact that Ethernet faces don't support marking is merely an implementation limitation (similarly, the protocol doesn't say that e.g. Unix faces cannot be permanent).

My design in note 3 has been updated to address Davide's comments in note 16.

If unset, the corresponding link service options will use the default value set in the GenericLinkService::Options constructor.

Mentioning "link service" and the constructor is also an implementation detail. Either choose a fixed default value (like FacePersistency does), or don't say anything (implying that the default is implementation-defined).

Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceStatus block.
Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceEventNotification block.

Are these optional or required?

Davide Pesavento wrote:

If unset, the corresponding link service options will use the default value set in the GenericLinkService::Options constructor.

Mentioning "link service" and the constructor is also an implementation detail. Either choose a fixed default value (like FacePersistency does), or don't say anything (implying that the default is implementation-defined).

Ok, I removed this point.

Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceStatus block.
Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceEventNotification block.

Are these optional or required?

Required.

To avoid blocking/delaying testbed deployment, I suggest splitting this issue in (at least) two commits: the first commit introduces the configuration file option to globally enable/disable marking, while the second commit implements the rest (face management + nfdc). The second commit can be further split into as many commits as you see fit. Testbed deployment needs only the first commit. Eventually, there can be a third (or n-th) commit that flips the default allowCongestionMarking from false to true.

Davide Pesavento wrote:

To avoid blocking/delaying testbed deployment, I suggest splitting this issue in (at least) two commits: the first commit introduces the configuration file option to globally enable/disable marking, while the second commit implements the rest (face management + nfdc). The second commit can be further split into as many commits as you see fit. Testbed deployment needs only the first commit. Eventually, there can be a third (or n-th) commit that flips the default allowCongestionMarking from false to true.

I've pushed a commit implementing the config file option method.

The config file option commit has been merged. The face management + nfdc commit is in the works at the moment.

Per Davide's suggestion in note 10, I have removed BaseCongestionMarkingInterval and DefaultCongestionThreshold from the FaceEventNotification block.

I added the design in note 3 to FaceMgmt, including assigning TLV-TYPEs for BaseCongestionMarkingInterval and DefaultCongestionThreshold.

Eric Newberry wrote:

I added the design in note 3 to FaceMgmt, including assigning TLV-TYPEs for BaseCongestionMarkingInterval and DefaultCongestionThreshold.

The description of BaseCongestionMarkingInterval doesn't explain what it means or how it's used.

Davide Pesavento wrote:

Eric Newberry wrote:

I added the design in note 3 to FaceMgmt, including assigning TLV-TYPEs for BaseCongestionMarkingInterval and DefaultCongestionThreshold.

The description of BaseCongestionMarkingInterval doesn't explain what it means or how it's used.

Fixed.

In FaceMgmt rev78, it is unclear what’s the semantics of omitted BaseCongestionMarkingInterval and DefaultCongestionThreshold in requests and responses.

Junxiao Shi wrote:

In FaceMgmt rev78, it is unclear what’s the semantics of omitted BaseCongestionMarkingInterval and DefaultCongestionThreshold in requests and responses.

This has been addressed in r79.

Eric Newberry wrote:

This has been addressed in r79.

r79 says:

the omitted static properties are left at the default values set by the link service.

"link service" is an implementation detail of NFD and should not be mentioned in the protocol spec. Just say that an implementation-defined default value is used.

If BaseCongestionMarkingInterval and DefaultCongestionThreshold are omitted in the response ControlParameters, this indicates that congestion marking is not supported

How do you intend to implement this?

If BaseCongestionMarkingInterval or DefaultCongestionThreshold are omitted in the ControlParameters sent with the creation command, the omitted static properties are left at the default values set by the link service.

This rule causes these fields to take on a default value and becomes non-empty. In other words, there exists a ControlParameters with filled values that is equivalent to a ControlParameters with these fields omitted,

if a static property specified in the request is not acceptable, the command fails with StatusCode 406.

Congestion mark parameters are static properties.
A face not supporting congestion marks cannot accept any value of these static properties.
Therefore, it becomes impossible to create a face if it does not support congestion marks.

Do not explain or argue here. Instead, make the protocol non ambiguous. Suggestion: qualify the first quote above to indicate that a default value presents only if the face supports congestion marks.

Flags: contains effective values for LocalFieldsEnabled and LpReliabilityEnabled bits.

Why not include “CongestionMarkingEnabled” bit as well?

Junxiao Shi wrote:

Congestion mark parameters are static properties.
A face not supporting congestion marks cannot accept any value of these static properties.
Therefore, it becomes impossible to create a face if it does not support congestion marks.

Why not? Just because some static properties are set does not mean they have to be utilized.

Why not? Just because some static properties are set does not mean they have to be utilized.

If the properties are set, the response shall contain them regardless of whether they are utilized.

If the response does not carry them, the default on request shall be “empty for faces not supporting congestion marking, implementation-defined for faces supporting congestion marking”. Setting these values to non-empty on a face not supporting congestion marking shall cause 406.

Junxiao Shi wrote:

Why not? Just because some static properties are set does not mean they have to be utilized.

If the properties are set, the response shall contain them regardless of whether they are utilized.

Yes, this is what I'm saying.

If the response does not carry them, the default on request shall be “empty for faces not supporting congestion marking, implementation-defined for faces supporting congestion marking”. Setting these values to non-empty on a face not supporting congestion marking shall cause 406.

Why does this cause a 406 if they can still be set on the face? Above you said a 406 would only be caused if the parameters are not acceptable. These parameters are perfectly acceptable, although they would not be used.

The management protocol has been updated in r80.

If the properties are set, the response shall contain them regardless of whether they are utilized.

Yes, this is what I'm saying.

Then why are BaseCongestionMarkingInterval and DefaultCongestionThreshold declared as “optional” in faces/create command response?

Junxiao Shi wrote:

If the properties are set, the response shall contain them regardless of whether they are utilized.

Yes, this is what I'm saying.

Then why are BaseCongestionMarkingInterval and DefaultCongestionThreshold declared as “optional” in faces/create command response?

Because an implementation of the management protocol is not required to support them, hence it should not be required to include them in the response.

Eric Newberry wrote:

The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.

Yes, we want it enabled by default. Plus the NFD devguide needs to be updated.

Davide Pesavento wrote:

Eric Newberry wrote:

The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.

Yes, we want it enabled by default. Plus the NFD devguide needs to be updated.

Should it be enabled by default in the GenericLinkService::Options constructor or in the individual protocol factories? I would assume the latter since not all transport types support it.

Eric Newberry wrote:

Davide Pesavento wrote:

Eric Newberry wrote:

The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.

Yes, we want it enabled by default. Plus the NFD devguide needs to be updated.

Should it be enabled by default in the GenericLinkService::Options constructor or in the individual protocol factories? I would assume the latter since not all transport types support it.

I suppose in the factories and in the configuration file (which takes precedence anyway). At that point the default in GenericLinkService::Options won't matter much...

Eric Newberry wrote:

Commit enabling congestion marking by default on supported faces has been pushed to Gerrit.

This change has been merged. I think this issue is completed now.

Eric Newberry wrote:

Eric Newberry wrote:

Commit enabling congestion marking by default on supported faces has been pushed to Gerrit.

This change has been merged. I think this issue is completed now.

Yeah looks completed to me. Good job!

What about the documentation? The NFD devguide has been neglected way too much lately.

Davide Pesavento wrote:

What about the documentation? The NFD devguide has been neglected way too much lately.

I agree something should be written about this in the devguide, but I think that's a separate issue.

Eric Newberry wrote:

but I think that's a separate issue.

We've usually handled documentation updates in the same issue as the implementation, but feel free to do whatever, I don't have any preference.

Davide Pesavento wrote:

Eric Newberry wrote:

but I think that's a separate issue.

We've usually handled documentation updates in the same issue as the implementation, but feel free to do whatever, I don't have any preference.

From what I understand, the devguide updates would be about congestion control in general, and not just the management updates. Therefore, I think it would be better suited for #1624. Just my thoughts.

Makes sense.

I added #4504 for the documentation.