core: Add support for traffic class selection #5148

jswaro · 2019-07-01T19:11:13Z

This commit introduces the capability to select a traffic class when
creating libfabric objects with the goal to allow applications to
utilize traffic shaping mechanisms exposed to providers.

In this proposal, the fi_domain, fi_tx_attr, and fi_rx_attributes are
modified to add the new fi_traffic_class structure to the structure definition.
The fi_traffic_class structure provides a 'type' field, indicating the type of
traffic class value that is represented in the 'value' field of the structure.
Two different types of traffic class are presented with this commit; FI_TC_TYPE_LABEL,
and FI_TC_TYPE_DSCP. Libfabric defines generalized traffic classes via community
defined labels and expected behavior for those labels, which fall under the category
of FI_TC_TYPE_LABEL. Providers may also define provider specific labels utilizing the
most significant 32 bits of the value field. To provide applications with greater
control over traffic class selection, this proposal exposes the ability to specify
a specify DSCP value to be associated with libfabric objects, using the value field set
to the DSCP value, and type set to FI_TC_TYPE_DSCP.

Additionally, to simplify the traffic class selection process, this commit introduces
a heirarchal inheritance model with the fi_traffic_class structure. When an fi_domain
is created, the application can choose to select a traffic class, or allow the provider
to select a default traffic class. The traffic class associated with the domain is used
as the default for all objects created within the domain. This allows an
application to request a general traffic class for most traffic flows, while
allowing the application to finely control traffic at the granularity of the tx context
or rx context.

The original proposal to OFIWG did not include the rx context modification. However,
multi-part data transfer operations such as rendezvous messaging benefit from the differentiation of
request and response traffic shaping flows. The traffic class field in the rx attribute is
only intended to apply to these multi-part data transfer operations and does not imply any
relation with the tx attribute of a peer endpoint.

Signed-off-by: James Swaro jswaro@cray.com

jswaro · 2019-07-01T19:12:08Z

@shefty Who else should we add to this PR as reviewers? This is a first pass at traffic class support for libfabric based on the previous OFIWG meetings.

shefty · 2019-07-01T19:20:36Z

I would post an email to the ofiwg mail list and point people to this PR. Then, anyone interested can come have a look.

jswaro · 2019-07-01T19:32:02Z

Done

include/rdma/fabric.h

include/rdma/fi_endpoint.h

man/fi_domain.3.md

include/rdma/fabric.h

man/fi_domain.3.md

include/rdma/fabric.h

man/fi_domain.3.md

man/fi_endpoint.3.md

jswaro · 2019-07-08T19:08:59Z

I do plan to squash this into a single commit when review is complete. The small commits are meant to show how the feedback is addressed until then. Just a heads up.

shefty · 2019-07-08T19:18:12Z

I'm basically reviewing the merged result only, so whichever is easier for you. I agree this should be squashed when merging.

include/rdma/fabric.h

man/fi_endpoint.3.md

This commit introduces the capability to select a traffic class when creating libfabric objects with the goal to allow applications to utilize traffic shaping mechanisms exposed to providers. In this proposal, the fi_domain, fi_tx_attr, and fi_rx_attributes are modified to add the new fi_traffic_class structure to the structure definition. The fi_traffic_class structure provides a 'type' field, indicating the type of traffic class value that is represented in the 'value' field of the structure. Two different types of traffic class are presented with this commit; FI_TC_TYPE_LABEL, and FI_TC_TYPE_DSCP. Libfabric defines generalized traffic classes via community defined labels and expected behavior for those labels, which fall under the category of FI_TC_TYPE_LABEL. Providers may also define provider specific labels utilizing the most significant 32 bits of the value field. To provide applications with greater control over traffic class selection, this proposal exposes the ability to specify a specify DSCP value to be associated with libfabric objects, using the value field set to the DSCP value, and type set to FI_TC_TYPE_DSCP. Additionally, to simplify the traffic class selection process, this commit introduces a heirarchal inheritance model with the fi_traffic_class structure. When an fi_domain is created, the application can choose to select a traffic class, or allow the provider to select a default traffic class. The traffic class associated with the domain is used as the default for all objects created within the domain. This allows an application to request a general traffic class for most traffic flows, while allowing the application to finely control traffic at the granularity of the tx context or rx context. The original proposal to OFIWG did not include the rx context modification. However, multi-part data transfer operations such as rendezvous messaging benefit from the differentiation of request and response traffic shaping flows. The traffic class field in the rx attribute is only intended to apply to these multi-part data transfer operations and does not imply any relation with the tx attribute of a peer endpoint. Signed-off-by: James Swaro <jswaro@cray.com>

