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
