Messages in this thread | | | Date | Sun, 13 Dec 2015 21:18:42 +0000 | From | Al Viro <> | Subject | [RFC] get_free_pages/free_pages signature change |
| |
All the way back to v0.01 the page allocator had been representing addresses as unsigned long. The interface went through several iterations; the current primitives are __get_free_pages() and free_pages(), with __get_free_page() and get_zeroed_page() as convenience wrappers for the former and free_page() - for the latter. Allocating functions are returning unsigned long; freeing ones take unsigned long as an argument.
Unsurprisingly, there are both the call sites that treat an address as integer and ones that treat it as a pointer. Whether we use unsigned long or void *, one of those classes will need to use explicit typecasts.
The thing is, "pointer" call sites heavily outnumber the "integer" ones. The places where we pass to e.g. free_pages() (unsigned long)pointer account for 80% of all call sites. For other functions in that set the percentages vary, but they all are well above 50%. Moreover, quite a bit of the rest comes from the places like unsigned long buf = get_zeroed_page(GFP_KERNEL); ... code using buf only in form of (char *)buf ... free_page(buf); If we count such places as "pointer" ones (they would also be better off with pointer-returning variant, but the change would have to be slightly more extensive), the fraction of "pointer" call sites for e.g. free_pages() goes at least up to 90%.
In other words, switching to void * for return values of allocating and argument of freeing functions would reduce the amount of boilerplate quite nicely. What's more, fewer casts means better chance for typechecking to catch more bugs. Example caught while looking into free_pages() situation: drivers/media/platform/davinci/dm644x_ccdc.c:ccdc_update_raw_params() has fpc_physaddr = (unsigned int *)config_params->fault_pxl.fpc_table_addr; fpc_virtaddr = (unsigned int *)phys_to_virt( (unsigned long)fpc_physaddr); /* * Allocate memory for FPC table if current * FPC table buffer is not big enough to * accommodate FPC Number requested */ if (raw_params->fault_pxl.fp_num != config_params->fault_pxl.fp_num) { if (fpc_physaddr != NULL) { free_pages((unsigned long)fpc_physaddr, which is obviously wrong. The values expected by phys_to_virt() and by free_pages() are not the same. In the same file, ccdc_close() does fpc_physaddr = (unsigned int *)config_params->fault_pxl.fpc_table_addr;
if (fpc_physaddr != NULL) { fpc_virtaddr = (unsigned int *) phys_to_virt((unsigned long)fpc_physaddr); free_pages((unsigned long)fpc_virtaddr, get_order(config_params->fault_pxl.fp_num * FP_NUM_BYTES)); which makes a lot more sense - the virtual address is passed to free_pages(), not the physical one. And no, on arm/davinci phys_to_virt() is not a no-op; the only reason why the ccdc_update_raw_params() does not blow up all the time is that normally it's only called once, with fpc_table_addr still being zero, so that free_pages() in there isn't reached at all. With fewer casts (starting with "make fpc_physaddr an integer, keep fpc_virtaddr a pointer, don't cast that crap at all") it would've been caught just fine...
I wonder if the conversion is feasible; what I've got so far seems to be doable with relatively little PITA.
1) __get_free_page()/__get_free_pages() is the easiest of the entire bunch - we can add get_free_page()/get_free_pages() that would return void * at any point (the former would require renaming local inlines with the same name in drivers/block/xen-blkback/blkback.c and drivers/xen/xen-scsiback.c, the latter is completely unused). Conversion to those could go piecemeal, the originals becoming something like #define __get_free_page(gfp) ((unsigned long)get_free_page(gfp)) in the commit introducing void * variant. Eventually we could drop those defines, or leave them for as long as we want - doesn't really matter.
2) get_zeroed_page() has 275 call sites at the moment. 205 of them immediately cast to a pointer. Out of remaining 70, about a half (29) is in two files among drivers/net/wireless/*/debugfs.c; both of those would be better off from switching to seq_file anyway, and that might be worth doing as preliminary step. With that done, it would be a matter of replacing return type of get_zeroed_page with void * and adding casts to unsigned long in remaining 41 locations. Most of those can be eliminated in immediate followups, but those can go into individual subsystems' trees; the flagday part would have to be at the very end of a merge window and it can be generated pretty much automatically.
3) free_page() and free_pages() are the trickiest part; the nasty part is that we have __free_page()/__free_pages(), which *do* take a pointer and not to the area being freed - to relevant struct page. I think it's still worth switching free_page/free_pages to void *, but I'd probably make them macros along the lines of extern void __free_page_misspelled(void); extern void __free_page_pointer(void *); #define free_page(p) \ __builtin_choose_expr( \ __builtin_types_compatible_p(struct page *,typeof(f)), \ __free_page_misspelled(), \ __builtin_choose_expr( \ __builtin_types_compatible_p(const struct page *,typeof(f)), \ __free_page_misspelled(), \ __free_page_pointer(p))) so that errors of that sort would still be caught.
This will have to be a flagday change, preferably right next to the get_zeroed_page() one. I'd done such a patch for free_pages(), and it ended up with 31 call site (of 231) with cast to void * and the rest with casts removed. free_page() one will be about three times bigger. I'm not sure what's the best tactics here - one solution is to have dumb and straightforward "add a cast to void * at every call site", immediately followed by a bunch of "collapse the useless casts" per-subsystem patches that would go into relevant trees. That's very hard to fuck up, but more noisy than it has to be. Another variant is to have removals of obvious casts from pointers to unsigned long mixed into the flagday one, but that's slightly more dangerous - there are places where the boilerplate cast to unsigned long in free_pages() argument is done even to unsigned long expression. Of course that requires the same care in followup patches in the dumb approach. And missed cast to void * would immediately show in the build - it's not as if it would be quiet.
In any case, all followup patches would be in the same cycle; I hadn't done the full proof-of-concept series yet, but free_pages() part (about a quarter of the total) had taken one afternoon. It's certainly feasible.
Impact on the out-of-tree code (including development branches in the trees that get pulled into mainline) would be trivial - loud noise on build fixed by a one-liner at the location the warning points to, possibly with a followup along the lines of "now we can drop those 5 casts simply by keeping the result of get_zeroed_page/get_free_page/etc. as struct foo * rather than unsigned long; let's do it".
What do you think of that plan? If the answer is "hell, no - leave it alone, we are not going to do that kind of change no matter what"... well, I can live with that, seeing that we'd lived with that boilerplate for what, 24 years by now? But if it's "such and such parts of that outline is no-go because of..." (or, better yet, "post the patch series and we'll see"), I'd like to try to get it done. Comments?
| |