jswaro · 2019-09-12T18:31:15Z

@shefty Ready for another round of reviews.

shefty

i think this is close. thanks

include/rdma/fabric.h

shefty · 2019-10-04T19:35:37Z

FYI - I'm pulling this into my tree to make some updates and will open a new PR. I want to get this merged soon, so it's ready before 1.9rc1.

jswaro · 2019-10-04T19:37:07Z

Before you merge it, I'll squash that last commit.

shefty · 2019-10-04T19:38:52Z

I was going to squash it.

jswaro · 2019-10-04T19:39:40Z

Ok, all yours. Would you mind copying me to the new PR?

shefty · 2019-10-04T19:42:37Z

Yes. I plan on tagging you and requesting your review.

shefty

See PR #5334. Posting comments that I had written as part of review. Those have been addressed in the updated PR.

man/fi_domain.3.md

shefty · 2019-10-04T19:24:34Z

man/fi_domain.3.md

+info->domain_attr.traffic_class = FI_TC_DSCP_SET(0x2e);
+ret = fi_domain(fabric, info, &domain, context);
+assert(ret == FI_SUCCESS);
+```


This is a sizeable amount of example code. It would be better to have a fabtests example than this much code added to the man page for a single field in the attribute structures. This is particularly true since the use of the field is straightforward and aligns with other attribute fields.

shefty · 2019-10-04T19:25:31Z

man/fi_domain.3.md

+traffic is considered to be undifferentiated.
+
+Traffic classes can be any DSCP value, any of the following defined classes of
+traffic, or any provider-specific definitions.


The use of FI_TC_DSCP_SET/GET need to be defined here.

shefty · 2019-10-04T19:26:04Z

src/abi_1_0.c

+        size_t                  size;
+        size_t                  iov_limit;
+        size_t                  rma_iov_limit;
+        uint32_t                tclass;


This is not a 1.2 field

I'm slightly confused. Perhaps you can walk me through this, but I thought 1.2 referred to the ABI version, not the API version.

shefty · 2019-10-04T19:26:12Z

src/abi_1_0.c

+        size_t                  rx_ctx_cnt;
+        size_t                  auth_key_size;
+        uint8_t                 *auth_key;
+        uint32_t                tclass;


Not a 1.2 field

shefty · 2019-10-04T19:26:22Z

src/abi_1_0.c

+        size_t                  auth_key_size;
+        size_t                  max_err_data;
+        size_t                  mr_cnt;
+        uint32_t                tclass;


not a 1.2 field. All of the above structures should be the exact structure that was defined prior to any changes being made. You can use the 1.8 release structures, for example.

shefty · 2019-10-04T19:28:22Z

man/fi_domain.3.md

+traffic, or any provider-specific definitions.
+
+All values other than 0 and less than FI_TC_LABEL are treated as DSCP values in
+the traffic class field.


I don't think this statement is true

Right, we changed that. Nice catch.

shefty · 2019-10-04T19:29:23Z

man/fi_domain.3.md

+
+*FI_TC_REALTIME_STREAM*
+: This class is used for data that requires dedicated bandwidth and little
+  jitter.


This should be removed, since it was removed from the API.

shefty · 2019-10-04T19:32:34Z

include/rdma/fabric.h

@@ -316,6 +316,22 @@ enum {
 	FI_PROTO_EFA
 };

+#define FI_TC_PROV_SPECIFIC (1UL << 31)


FI_PROV_SPECIFIC is already defined by the API. No need for an overlapping definition.

jswaro · 2019-10-07T15:14:34Z

@shefty : Are we still modifying this PR with #5334 open?

shefty · 2019-10-07T16:18:35Z

I think we can close this and update 5334. I left the comments here, so you could see why I made the changes I did in 5334. Everything has been addressed there. So please review that PR and respond with any issues.

jswaro · 2019-10-07T16:19:34Z

Closed. Follow up with #5334

jswaro requested review from shefty and a-ilango July 1, 2019 19:11

shefty reviewed Jul 2, 2019

View reviewed changes

include/rdma/fabric.h Outdated Show resolved Hide resolved

shefty reviewed Jul 8, 2019

View reviewed changes

man/fi_domain.3.md Outdated Show resolved Hide resolved

shefty reviewed Jul 8, 2019

View reviewed changes

jswaro force-pushed the feature/traffic_classes branch from a47a04f to 9822769 Compare August 5, 2019 18:13

shefty reviewed Aug 29, 2019

View reviewed changes

include/rdma/fabric.h Outdated Show resolved Hide resolved

shefty reviewed Sep 3, 2019

View reviewed changes

man/fi_endpoint.3.md Outdated Show resolved Hide resolved

jswaro force-pushed the feature/traffic_classes branch from 4bd4466 to 847d697 Compare September 11, 2019 15:03

jswaro mentioned this pull request Sep 11, 2019

prov/verbs: feature request - support for defining PKEYs and SL #5263

Closed

shefty reviewed Sep 19, 2019

View reviewed changes

include/rdma/fabric.h Outdated Show resolved Hide resolved

include/rdma/fabric.h Outdated Show resolved Hide resolved

removed tclass from ep and realtime_stream enum

d485062

shefty mentioned this pull request Oct 4, 2019

core: Add support for traffic class selection #5334

Merged

shefty reviewed Oct 4, 2019

View reviewed changes

jswaro closed this Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Add support for traffic class selection #5148

core: Add support for traffic class selection #5148

jswaro commented Jul 1, 2019

jswaro commented Jul 1, 2019

shefty commented Jul 1, 2019

jswaro commented Jul 1, 2019

jswaro commented Jul 8, 2019

shefty commented Jul 8, 2019

jswaro commented Sep 12, 2019

shefty left a comment

shefty commented Oct 4, 2019

jswaro commented Oct 4, 2019

shefty commented Oct 4, 2019 via email

jswaro commented Oct 4, 2019

shefty commented Oct 4, 2019

shefty left a comment

shefty Oct 4, 2019

shefty Oct 4, 2019

shefty Oct 4, 2019

jswaro Oct 7, 2019

shefty Oct 4, 2019

shefty Oct 4, 2019

shefty Oct 4, 2019

jswaro Oct 7, 2019

shefty Oct 4, 2019

jswaro Oct 7, 2019

shefty Oct 4, 2019

jswaro commented Oct 7, 2019

shefty commented Oct 7, 2019

jswaro commented Oct 7, 2019

core: Add support for traffic class selection #5148

core: Add support for traffic class selection #5148

Conversation

jswaro commented Jul 1, 2019

jswaro commented Jul 1, 2019

shefty commented Jul 1, 2019

jswaro commented Jul 1, 2019

jswaro commented Jul 8, 2019

shefty commented Jul 8, 2019

jswaro commented Sep 12, 2019

shefty left a comment

Choose a reason for hiding this comment

shefty commented Oct 4, 2019

jswaro commented Oct 4, 2019

shefty commented Oct 4, 2019 via email

jswaro commented Oct 4, 2019

shefty commented Oct 4, 2019

shefty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jswaro commented Oct 7, 2019

shefty commented Oct 7, 2019

jswaro commented Oct 7, 2019