Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Eugen Hristev <eugen.hristev@linaro.org>]

On 6/14/25 00:10, Thomas Gleixner wrote:
> On Fri, Jun 13 2025 at 17:33, Eugen Hristev wrote:
>> On 5/7/25 13:27, Eugen Hristev wrote:
>>>> Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
>>>> kmemdump specific section and then kmemdump can just walk that section
>>>> and dump stuff. No magic register functions and no extra storage
>>>> management for static/global variables.
>>>>
>>>> No?
>>>
>>> Thank you very much for your review ! I will try it out.
>>
>> I have tried this way and it's much cleaner ! thanks for the
>> suggestion.
> 
> Welcome.
> 
>> The thing that I am trying to figure out now is how to do something
>> similar for a dynamically allocated memory, e.g.
>> void *p = kmalloc(...);
>> and then I can annotate `p` itself, it's address and size, but what I
>> would also want to so dump the whole memory region pointed out by p. and
>> that area address and size cannot be figured out at compile time hence I
>> can't instantiate a struct inside the dedicated section for it.
>> Any suggestion on how to make that better ? Or just keep the function
>> call to register the area into kmemdump ?
> 
> Right. For dynamically allocated memory there is obviously no compile
> time magic possible.
> 
> But I think you can simplify the registration for dynamically allocated
> memory significantly.
> 
> struct kmemdump_entry {
> 	void			*ptr;
>         size_t			size;
>         enum kmemdump_uids	uid;
> };
> 
> You use that layout for the compile time table and the runtime
> registrations.
> 
> I intentionally used an UID as that avoids string allocation and all of
> the related nonsense. Mapping UID to a string is a post processing
> problem and really does not need to be done in the kernel. The 8
> character strings are horribly limited and a simple 4 byte unique id is
> achieving the same and saving space.
> 
> Just stick the IDs into include/linux/kmemdump_ids.h and expose the
> content for the post processing machinery.
> 
> So you want KMEMDUMP_VAR() for the compile time created table to either
> automatically create that ID derived from the variable name or you add
> an extra argument with the ID.

First of all, thank you very much for taking the time to think about this !

In KMEMDUMP_VAR, I can use __UNIQUE_ID to derive something unique from
the variable name for the table entry.

The only problem with having something like

#define KMEMDUMP_VAR(sym) \
	 static struct entry __UNIQUE_ID(kmemdump_entry_##sym) ...

is when calling it with e.g. `init_mm.pgd` which will make the `.`
inside the name and that can't happen.
So I have to figure a way to remove unwanted chars or pass a name to the
macro.

I cannot do something like
static  void * ptr = &init_mm.pgd;
and then
KMEMDUMP_VAR(ptr)
because ptr's dereferencing can't happen at compile time to add it's
value into the table entry.

> 
> kmemdump_init()
>         // Use a simple fixed size array to manage this
>         // as it avoids all the memory allocation nonsense
>         // This stuff is neither performance critical nor does allocating
>         // a few hundred entries create a memory consumption problem
>         // It consumes probably way less memory than the whole IDR/XARRAY allocation
>         // string duplication logic consumes text and data space.
> 	kmemdump_entries = kcalloc(NR_ENTRIES, sizeof(*kmemdump_entries), GFP_KERNEL);
> 
> kmemdump_register(void *ptr, size_t size, enum kmemdump_uids uid)
> {
>         guard(entry_mutex);
> 
> 	entry = kmemdump_find_empty_slot();
>         if (!entry)
>         	return;
> 
>         entry->ptr = ptr;
>         entry->size = size;
>         entry->uid = uid;
> 
>         // Make this unconditional by providing a dummy backend
>         // implementation. If the backend changes re-register all
>         // entries with the new backend and be done with it.
>         backend->register(entry);
> }
> 
> kmemdump_unregister(void *ptr)
> {
>         guard(entry_mutex);
>         entry = find_entry(ptr);
>         if (entry) {
>                 backend->unregister(entry);
>         	memset(entry, 0, sizeof(*entry);
>         }
> }
> 
> You get the idea.
> 
> Coming back to the registration at the call site itself.
> 
>        struct foo = kmalloc(....);
> 
>        if (!foo)
>        		return;
> 
>        kmemdump_register(foo, sizeof(*foo), KMEMDUMP_ID_FOO);
> 
> That's a code duplication shitshow. You can wrap that into:
> 
>        struct foo *foo = kmemdump_alloc(foo, KMEMDUMP_ID_FOO, kmalloc, ...);
> 
> #define kmemdump_alloc(var, id, fn, ...)				\
> 	({								\
>         	void *__p = fn(##__VA_ARGS__);				\
> 									\
>                 if (__p)						\
>                 	kmemdump_register(__p, sizeof(*var), id);	\
> 		__p;
>         })
> 

I was thinking into a new variant of kmalloc, like e.g. kdmalloc() which
would be a wrapper over kmalloc and also register the region into
kmemdump like you are suggesting.
It would be like a `dumpable` kmalloc'ed memory.
And it could take an optional ID , if missing, it could generate one.

However this would mean yet another k*malloc friend, and it would
default to usual kmalloc if CONFIG_KMEMDUMP=n .
I am unsure whether this would be welcome by the community

Let me know what you think.

Thanks again !
Eugen

> or something daft like that. And provide the matching magic for the free
> side.
> 
> Thoughts?
> 
> Thanks,
> 
>         tglx