Re: [PATCH v6 2/9] drm/xe: Add ASCII85 dump helper function

[John Harrison <john.c.harrison@intel.com>]

On 8/30/2024 16:33, John Harrison wrote:
> On 8/30/2024 14:56, Matthew Brost wrote:
>> On Thu, Aug 29, 2024 at 11:23:03PM -0700, John.C.Harrison@Intel.com 
>> wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> There is a need to include the GuC log and other large binary objects
>>> in core dumps and via dmesg. So add a helper for dumping to a printer
>>> function via conversion to ASCII85 encoding.
>>>
>>> Another issue with dumping such a large buffer is that it can be slow,
>>> especially if dumping to dmesg over a serial port. So add a yield to
>>> prevent the 'task has been stuck for 120s' kernel hang check feature
>>> from firing.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_devcoredump.c | 76 
>>> +++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_devcoredump.h |  5 ++
>>>   2 files changed, 81 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c 
>>> b/drivers/gpu/drm/xe/xe_devcoredump.c
>>> index bdb76e834e4c..eec7b89ab48b 100644
>>> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
>>> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
>>> @@ -6,6 +6,7 @@
>>>   #include "xe_devcoredump.h"
>>>   #include "xe_devcoredump_types.h"
>>>   +#include <linux/ascii85.h>
>>>   #include <linux/devcoredump.h>
>>>   #include <generated/utsrelease.h>
>>>   @@ -310,3 +311,78 @@ int xe_devcoredump_init(struct xe_device *xe)
>>>   }
>>>     #endif
>>> +
>>> +/**
>>> + * xe_print_blob_ascii85 - print a BLOB to some useful location in 
>>> ASCII85
>>> + *
>>> + * The output is split to multiple lines because some print 
>>> targets, e.g. dmesg
>>> + * cannot handle arbitrarily long lines. Note also that printing to 
>>> dmesg in
>>> + * piece-meal fashion is not possible, each separate call to 
>>> drm_puts() has a
>>> + * line-feed automatically added! Therefore, the entire output line 
>>> must be
>>> + * constructed in a local buffer first, then printed in one atomic 
>>> output call.
>>> + *
>>> + * There is also a scheduler yield call to prevent the 'task has 
>>> been stuck for
>>> + * 120s' kernel hang check feature from firing when printing to a 
>>> slow target
>>> + * such as dmesg over a serial port.
>>> + *
>>> + * TODO: Add compression prior to the ASCII85 encoding to shrink 
>>> huge buffers down.
>>> + *
>>> + * @p: the printer object to output to
>>> + * @blob: the Binary Large OBject to dump out
>>> + * @offset: offset in bytes to skip from the front of the BLOB, 
>>> must be a multiple of sizeof(u32)
>>> + * @size: the size in bytes of the BLOB, must be a multiple of 
>>> sizeof(u32)
>>> + */
>>> +void xe_print_blob_ascii85(struct drm_printer *p, const void *blob, 
>>> size_t offset, size_t size)
>>> +{
>>> +    const u32 *blob32 = (const u32 *)blob;
>>> +    char buff[ASCII85_BUFSZ], *line_buff;
>>> +    size_t line_pos = 0;
>>> +
>>> +#define DMESG_MAX_LINE_LEN    800
>>> +#define MIN_SPACE        (ASCII85_BUFSZ + 2)        /* 85 + "\n\0" */
>>> +
>>> +    if (size & 3)
>>> +        drm_printf(p, "Size not word aligned: %zu", size);
>>> +    if (offset & 3)
>>> +        drm_printf(p, "Offset not word aligned: %zu", size);
>>> +
>>> +    line_buff = kzalloc(sizeof(DMESG_MAX_LINE_LEN), GFP_KERNEL);
>>> +    if (IS_ERR(line_buff)) {
>>> +        drm_printf(p, "Failed to allocate line buffer: %pe", 
>>> line_buff);
>>> +        return;
>>> +    }
>>> +
>>> +    blob32 += offset / sizeof(*blob32);
>>> +    size /= sizeof(*blob32);
>>> +
>>> +    while (size--) {
>>> +        u32 val = *(blob32++);
>>> +
>>> +        strscpy(line_buff + line_pos, ascii85_encode(val, buff),
>>> +            DMESG_MAX_LINE_LEN - line_pos);
>>> +        line_pos += strlen(line_buff + line_pos);
>>> +
>>> +        if ((line_pos + MIN_SPACE) >= DMESG_MAX_LINE_LEN) {
>>> +            line_buff[line_pos++] = '\n';
>>> +            line_buff[line_pos++] = 0;
>>> +
>>> +            drm_puts(p, line_buff);
>>> +
>>> +            line_pos = 0;
>>> +
>>> +            /* Prevent 'stuck thread' time out errors */
>>> +            cond_resched();
>>> +        }
>>> +    }
>>> +
>>> +    if (line_pos) {
>>> +        line_buff[line_pos++] = '\n';
>>> +        line_buff[line_pos++] = 0;
>>> +
>>> +        drm_puts(p, line_buff);
>>> +    }
>>> +
>>> +    kfree(line_buff);
>> Dive by comment kvfree per CI. Will try to plan sometime soon to give
> Not for a kzalloc. That's just a kmalloc(GFP_ZERO) which means kfree. 
> Although I think the IS_ERR test is not actually correct. It should 
> just be testing for null. Not sure how that would result in the CI 
> failure though.
>
> John.
>
The boom in CI is because the alloc is for 'sizeof(DMESG_MAX_LINE_LEN)' 
which is 4 not 800! Doh! Not sure how come it didn't blow up in my local 
testing. I've dumped out some pretty huge logs without issue. Just lucky 
I guess?

John.