Re: [PATCH] mm: replace mmap_sem for mm->exe_file serialization

[Konstantin Khlebnikov]

On Fri, Feb 27, 2015 at 9:34 PM, Davidlohr Bueso <dave@xxxxxxxxxxxx> wrote:
> On Fri, 2015-02-27 at 18:36 +0100, Oleg Nesterov wrote:
>> On 02/26, Cyrill Gorcunov wrote:
>> >
>> > On Thu, Feb 26, 2015 at 11:36:57AM -0800, Davidlohr Bueso wrote:
>> > > We currently use the mmap_sem to serialize the mm exe_file.
>> > > This is atrocious and a clear example of the misuses this
>> > > lock has all over the place, making any significant changes
>> > > to the address space locking that much more complex and tedious.
>> > > This also has to do of how we used to check for the vma's vm_file
>> > > being VM_EXECUTABLE (much of which was replaced by 2dd8ad81e31).
>> > >
>> > > This patch, therefore, removes the mmap_sem dependency and
>> > > introduces a specific lock for the exe_file (rwlock_t, as it is
>> > > read mostly and protects a trivial critical region). As mentioned,
>> > > the motivation is to cleanup mmap_sem (as opposed to exe_file
>> > > performance).
>>
>> Well, I didn't see the patch, can't really comment.
>>
>> But I have to admit that this looks as atrocious and a clear example of
>> "lets add yet another random lock which we will regret about later" ;)
>>
>> rwlock_t in mm_struct just to serialize access to exe_file?
>
> I don't see why this is a random lock nor how would we regret this
> later. I regret having to do these kind of patches because people were
> lazy and just relied on mmap_sem without thinking beyond their use case.

That's history: exe_file had direct relation to mm->mmap_sem,
that was file from first executable vma. After my patch it's less
related to vmas.

> As mentioned I'm also planning on creating an own sort of
> exe_file_struct, which would be an isolated entity (still in the mm
> though), with its own locking and prctl bits, that would tidy mm_struct
> a bit. RCU was something else I considered, but it doesn't suite well in
> all paths and we would still need a spinlock when updating the file
> anyway.

Please don't. What's wrong with mmap_sem?

Do you want optimize reading mm->exe_file?
Then you should use rcu for that: struct file is rcu-protected thing.
See fget(), you could do something like that.

>
> If you have a better suggestion please do tell.
>
>>
>> > A nice side effect of this is that we avoid taking
>> > > the mmap_sem (shared) in fork paths for the exe_file handling
>> > > (note that readers block when the rwsem is taken exclusively by
>> > > another thread).
>>
>> Yes, this is ugly. Can't we kill this dup_mm_exe_file() and copy change
>> dup_mmap() to also dup ->exe_file ?
>>
>> > Hi Davidlohr, it would be interesting to know if the cleanup
>> > bring some performance benefit?
>>
>> To me the main question is whether the patch makes this code simpler
>> or uglier ;)
>
> Its much beyond that. As mentioned, for any significant changes to the
> mmap_sem locking scheme, this sort of thing needs to be addressed first.
>
> Thanks,
> Davidlohr
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/