On 11/24/21 19:32, Matthew Wilcox wrote:
On Wed, Nov 24, 2021 at 02:45:59PM -0800, John Hubbard wrote:@@ -2201,13 +2201,12 @@ static int __ref try_remove_memory(u64 start, u64 size) */ void __remove_memory(u64 start, u64 size) { - + int ret = try_remove_memory(start, size); /* * trigger BUG() if some memory is not offlined prior to calling this * function */ - if (try_remove_memory(start, size)) - BUG(); + BUG_ON(ret); }I'd rather leave it the way it is. I don't see why the version you propose is better.
In isolation, it's *not* better. It's only potentially useful in the context of "code plus tools". That is to say, if the coccinelle change request were rejected, then this provides a way forward that is not worse than the existing code, and also works around the warning.
...and by the way, while going to type that, I immediately stumbled upon another pre-existing case of this sort of thing, in try_remove_memory(), which does this: static int __ref try_remove_memory(u64 start, u64 size) { struct vmem_altmap mhp_altmap = {}; struct vmem_altmap *altmap = NULL; unsigned long nr_vmemmap_pages; int rc = 0, nid = NUMA_NO_NODE; BUG_ON(check_hotplug_memory_range(start, size));That needs to be fixed.
Yes it does. :) I pointed it out in hopes that Chiminghao might be inspired to go find and fix some of these. thanks, -- John Hubbard NVIDIA
