RFC: StreamExecutor C API

[annarev]

The feedback phase will be open for two weeks until Tuesday June 30, 2020.

Status	Accepted
RFC #	257
Author(s)	Anna Revinskaya (annarev@google.com), Penporn Koanantakool (penporn@google.com), Yi Situ (yisitu@google.com), Russell Power (power@google.com)
Sponsor	Gunhan Gulsoy (gunan@google.com)
Updated	2020-09-08

Objective

Provide basic device management C API to allow new devices to modularly connect
to the current TensorFlow runtime.

[jzhoulon]

Hi, @annarev It seems that block_host_until_done is removed, can we keep this API? block_host_until_done is a more large-grained API, it provides more flexibility for the backend to implement, and lots of device runtime already provide the capability to match this API(CUDA: cuStreamSynchronize(stream),SYCL queue.wait() ). I saw current implementation is composed of create_event+ record_event + block_host_for_event + delete event, that means each back-end needs to implements all of these API, which limits the flexibility, and I am not sure whether it will bring some performance penalty (for DPC++, we need to enqueue a barrier to retrieve the event)

[annarev]

Added here now:
https://cs.opensource.google/tensorflow/tensorflow/+/master:tensorflow/c/experimental/stream_executor/stream_executor.h;l=331?q=stream_executor.h

[annarev]

I am planning to add the following SP_DeviceDescription struct. I made some fields optional and some required. @wchao1115, @kulinseth, @jzhoulon let me know if this looks reasonable (for e.g. if some other fields should be optional as well, such as pci_bus_id or if some optional fields should be required).

typedef struct SP_DeviceDescription {
  size_t struct_size;
  
  // Device hardware name. Used for printing.
  // Must be null-terminated.
  char* name;
  
  // Device vendor name. Used for printing.
  // Must be null-terminated.
  char* device_vendor;

  // Returns the PCI bus identifier for this device, of the form
  // [domain]:[bus]:[device].[function]
  // Used for printing.
  // Must be null-terminated.
  char* pci_bus_id;

  TF_Bool has_numa_node;  // True if `numa_node` is set.
  // Returns the NUMA node associated with this device, for use in
  // determining socket locality.
  int numa_node;

  TF_Bool has_memory_bandwidth;  // True if `memory_bandwidth` is set.
  // Device's memory bandwidth in bytes/sec.  (This is for reads/writes to/from
  // the device's own memory, not for transfers between the host and device.)
  int64_t memory_bandwidth;

  TF_Bool has_gflops;  // True if `gflops` field is set.
  // Estimate of floating point operations per second for this device * 10e-9.
  double gflops;
} SP_DeviceDescription;

[kulinseth]

I am planning to add the following SP_DeviceDescription struct. I made some fields optional and some required. @wchao1115, @kulinseth, @jzhoulon let me know if this looks reasonable (for e.g. if some other fields should be optional as well, such as pci_bus_id or if some optional fields should be required).

Thanks!. This looks good to me. Just to confirm, all the versioning information is subsumed in struct_size, and we don't need any extra fields in DeviceDescription for version compatibility. Is that correct?

[annarev]

I am planning to add the following SP_DeviceDescription struct. I made some fields optional and some required. @wchao1115, @kulinseth, @jzhoulon let me know if this looks reasonable (for e.g. if some other fields should be optional as well, such as pci_bus_id or if some optional fields should be required).

Thanks!. This looks good to me. Just to confirm, all the versioning information is subsumed in struct_size, and we don't need any extra fields in DeviceDescription for version compatibility. Is that correct?

Yes, we rely on struct_size and version value in SE_PlatformRegistrationParams.

[annarev]

I updated the doc to add a way to use a custom allocator:
21be6e9
Addition of a custom allocator was requested in Pluggable Device RFC:
#262

[penpornk]

This RFC has been up for a while. The implementation has even been checked in over a month ago. I think we should wrap this up (i.e., merge this PR).

Changes to StreamExecutor C API since the last combined design review meeting with PluggableDevice on 8/13/20:

• Added more explanation of the overall functionality.
• Added more clarifications on versioning strategy.
• Corrected typos. Minor functionality additions such as block_host_until_done, unified_memory_allocate/deallocate, etc.

If anyone has anything else that needs to be discussed/resolved before this RFC can be merged (i.e., fundamental issues), please explicitly say so here by this Thursday, 9/24 at 11:59pm PT.

[kulinseth]

If anyone has anything else that needs to be discussed/resolved before this RFC can be merged (i.e., fundamental issues), please explicitly say so here by this Thursday, 9/24 at 11:59pm PT.

Thanks Penporn, for reaching out. Nothing from our side.

[penpornk]

@kulinseth Thank you for the quick confirmation! :)

@ematejska @alextp I believe there is no outstanding issue now. Could we move forward?

[alextp]

Yes, let's merge the PR.

…

On Fri, Sep 25, 2020 at 10:13 AM Penporn Koanantakool < ***@***.***> wrote: @kulinseth <https://github.com/kulinseth> Thank you for the quick confirmation! :) @ematejska <https://github.com/ematejska> @alextp <https://github.com/alextp> I believe there is no outstanding issue now. Could we move forward? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#257 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABHRL4JHCQHDI5DPSTMLLSHTFTRANCNFSM4N4SSC6Q> .

-- - Alex

[theadactyl]

LGTM

[wdfnst]

@penpornk Hi, is there any PluggableDevice template for beginner? TKS