On Wed, 21 Mar 2018, Yang Shi wrote: Please CC everyone involved on the full patch set next time. I had to dig the rest out from my lkml archive to get the context. > Pass "true" to do_munmap() to not do unlock/relock to mmap_sem when > manipulating mpx map. > This is API change only. This is wrong. You cannot change the function in one patch and then clean up the users. That breaks bisectability. Depending on the number of callers this wants to be a single patch changing both the function and the callers or you need to create a new function which has the extra argument and switch all users over to it and then remove the old function. > @@ -780,7 +780,7 @@ static int unmap_entire_bt(struct mm_struct *mm, > * avoid recursion, do_munmap() will check whether it comes > * from one bounds table through VM_MPX flag. > */ > - return do_munmap(mm, bt_addr, mpx_bt_size_bytes(mm), NULL); > + return do_munmap(mm, bt_addr, mpx_bt_size_bytes(mm), NULL, true); But looking at the full context this is the wrong approach. First of all the name of that parameter 'atomic' is completely misleading. It suggests that this happens in fully atomic context, which is not the case. Secondly, conditional locking is frowned upon in general and rightfully so. So the right thing to do is to leave do_munmap() alone and add a new function do_munmap_huge() or whatever sensible name you come up with. Then convert the places which are considered to be safe one by one with a proper changelog which explains WHY this is safe. That way you avoid the chasing game of all existing do_munmap() callers and just use the new 'free in chunks' approach where it is appropriate and safe. No suprises, no bisectability issues.... While at it please add proper kernel doc documentation to both do_munmap() and the new function which explains the intricacies. Thanks, tglx
