[Beignet] [PATCH] Fix the printf buffer size bug.
He Junyan
junyan.he at inbox.com
Tue Jan 13 00:03:51 PST 2015
Sorry to add V2 comment, just ignore this.
On 二, 2015-01-13 at 15:40 +0800, junyan.he at inbox.com wrote:
> From: Junyan He <junyan.he at linux.intel.com>
>
> We can not know the accurate size of the printf buffer
> size before run the kernel. Sometimes, especially when
> the global work items size is huge, the output buffer
> is not enough and the print message logic will cause the
> segment fault.
> We increase the printf buffer to 16M at most and add out
> of range check to avoid crash.
>
> Signed-off-by: Junyan He <junyan.he at linux.intel.com>
> ---
> backend/src/backend/program.cpp | 5 +++--
> backend/src/backend/program.h | 2 +-
> backend/src/backend/program.hpp | 4 ++--
> backend/src/ir/printf.cpp | 16 ++++++++++------
> backend/src/ir/printf.hpp | 2 +-
> src/cl_command_queue.c | 5 +++--
> src/cl_command_queue_gen7.c | 2 +-
> src/cl_driver.h | 2 +-
> src/intel/intel_gpgpu.c | 8 +++++++-
> 9 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/backend/src/backend/program.cpp b/backend/src/backend/program.cpp
> index b22e4a6..38ce9c8 100644
> --- a/backend/src/backend/program.cpp
> +++ b/backend/src/backend/program.cpp
> @@ -1068,12 +1068,13 @@ namespace gbe {
>
> static void kernelOutputPrintf(void * printf_info, void* index_addr,
> void* buf_addr, size_t global_wk_sz0,
> - size_t global_wk_sz1, size_t global_wk_sz2)
> + size_t global_wk_sz1, size_t global_wk_sz2,
> + size_t output_sz)
> {
> if (printf_info == NULL) return;
> ir::PrintfSet *ps = (ir::PrintfSet *)printf_info;
> ps->outputPrintf(index_addr, buf_addr, global_wk_sz0,
> - global_wk_sz1, global_wk_sz2);
> + global_wk_sz1, global_wk_sz2, output_sz);
> }
>
> static void kernelGetCompileWorkGroupSize(gbe_kernel gbeKernel, size_t wg_size[3]) {
> diff --git a/backend/src/backend/program.h b/backend/src/backend/program.h
> index 6acc010..dc5662f 100644
> --- a/backend/src/backend/program.h
> +++ b/backend/src/backend/program.h
> @@ -157,7 +157,7 @@ typedef uint32_t (gbe_get_printf_sizeof_size_cb)(void* printf_info);
> extern gbe_get_printf_sizeof_size_cb *gbe_get_printf_sizeof_size;
>
> typedef void (gbe_output_printf_cb) (void* printf_info, void* index_addr, void* buf_addr,
> - size_t global_wk_sz0, size_t global_wk_sz1, size_t global_wk_sz2);
> + size_t global_wk_sz0, size_t global_wk_sz1, size_t global_wk_sz2, size_t outbuf_sz);
> extern gbe_output_printf_cb* gbe_output_printf;
>
> /*! Create a new program from the given source code (zero terminated string) */
> diff --git a/backend/src/backend/program.hpp b/backend/src/backend/program.hpp
> index 66f90aa..cff2463 100644
> --- a/backend/src/backend/program.hpp
> +++ b/backend/src/backend/program.hpp
> @@ -155,10 +155,10 @@ namespace gbe {
> }
>
> void outputPrintf(void* index_addr, void* buf_addr, size_t global_wk_sz0,
> - size_t global_wk_sz1, size_t global_wk_sz2) {
> + size_t global_wk_sz1, size_t global_wk_sz2, size_t output_sz) {
> if(printfSet)
> printfSet->outputPrintf(index_addr, buf_addr, global_wk_sz0,
> - global_wk_sz1, global_wk_sz2);
> + global_wk_sz1, global_wk_sz2, output_sz);
> }
>
> ir::FunctionArgument::InfoFromLLVM* getArgInfo(uint32_t id) const { return &args[id].info; }
> diff --git a/backend/src/ir/printf.cpp b/backend/src/ir/printf.cpp
> index 0e9f81c..fa108dc 100644
> --- a/backend/src/ir/printf.cpp
> +++ b/backend/src/ir/printf.cpp
> @@ -105,16 +105,20 @@ namespace gbe
> #define PRINT_SOMETHING(target_ty, conv) do { \
> if (!vec_i) \
> pf_str = pf_str + std::string(#conv); \
> - printf(pf_str.c_str(), \
> - ((target_ty *)((char *)buf_addr + sizeOfSize * global_wk_sz0 * global_wk_sz1 * global_wk_sz2 * n \
> - + slot.state->out_buf_sizeof_offset * \
> - global_wk_sz0 * global_wk_sz1 * global_wk_sz2)) \
> - [(k*global_wk_sz0*global_wk_sz1 + j*global_wk_sz0 + i) * vec_num + vec_i]);\
> + char *ptr = ((char *)buf_addr + sizeOfSize * global_wk_sz0 * global_wk_sz1 * global_wk_sz2 * n \
> + + slot.state->out_buf_sizeof_offset * \
> + global_wk_sz0 * global_wk_sz1 * global_wk_sz2); \
> + target_ty* obj_ptr = ((target_ty *)ptr) + (k*global_wk_sz0*global_wk_sz1 + j*global_wk_sz0 + i) * vec_num + vec_i; \
> + if ((char *)obj_ptr + sizeof(target_ty) > (char *)buf_addr + output_sz) { \
> + printf("\n\n!!!The printf message is out of range because of the limited buffer, ignore.\n"); \
> + return; \
> + } \
> + printf(pf_str.c_str(), *obj_ptr); \
> } while (0)
>
>
> void PrintfSet::outputPrintf(void* index_addr, void* buf_addr, size_t global_wk_sz0,
> - size_t global_wk_sz1, size_t global_wk_sz2)
> + size_t global_wk_sz1, size_t global_wk_sz2, size_t output_sz)
> {
> LockOutput lock;
> size_t i, j, k;
> diff --git a/backend/src/ir/printf.hpp b/backend/src/ir/printf.hpp
> index cc1f8dc..f6c6bcf 100644
> --- a/backend/src/ir/printf.hpp
> +++ b/backend/src/ir/printf.hpp
> @@ -253,7 +253,7 @@ namespace gbe
> }
>
> void outputPrintf(void* index_addr, void* buf_addr, size_t global_wk_sz0,
> - size_t global_wk_sz1, size_t global_wk_sz2);
> + size_t global_wk_sz1, size_t global_wk_sz2, size_t output_sz);
>
> private:
> vector<PrintfFmt> fmts;
> diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c
> index 6103909..89afa07 100644
> --- a/src/cl_command_queue.c
> +++ b/src/cl_command_queue.c
> @@ -216,7 +216,8 @@ LOCAL void
> cl_command_queue_flush_gpgpu(cl_command_queue queue, cl_gpgpu gpgpu)
> {
> size_t global_wk_sz[3];
> - void* printf_info = cl_gpgpu_get_printf_info(gpgpu, global_wk_sz);
> + size_t outbuf_sz = 0;
> + void* printf_info = cl_gpgpu_get_printf_info(gpgpu, global_wk_sz, &outbuf_sz);
>
> cl_gpgpu_flush(gpgpu);
>
> @@ -227,7 +228,7 @@ cl_command_queue_flush_gpgpu(cl_command_queue queue, cl_gpgpu gpgpu)
> buf_addr = cl_gpgpu_map_printf_buffer(gpgpu, 1);
>
> interp_output_printf(printf_info, index_addr, buf_addr, global_wk_sz[0],
> - global_wk_sz[1], global_wk_sz[2]);
> + global_wk_sz[1], global_wk_sz[2], outbuf_sz);
>
> cl_gpgpu_unmap_printf_buffer(gpgpu, 0);
> if (interp_get_printf_sizeof_size(printf_info))
> diff --git a/src/cl_command_queue_gen7.c b/src/cl_command_queue_gen7.c
> index 734267a..eec39b4 100644
> --- a/src/cl_command_queue_gen7.c
> +++ b/src/cl_command_queue_gen7.c
> @@ -281,7 +281,7 @@ cl_bind_printf(cl_gpgpu gpgpu, cl_kernel ker, void* printf_info, int printf_num,
> if (buf_size < 1*1024)
> buf_size = 1*1024*1024;
> else
> - buf_size = 4*1024*1024; //at most.
> + buf_size = 16*1024*1024; //at most.
>
> if (offset > 0) {
> if (cl_gpgpu_set_printf_buffer(gpgpu, 1, buf_size, offset, interp_get_printf_buf_bti(printf_info)) != 0)
> diff --git a/src/cl_driver.h b/src/cl_driver.h
> index f13ebee..c88b9be 100644
> --- a/src/cl_driver.h
> +++ b/src/cl_driver.h
> @@ -266,7 +266,7 @@ typedef int (cl_gpgpu_set_printf_info_cb)(cl_gpgpu, void *, size_t*);
> extern cl_gpgpu_set_printf_info_cb *cl_gpgpu_set_printf_info;
>
> /* Get the last printfset pointer */
> -typedef void* (cl_gpgpu_get_printf_info_cb)(cl_gpgpu, size_t*);
> +typedef void* (cl_gpgpu_get_printf_info_cb)(cl_gpgpu, size_t*, size_t*);
> extern cl_gpgpu_get_printf_info_cb *cl_gpgpu_get_printf_info;
>
> /* Will spawn all threads */
> diff --git a/src/intel/intel_gpgpu.c b/src/intel/intel_gpgpu.c
> index 1836bfd..8cc660b 100644
> --- a/src/intel/intel_gpgpu.c
> +++ b/src/intel/intel_gpgpu.c
> @@ -1896,11 +1896,17 @@ intel_gpgpu_set_printf_info(intel_gpgpu_t *gpgpu, void* printf_info, size_t * gl
> }
>
> static void*
> -intel_gpgpu_get_printf_info(intel_gpgpu_t *gpgpu, size_t * global_sz)
> +intel_gpgpu_get_printf_info(intel_gpgpu_t *gpgpu, size_t * global_sz, size_t *outbuf_sz)
> {
> global_sz[0] = gpgpu->global_wk_sz[0];
> global_sz[1] = gpgpu->global_wk_sz[1];
> global_sz[2] = gpgpu->global_wk_sz[2];
> +
> + if (gpgpu->printf_b.bo)
> + *outbuf_sz = gpgpu->printf_b.bo->size;
> + else
> + *outbuf_sz = 0;
> +
> return gpgpu->printf_info;
> }
>
More information about the Beignet
mailing list