Logger callback function sink #1748

martygrant · 2024-06-12T09:54:36Z

This PR implements #1330 through a new logger sink: a user configurable callback. It introduces some spec additions:

typedef void (*ur_logger_output_callback_t)(ur_logger_level_t level, const char *pLoggerMsg, void *pUserData)
urSetLoggerCallback(ur_adapter_handle_t hAdapter, ur_logger_output_callback_t pfnLoggerCallback, void *pUserData, ur_logger_level_t level
)`
urSetLoggerCallbackLevel(ur_adapter_handle_t hAdapter, ur_logger_level_t level)
typedef enum ur_logger_level_t (moved the logger::level enum into the spec)

This new logger sink will only be constructed once a user makes a call to urSetLoggerCallback, supplying their own callback function. They can set the minimum logging level through urSetLoggerCallbackLevel. Any subsequent logging calls will additionally make a call to the supplied callback where the log level, message and user data will be sent.

A new test suite LoggerWithCallbackSink has been added to test this new functionality.

intel/llvm PR: intel/llvm#14422

scripts/core/loader.yml

test/unit/logger/logger.cpp

pbalcer · 2024-06-12T13:32:10Z

source/loader/ur_lib.cpp

+        return UR_RESULT_ERROR_INVALID_NULL_POINTER;
+    }
+
+    logger::get_logger().setCallbackSinkFunction(pfnLoggerCallback);


There is more than one logger. At the very least, each adapter will have its own logger instance, so this API needs to be in DDI and propagated to all adapters.

I meant to highlight this as a question because I wasn't sure about how to get the callback function pointer into the logger as get_logger takes a name parameter. I can't see any loader references in DDI tables, would I need to add one for the loader or should the entry point be part of another group maybe?

This is not really a loader API, so I'd call this something like urSetLoggerCallback and remove loader_only from the spec. This will generate a DDI function where you can implement this for each adapter. And each adapter will then need to have the logger::get_logger().setSink() call in their implementation. Because the instance returned by logger::get_logger() is different for each adapter.

Additionally, if you want the application to be able to also get callbacks on loader-related messages, you'd need to make an if in the template file to call logger::get_logger().setSink() at the loader layer, before the function is forwarded to the adapter-specific implementation.

I've updated this as per your comments here and our conversation on Teams, each adapter implements this new entry point now. Thanks for your help!

pbalcer · 2024-06-12T13:37:27Z

If the idea is for the SYCL to hoist UR logs and print them using its own facility, wouldn't it make sense for urLoaderConfigSetLoggerCallback to override (or be in addition to) the default sink so that the users only ever need to interact with SYCL interfaces?

martygrant · 2024-06-12T15:56:32Z

@pbalcer thanks for your comments! I'm not sure about this one maybe you can clarify?

If the idea is for the SYCL to hoist UR logs and print them using its own facility, wouldn't it make sense for urLoaderConfigSetLoggerCallback to override (or be in addition to) the default sink so that the users only ever need to interact with SYCL interfaces?

I think you mean so the user at the SYCL level doesn't need to interact with UR directly right? I'm not sure if that would be possible for this case as the callback would need to be passed through programmatically?

pbalcer · 2024-06-12T16:20:06Z

@pbalcer thanks for your comments! I'm not sure about this one maybe you can clarify?

If the idea is for the SYCL to hoist UR logs and print them using its own facility, wouldn't it make sense for urLoaderConfigSetLoggerCallback to override (or be in addition to) the default sink so that the users only ever need to interact with SYCL interfaces?

I think you mean so the user at the SYCL level doesn't need to interact with UR directly right? I'm not sure if that would be possible for this case as the callback would need to be passed through programmatically?

The way you implemented it right now, for a SYCL end-user (I don't mean developer of SYCL runtime) to receive any sort of messages from the UR layer, they'd have to set UR_LOG_FOO=callback env variable. If the plan for the callback is to pass the responsibility for emitting logs to SYCL, it would make more sense to me if SYCL was also responsible for controlling the verbosity of those logs, using whatever methods that exist today. This way, end-users interact only with the highest level of abstraction (SYCL), not having to be aware of the underlying implementation (UR).

But for that to be possible, the set callback function needs to always work, independent of the UR-level log setting.

pbalcer · 2024-06-12T16:25:40Z

scripts/core/loader.yml

+      desc: "[in] Function pointer to callback from the logger."
+    - type: void*
+      name: pUserData
+      desc: "[in][out][optional] pointer to data to be passed to callback"


The way this is implemented right now is not thread safe with respect to any other API call. This should be stated clearly in the docs.
(Ideally we'd figure out how to make it thread-safe though, but I don't see a simple way other than having a mutex around the logger, and that's just iffy).

I've updated the description here to say the creator of the callback function should be responsible for making it thread-safe.

include/ur_api.h

scripts/core/adapter.yml

scripts/core/registry.yml

omarahmed1111 · 2024-07-10T10:57:36Z

There is a workaround for warnings for now in this PR. and it is probably going to be merged first so could you delete it in this PR once merged as this should fix it properly @martygrant? Thanks!

Edit: After a rethought that would cause e2e test failures as this will be merged after the deleting PI PR. So, we should attach the deletion of the workaround with changing the adapters to use this new logger mechanism and not in this PR. I added an issue for more context on this here.

source/common/logger/ur_sinks.hpp

pbalcer · 2024-07-10T14:24:22Z

include/ur_api.h

+/// @brief Set a callback function for use by the logger to retrieve logging output.
+///        It is a requirement that the callback function is thread safe and the
+///        creator of the function will be responsible for this.
+///


One more thing to document is that calling this, urAdapterSetLoggerCallback, and urAdapterSetLoggerCallbackLevel functions is not thread-safe with respect to any other function in the API. The reason is that they are modifying global state without using atomics or grabbing a lock. In other words, when something is writing a log, you may inadvertently change the logger callback pointer.

Okay no problem, I reworded the description for urAdapterSetLoggerCallback but will add it to urAdapterSetLoggerCallbackLevel as well. It should specifically say not "thread safe with respect to any other function in the API"? Should the explanation be added here or is that enough?

Most people understand thread-safe functions as safe to be invoked, with respect to themselves, concurrently by multiple threads. And that is how POSIX defines it. For example, you can't call strtok() from multiple threads and expect it to work correctly, but you can call strtok() in one thread, and e.g., write() in another. In this case, the situation is a bit different, since urAdapterSetLoggerCallback is not safe to call even with respect to other UR API functions (that are otherwise thread-safe). Small caveat is that implementations may choose to limit this unsafety to be per-adapter.

In my opinion this break the principle of least surprise, and should be thoroughly documented. Saying "not thread-safe" is not enough.

Ideally we'd fix this - one approach would be to only allow setting the callback as part of the loader config (and then somehow pass that callback to the adapter during initialization... won't be trivial), and then change the logger level variable to be an atomic.

Thanks Piotr. @kbenzie do you have any thoughts on this? Would we want to modify the design now or just get this PR in as it is now and modify it later? If the latter then I'll move these comments to a new issue to track this.

Lets revisit making this decision until after intel/llvm#14145 is merged.

…back function. Configurable through two new entry points with adapter implementations: urAdapterSetLoggerCallback() and urAdapterSetLoggerCallbackLevel(). Moved logger::level enum to the spec, named ur_logger_level_t. Added new unit test suite for these entry points.

Co-authored-by: Kenneth Benzie (Benie) <[email protected]>

…Move entry points from global ddi table to adapter. Rename the callback function pointer to ur_logger_callback_t.

kbenzie reviewed Jun 12, 2024

View reviewed changes

scripts/core/loader.yml Outdated Show resolved Hide resolved

github-actions bot added loader Loader related feature/bug common Changes or additions to common utilities specification Changes or additions to the specification labels Jun 12, 2024

pbalcer reviewed Jun 12, 2024

View reviewed changes

martygrant force-pushed the martin/loggerCallbackSink branch from e060f14 to 31d4486 Compare June 12, 2024 15:50

pbalcer reviewed Jun 12, 2024

View reviewed changes

martygrant mentioned this pull request Jun 17, 2024

Add checks to logger unit tests #1764

Closed

github-actions bot added level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues native-cpu Native CPU adapter specific issues labels Jun 19, 2024

martygrant force-pushed the martin/loggerCallbackSink branch from b006f18 to 1151162 Compare July 3, 2024 13:59

github-actions bot added the sanitizer Sanitizer layer issues/changes/specification label Jul 3, 2024

martygrant force-pushed the martin/loggerCallbackSink branch 3 times, most recently from 7678217 to a8081b5 Compare July 3, 2024 15:30

martygrant mentioned this pull request Jul 3, 2024

[UR] Logger callback function sink intel/llvm#14422

Draft

martygrant marked this pull request as ready for review July 3, 2024 16:35

martygrant requested review from a team as code owners July 3, 2024 16:35

martygrant requested a review from mmoadeli July 3, 2024 16:35

martygrant changed the title ~~WIP Logger callback function sink~~ Logger callback function sink Jul 3, 2024

frasercrmck reviewed Jul 3, 2024

View reviewed changes

include/ur_api.h Outdated Show resolved Hide resolved

martygrant force-pushed the martin/loggerCallbackSink branch from a8081b5 to 808e73a Compare July 3, 2024 16:48

kbenzie reviewed Jul 4, 2024

View reviewed changes

scripts/core/adapter.yml Show resolved Hide resolved

kbenzie reviewed Jul 4, 2024

View reviewed changes

scripts/core/adapter.yml Outdated Show resolved Hide resolved

scripts/core/adapter.yml Show resolved Hide resolved

scripts/core/adapter.yml Outdated Show resolved Hide resolved

scripts/core/adapter.yml Outdated Show resolved Hide resolved

martygrant force-pushed the martin/loggerCallbackSink branch 2 times, most recently from 3cfc04c to b4a2faf Compare July 5, 2024 15:06

kbenzie reviewed Jul 5, 2024

View reviewed changes

scripts/core/registry.yml Outdated Show resolved Hide resolved

martygrant force-pushed the martin/loggerCallbackSink branch 3 times, most recently from 7c297fc to 8a04428 Compare July 10, 2024 09:56

omarahmed1111 approved these changes Jul 10, 2024

View reviewed changes

pbalcer approved these changes Jul 10, 2024

View reviewed changes

source/common/logger/ur_sinks.hpp Show resolved Hide resolved

pbalcer reviewed Jul 10, 2024

View reviewed changes

omarahmed1111 mentioned this pull request Jul 10, 2024

Delete the workaround for handling warnings inside the loader #1851

Open

pbalcer approved these changes Jul 12, 2024

View reviewed changes

martygrant force-pushed the martin/loggerCallbackSink branch from 8a04428 to 1cf3c31 Compare August 23, 2024 10:00

martygrant force-pushed the martin/loggerCallbackSink branch from 1cf3c31 to d8bf577 Compare September 9, 2024 11:53

martygrant and others added 3 commits October 17, 2024 16:10

Apply suggestions from code review

02c0aa0

Co-authored-by: Kenneth Benzie (Benie) <[email protected]>

Rerun generate target after applying suggestions from Github web ui. …

8936426

…Move entry points from global ddi table to adapter. Rename the callback function pointer to ur_logger_callback_t.

martygrant force-pushed the martin/loggerCallbackSink branch from d8bf577 to 8936426 Compare October 17, 2024 15:11

Update tracing mako template to account to use new logger level enum.

e592c3d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logger callback function sink #1748

Logger callback function sink #1748

martygrant commented Jun 12, 2024 •

edited

Loading

pbalcer Jun 12, 2024

martygrant Jun 12, 2024

pbalcer Jun 12, 2024 •

edited

Loading

martygrant Jul 4, 2024

pbalcer commented Jun 12, 2024

martygrant commented Jun 12, 2024

pbalcer commented Jun 12, 2024

pbalcer Jun 12, 2024

martygrant Jul 4, 2024

omarahmed1111 commented Jul 10, 2024 •

edited

Loading

pbalcer Jul 10, 2024 •

edited

Loading

martygrant Jul 10, 2024

pbalcer Jul 10, 2024 •

edited

Loading

martygrant Jul 15, 2024

kbenzie Jul 15, 2024

Logger callback function sink #1748

Are you sure you want to change the base?

Logger callback function sink #1748

Conversation

martygrant commented Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbalcer Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbalcer commented Jun 12, 2024

martygrant commented Jun 12, 2024

pbalcer commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarahmed1111 commented Jul 10, 2024 • edited Loading

pbalcer Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbalcer Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martygrant commented Jun 12, 2024 •

edited

Loading

pbalcer Jun 12, 2024 •

edited

Loading

omarahmed1111 commented Jul 10, 2024 •

edited

Loading

pbalcer Jul 10, 2024 •

edited

Loading

pbalcer Jul 10, 2024 •

edited

Loading