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

[Konstantin Khlebnikov]

On Wed, Mar 11, 2015 at 3:40 PM, Davidlohr Bueso <dave@xxxxxxxxxxxx> wrote:
> On Wed, 2015-03-11 at 15:21 +0300, Konstantin Khlebnikov wrote:
>> 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.
>
> Indeed. Yet I'm not changing the exe_file address space semantics at
> all.
>
>>
>> > 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?
>
> No, I want to get rid of certain things being done under mmap_sem,
> that's all. This is not performance motivated, it's to allow future work
> on lock breaking. I've just yesterday explained this at lsfmm (and not
> only related to exe_file). In any case I've clean up this patch and
> added more on top to create a friendlier interface, I'll send that out a
> bit later.
>
>> Then you should use rcu for that: struct file is rcu-protected thing.
>> See fget(), you could do something like that.
>
> As mentioned, not all exe paths are RCU friendly ;) We'd at least need
> srcu, but that's neither here nor there. A rwlock is suficient to get
> the job done and we really need not care much about optimizing this
> particular file further.

I mean you could make mm->exe_file rcu protected pointer and use
everywhere get_mm_exe_file() which grabs file refcount under rcu and
returns pointer.

>
> Thanks,
> Davidlohr
>
--
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/