fix(gpu): fix memory leak and buffer sizing in f128 FFT host functions - #3825
fix(gpu): fix memory leak and buffer sizing in f128 FFT host functions#3825pdroalves wants to merge 4 commits into
Conversation
|
✅ Backward-compat snapshot: everything looks good! No backward-compatibility issues detected. |
|
Forward Compatibility Matrix
|
guillermo-oyarzun
left a comment
guillermo-oyarzun
left a comment
There was a problem hiding this comment.
Good tricky catches, if we end up removing the host calls, then i would suggest to squash the commits.
| } | ||
|
|
||
| template <class params> | ||
| __host__ void host_fourier_transform_forward_as_integer_f128( |
There was a problem hiding this comment.
actually this function seems like dead code, probably that's why we didn't notice the problem, better to remove it in my opinion, unless @bbarbakadze left it there for some reason?
There was a problem hiding this comment.
We are not using it anywhere so I guess we can remove it.
There was a problem hiding this comment.
I think it's used in the C++ tests. Let me double check.
There was a problem hiding this comment.
So, that method is only used in cuda_fourier_transform_forward_as_integer_f128_async, which is a method we expose to core_crypto to run the FFT. We don't use it anywhere, but I don't know if we shouldn't just keep it in case we want someday.
| double2 *buffer = (double2 *)cuda_malloc_async(0, stream, gpu_index); | ||
| // global scratch is only allocated on the NOSM paths below; cuda_drop_async | ||
| // accepts the nullptr left by the FULLSM paths | ||
| double2 *buffer = nullptr; |
There was a problem hiding this comment.
i guess this buffer thing here was not much of a problem in practice cause we don't use the NOSM flavor, and in the worse case we were allocating a 0 size. Your version seems cleaner
3 participants
This PR fixes several memory management issues in the f128 FFT host functions and the 128-bit BSK conversion, found while investigating a memory leak in
host_fourier_transform_forward_as_torus_f128.Memory leak in the f128 FFT host functions
host_fourier_transform_forward_as_integer_f128,host_fourier_transform_forward_as_torus_f128, andhost_fourier_transform_backward_as_torus_f128allocated a global scratchbufferfor the FFT kernel but never freed it. The cleanup block only releasedd_standard,d_re0/d_re1/d_im0/d_im1.Scratch buffer allocated in elements instead of bytes
The same
bufferwas sized withsafe_mul(number_of_samples, N/2, 4), butcuda_malloc_asynctakes bytes, so the allocation was 8x too small (missing thesizeof(double)factor). Any call taking the no-shared-memory (NOSM) kernel path wrote out of bounds. The same defect existed inconvert_u128_to_f128_and_forward_fft_128(bootstrapping_key.cuh), which is used by the 128-bit BSK conversion.Device buffers not sized for batched calls
The three FFT host functions launch
number_of_sampleskernel blocks, each indexing its own sample slice, but all device buffers and host/device copies were sized for a single sample. Calls withnumber_of_samples > 1wrote out of bounds. Buffers and copies are now sized for the whole batch. (Current callers, the GPU FFT tests, pass 1, so this was latent.)Smaller cleanups
cuda_convert_lwe_programmable_bootstrap_keyalso initializes its scratch pointer tonullptrinstead of a 0-byte allocation that was overwritten (and lost) by the NOSM branches.cuda_drop_with_size_tracking_asyncis now an explicit no-op for null pointers, sincecudaFreeAsync(unlikecudaFree) is not documented to accept them.check_cuda_error(cudaGetLastError())after kernel launches in the twoas_torusFFT variants, and replaced C-style casts withstatic_castin the touched code.closes: please link all relevant issues
PR content/description
Check-list: