| Message ID | 20220811153632.0ce73f72.alex.williamson@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] VFIO updates for v6.0-rc1 (part 2) | expand |
On Thu, Aug 11, 2022 at 2:36 PM Alex Williamson <alex.williamson@redhat.com> wrote: > > - Rename vfio source file to more easily allow additional source > files in the upcoming development cycles (Jason Gunthorpe) > > ---------------------------------------------------------------- > Jason Gunthorpe (1): > vfio: Move vfio.c to vfio_main.c > > drivers/vfio/Makefile | 2 ++ > drivers/vfio/{vfio.c => vfio_main.c} | 0 Guys, why do you do this ludicrously redundant file naming? The directory is called "vfio". Why is every file in it called "vfio_xyzzy.c"? This is a bad pattern, and I don't understand why you do this and continue to just make it worse. We don't have "drivers/block/block_floppy.c". We don't have "kernel/kernel_exit.c". And then when somebody finally points out that "vfio/vfio.c" is kind of silly and bad naming practice because it doesn't say what the file is all about, instead of realizing what the problem is, you just continue the same broken pattern. Is vfio the only subsystem that does this? No. We have the same odd pattern in "drivers/leds/leds-xyzzy.c" and a few other subsystems, and I don't understand it there either. I don't care that much, because I never touch those files, but if I did, I would have complained long ago about how auto-complete of filenames is broken because of that silly non-unique and pointless prefix that is just repeated mindlessly over and over again. So I've pulled this, since hey, "maintainer preference" and me not really having a lot of reason to *care*, but when I get this kind of pure rename pull request, I just have to pipe up about how silly and pointless the new name seems to be. Am I the only one that just uses auto-complete for everything when I'm off editing files in a terminal? And if you don't use autocomplete, and actually type things out fully, doesn't that double redundant 'vtio' bother you even *more*? I'm just confused and wondering about this all, since it seems so *odd*. It's like people have entirely missed what the point of using directories to give you a hierarchy of things is all about.. Linus
On Fri, Aug 12, 2022 at 09:35:34AM -0700, Linus Torvalds wrote: > On Thu, Aug 11, 2022 at 2:36 PM Alex Williamson > <alex.williamson@redhat.com> wrote: > > > > - Rename vfio source file to more easily allow additional source > > files in the upcoming development cycles (Jason Gunthorpe) > > > > ---------------------------------------------------------------- > > Jason Gunthorpe (1): > > vfio: Move vfio.c to vfio_main.c > > > > drivers/vfio/Makefile | 2 ++ > > drivers/vfio/{vfio.c => vfio_main.c} | 0 > > Guys, why do you do this ludicrously redundant file naming? > > The directory is called "vfio". > > Why is every file in it called "vfio_xyzzy.c"? I think this is partly a historical artifact because each of the files in the directory are compiling to actual kernel modules so the file name has to have vfio_ in it to fit into the global module namespace. Now, there is no reason I can see for all these files to be different modules, but that is how it is, and because we have some module parameters changing it is API breaking.. > So I've pulled this, since hey, "maintainer preference" and me not > really having a lot of reason to *care*, but when I get this kind of > pure rename pull request, I just have to pipe up about how silly and > pointless the new name seems to be. We can start to fix it up. I'm working on splitting that file up further right now, so I will name the new ones container.c, group.c and then we can resonably rename what is left as device.c - this would logically match the object structure in the code at least. Newer parts are already using non-prefix'd names: $ ls drivers/vfio/pci/mlx5/ cmd.c cmd.h Kconfig main.c Makefile > And if you don't use autocomplete, and actually type things out fully, > doesn't that double redundant 'vtio' bother you even *more*? Honestly, not really.. It is is just yet another tab in the bash autocomplete, and vscode's filename searcher makes it irrelevant. I'll watch out for it in future, eg I see I just merged drivers/infiniband/hw/erdma/ and they used prefixes too. Thanks, Jason
The pull request you sent on Thu, 11 Aug 2022 15:36:32 -0600:
> https://github.com/awilliam/linux-vfio.git tags/vfio-v6.0-rc1pt2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/d16b418fac3de0d2ac854b3a9a1a59a0ebf2a0e9
Thank you!
On Fri, Aug 12, 2022 at 9:58 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > I think this is partly a historical artifact because each of the files > in the directory are compiling to actual kernel modules so the file > name has to have vfio_ in it to fit into the global module namespace. > > Now, there is no reason I can see for all these files to be different > modules, but that is how it is, and because we have some module > parameters changing it is API breaking.. Oh, the module name issue is absolutely real. But we actually have good tools for that in the kernel build system, because we've had that issue for so long, and because it's not at all uncommon that one single kernel module needs to be built up from multiple different object files. I guess it does require an extra lines in the Makefile. Maybe we could improve on that, but that extra line does end up having real advantages in that it makes for a lot of flexibility (see below). Attached is a truly *stupid* patch just to show the concept. I literally just picked one vfio file at random to convert. The point being that this approach (a) makes it very easy to have the file naming you like (b) makes it *very* easy to have common helper libraries that get linked into the modules (c) also means that it's now basically trivial to split any of these drivers into multiple files, exactly because the file name isn't tied to the module name where that extra line is exactly what makes (b) and (c) so trivial. Now, don't get me wrong: this patch is *not* meant to be a "please do this". If people really like the current odd file naming policy, it's really not a lot of skin off my nose. So the attached patch is literally meant to be a "look, if you want to do this, it's really this simple". And you can easily do it one driver at a time, possibly when you have a "oops, this one driver is getting a bit large, so I'd like to split it up". Also, it should go without mention that I've not actually *tested* this patch. I did do a full build, and that full build results in a vfio_iommu_type1.ko module as expected, but there might be something I overlooked. There's also some build overhead from the indirection, but hey, what else is new. Our Makefiles are actually quite powerful, but they do make 'GNU make' spend a *lot* of time doing various string tricks. Linus
Hi Linus, We've had a request to get this into the current merge window to ease re-bases in the next development cycle. Thanks, Alex The following changes since commit c8a684e2e110376c58f0bfa30fb3855d1e319670: Merge tag 'leds-5.20-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds (2022-08-08 11:36:21 -0700) are available in the Git repository at: https://github.com/awilliam/linux-vfio.git tags/vfio-v6.0-rc1pt2 for you to fetch changes up to 0f3e72b5c8cfa0b57dc4fc7703a0a42dbc200ba9: vfio: Move vfio.c to vfio_main.c (2022-08-08 14:33:41 -0600) ---------------------------------------------------------------- VFIO updates for v6.0-rc1 (part 2) - Rename vfio source file to more easily allow additional source files in the upcoming development cycles (Jason Gunthorpe) ---------------------------------------------------------------- Jason Gunthorpe (1): vfio: Move vfio.c to vfio_main.c drivers/vfio/Makefile | 2 ++ drivers/vfio/{vfio.c => vfio_main.c} | 0 2 files changed, 2 insertions(+) rename drivers/vfio/{vfio.c => vfio_main.c} (100%)