instead of printing, log metrics into array and transfer them back to host - #8
instead of printing, log metrics into array and transfer them back to host#8craiig wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
The reason NCI is failing is because the output file length is too large, which is a check that happens in setup_output_descriptor(). We don't actually check the return of this function, so it fails silently and still executes the decompression but doesn't write out to the file.
In this case the output file size is ~30MB, but I've limited both the output and input sizes to 16MB per DPU. We can bump up the limit to higher, and they don't need to be the same for input and output.
My only concern before was that an input file would be accepted that resulted in a decompressed file that was too large to fit. But we have checks for both of these things, so it's not a big deal. You can fix this before merging if you want, or you can merge and I can fix this.
EDIT: I made this fix in my latest commit to master
| uint8_t idx = me(); | ||
|
|
||
| printf("DPU starting, tasklet %d\n", idx); | ||
| /* printf("DPU starting, tasklet %d\n", idx); */ |
There was a problem hiding this comment.
I would just remove these commented out print statements. I've never been a fan of keeping commented out code around just in case we need it later.
That's what version control is for :D
| { | ||
| struct in_buffer_context input; | ||
| struct out_buffer_context output; | ||
| /* perfcounter_config(COUNT_CYCLES, true); */ |
| #define TOTAL_NR_TASKLETS (NR_DPUS * NR_TASKLETS) | ||
|
|
||
| /* This should be coming from the config but the include isn't working for me. */ | ||
| /* #include <perfcounter.h> */ |
There was a problem hiding this comment.
I didn't check, but I believe <perfcounter.h> is a "dpu only" header, and this file is compiled for the host.
| } | ||
|
|
||
| /* flip between instructions and cycles on different DPUs */ | ||
| perfcounter_config_t perf_ctr_config = (dpu_idx % 2) == 0 ? COUNT_CYCLES : COUNT_INSTRUCTIONS; |
There was a problem hiding this comment.
this is correct, but I'm used to seeing: (dpu_idx & 1) ? x : y;
| DPU_ASSERT(dpu_copy_to(dpu, "output_offset", 0, output_offset[dpu_idx], sizeof(uint32_t) * NR_TASKLETS)); | ||
| DPU_ASSERT(dpu_copy_to(dpu, "output_length", 0, &output_length, sizeof(uint32_t))); | ||
| DPU_ASSERT(dpu_copy_to(dpu, "output_buffer", 0, &output_buffer_start, sizeof(uint32_t))); | ||
| /* copy metrics just to basically zero the buffers out? */ |
There was a problem hiding this comment.
This comment doesn't sound very confident. Is that why you are copying the buffers?
3 participants
I'm concerned that calling printf() from multiple tasklets could significantly delay the pipeline, taking up slots that other tasklets could use for processing, and skewing the results. Instead, let's eliminate printf from the execution and just write the perf counter back to mram for the host to pick up and report.
Eventually, I think we could use the perf counter results on the host-side to do intelligent tasklet tuning, and the start of it will be collecting some detailed info like this. I've just been running this in simulation, so we'd need to look at these numbers from real execution to make some inferences about behaviour.
For some reason, NCI is failing on this push, so I guess it is not totally safe to merge. Any clue as to why? Other tests seem to work.