Add API dnnl_threadpool_interop_set_scratchpad_concurrency - #316
Add API dnnl_threadpool_interop_set_scratchpad_concurrency#316sunxiaoxia2022 wants to merge 2 commits into
Conversation
| CHECK(brgemm_convolution_bwd_utils::init_conf(jcp_, isa, desc_, | ||
| diff_dst_md_, weights_md_, diff_src_md_, bias_md_, attr_, | ||
| dnnl_get_max_threads())); | ||
| dnnl_get_max_threads_for_scratchpad())); |
There was a problem hiding this comment.
Consider using dnnl_get_current_num_threads() instead if I understand what this PR tries to solve.
There was a problem hiding this comment.
I think dnnl_get_current_num_threads() would be incorrect here because this path is used during init_conf()/scratchpad estimation, outside an active parallel region. In the threadpool runtime that can evaluate to 1 when no active threadpool is bound, which would underestimate scratchpad needs. Here we need the scratchpad-specific assumed concurrency, so dnnl_get_max_threads_for_scratchpad() is intentional.
There was a problem hiding this comment.
Then I don't understand the technical problem in the first place. Why using dnnl_threadpool_interop_set_max_concurrency doesn't help to propagate the needed number of threads in here which gets accounted through original dnnl_get_max_threads()?
There was a problem hiding this comment.
I updated the root cause of this issue in description. Please take a look.
There was a problem hiding this comment.
Thank you for the updated description.
It looks like that the primitive allocates scratchpad per thread and the number of threads was overestimated due to the user call.
I believe the proposed solution is unreliable as scratchpad is not directly controlled by the user and tipping on how many threads will be actually used over the number passed through API as real worker-thread * 32 seems to me rather a hack than an actual fix.
Besides, the fix in its current way won't make to the upstream version and will live in this limbo forever which doesn't sound like a good resolution to me. I'm probably fine with this solution as a very temporary fix but I recommend to look into a longer-term resolution, for example thread-agnostic scratchpad allocation.
| CHECK(brgemm_convolution_bwd_utils::init_conf(jcp_, isa, desc_, | ||
| diff_dst_md_, weights_md_, diff_src_md_, bias_md_, attr_, | ||
| dnnl_get_max_threads())); | ||
| dnnl_get_max_threads_for_scratchpad())); |
There was a problem hiding this comment.
Should all scratchpad-related sizing paths that currently use dnnl_get_max_threads() be switched to dnnl_get_max_threads_for_scratchpad() as well? Right now only jit_brgemm_conv_bwd_strided.cpp is updated.
There was a problem hiding this comment.
I think a full migration is much larger than replacing one API call. There are many scratchpad-sizing paths across different primitives and runtimes. To keep risk manageable and unblock the current bug, this PR applies a minimal targeted fix.
There was a problem hiding this comment.
Agreed that this PR is a temporary fix for the concrete issue at hand.
liubo-intel
left a comment
liubo-intel
left a comment
There was a problem hiding this comment.
@sunxiaoxia2022 could you please also fix the CI failed Clang-Format issue?
3 participants
Description
Problem
On CPU with adaptive threading, model load could fail in deconvolution descriptor creation because oneDNN reported a multi-GB scratchpad requirement for a single
ConvolutionBackpropDatanode.Root Cause
This issue only happens with TBB_ADAPTIVE when the CPU partitioner runs in AUTO mode.
The CPU plugin calls
dnnl_threadpool_interop_set_max_concurrency(int max_concurrency)to set the number of threads for oneDNN.In partitioner=AUTO mode, max_concurrency=real worker-thread * 32.
Some oneDNN primitives use the inflated virtual concurrency to decide implementation details and scratchpad size.
As a result, descriptors could be built against an inflated thread count, which amplified scratchpad requirements and caused large memory allocations.
Simply put, oneDNN should use virtual concurrency to decide implementation details, and use real worker-thread to calculate scratchpad size.
So this pull request introduces new functionality to control and query the maximum concurrency used by oneDNN when computing scratchpad sizes outside parallel calls. It adds thread-local APIs for setting and getting this "scratchpad concurrency," updates internal utilities to support the feature, and integrates the new API into convolution code paths that allocate scratchpad memory.
Fixes
Fix compilation of small UNet crashes with 2026.0 and later on 16GB MTL
CVS-187773
Dependency PR: PR#36435
Checklist
General
make testandmake test_benchdnn_*) pass locally for each commit?Performance improvements
New features
Bug fixes
RFC PR