On 31.07.19 15:33, Michal Hocko wrote: > On Wed 31-07-19 15:18:42, David Hildenbrand wrote: >> On 31.07.19 15:14, Michal Hocko wrote: >>> On Wed 31-07-19 15:02:49, David Hildenbrand wrote: >>>> On 31.07.19 14:53, Michal Hocko wrote: >>>>> On Wed 31-07-19 14:32:01, David Hildenbrand wrote: >>>>>> Let's document why we take the lock here. If we're going to overhaul >>>>>> memory hotplug locking, we'll have to touch many places - this comment >>>>>> will help to clairfy why it was added here. >>>>> >>>>> And how exactly is "lock for consistency" comment going to help the poor >>>>> soul touching that code? How do people know that it is safe to remove it? >>>>> I am not going to repeat my arguments how/why I hate "locking for >>>>> consistency" (or fun or whatever but a real synchronization reasons) >>>>> but if you want to help then just explicitly state what should done to >>>>> remove this lock. >>>>> >>>> >>>> I know that you have a different opinion here. To remove the lock, >>>> add_memory() locking has to be changed *completely* to the point where >>>> we can drop the lock from the documentation of the function (*whoever >>>> knows what we have to exactly change* - and I don't have time to do that >>>> *right now*). >>> >>> Not really. To remove a lock in this particular path it would be >>> sufficient to add >>> /* >>> * Although __add_memory used down the road is documented to >>> * require lock_device_hotplug, it is not necessary here because >>> * this is an early code when userspace or any other code path >>> * cannot trigger hotplug operations. >>> */ >> >> Okay, let me phrase it like this: Are you 100% (!) sure that we don't >> need the lock here. I am not - I only know what I documented back then >> and what I found out - could be that we are forgetting something else >> the lock protects. >> >> As I already said, I am fine with adding such a comment instead. But I >> am not convinced that the absence of the lock is 100% safe. (I am 99.99% >> sure ;) ). > > I am sorry but this is a shiny example of cargo cult programming. You do > not add locks just because you are not sure. Locks are protecting data > structures not code paths! If it is not clear what is actually protected > then that should be explored first before the lock is spread "just to be > sure" > > Just look here. You have pushed that uncertainty to whoever is going > touch this code and guess what, they are going to follow that lead and > we are likely to grow the unjustified usage and any further changes will > be just harder. I have seen that pattern so many times that it is even > not funny. And that's why I pushed back here. > > So let me repeat. If the lock is to stay then make sure that the comment > actually explains what has to be done to remove it because it is not > really required as of now. > The other extreme I saw: People dropping locks because they think they can be smart but end up making developers debug crashes for months (I am not lying). As I want to move on with this patch and have other stuff to work on, I will adjust the comment you gave and add that instead of the lock. -- Thanks, David / dhildenb
