Re: [RFC PATCH 0/6] debugfs: Replace dentry with an opaque handle in debugfs API

[David Reaver]

Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:

>
> First off, many thanks for attempting this, I didn't think it was ready
> to even be attempted, so it's very nice to see this.
>

No problem, and thank you for taking a look!

> That being said, I agree with Al, we can't embed a dentry in a structure
> like that as the lifecycles are going to get messy fast.
>

Ack, I'll do something different in v2.

For my own education: what goes wrong with lifecycles with this embed?
Feel free to point me at a doc or something.

Also, Al and Greg, would wrapping a pointer be fine?

	struct debugfs_node {
		struct dentry *dentry;
	};

I was trying to do the simplest thing possible so the bulk of the change
was mechanical. Wrapping a pointer is slightly more complicated because
we have to deal with memory allocation, but it is still totally doable.

> Also, your replacement of many of the dentry functions with wrappers
> seems at bit odd, ideally you would just return a dentry from a call
> like "debugfs_node_to_dentry()" and then let the caller do with it what
> it wants to, that way you don't need to wrap everything.
>

Understood. I considered exposing the underlying dentry as a "dirty
backdoor" around the opaque wrapper, so I was trying to minimize it :)
I'm happy to undo some of these wrappers though, it will make the change
simpler.

> And finally, I think that many of the places where you did have to
> convert the code to save off a debugfs node instead of a dentry can be
> removed entirely as a "lookup this file" can be used instead.  I was
> waiting for more conversions of that logic, removing the need to store
> anything in a driver/subsystem first, before attempting to get rid of
> the returned dentry pointer.
>

Yeah this is a great idea, and could even be done in a few patches
outside of this large migration patch series if necessary. I'll
investigate.

> As an example of this, why not look at removing almost all of those
> pointers in the relay code?  Why is all of that being stored at all?
>

I'll take another look at the relay code as well and see if I can remove
the pointers.

> Oh, also, all of those forward declarations look really odd, something
> feels wrong with needing that type of patch if we are doing things
> right.  Are you sure it was needed?
>

I agree with this sentiment, and I discussed this in the cover letter a
bit under the section "#includes and #defines". The need for peppering
temporary #defines (for intermediate commits) and forward declarations
around is my least favorite part of this patch series.

I am indeed sure they are needed in most cases. I'll give a few examples
for both the temporary #defines Coccinelle adds and the forward
declarations that replace the #defines in the last commit:

1. If you remove the forward declaration (or the corresponding temporary
   #define in the Coccincelle commit) in
   drivers/gpu/drm/xe/xe_gsc_debugfs.h, you get this compilation error:

   drivers/gpu/drm/xe/xe_gsc_debugfs.h:12:57: error: ‘struct debugfs_node’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      12 | void xe_gsc_debugfs_register(struct xe_gsc *gsc, struct debugfs_node *parent);

   gcc does not like implicitly-defined types inside of function
   arguments. As far as I can tell, we only get this error for function
   arguments; this is apparently okay for a top-level declaration, like:

   struct debugfs_node *my_root_node;


2. In the Coccinelle commit, if you remove the #define debugfs_node from
   include/linux/fault-inject.h, you get errors of this sort:

   mm/fail_page_alloc.c:55:13: error: assignment to ‘struct dentry *’ from incompatible pointer type ‘struct debugfs_node *’ [-Werror=incompatible-pointer-types]
      55 |         dir = fault_create_debugfs_attr("fail_page_alloc", NULL,
         |             ^

   Because the #define is not in scope, the compiler is assuming we are
   implicitly defining a new type.


The Coccinelle script adds a forward declaration of struct debugfs_node
wherever there was one for struct dentry. This is just a heuristic I
found that seemed to do the job and was easy to automate.

I originally did this whole patch series in reverse, where we
immediately make struct debugfs_node, migrate debugfs internals, and
migrate all users of the API, but that leads to one very large commit
and appeared harder to review to me. I went with this intermediate
#define idea so the commits could be split up and each commit would
compile, but I don't like the little bit of extra complexity it adds.

I'm open to any other migration ideas folks have! I'm not tied to these
two plans at all.

Thanks,
David Reaver